Adopt AEP 231 - Batch Get #177

mkistler · 2024-04-22T14:26:45Z

Adopt AEP 231 - Batch Get

🍱 Types of changes

What types of changes does your code introduce to AEP? Put an x in the boxes
that apply

Enhancement
New proposal
Migrated from google.aip.dev
Chore / Quick Fix

📋 Your checklist for this pull request

Please review the AEP Style and Guidance for
contributing to this repository.

General

Basic Guidance is met.
Ensure that your PR
references AEPs
correctly.
My code has been formatted
(usually prettier -w .)

Additional checklist for a new AEP

A new AEP should be no more than two pages if printed out.
Ensure that the PR is editable by maintainers.
Ensure that File structure
guidelines are met.
Ensure that
Document structure
guidelines are met.

💝 Thank you!

rofrankel · 2024-04-26T19:52:40Z

aep/general/0231/aep.md.j2

+Some APIs need to allow users to get a specific set of resources at a
+consistent time point (e.g. using a read transaction). A batch get method


We removed this transactionality requirement at Roblox, because we have backends that don't support it, and also because sometimes it's useful to be able to fetch many resources in a single request even without a consistency guarantee.

WDYT?

If the transactional guarantee is dropped, how does "batchGet" differ from "List" with a filter that specifies the resource names/paths?

That's a very fair point. It's functionally equivalent (but may be able to be implemented much more efficiently, which is sometimes important - though I suppose one could write code that, at least under normal circumstances, detects when a filtered list request is equivalent to a batch get based on the filter provided).

We actually discussed this in general in the meeting today. @toumorokoshi proposed that maybe we factor out the guidance about transactionality, explain that it's optional for each batch method, and then describe a pattern for partial failures to be used for non-transactional batch operations.

If we merge this as-is and then remove this requirement, is that a breaking change?

Maybe it is if we add a requirement that transactional batch gets must document that they're transactional, because someone could go and implement one that doesn't document it?

Alternatively, maybe we could have different operation names for the non-transactional cases (e.g. MultiGetBooks vs. BatchGetBooks -- or the other way around, BatchGetBooks vs. TransactionalBatchGetBooks)?

If the transactional guarantee is dropped, how does "batchGet" differ from "List" with a filter that specifies the resource names/paths?

I find this very compelling. Given a not-necessarily-transactional List operation, BatchGet makes sense only with such a guarantee.

aep/general/0231/aep.md.j2

rofrankel · 2024-04-26T20:00:28Z

aep/general/0231/aep.md.j2

+- The response schema **must** be an array with each item containing one of the
+  requested resources.


Is this structurally different from the response you'd get with a protobuf API? It seems like it: {"books": [...]} vs. [...].

I know we probably can't get full JSON parity between OAS and transcoded gRPC, but nevertheless it seems unfortunate to diverge here unless there's a good reason to do so.

The bare array response comes straight from the Google AIP, so I assume there is a way to do this in protobuf.

But I think what you are getting at is that this differs from the standard List method, which returns an object with a top level array property.

I just checked JJ's book and it looks like his definition of batch get returns an object with a top-level array property:

interface BatchGetChatRoomsResponse { resources: ChatRooms[]; }

So changing to be consistent with the standard List method seems reasonable.

It doesn't look like that change was actually made...?

Also, it's not just List - the BatchGetBooksResponse defined above follows the same pattern.

rofrankel · 2024-04-26T20:01:32Z

aep/general/0231/aep.md.j2

+
+- **2024-04-22:** Adopt from Google AIP https://google.aip.dev/231 with the
+  following changes:
+  - Dropped the "nested requests" pattern.


Because the AIP says "This is generally discouraged unless your use case really requires it." This made it sound like it was added for some strange use case at Google.

Also, this is going to be messy in HTTP. Does this still use an HTTP get method? If so, I hope the array of requests is not sent in the body -- HTTP does not allow GET to have a request body. But if not then I suppose they are sent in the query string, and that is really ugly. OAS does allow parameters of type "deep object" but the behavior of these is ill-defined.

