-
Notifications
You must be signed in to change notification settings - Fork 489
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
AIP-4236 - API Versioning for Version-aware clients #1331
Changes from 3 commits
5008dba
160884c
ff7f9bc
17305e5
1910500
8251167
60a868c
48e63e4
9a8ebee
67e664a
f46c82e
3db4698
a228cba
2e79b89
15e5eae
e7c97cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,36 @@ | ||||||||||||
--- | ||||||||||||
id: 4236 | ||||||||||||
state: reviewing | ||||||||||||
created: 2024-03-25 | ||||||||||||
--- | ||||||||||||
|
||||||||||||
# Version-aware clients | ||||||||||||
|
||||||||||||
APIs may choose to annotate services with [google.api.api_version]. If | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think we still need the empty Also let's format the annotation name as code, might require updating the definition text as well. Applies here and throughout. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Don't use requirement words in non-requirement contexts |
||||||||||||
[google.api.api_version] is specified, version-aware clients **must** include | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to hyperlink every use of |
||||||||||||
the value of [google.api.api_version] in the request to the API. This allows | ||||||||||||
services to abide by the schema and behavior of the service at the time that | ||||||||||||
this API version was deployed. The format of the API version must be treated | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I'm guessing this is a requirement?
noahdietz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
as opaque by clients. Services may use a format with an apparent structure, | ||||||||||||
but clients must not rely on this to determine components within an API version, | ||||||||||||
or attempt to construct other valid API versions. | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Let's just tighten up the guidance, be a bit more explicit, and explain things more in |
||||||||||||
|
||||||||||||
### Expected Generator and Client Library Behavior | ||||||||||||
|
||||||||||||
If a service is annotated with [google.api.api_version], client library | ||||||||||||
generators **must** include the version in requests sent to the API via | ||||||||||||
the HTTP Header `X-Goog-Api-Version`. | ||||||||||||
noahdietz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
|
||||||||||||
For HTTP Requests, an HTTP Query Parameter `$apiVersion` can be used | ||||||||||||
noahdietz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
instead of the `X-Goog-Api-Version` HTTP header. | ||||||||||||
|
||||||||||||
Requests which contain the API version, must include either an HTTP query | ||||||||||||
noahdietz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
parameter `$apiVersion` or HTTP header `X-Goog-Api-Version`, but not both. | ||||||||||||
noahdietz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
|
||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To guide consistent behavior across languages when user attempts to set a user header of the same key, this could be to override an existing version that ships with client library, or attempting to add this header when server does not opt in for this feature.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer not to add that, personally. There's only so much you can do to protect users who are determined to do weird things. The chances of someone accidentally setting that header are very slim - and if someone really wants to do so, they can always send a request directly from an HTTP client. We definitely shouldn't provide any way of specifically overriding the header - but I don't think "code that allows headers to be specified" should be modified to try to prevent this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree on this. On the other hand, it also seems valuable to give user usable feedback if we don't intend have this overriden. To add a bit context in Java, we already have some logic trying to resolve conflict between internal header set by defaults and user headers. I am inclined to add logic to throw exception when user attempt to add a header with key "x-goog-api-version" (draft pr). Alternative to this is no change to this existing logic, which will throw exception only when internal header has this key (that's when service has a api version). I don't feel too strongly, but the first one gives a bit more consistent feedback. I realize this is only dealing with an edge case, but also feel that this case is language agnostic, so wanted to check if we want to deal with it consistently across languages. WDTY? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if Java wants to attempt to stop the user from doing it, that's fine - I don't think it should be in the AIP. |
||||||||||||
Generated documentation for a given service can include the value of | ||||||||||||
noahdietz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
[google.api.api_version], if it exists in the source protos. | ||||||||||||
|
||||||||||||
The format of the API version must be treated as opaque by clients. Generators | ||||||||||||
noahdietz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
and clients must not use the value of [google.api.api_version] to make decisions. | ||||||||||||
noahdietz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
|
||||||||||||
[google.api.api_version]: https://github.com/googleapis/googleapis/blob/master/google/api/client.proto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a bunch of comments re:guidance appearing in this preamble - perhaps that means the preamble needs to be cut down/generalized/removed/converted into guidance. We don't want to repeat ourselves, we don't want to spread guidance out into various places, and we want to be succinct. Adding a
Rationale
section go/aip/8#rationale is probably where you want to be explaining the "why"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing @noahdietz ! Please let me know if this latest version reads better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I does! A few more tweaks thanks Tony :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please could you take another look?