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.

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

Adopt AEP 231 - Batch Get #177

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
123 changes: 77 additions & 46 deletions aep/general/0231/aep.md.j2
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,40 @@ provides this functionality.
APIs **may** support batch get to retrieve a consistent set of resources.

- The method's name **must** begin with `BatchGet`. The remainder of the method
name **should** be the plural form of the resource being retrieved.
name **must** be the plural form of the resource being retrieved.
- The HTTP verb **must** be `GET`.
- The HTTP URI **must** end with `:batchGet`.
- 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`.
mkistler marked this conversation as resolved.
Show resolved Hide resolved
- The URI path **must** represent the collection for the resource, matching the
collection used for simple CRUD operations. If the operation spans parents, a
[wilcard](./reading-across-collections) **may** be accepted.
- There **must not** be a request body.
- If a GET request contains a body, the body **must** be ignored, and **must not** cause an error.
- The operation **must** be atomic: it **must** fail for all resources or
succeed for all resources (no partial success). For situations requiring
partial failures, `List` (AEP-132) methods **should** be used.
- If the operation covers multiple locations and at least one location is
down, the operation **must** fail.
- If a GET request contains a body, the body **must** be ignored, and **must
not** cause an error.

### Request

The request for a batch get method **should** be specified with the following
pattern:

- The request **must** include an array parameter which accepts the resource
paths specifying the resources to retrieve. The parameter **should** be named
`paths`.
mkistler marked this conversation as resolved.
Show resolved Hide resolved
- If no resource paths are provided, the API **should** error with
`INVALID_ARGUMENT`.
- The parameter **should** be required.
mkistler marked this conversation as resolved.
Show resolved Hide resolved
- The parameter **should** identify the [resource type](./paths) that it
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does "identify" mean in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This language is exactly the same as the AIP.

Copy link
Member

Choose a reason for hiding this comment

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

It is a little vague as to what this term means - I think it means "document" here.

Suggested change
- The parameter **should** identify the [resource type](./paths) that it
- The documentation for the parameter **should** include the [resource type](./paths) that it

But either way we can follow-up on this, I don't think it's a blocker.

references.
- The parameter should define the pattern of the resource path values.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this mean, concretely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes from the statement in the original AIP:

The comment for the field should document the resource pattern.

So this could mean in the description for the parameter or it could be more formal like a "pattern" keyword in the schema of the parameter.

Copy link
Member

Choose a reason for hiding this comment

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

I think there's two options here:

  1. leave it as is, since it's a generic description of the operation.
  2. remove this guidance and move it into protocol-specific descriptions, so we can tie it with precise recommendations.

I'm comfortable with either.

- The parameter should define the maximum number of paths allowed.
Comment on lines +32 to +35
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these three bullet points all about documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some ways all of this is about documentation, right? But in OpenAPI / JSON Schema many of these things can be described formally, such as with a maxItems keyword in an array schema.

Copy link
Member

Choose a reason for hiding this comment

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

could we just update it to specify documentation? we could even qualify it in the way you described - if the protocol / description supports a way to describe it, use that. Otherwise use documentation.

- Other parameters besides `paths` **may** be "hoisted" from the [standard Get
request][get-request].
Comment on lines +36 to +37
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Other parameters besides `paths` **may** be "hoisted" from the [standard Get
request][get-request].

I don't think this is explained very well - and I think removing unclear guidance is better than including it? we can re-add with a clearer description.

I don't see any documented additional parameters in the Get, so not sure if the guidance is even relevant: https://aep.dev/131/

- Batch get **should not** support pagination because transactionality across
mkistler marked this conversation as resolved.
Show resolved Hide resolved
API calls would be extremely difficult to implement or enforce, and the
request defines the exact scope of the response anyway.
- The request **must not** contain any other required parameters, and **should
not** contain other optional parameters except those described in this or
another AEP.

{% tab proto -%}

```proto
Expand Down Expand Up @@ -58,8 +73,8 @@ message BatchGetBooksRequest {
}
```

- The request and response schemas **must** match the method name, with `Request` and
`Response` suffixes.
- The request and response schemas **must** match the method name, with
`Request` and `Response` suffixes.
mkistler marked this conversation as resolved.
Show resolved Hide resolved
- A `parent` field **should** be included, unless the resource being retrieved
is a top-level resource, to facilitate inclusion in the URI as well to permit
a single permissions check. If a caller sets this field, and the parent
Expand All @@ -69,49 +84,23 @@ message BatchGetBooksRequest {
- The field **should** identify the [resource type][aep-122-parent] that it
references.
- The comment for the field **should** document the resource pattern.
- The request **must** include a repeated field which accepts the resource
paths specifying the resources to retrieve. The field **should** be named
`paths`.
- If no resource paths are provided, the API **should** error with
`INVALID_ARGUMENT`.
- The field **should** be required.
- The field **should** identify the [resource type][aep-122-paths] that it
references.
- The comment for the field **should** document the resource pattern.
- The comment for the field **should** document the maximum number of
paths allowed.
- There **must not** be a body key in the `google.api.http` annotation.

{% tab oas %}
mkistler marked this conversation as resolved.
Show resolved Hide resolved

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

- The request **must** include a query parameter which accepts an array of
- The `paths` parameter **must** be a query parameter which accepts an array of
resource paths specifying the resources to retrieve. The parameter **should**
be named `paths`.
- If no resource paths are provided, the API **should** error with status
code `400 Bad Request`.
- If the collection in the path of any resource does not match the collection
of the request URL, the request **must** fail.
- The parameter **should** be required.
- The parameter description **should** identify the [resource type][aep-122-paths]
that it references.
- The parameter `description` **should** document the pattern for path values.
- The parameter `schema` **should** include a `maxItems` attribute to specify
the maximum number of paths allowed.
- The method definition **must not** have a `requestBody` defined.

{% endtabs %}

- Other parameters besides `paths` **may** be "hoisted" from the [standard Get
request][get-request].
- Batch get **should not** support pagination because transactionality across
API calls would be extremely difficult to implement or enforce, and the
request defines the exact scope of the response anyway.
- The request **must not** contain any other required parameters, and **should
not** contain other optional parameters except those described in this or
another AEP.

### Response

The response for a batch get method **should** be specified with the following
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The response for a batch get method **should** be specified with the following
The response for a batch get method **must** be specified with the following

Feels weird to have a should that includes multiple musts. Why not enforce the pattern?

Expand All @@ -126,24 +115,66 @@ message BatchGetBooksResponse {
}
```

