-
Notifications
You must be signed in to change notification settings - Fork 247
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
How should we provide "typed" access to response headers? #1826
Comments
Most Azure SDK languages don't provide typed header access, and I really don't want to get providing some mapping of header names to types necessarily. Headers can be malformed and lookup can be slower with lots of headers (or we hash, but if people don't need O(1) access to all headers it's a waste of time). I prototyped option 3 but there wasn't a lot of backing for it back then. We could reconsider but I worry this sets a precedent that we should do it for all client libraries and I think that's overly verbose. We end up generating a lot of types which pollutes the I think for now we stick with option 1. For most client libraries, at least, the headers are often useless for apps (and most that are somewhat useful are opaque anyway). We could add parsing to headers with some blanket |
The "zero cost" angle is a good one to raise. The Rust ecosystem definitely prioritizes opt-in costs as much as possible. Here's a possible modified proposal that comes to mind, and I'm OK with sitting on it until we feel like it's really necessary (based on customer feedback): Option 5:
|
That I like! |
Ok, I can get behind that. It's a fair bit different from how other Cosmos SDKs do it, but I agree we're largely outliers here from my initial looking. Most importantly, it feels Rustier than all the other options above. |
Many Azure services do leverage response headers and a BIG feature of SDK client libraries is to parse these and return some structure with fields so that client code can use code completion and compile-time type-safety. I think it would be a huge mistake for Rust not to offer this for response headers. It's true that most (perhaps all) of our current SDK languages parse these headers by default. I would NOT encode these values into the resource model type as they serve vastly different purposes. Response headers are in response to a specific operation (like a specific create, update, read, or delete operation) while the resource model is operation agnostic and is frequently round-tripped; for example, GET a resource, modify it, and then update (PATCH) it back. |
To iterate through byte sequences for every response to parse headers early for every response is a massive waste of CPU time. I hope not all languages do this. I know for certain .NET doesn't. It doesn't even provide any typed headers unless you specifically pass raw Rust is at least going beyond this. It's parsing known headers (and supports customer-provided types) only when needed. That's far more performant since the vast majority of service clients won't need to parse headers as anything more than strings. Even all the etag-related headers are otherwise opaque. |
I think we've found a good balance between the needs of our customers and the expectations of the Rust community. Pre-emptively parsing headers would be surprising to Rust users. The approach we have allows for fairly easy typed access. If we do nothing else, headers like the Request Charge in Cosmos require only that you know the target type (and not anything about the header name or expected format): let request_charge: RequestCharge = response.headers().get()?; I think we stick with that for now. If we get feedback that this is hard to discover, we could build extension traits on the |
Cosmos DB provides a lot of information in HTTP headers. For example, when modifying items, the
x-ms-session-token
header returns a token that can be provided in future requests to maintain "session consistency" (so future requests "see" modifications made earlier in the session). We also return metadata like the number of RUs consumed by a request (the "request charge"), the etag for a newly-created document, the Activity ID for tracing, etc. Most of our language SDKs provide access to these values using typed responses of some kind.Other clients handle this using inheritance or embedding. For example, the Go client returns an
azcosmos.Response
which embeds the Azure Core response and adds first-class properties for these. The .NET client returns a response type that inherits from the Azure Core response.What I'm seeking here is to figure out the ideal way to represent this in the Rust SDK. There are a few options:
Option 1: Do nothing.
Users can retrieve these values from
Response<T>
using the header names.I don't really like this option because it's a significant loss of functionality compared to the other SDKs. However, it's certainly one of the simplest options.
For the cosmos example, if you wanted to access the session token or request charge while still deserializing the body, you'd write something like this:
Option 2: Encode these values in the Model type
In this option, instead of returning
azure_core::Response<T>
, we'd returnazure_core::Response<ItemResponse<T>>
, whereItemResponse
contains properties for things like the session token.This is the most complicated option and requires some signficant changes. We'd need
azure_core::Model::from_response_body
to take the fullResponse
instead, and we'd need to write our own custom logic to pull the headers and then deserialize the body. If a user changed the deserialize type, usingdeserialize_body_into
, they'd lose this data.NOTE: This is what I did for
azure_data_cosmos::clients::ContainerClient::query_items
but I don't think it works outside of paged responses. It worked there because we returnPageable<QueryResults<T>>
, which doesn't provide access to the underlying responses (unless we returnPageable<Response<T>>
, I suppose).For the cosmos example, if you wanted to access the session token or request charge while still deserializing the body, you'd write something like this:
Option 3: Create custom
Response
typesInstead of
azure_core::Response
, methods that want to provide typed access to response headers would return their own struct, which wrapsazure_core::Response<T>
(for example,azure_data_cosmos::ItemResponse<T>
). To be ergonomic, I think these structs would have to implementDeref<Target = azure_core::Response>
so that you can call methods likedeserialize_body()
.I don't mind this option. It's the most similar to how the Go SDK does it. It feels like inheritance, which is not very idiomatic in Rust, but it has the least impact on service client methods that don't need to return custom data from headers.
For the cosmos example, if you wanted to access the session token or request charge while still deserializing the body, you'd write something like this:
Option 4: Add a "Detail" value to `Response
This would add a new type parameter and field to
Response<T>
. The new type,Response<T, D = ()>
, would be able to store some kind of service-specific details derived from the response headers. This detail would be provided by the service client, which would parse the headers. The pipeline would returnResponse<T, ()>
, and we'd define aResponse<T, ()>::with_detail<D>(detail D) -> Response<T, D>
method that "sets" the details for a response.Response<T, ()>
would have a nonsensedetail()
method that returns()
, which is harmless but could cause confusion in rust-analyzer completion prompts.For the cosmos example, if you wanted to access the session token or request charge while still deserializing the body, you'd write something like this:
Of these options, I'm leaning strongest towards Option 3 at this point, though I've waffled back and forth between that and Option 4 as I've thought through this and typed out this issue. If we do Option 3, implementing
Deref
or reimplementing some methods fromResponse<T>
feels more ergonomic, but using Deref in this way is often considered an anti-pattern.The text was updated successfully, but these errors were encountered: