per operation Response types for generated clients

[ctaggart]

This implements the first part of #1053 design.

RequestBuilder
fn send(self) -> Response

Response(azure_core::Response)
fn into_body(self) -> ParsedBody
fn into_raw_reponse(self) -> azure_core::Response
fn as_raw_reponse(&self) -> &azure_core::Response

[bmc-msft]

Can you discuss why this change is needed? It would be helpful to understand the need, as this is increasing the amount of boiler plate developers will need to write. Consider the existing Application Insights example:

let response = client.query_client().execute(app_id, body).into_future().await?;

With this change, the example looks like this:

let query_results = client.query_client().execute(app_id, body).into_future().await?.into_body().await?;

[cataggar]

Thanks for the feedback @bmc-msft. I added a .into_body() helper to the RequestBuilder that does the same thing as into_future() was doing before which is to send the request and get back the response body. It is back to the same amount of boilerplate.

let response = client.query_client().execute(app_id, body).into_body().await?;

The send() function now returns a response. The response with allow access to the typed headers (next PR) and the raw response if needed. This is the same design as the .NET rest clients #1039.

[yoshuawuyts]

This PR contains a million-line diff, which is incredibly hard to review. Can you walk us through the API changes you're proposing here? I'd like to be able to understand the proposed API design and the motivation for it.

As we discussed in the meeting on Tuesday, there are several ways which would make it easier to talk about API design changes:

• Instead of generating the changes for all files, generate a diff for a single file, or possibly even a single SDK. This allows us to zoom in on a single set of changes and discuss them in detail.
• Alternatively: instead of proposing the API changes for the auto-generated SDKs, file a PR to make the changes to the manually authored SDKs first. It makes sense to not go down this path if you're not comfortable with the manually authored SDKs.
• Author a document or PR description outlining the changes you're making with clear descriptions of the problems and proposed solutions. I would say this is a requirement for API changes, as the auto-generated and manually-authored crates should maintain similar user-facing APIs.

As @rylev mentioned on Tuesday: it's fine for the manually-authored and automatically generated SDKs to deviate in their APIs. But this should be the result of a conscious decision making process, and not because we simply didn't communicate about the design.

Thanks!

[cataggar]

Instead of generating the changes for all files, generate a diff for a single file, or possibly even a single SDK. This allows us to zoom in on a single set of changes and discuss them in detail.

@yoshuawuyts, I opened up #1043 to make is easier to zoom in on a single set of changes for the strorage Service_GetAccountInfo operation.

I would say this is a requirement for API changes, as the auto-generated and manually-authored crates should maintain similar user-facing APIs.

The main difference is the SDKs combine data from the response headers and the response bodies for their responses. The generated rest clients allow type-safe access to individual response headers and response body. In #1039, the ToAccountInfo is the manually written conversion function.

        internal static AccountInfo ToAccountInfo(this ResponseWithHeaders<ServiceGetAccountInfoHeaders> response)
        {
            if (response == null)
            {
                return null;
            }
            return new AccountInfo
            {
                SkuName = response.Headers.SkuName.GetValueOrDefault(),
                AccountKind = response.Headers.AccountKind.GetValueOrDefault(),
                IsHierarchicalNamespaceEnabled = response.Headers.IsHierarchicalNamespaceEnabled.GetValueOrDefault()
            };
        }

[cataggar]

The convenience function that sends the request and returns the body is now into_body.

[rylev]

Why is into_body necessary (into_future is temporary until std::future::IntoFuture arrives on stable - it will soon go away). Why would there be a send and an into_body? Why not always just return the Response type?

[cataggar]

Operations like Service_GetAccountInfo do not have response bodies. All of the retuned data is in the response headers, which are not currently accessible. The plan is make them available, probably via Response::headers(&self) which follows the AutoRest C# design. I'm not sure what into_future should map to and would prefer to add it back once we figure that out.

[ctaggart]

When cadl replaces openapi for the specifications, it will be possible to model the responses the way we with them to be in the SDKs. I created an example for operation in #1043 (comment) .

[cataggar]

Another example.

[cataggar]

@rylev, I provided an overview and switched back to into_future as we discussed today. Please review/approve.

Overview provided.

[bmc-msft]

This requires the user know where the response data comes from, either from the header or body, which seems sub-optimal. The data representation from the HTTP response should be inconsequential to the API consumer.

If we consider some of the other SDKs, this is handled in a handful of different ways.

1. Golang: appears to provide the parsed response
   https://github.com/Azure-Samples/azure-sdk-for-go-samples/blob/d9f41170eaf6958209047f42c8ae4d0536577422/services/keyvault/examples/go-keyvault-msi-example.go#L82-L89
2. Python: if you provide the argument raw, returns the response with the expectation that the user deserializes it, otherwise the consumer gets a fully contextualized response.
3. Javascript: Users register a callback where you are provided the deserialized response or the base response stream.

