azure-sdk-for-net: LLC does not add api-version to LRO URI in `UpdateStatus()`
While working on the LLC for Question Answering authoring, @AhmedLeithy found that the api-version
query parameter was not being added to the URI returned by the Operation-Location
header. This was causing a 404 but, even if the service handled that, assuming the latest api-version could result in unexpected response models.
Talking with Jeff and Johan, an api-version
in a Operation-Location is not required for LROs, so the client should add one if not already set (some services do pass the api-version
back e.g., Key Vault).
On a related note, the nextLink
model property for paged results should include an api-version
, but it might be good to implement the same behavior described in this bug just to make sure we are consistent.
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Comments: 18 (18 by maintainers)
The API version is set in the ClientOption when creating a Client, and all requests sent by the client would use that version except the LRO cases we discuss here. If I understand correctly, we need to either add the API version or replace it for the link returned from service for LRO. That would be a simple change from https://github.com/Azure/autorest.csharp/pull/1749.
@JeffreyRichter I do not see the logic here. This is completely contrary to
Operation-Location
being an “opaque absolute URL”. We are now asking users to crack apart the URL and add a query parameter before issuing the GET. At the very least this breaks with our previous guidance.Can we please review this in the API Stewardship leadership team before we advise teams to go off and implement this guidance?
It was decided in https://github.com/Azure/azure-sdk/issues/3817 that the client should always set the
api-version
query parameter (not path parameters, though) if one was passed originally, which should always be the case for data plane clients.Cross-linking Azure/azure-sdk#3809 since we may want to pull this back in depending on how that discussion turns out.
The guidance should be the same: replace the version path segment with the version the client wants to use. How hard is this too implement?
Notes from API Stewardship Board Discussion on 4-Jan-22
We are aligned on: For NEW services that use the API Version in the query parameter:
Open Issue to be investigated further (in off-line thread):
Operation-Location
Recording Link Here
HTTP API consumers are going to assume that URLs that come back in a response are opaque.
If it is expected that clients can share URLs across different SDK versions, then SDKs should have the ability to remove any existing api-version and apply their own api-version. This would also allow for SDKs to detect incompatible versions.
@heaths my personal take on what @JeffreyRichter and @johanste might have meant for “api-version in a Operation-Location is not required for LROs”, is that most of the
Operation-Location
will point to an guid or some form of unique identifier, that makes the call explicit and then do not require extra metadata to be understood correctly (like api-version). It doens’t interpret to me, as the SDK should addapi-version
if it was not there.Operation-Location
should point to an absolute URL at all time, and SDK should not change it.See: https://github.com/microsoft/api-guidelines/blob/50b14d32a1a5192213272c10371d1d82de02b296/azure/ConsiderationsForServiceDesign.md#long-running-operations-with-status-monitor
Running autorest on the PR would produce the client, but running live tests against QA currently isn’t trivial. Instead, I wrote up a simple repro: https://github.com/heaths/Issue25411. It uses a swagger file that mimics the behavior and does not return an
api-version
for theOperation-Location
header.You can simply build and run the “Issue25411” project (the service), wait till it starts, then run the “Jobs” project - a console + client library sample. If the Issue25411 isn’t bound to
localhost:7041
be sure to change the launch settings for the “Jobs” project. The call to wait on the operation will fail with an HTTP 400, which I’m effectively simulating what the Question Answering service currently does (though, it returns a 404, but @johanste said this should at least be a 400 - either way, it errs).