-
Notifications
You must be signed in to change notification settings - Fork 526
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
Conversation
|
||
Requests which contain the API version, must include either an HTTP query | ||
parameter `$apiVersion` or HTTP header `X-Goog-Api-Version`, but not both. | ||
|
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.
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.
Let's state something along the lines of:
Generated client library should not let users override the `X-Goog-Api-Version` HTTP header. This should be regardless if the service is annotated with [google.api.api_version]. |
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'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 comment
The reason will be displayed to describe this comment to others. Learn more.
There's only so much you can do to protect users who are determined to do weird things.
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 comment
The 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.
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'm generally happy with this as a first pass. I suspect I may have some suggested changes over time, but that's a different matter :)
|
||
Requests which contain the API version, must include either an HTTP query | ||
parameter `$apiVersion` or HTTP header `X-Goog-Api-Version`, but not both. | ||
|
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'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.
@noahdietz PTAL |
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 review all comments before responding to an individual one. Some are on the same line and impact each other.
aip/client-libraries/4236.md
Outdated
|
||
# 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 comment
The reason will be displayed to describe this comment to others. Learn more.
APIs may choose to annotate services with [google.api.api_version]. If | |
APIs may choose to annotate services with [`google.api.api_version`][]. If |
I think we still need the empty []
for an implicit link.
Also let's format the annotation name as code, might require updating the definition text as well.
Applies here and throughout.
aip/client-libraries/4236.md
Outdated
|
||
# 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 comment
The reason will be displayed to describe this comment to others. Learn more.
APIs may choose to annotate services with [google.api.api_version]. If | |
APIs can annotate services with [google.api.api_version]. If |
Don't use requirement words in non-requirement contexts
aip/client-libraries/4236.md
Outdated
# Version-aware clients | ||
|
||
APIs may choose to annotate services with [google.api.api_version]. If | ||
[google.api.api_version] is specified, version-aware clients **must** include |
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.
Do we need to hyperlink every use of google.api.api_version
?
aip/client-libraries/4236.md
Outdated
[google.api.api_version] is specified, version-aware clients **must** include | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
this API version was deployed. The format of the API version must be treated | |
this API version was deployed. The format of the API version **must** be treated |
I'm guessing this is a requirement?
created: 2024-05-16 | ||
--- | ||
|
||
# Version-aware clients |
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.
Just one more comment - I liked that opaque value requirement in the guidance, it is important stuff
aip/client-libraries/4236.md
Outdated
Clients must treat the value of `google.api.api_version` as opaque to ensure robust | ||
compatibility. This means that the specific format or structure of the version string | ||
should not be parsed or interpreted for any purpose beyond identifying the intended API | ||
version. By relying on this opaque identifier, clients avoid making assumptions about the |
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.
So these first two sentences would be really good in the guidance section e.g.
Clients **must** treat the value of `gogle.api.api_version as opaque. Specifically, the value **must not**
be interpreted or parsed in any way.
Then this subsection could start:
Treating the `google.api.api_version` value as opaque is important to ensuring robust
compatibility guarantees. By relying on....
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.
Fixed in a228cba
Co-authored-by: Noah Dietz <[email protected]>
Co-authored-by: Noah Dietz <[email protected]>
The following is an AIP for the API Versioning feature in client library generators.
Googlers see b/330951736