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

Change Request: Clients should add/update API version for paging, LROs #3817

Closed
heaths opened this issue Jan 20, 2022 · 4 comments
Closed

Change Request: Clients should add/update API version for paging, LROs #3817

heaths opened this issue Jan 20, 2022 · 4 comments
Assignees
Labels
architecture board-review Request for an Architectural Board Review

Comments

@heaths
Copy link
Member

heaths commented Jan 20, 2022

The Basics

  • Which design guideline is affected?

We're proposing a change to the Service API Versions general guidelines, as available already in PR #3809.

  • Which target languages are affected?

All languages.

  • Describe the change:

Clients should add or update the api-version string on any LRO or paging request with the API version passed (or defaulted) to the client upon instantiation. It's also been suggested that any API version in the URI path should be updated, though more hints to code generation may be required.

Reasoning

We recently discovered that some services weren't sending back the api-version in Operation-Location headers, which then caused a service error since we weren't sending it back. After a discussion over email with @mikekistler, @JeffreyRichter, and @markweitzel, it was proposed that there's good reason for a service not to back the api-version but instead have the client library always add/update it - at least for the api-version query string parameter if not in the URI path for those services doing that e.g., api/v2/resource/:action.

The reason given was that one version of an SDK may start an LRO (especially a VLRO), but another version using another api-version may want to complete it, i.e. that the status update and final resource fetch for an LRO may be divorced from the original request to start the LRO. The same argument didn't make as much sense for paging, but it was agreed that we should be consistent.

This was blocking Cognitive Services - Question Answering, but the service team decided to send the api-version back in the Operation-Location header for the LRO. But we would like to discuss merging #3809 and making changes to code generation (and helper APIs, where needed) such that all clients do this for LROs and paging. This way we have a consistent pattern across Azure services and client libraries.

@heaths heaths added architecture board-review Request for an Architectural Board Review labels Jan 20, 2022
@mikekistler
Copy link
Member

Since I was @-mentioned I want to make a slight correction to the record. What was agreed was that the Azure SDKs should replace or add the api-version – because our SDKs provide externals for serializing and handing off the Operation-Location URL (as a continuation token?) to another process which might be using a different api-version.

There was not agreement on whether api-version should our should not be included in the Operation-Location or nextLink URLs returned from the service. That was discussed at length in a recent API Stewardship meeting and there were strong opinions on both sides so we simply agreed to defer it.

@lilyjma
Copy link
Contributor

lilyjma commented Feb 2, 2022

scheduled for 2/7 3-4pm pst

@tg-msft
Copy link
Member

tg-msft commented Feb 9, 2022

Recording[MS INTERNAL ONLY]

We settled on:

  • Clients should add or set the api-version query parameter on next links and operation locations (to the API version the client was configured to use)
  • Clients should not change versions specified in the path (we'll need a separate meeting to discuss that)

@heaths
Copy link
Member Author

heaths commented Feb 9, 2022

  • Clients should not change versions specified in the path (we'll need a separate meeting to discuss that)

@tg-msft would you or the other architects agree that this point isn't a "should not" / "must not" requirement, but perhaps just leave it out of PR #3809 as I've suggested (edited today)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture board-review Request for an Architectural Board Review
Projects
None yet
Development

No branches or pull requests

6 participants