Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(151): removing duplicate status code guidance #227

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+67 −81
Open

fix(151): removing duplicate status code guidance #227

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 67 additions & 81 deletions aep/general/0151/aep.md.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,41 +7,89 @@ return some kind of promise to the user, and allow the user to check back in
later.

The long-running request pattern is roughly analogous to a [Future][] in Python
or Java, or a [Node.js Promise][]. Essentially, the user is given a token that
can be used to track progress and retrieve the result.
or Java, or a [Node.js Promise][]: the user is given a token that can be used to
track progress and retrieve the result.

## Guidance

Operations that might take a significant amount of time to complete **should**
return a `202 Accepted` response along with an `Operation` resource that can be
used to track the status of the request and ultimately retrieve the result.
return an `Operation` resource that can be used to track the status of the
request and ultimately retrieve the result.

Any single operation defined in an API surface **must** either _always_ return
`202 Accepted` along with an `Operation`, or _never_ do so. A service **must
not** return a `200 OK` response with the result if it is "fast enough", and
`202 Accepted` if it is not fast enough, because such behavior adds significant
burdens for clients.
A single method defined in an API surface **must** either _always_ return an
`Operation`, or _never_ do so. As an example, a long-running operation **must
not** return a response with the result of the operation if the operation
completes quickly.

**Note:** User expectations can vary on what is considered "a significant
amount of time" depending on what work is being done. A good rule of thumb is
10 seconds.
### Interface Definitions

### Operation representation
{% tab proto %}

The response to a long-running request **must** be an [`Operation`][Operation].
When using protocol buffers, the common component
[`aep.api.Operation`][aep.api.Operation] is used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Is this undefined, or am I failing to find it? cf GitHub and Buf

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be fixed after aep-dev/aep-components#8 - but @rofrankel could confirm.


{% tab proto %}
{% sample 'lro.proto', 'rpc WriteBook' %}

Protocol buffer APIs **must** use the common component
[`aep.api.Operation`][aep.api.Operation].
- The response type **must** be `aep.api.Operation`. The `Operation` proto
definition **should not** be copied into individual APIs; prefer to use a
single copy (in monorepo code bases), or remote dependencies via a tool like
[Buf][buf.build].
- The response **must not** be a streaming response.
- The method **must** include a [`aep.api.operation_info`][lro] annotation,
which **must** define both response and metadata types.
- The response and metadata types **must** be defined in the file where the
RPC appears, or a file imported by that file.
- If the response and metadata types are defined in another package, the
fully-qualified message name **must** be used.
- The response type **should not** be `google.protobuf.Empty` (except for
[`Delete`][aep-135] methods), unless it is certain that response data will
_never_ be needed. If response data might be added in the future, define an
empty message for the RPC response and use that.
- The metadata type is used to provide information such as progress, partial
failures, and similar information on each `GetOperation` call. The metadata
type **should not** be `google.protobuf.Empty`, unless it is certain that
metadata will _never_ be needed. If metadata might be added in the future,
define an empty message for the RPC metadata and use that.
- APIs with messages that return `Operation` **must** implement the
`GetOperation` method of the [`Operations`][lro] service, and **may**
implement the other methods defined in that service. Individual APIs **must
not** define their own interfaces for long-running operations to avoid
inconsistency.

{% tab oas %}

