Closed cataggar closed 1 year ago
I like where we ended up in Go a lot. A Client has a Foo operation which returns a FooResponse (or an error). The Foo response has fields in it for the deserialized response headers and another struct field for the deserialized body. You can see some examples in this Go client library
It would be nice to add a context-helper that lets the customer get at the raw HTTP response object if there is one idiomatic to Rust that we can leak out of our API. We offer this in Go here.
This works very well (and is version tolerant) for everything except blob upload/download operations.
Thanks for writing this up!
I have a few questions:
into_future
and send
? Can't the ParsedResponse
contain a response
field which holds on to the raw Response
? I suppose in order to get this to work you would need to consume the Response
's body and so into_future().raw_response.into_body()
would not really work. I wonder if this papercut is worth not having two different methods for kicking off the request.ParsedHeaders
and Headers
type you reference above. Are those the same thing and this is just a typo?ParsedBody
and such are just placeholder names? Would these have more descriptive names?Thanks @JeffreyRichter & @rylev for the feedback.
The main use case is 1. Use cases 2 & 3 allow the user to choose what to parse or parse themselves. May be the spec is wrong and parsing of a header fails. May be it is a performance optimization and they do not want to pay to collect the response body and deserialize it according to the spec. This design allows the user to opt out of collect
ing the response body and parsing it.
Do we need a separate
into_future
andsend
?
Yes. The implementation of into_future
will call send
and then into_parsed_response
. It makes it easy for use case 1 by avoiding two function calls. Allowing access to send
provides for use cases 2 and 3.
Can't the
ParsedResponse
contain aresponse
field which holds on to the rawResponse
? I suppose in order to get this to work you would need to consume theResponse
's body and sointo_future().raw_response.into_body()
would not really work. I wonder if this papercut is worth not having two different methods for kicking off the request.
That is correct. This does not work because the response body gets consumed. It also does not give the user the option to opt out.
Would this general pattern work well for the hand rolled SDKs? If not, why not?
Yes, the general pattern would work well for the hand rolled SDKs. The main difference is that that the SDKs should provide a response model that can be converted from a ParsedResponse
.
What's the difference between the ParsedHeaders and Headers type you reference above. Are those the same thing and this is just a typo?
ParsedHeaders
contains all of the parsed response headers. Headers
contains a function for each header, allowing the user to choose what to parse. The example I gave was for a x-ms-request-id
header. Headers
contains a function fn x_ms_request_id(&self) -> azure_core::Result<&str>
. ParsedHeaders
is that would contain owned data for all of the defined response headers. It would contain x_ms_request_id: Option<String>
in this case.
I assume ParsedBody and such are just placeholder names? Would these have more descriptive names?
Yes & yes.
In the example that @JeffreyRichter linked, for the "Azure App Settings" example from the Azure SDK for Go, this is the example for adding a configuration setting:
resp, err := client.AddSetting(
context.TODO(),
"key",
to.Ptr("value"),
&azappconfig.AddSettingOptions{
Label: to.Ptr("label"),
})
if err != nil {
panic(err)
}
fmt.Println(*resp.Key)
fmt.Println(*resp.Label)
fmt.Println(*resp.Value)
The response directly shows the Key, Label, and Value which are values returned from the API. This doesn't require the user to first call into something similar to into_parsed_response
.
@bmc-msft, that function is not generated. The function AddSettings uses a generated client to call PutKeyValue. It maps the PutKeyValue data plane operation. The function returns a response type of:
// AzureAppConfigurationClientPutKeyValueResponse contains the response from method AzureAppConfigurationClient.PutKeyValue.
type AzureAppConfigurationClientPutKeyValueResponse struct {
KeyValue
// ETag contains the information returned from the ETag header response.
ETag *string
// SyncToken contains the information returned from the Sync-Token header response.
SyncToken *string
}
The generated client function would be similar to how it currently is:
Except the response of into_future
would be:
struct ParsedResponse {
headers: ParsedHeaders,
body: KeyValue
}
struct ParsedHeaders {
e_tag: String,
sync_token: String,
}
I think you may be suggesting that the response should look like this?
struct ParsedResponse {
key_value: KeyValue,
e_tag: String,
sync_token: String
}
Is that what you propose? I worry about name collisions, but we could try it.
I know nothing about Rust, but the discussion seems reasonable to me :).
One thing I would add is streamable response (blob download). In Python, this has been something a little tricky to model, as it means you get a response with an open socket, and give control to the customer on when to close it. We didn't want a different response type, meaning our reponses have various "is_close" and funny thing like that to support all scenarios. You could technically want to support a download of JSON as a stream, and Python would do that is you pass stream=True
to the call (don't pre-load and pre-parse the body as JSON, and keep the socket instead).
In yesterday's meeting, the feedback was the primary use case is to get back the ParsedBody
. Headers are only needed sometimes. Reviewing the #1084 changes, we can see that the majority of headers like azure-asyncoperation
, location
, retry-after
, etag
, last-modified
are meant for the tooling. Given this, into_future
should return the ParsedBody
like before. The design to ship in the September release becomes:
RequestBuilder
fn into_future(self) -> ParsedBody
fn send(self) -> Response
Response(azure_core::Response)
fn headers(&self) -> Headers
fn into_body(self) -> ParsedBody
fn into_raw_reponse(self) -> azure_core::Response
fn as_raw_reponse(&self) -> &azure_core::Response
Headers(&azure_core::headers::Headers)
fn x_ms_request_id(&self) -> azure_core::Result<&str>
While I agree that headers are mostly of secondary importance, that's not always the case. For example, when getting account information about a blob storage account, the body is empty, and the headers contain the relevant information (e.g., sku_name
and account_kind
). Are we sure that we want to require the user to understand this and to use send
instead of into_future
?
Yes, I think it is fine. For blob storage accounts, we provide a more friendly SDK on top of the generated crate like the other languages.
In the case of APIs where the body reply is known to be empty, could into_future return the header?
We're talking about what the code generator should do, right? Or are we talking about guidelines that engineers would follow when wrapping generated code?
Also, I don't think you should be getting creative here. A lot of thinking went into our SDKs and how they are generated/presented to users. Rust should do the same thing but in a Rust-way. Please do what Go and other code-generated SDKs and what we discuss on the call. It makes the most sense from an HTTP perspective - let's not make value judgements on which headers are for tooling and which contain useful information. Let's just treat all headers as headers and bodies as bodies. When the SDK team has placed value judgements on things, we have frequently gotten burned by this and then our hands are tied. For example, a service decides in the future to return a really important header and now it is downplayed by the SDK API surface or vice versa. You must always consider the how things might change in the future and we don't want to break customers so we need to just keep things simple, stick with HTTP semantics and not make our own point-in-time value judgements to ensure sustainable success.
Overview
Following @yoshuawuyts advice, this is my attempt to describe the problem and offer a proposed solution. From working on #1040, and doing analysis of AutoRest generated code for other languages, it has become clear that there are some different use cases for HTTP responses.
SDK Responses vs OpenaAPI Generated Responses
The generated responses are limited to what is provided by the OpenAPI specifications. Data is provided in both the response headers and the response body. The SDKs abstract this away into response models that combine data from headers and the body. Cadl will make it possible to specify the response models, but OpenAPI is what we currently have.
Current Design
Currently, the parsed response body is returned. There is no access to data from the headers. There is no access to the raw response. In pseudo code:
Proposed Design
Make it is easy to get the fully parsed response. Make it possible to choose what to parse. Make it possible to get the raw response. In pseudo code:
.into_future()
and it returns theParsedResponse
. It has aheaders
member if the operation has response headers. It has abody
if the operation has a response body..send
instead of.into_future
, which returns aResponse
. They can use.headers()
to access individual headers..send
and theninto_raw_response
to get access to theazure_core::Response
.