It appears the primary concern would be addressed if we provided either the contextualized/parsed response or a raw version. Can we do something akin to std::str::parse, and rely on type inference (or turbofishing) to know which response the user wants?

[cataggar]

This requires the user know where the response data comes from, either from the header or body, which seems sub-optimal. The data representation from the HTTP response should be inconsequential to the API consumer.

That is true for SDKs, but not the generated clients. AutoRest clients make this distinction. See ResponseWithHeaders in the #1039 example. The generated clients represent the openapi in Rust code. Some .NET APIs call it response value instead of response body.

Golang: appears to provide the parsed response

The generated GetSecret is:

// GetSecret - The GET operation is applicable to any secret stored in Azure Key Vault. This operation requires the secrets/get
// permission.
// If the operation fails it returns an *azcore.ResponseError type.
// Generated from API version 7.3
// name - The name of the secret.
// version - The version of the secret. This URI fragment is optional. If not specified, the latest version of the secret
// is returned.
// options - GetSecretOptions contains the optional parameters for the Client.GetSecret method.
func (client *Client) GetSecret(ctx context.Context, name string, version string, options *GetSecretOptions) (GetSecretResponse, error) {
	req, err := client.getSecretCreateRequest(ctx, name, version, options)
	if err != nil {
		return GetSecretResponse{}, err
	}
	resp, err := client.pl.Do(req)
	if err != nil {
		return GetSecretResponse{}, err
	}
	if !runtime.HasStatusCode(resp, http.StatusOK) {
		return GetSecretResponse{}, runtime.NewResponseError(resp)
	}
	return client.getSecretHandleResponse(resp)
}

It returns a response type named GetSecretResponse:

// GetSecretResponse contains the response from method Client.GetSecret.
type GetSecretResponse struct {
	SecretBundle
}

The SecretBundle is the response body. The spec for the operation does not return any headers. To skip the deserialization, you would need to call getSecretCreateRequest yourself.

For these Rust operation Response types. The intension it to provide into_raw_response and as_raw_response with Into<azure_core::Response> and AsRef<azure_core::Response> as well. I can update the pull request.

[cataggar]

Looking at generated JavaScript clients, it looks like primary way to respond is with a custom response that contains both the parsed body and parsed headers. Using getStatistics as an example:

function getStatistics(options?: ServiceGetStatisticsOptions): Promise<ServiceGetStatisticsResponse>

type ServiceGetStatisticsResponse = ServiceGetStatisticsHeaders &
  BlobServiceStatistics & {
    _response: HttpResponse & {
      bodyAsText: string,
      parsedBody: BlobServiceStatistics,
      parsedHeaders: ServiceGetStatisticsHeaders,
    },
  }

We could provide something similar from into_future.

[cataggar]

Design with Parsed Response

Here is a proposed design that provides for parsed responses similar to the generated JavaScript clients. Each operation would have the following:

RequestBuilder
fn into_future(self) -> ParsedResponse
fn send(self) -> Response

ParsedResponse
headers: ParsedHeaders
body: T

ParsedHeaders
x_ms_request_id: Option<String>

Response(azure_core::Response)
fn headers(&self) -> Headers
fn parsed_headers(&self) -> ParsedHeaders
fn into_body(self) -> T
fn into_parsed_response(self) -> ParsedResponse
fn into_raw_reponse(self) -> azure_core::Response

Headers(&azure_core::headers::Headers)
fn x_ms_request_id(&self) -> azure_core::Result<&str>

If we go with this design, it can be broken up into multiple pull requests. This pull request would change to send and we would reserve into_future for a ParsedResponse.

[cataggar]

I converted this to a draft while we come to agreement on the proposed design in #1053.

[cataggar]

This implements the first part of the #1053 design. It should be good to merge now. @rylev, @bmc-msft

[rylev]

I'm fine with merging this if it unblocks folks as long as no one is under the illusion that merging this means we've decided on an actual design which we're still discussing in #1053. Waiting for @bmc-msft's thoughts.

[bmc-msft]

I recognize this is a work-in-progress, but is a step backwards in terms of usability as it stands right now.

Currently, nearly all of the published generated crates are broken (fixed in #1020). Can an update be published out before merging this work?

[cataggar]

I recognize this is a work-in-progress, but is a step backwards in terms of usability as it stands right now.

I left RequestBulider::into_body in place to prevent it being a step backwards. I intend to add the other features soon. I just need to get unblocked on this review.

I'm fine with merging this if it unblocks folks as long as no one is under the illusion that merging this means we've decided on an actual design which we're still discussing in #1053.

Sure. We should come to an agreement before the September release in a week or two.

While I have concerns with this PR as is, I understand it's a work in progress.

[bmc-msft]

Based off of an offline discussion, I am marking this approved as to unblock @cataggar