Adopt AEP 231 - Batch Get #177
Conversation
Co-authored-by: Richard Frankel <[email protected]>
|
@rofrankel Please re-review. |
Co-authored-by: Richard Frankel <[email protected]>
|
@rofrankel Thanks for the re-review. I addressed your comments and asked for a new review. |
|
I made the updates we discussed three weeks ago. Please re-review and let me know if there is any additional feedback. |
| - The URI path **should** represent the collection for the resource, matching | ||
| the collection used for simple CRUD operations. If the operation spans | ||
| parents, a dash (`-`) **may** be accepted as a wildcard. | ||
| - The HTTP URI **must** end with `:BatchGet`. |
There was a problem hiding this comment.
Why the casing change here? aep.dev/custom-methods still uses camelCase rather than PascalCase.
| - The request **must** include an array parameter which accepts the resource | ||
| paths specifying the resources to retrieve. The parameter **should** be named | ||
| `paths`. |
There was a problem hiding this comment.
| - The request **must** include an array parameter which accepts the resource | |
| paths specifying the resources to retrieve. The parameter **should** be named | |
| `paths`. | |
| - The request **must** include an array field which accepts the resource | |
| paths specifying the resources to retrieve. The field **should** be named | |
| `paths`. |
I think we use "field" reasonably consistently across the other AEPs. (Suggestion applies to below uses of "parameter" as well.)
| `paths`. | ||
| - If no resource paths are provided, the API **should** error with | ||
| `INVALID_ARGUMENT`. | ||
| - The parameter **should** be required. |
There was a problem hiding this comment.
Why not must? And is this redundant with the previous bullet point?
| - If no resource paths are provided, the API **should** error with | ||
| `INVALID_ARGUMENT`. | ||
| - The parameter **should** be required. | ||
| - The parameter **should** identify the [resource type](./paths) that it |
There was a problem hiding this comment.
What does "identify" mean in this context?
| - The parameter **should** be required. | ||
| - The parameter **should** identify the [resource type](./paths) that it | ||
| references. | ||
| - The parameter should define the pattern of the resource path values. |
There was a problem hiding this comment.
What does this mean, concretely?
| - The response schema **must** be `type: object` with a single array property | ||
| with each item containing one of the requested resources. |
There was a problem hiding this comment.
This seems substantially different from the proto guidance:
- OAS responses only have an array, where proto responses just have to include the repeated field.
- The OAS array just has the requested resource; the proto repeated field has either the resource or an error object.
These both seem like they should be the same across IDLs - no?
| "rating": 9.6 | ||
| }, | ||
| { | ||
| "code": "NotFound", |
There was a problem hiding this comment.
Should this be 404 rather than "NotFound"? Or if not, where are these codes defined? Is there a standard, or is it up to each BatchGet method to define its error codes?
| - The array of resources **must** be named `results` and contain resources with | ||
| no additional wrapping. | ||
| - There must not be a `nextPageToken` field as batch get operations are not | ||
| pageable. |
There was a problem hiding this comment.
This doesn't seem OAS-specific - can we factor it out of the tab?
|
|
||
| The batch get method may support a transactional form of operation where the | ||
| get either succeeds for all requested resources or fails. When supported, the | ||
| method should define a boolean parameter `asTransaction` that the user must |
There was a problem hiding this comment.
I think transactional might be more consistent with the general AEP style than asTransaction, since the AEPs try to avoid prepositions in field names in general. (But I don't remember if we already discussed this...)
| The batch get method may support a transactional form of operation where the | ||
| get either succeeds for all requested resources or fails. When supported, the | ||
| method should define a boolean parameter `asTransaction` that the user must | ||
| specify with the value `true` to request transactional operation. |
There was a problem hiding this comment.
| specify with the value `true` to request transactional operation. | |
| specify with the value `true` to request a transactional operation. |
Adopt AEP 231 - Batch Get
🍱 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 .)Additional checklist for a new AEP
guidelines are met.
Document structure
guidelines are met.
💝 Thank you!