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)

Most upvoted comments

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?

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:

  • ALL clients should replace the API version in the URL in the query parameter if they are bound to a specific API version.

Open Issue to be investigated further (in off-line thread):

  • Services SHOULD NOT return a query parameter in the Operation-Location
  • The status monitor structure is NOT described in the Open API document.
  • We need to determine guidance on services that put the version in the path, e.g. cognitive.

Recording Link Here

HTTP API consumers are going to assume that URLs that come back in a response are opaque.

Imagine that a V1 client initiates the LRO and then a V2 client wants to poll for the completion - this should work.

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 add api-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

The resource validates the request and initiates the operation processing. It sends a response to the client with a 202-Accepted HTTP status code. Included in this response is an Operation-location response header with the absolute URL of status monitor for this specific operation.

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 the Operation-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).