fix(151): removing duplicate status code guidance #227
Conversation
The guidance around LROs described the 202 status code for gRPC, which is not relevant. Since this is already documented in the "Interface Definitions" section, removing the duplicate guidance seemed the best option - it will minimize future maintenance. Helps address aep-dev#224.
|
|
||
| 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. |
There was a problem hiding this comment.
I think this will be fixed after aep-dev/aep-components#8 - but @rofrankel could confirm.
|
|
||
| {% tab oas %} | ||
|
|
||
| OpenAPI services **must** use this [`JSON Schema Operation`][JSON Schema | ||
| Operation] schema. | ||
| {% sample 'lro.oas.yaml', 'paths' %} |
There was a problem hiding this comment.
thought: Should the Operation part of the lro.oas.yaml example be extracted to a common component for re-use?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Same question for the response schema.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The guidance around LROs described the 202 status code for gRPC, which is not relevant. Since this is already documented in the "Interface Definitions" section, removing the duplicate guidance seemed the best option - it will minimize future maintenance.
Helps address #224.
🍱 Types of changes
What types of changes does your code introduce to AEP? Put an
xin the boxesthat apply
📋 Your checklist for this pull request
Please review the AEP Style and Guidance for
contributing to this repository.
General
references AEPs
correctly.
(usually
prettier -w .)