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!

aep/general/0231/aep.md.j2

Co-authored-by: Richard Frankel <[email protected]>

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

@rofrankel Please re-review.

aep/general/0231/aep.md.j2

Co-authored-by: Richard Frankel <[email protected]>

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

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

mkistler · 2024-09-13T18:21:47Z

I made the updates we discussed three weeks ago. Please re-review and let me know if there is any additional feedback.

aep/general/0231/aep.md.j2

rofrankel · 2024-09-20T19:12:36Z

aep/general/0231/aep.md.j2

+  - 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


What does "identify" mean in this context?

This language is exactly the same as the AIP.

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.

rofrankel · 2024-09-20T19:12:55Z

aep/general/0231/aep.md.j2

+  - 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.


What does this mean, concretely?

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.

I think there's two options here:

leave it as is, since it's a generic description of the operation.

remove this guidance and move it into protocol-specific descriptions, so we can tie it with precise recommendations.

I'm comfortable with either.

rofrankel · 2024-09-20T19:18:10Z

aep/general/0231/aep.md.j2

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


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?

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.

aep/general/0231/aep.md.j2

mkistler · 2024-09-27T19:31:43Z

@rofrankel Thanks for the comments. I've addressed or answered all that I could. I will need help on the proto definition of the response.

I will tag you for re-review.

toumorokoshi

Thank you for all the work! The page doesn't seem to render for me today, so I think that change should be made at least.

The rest are pretty minor, and I'll watch this PR closely on replies so we can resolve them quickly. Thank you!

toumorokoshi · 2024-10-18T17:45:18Z

aep/general/0231/aep.md.j2

+
+{% tab oas %}
+
+{% sample 'batchget.oas.yaml', 'paths./publishers/{publisherId}/books:BatchGet.get.responses.200.content.application/json' %}


Suggested change

{% sample 'batchget.oas.yaml', 'paths./publishers/{publisherId}/books:BatchGet.get.responses.200.content.application/json' %}

{% sample 'batchget.oas.yaml', 'paths./publishers/{publisherId}/books:BatchGet' %}

This is broken for me - I get a template syntax error. The site-generator matching is pretty rudimentary, I think it only does a substring match of some kind. Something to look into CC @rambleraptor, but the only blocker is the breaking of rendering.

toumorokoshi · 2024-10-18T17:47:03Z

aep/general/0231/aep.md.j2

+pattern:
+
+- The request **must** include an array field which accepts the resource paths
+  specifying the resources to retrieve. The field **should** be named `paths`.


Suggested change

specifying the resources to retrieve. The field **should** be named `paths`.

specifying the resources to retrieve. The field **must** be named `paths`.

Is there a good reason to not require it be named paths?

toumorokoshi · 2024-10-18T17:49:35Z

aep/general/0231/aep.md.j2

+  - 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


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.

toumorokoshi · 2024-10-18T17:51:38Z

aep/general/0231/aep.md.j2

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


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/

toumorokoshi · 2024-10-18T17:53:15Z

aep/general/0231/aep.md.j2

+- 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`.


Suggested change

- 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`.

- The `paths` parameter **must** be a query parameter which accepts an array of

resource paths specifying the resources to retrieve.

The guidance around the naming of the field already exists above - so remove the redundant one for conciseness?

toumorokoshi · 2024-10-18T17:54:06Z

aep/general/0231/aep.md.j2

+
+### Response
+
+The response for a batch get method **should** be specified with the following


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?

toumorokoshi · 2024-10-18T17:54:29Z

aep/general/0231/aep.md.j2

+  the paths in the request.
+- 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


Suggested change

- There must not be a `nextPageToken` field as batch get operations are not

- There **must not** be a `nextPageToken` field as batch get operations are not

toumorokoshi · 2024-10-18T17:55:53Z

aep/general/0231/aep.md.j2

+
+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 `transactional` that the user must


Suggested change

method should define a boolean parameter `transactional` that the user must

method must define a boolean parameter `transactional` that the user must

If we don't require that - it might be difficult for clients to understand the purpose of the value and use it.

Or we could require an annotation to specify this is the "transactional" parameter?

toumorokoshi · 2024-10-18T17:56:57Z

aep/general/0231/aep.md.j2

+  - The parameter **should** identify the [resource type](./paths) that it
+    references.
+  - The parameter should define the pattern of the resource path values.
+  - The parameter should define the maximum number of paths allowed.


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.

toumorokoshi · 2024-10-18T17:58:22Z

aep/general/0231/aep.md.j2

+  - 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.


I think there's two options here:

leave it as is, since it's a generic description of the operation.

remove this guidance and move it into protocol-specific descriptions, so we can tie it with precise recommendations.

I'm comfortable with either.

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 <[email protected]>

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 <[email protected]>

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

mkistler added 2 commits June 7, 2024 11:21

Changed shoulds to musts as discussed in weekly meeting

4d4be17

Resolve outstanding issues for Batch Get

f1c1bba

rofrankel reviewed Sep 20, 2024

View reviewed changes

Address PR review comments

92af486

mkistler requested a review from rofrankel September 27, 2024 19:31

toumorokoshi requested changes Oct 18, 2024

View reviewed changes

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

mkistler commented May 10, 2024

mkistler commented May 17, 2024

mkistler commented Sep 13, 2024

rofrankel Sep 20, 2024

mkistler Sep 27, 2024

toumorokoshi Oct 18, 2024

rofrankel Sep 20, 2024

mkistler Sep 27, 2024

toumorokoshi Oct 18, 2024

rofrankel Sep 20, 2024

mkistler Sep 27, 2024

mkistler commented Sep 27, 2024

toumorokoshi left a comment

toumorokoshi Oct 18, 2024

toumorokoshi Oct 18, 2024

toumorokoshi Oct 18, 2024

toumorokoshi Oct 18, 2024

toumorokoshi Oct 18, 2024

toumorokoshi Oct 18, 2024

toumorokoshi Oct 18, 2024

toumorokoshi Oct 18, 2024

toumorokoshi Oct 18, 2024

toumorokoshi Oct 18, 2024

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

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


		{% tab oas %}

		{% sample 'batchget.oas.yaml', 'paths./publishers/{publisherId}/books:BatchGet.get.responses.200.content.application/json' %}

	specifying the resources to retrieve. The field should be named `paths`.
	specifying the resources to retrieve. The field must be named `paths`.

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


		### Response

		The response for a batch get method should be specified with the following

	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

	- There must not be a `nextPageToken` field as batch get operations are not
	- There must not be a `nextPageToken` field as batch get operations are not

	method should define a boolean parameter `transactional` that the user must
	method must define a boolean parameter `transactional` that the user must

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

mkistler commented May 10, 2024

mkistler commented May 17, 2024

mkistler commented Sep 13, 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

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 Sep 27, 2024

toumorokoshi left a 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

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