- The response message **must** include one repeated field corresponding to the
resources being retrieved.
- The response message **must** include one repeated field where its items may
either be a retrieved resource or an error structure describing an error that
occurred attempting to retrieve the resource. The error alternative would
only be present for non-transactional batch gets.

{% tab oas %}

- The response schema **must** be an array with each item containing one of the
requested resources.
- The response schema **must** be `type: object` with a single array property
with each item containing one of the requested resources.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems substantially different from the proto guidance:

  1. OAS responses only have an array, where proto responses just have to include the repeated field.
  2. 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should be the same. I think the OAS description is what we want. I'll gladly fix the proto description if someone can tell me what it should be.


```json
{
"results": [
{
"name": "publishers/lacroix/books/les-mis",
"isbn": "978-037-540317-0",
"title": "Les Misérables",
"authors": ["Victor Hugo"],
"rating": 9.6
},
{
"code": "NotFound",
mkistler marked this conversation as resolved.
Show resolved Hide resolved
"message": "The resource is not available"
}
{
"name": "publishers/lacroix/books/hunchback-of-notre-dame",
"isbn": "978-140-274575-1",
"title": "The Hunchback of Notre Dame",
"authors": ["Victor Hugo"],
"rating": 9.3
}
]
}
```

- 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.
mkistler marked this conversation as resolved.
Show resolved Hide resolved

{% endtabs %}

- The order of resources in the response **must** be the same as the paths in
the request.
- The order of resources/error objects in the response **must** be the same as
the paths in the request.

### Support for transactional get

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
mkistler marked this conversation as resolved.
Show resolved Hide resolved
specify with the value `true` to request transactional operation.
mkistler marked this conversation as resolved.
Show resolved Hide resolved

## Changelog

- **2024-04-22:** Adopt from Google AIP https://google.aip.dev/231 with the
following changes:
- Dropped the "nested requests" pattern.
mkistler marked this conversation as resolved.
Show resolved Hide resolved
- Changed the response schema to an object with `results` array property.
- Made transactional operation optional and controlled by asTransaction
parameter.

<!-- Links -->

Expand Down
14 changes: 11 additions & 3 deletions aep/general/0231/batchget.oas.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,19 @@ paths:
content:
application/json:
schema:
type: array
items:
$ref: '#/components/schemas/Book'
type: object
properties:
results:
type: array
items:
oneOf:
- $ref: '#/components/schemas/Book'
- $ref: '#/components/schemas/Error'
components:
schemas:
Book:
description: A representation of a single book.
type: object
properties:
name:
type: string
Expand All @@ -61,3 +67,5 @@ components:
type: number
format: float
description: The rating assigned to the book.
Error:
type: object
Loading