Co-authored-by: Richard Frankel <richard@frankel.tv>

mkistler · 2024-05-10T11:41:41Z

@rofrankel Please re-review.

aep/general/0231/aep.md.j2

rofrankel · 2024-05-10T20:51:22Z

aep/general/0231/aep.md.j2

+- The request and response schemas **must** match the method name, with `Request` and
+  `Response` suffixes.
+- 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
+  collection in the path of any resource being retrieved does not match, the
+  request **must** fail.
+  - This field **should** be required if only 1 parent per request is allowed.
+  - 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.


Can this all be factored out of the proto tab into general guidance?

It cannot, because the OAS guidance does not go in a comment field or even in the description, but in a maxItems schema attribute.

Can we perhaps find a generic way to describe that? (I think we probably have an equivalent protovalidate annotation for protobuf.)

aep/general/0231/aep.md.j2

rofrankel · 2024-05-10T20:54:15Z

aep/general/0231/aep.md.j2

+- The response schema **must** be an array with each item containing one of the
+  requested resources.


It doesn't look like that change was actually made...?

Also, it's not just List - the BatchGetBooksResponse defined above follows the same pattern.

Co-authored-by: Richard Frankel <richard@frankel.tv>

mkistler · 2024-05-17T14:43:22Z

@rofrankel Thanks for the re-review. I addressed your comments and asked for a new review.

mkistler added 3 commits April 12, 2024 12:15

Copy Google AIP 231

d22265b

Mechanical AIP to AEP updates

d391a22

Add OAS-specific guidance

15ac5e9

mkistler requested a review from a team as a code owner April 22, 2024 14:26

mkistler requested review from rofrankel and toumorokoshi April 22, 2024 14:27

mkistler linked an issue Apr 26, 2024 that may be closed by this pull request

Adopt AIP-0231 Batch methods: Get #42

Open

rofrankel requested changes Apr 26, 2024

View reviewed changes

mkistler and others added 3 commits April 27, 2024 08:25

Apply suggestions from PR review

33dbbab

Co-authored-by: Richard Frankel <richard@frankel.tv>

Change name to path

8a0b163

Updates to address PR comments

b88e066

mkistler requested a review from rofrankel April 27, 2024 14:33

rofrankel requested changes May 10, 2024

View reviewed changes

mkistler and others added 2 commits May 17, 2024 08:28

Apply suggestions from PR review

9922cee

Co-authored-by: Richard Frankel <richard@frankel.tv>

Change response schema to be consistent with List method

cc4f44d

mkistler force-pushed the mdk/aep-231 branch from bbfb63d to cc4f44d Compare May 17, 2024 14:42

mkistler requested a review from rofrankel May 17, 2024 14:42

Changed shoulds to musts as discussed in weekly meeting

4d4be17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt AEP 231 - Batch Get #177

Adopt AEP 231 - Batch Get #177

mkistler commented Apr 22, 2024

rofrankel Apr 26, 2024

mkistler Apr 27, 2024

rofrankel May 10, 2024

rofrankel May 10, 2024 •

edited

gibson042 May 10, 2024

rofrankel Apr 26, 2024

mkistler Apr 27, 2024

rofrankel May 10, 2024

rofrankel Apr 26, 2024

mkistler Apr 27, 2024

mkistler Apr 27, 2024

mkistler commented May 10, 2024

rofrankel May 10, 2024

mkistler May 17, 2024

rofrankel May 24, 2024

rofrankel May 10, 2024

mkistler commented May 17, 2024

		Some APIs need to allow users to get a specific set of resources at a
		consistent time point (e.g. using a read transaction). A batch get method

		- The response schema must be an array with each item containing one of the
		requested resources.

Adopt AEP 231 - Batch Get #177

Are you sure you want to change the base?

Adopt AEP 231 - Batch Get #177

Conversation

mkistler commented Apr 22, 2024

🍱 Types of changes

📋 Your checklist for this pull request

General

Additional checklist for a new AEP

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofrankel May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkistler commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkistler commented May 17, 2024

rofrankel May 10, 2024 •

edited