OpenAPI services **must** use this [`JSON Schema Operation`][JSON Schema
Operation] schema.
{% sample 'lro.oas.yaml', 'paths' %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: Should the Operation part of the lro.oas.yaml example be extracted to a common component for re-use?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: The operation schema here doesn't utilize oneOf error/response the same way the protobuf schema does. Is this deliberate or an oversight? Is there a limit to the usefulness of oneOf I'm unaware of?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's an oversight, I'd guess - this PR was primarily meant to just fix the duplicate status code guidance, but we would maybe scope the PR bigger and just to re-review /fix all the issues you mentioned. Would you prefer that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No preference. I'm generally OK with things noticed in PR but out of scope becoming issues to be resolved later or follow on PRs. Or not resolved at all if I'm seeing problems where there are none.


- `202` **must** be the only success status code defined.
- The `202` response **must** define an `application/json` response body and no
other response content types.
- The response body schema **must** be an object with `path`, `done`, `error`,
and `response` properties as described above for an `Operation`.
- The response body schema **may** contain an object property named `metadata`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Should that schema be included in this schema using $ref to ensure the responses can be validated? Should it be referenced somehow like it is in the protobuf option to allow re-usability of the Operation? Something else entirely?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question for the response schema.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm.. maybe @rofrankel might have an opinion here? or @rambleraptor who's been working on a couple clients. Or @wora who's worked on AIPs.

It does feel like we should be describing the schema where possible. Protobuf schemas suffer a little bit from lack of dynamicism. Ideally we'd probably have individual LRO objects for each operation, but I imagine that being cumbersome is why aip.dev didn't recommend that.

The thing about LROs (to me) is that it's generally just a pattern to help combat disconnects. If a generated / dynamic client sees that the response is an LRO, all it would do is switch from extracting the response from the original request, to polling and extracting the response from the result of the LRO. Any metadata attached would generally be unused.

If you want clients to use the information, you'd be better off adding it to the response of the original request, not metadata.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

among the specific examples given - progress and metadata are both examples to me of common elements that would probably better off standardized.

For example, and LRO with a status string should be consumed by clients and printed as a way for them to know that the request didn't hang.

to hold service-specific metadata associated with the operation, for example
progress information and common metadata such as create time. The service
**should** define the contents of the `metadata` object in a separate schema,
which **should** specify `additionalProperties: true` to allow for future
extensibility.
- The `response` property **must** be a schema that defines the success
response for the operation. For an operation that typically gives a
`204 No Content` response, such as a `Delete`, `response` should be defined
as an empty object schema. For a standard `Get/Create/Update` operation,
`response` should be a representation of the resource.
- If a service has any long running operations, the service **must** define an
`Operation` resource with a `list` operation to retrieve a potentially
filtered list of operations and a `get` operation to retrieve a specific
operation by its `path`.

{% endtabs %}

### When to use an long-running operation

User expectations can vary on what is considered a significant amount of time
depending on what work is being done. Generally, a request **should** complete
10 seconds, as the longer period of time may block clients or by perceived by a
user as a hung request.

### Querying an operation

The service **must** provide an endpoint to query the status of the operation,
Expand Down Expand Up @@ -96,68 +144,6 @@ Errors that occur over the course of a request **may** be placed in the
metadata message. The errors themselves **must** still be represented with a
canonical error object.

## Interface Definitions

{% tab proto %}

When using protocol buffers, the common component
[`aep.api.Operation`][aep.api.Operation] is used.

{% sample 'lro.proto', 'rpc WriteBook' %}

- The response type **must** be `aep.api.Operation`. The `Operation` proto
definition **should not** be copied into individual APIs; prefer to use a
single copy (in monorepo code bases), or remote dependencies via a tool like
[Buf][buf.build].
- The response **must not** be a streaming response.
- The method **must** include a [`aep.api.operation_info`][lro] annotation,
which **must** define both response and metadata types.
- The response and metadata types **must** be defined in the file where the
RPC appears, or a file imported by that file.
- If the response and metadata types are defined in another package, the
fully-qualified message name **must** be used.
- The response type **should not** be `google.protobuf.Empty` (except for
[`Delete`][aep-135] methods), unless it is certain that response data will
_never_ be needed. If response data might be added in the future, define an
empty message for the RPC response and use that.
- The metadata type is used to provide information such as progress, partial
failures, and similar information on each `GetOperation` call. The metadata
type **should not** be `google.protobuf.Empty`, unless it is certain that
metadata will _never_ be needed. If metadata might be added in the future,
define an empty message for the RPC metadata and use that.
- APIs with messages that return `Operation` **must** implement the
`GetOperation` method of the [`Operations`][lro] service, and **may**
implement the other methods defined in that service. Individual APIs **must
not** define their own interfaces for long-running operations to avoid
inconsistency.

{% tab oas %}

{% sample 'lro.oas.yaml', 'paths' %}

- `202` **must** be the only success status code defined.
- The `202` response **must** define an `application/json` response body and no
other response content types.
- The response body schema **must** be an object with `path`, `done`, `error`,
and `response` properties as described above for an `Operation`.
- The response body schema **may** contain an object property named `metadata`
to hold service-specific metadata associated with the operation, for example
progress information and common metadata such as create time. The service
**should** define the contents of the `metadata` object in a separate schema,
which **should** specify `additionalProperties: true` to allow for future
extensibility.
- The `response` property **must** be a schema that defines the success
response for the operation. For an operation that typically gives a
`204 No Content` response, such as a `Delete`, `response` should be defined
as an empty object schema. For a standard `Get/Create/Update` operation,
`response` should be a representation of the resource.
- If a service has any long running operations, the service **must** define an
`Operation` resource with a `list` operation to retrieve a potentially
filtered list of operations and a `get` operation to retrieve a specific
operation by its `path`.

{% endtabs %}

<!-- prettier-ignore-start -->
[google.rpc.Status]: https://github.com/googleapis/api-common-protos/blob/master/google/rpc/S.proto
[lro]: https://github.com/aep-dev/aep/blob/main/proto/aep-api/aep/api/operations.proto
Expand Down
Loading