Azure / azure-sdk-for-rust

This repository is for active development of the *unofficial* Azure SDK for Rust. This repository is *not* supported by the Azure SDK team.
MIT License
697 stars 240 forks source link

How should we provide "typed" access to response headers? #1826

Open analogrelay opened 4 days ago

analogrelay commented 4 days ago

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:

let response = client.create_item(...).await.unwrap();
println!("Request Charge: {}", response.headers().get_as::<f32>(REQUEST_CHARGE).unwrap());
println!("Session Token: {}", response.headers().get_str(SESSION_TOKEN).unwrap());
println!("Created Item: {:?}", response.deserialize_body().await.unwrap());

Option 2: Encode these values in the Model type

In this option, instead of returning azure_core::Response<T>, we'd return azure_core::Response<ItemResponse<T>>, where ItemResponse 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 full Response 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, using deserialize_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 return Pageable<QueryResults<T>>, which doesn't provide access to the underlying responses (unless we return Pageable<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:

let response = client.create_item(...).await.unwrap();
let model = response.deserialize_body().await.unwrap();
println!("Request Charge: {}", model.request_charge);
println!("Session Token: {}", model.session_token);
println!("Created Item: {:?}", model.item);

Option 3: Create custom Response types

Instead of azure_core::Response, methods that want to provide typed access to response headers would return their own struct, which wraps azure_core::Response<T> (for example, azure_data_cosmos::ItemResponse<T>). To be ergonomic, I think these structs would have to implement Deref<Target = azure_core::Response> so that you can call methods like deserialize_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:

let response = client.create_item(...).await.unwrap();
println!("Request Charge: {}", response.request_charge());
println!("Session Token: {}", response.session_token());

// If we provide a Deref impl OR reimplement some methods from `Response` manually.
println!("Created Item: {:?}", response.deserialize_body().await.unwrap());

// If we don't provide a Deref impl
println!("Created Item: {:?}", response.raw_response().deserialize_body().await.unwrap());

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 return Response<T, ()>, and we'd define a Response<T, ()>::with_detail<D>(detail D) -> Response<T, D> method that "sets" the details for a response.

Response<T, ()> would have a nonsense detail() 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:

let response = client.create_item(...).await.unwrap();
println!("Request Charge: {}", response.detail().request_charge());
println!("Session Token: {}", response.detail().session_token());
println!("Created Item: {:?}", response.deserialize_body().await.unwrap());

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 from Response<T> feels more ergonomic, but using Deref in this way is often considered an anti-pattern.

heaths commented 4 days ago

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 models module (these definitely shouldn't go into the crate root) /cc @JeffreyRichter @RickWinter.

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 FromStr, for example, to make this easier and idiomatic so - when needed, and supported by examples (docs are often our fallback for teaching concepts - people can use them at zero cost to anyone else who doesn't need them.

analogrelay commented 4 days ago

We could add parsing to headers with some blanket FromStr, for example, to make this easier and idiomatic so - when needed, and supported by examples (docs are often our fallback for teaching concepts - people can use them at zero cost to anyone else who doesn't need them.

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: Headers::get<T>

We currently have get_as<V, E> which accepts any type V: FromStr<Err = E>, but that still requires knowing the name. We can (and will) provide constants, but it still gets verbose:

let request_charge = response.headers().get_as<f32>(REQUEST_CHARGE)?;

We could define a new trait FromHeaders that allows for opt-in typed reading of headers:

pub trait FromHeaders {
    type Error;

    fn from_headers(headers: &Headers) -> Result<Self, Self::Error>;
}

And then add a method to Headers:

impl Headers {
    pub fn get<T: FromHeaders>(&self) -> Result<T, T::Error>;
}

Getting the Request Charge or Session Token for a response is a fairly common pattern, so we'd define custom types for that. However, it would have no impact on those who don't want or need to provide that kind of access. Nor does it impose the lookup and parsing impact on those who don't want the value:

let request_charge: RequestCharge = response.headers().get()?;

That approach also avoids requiring the user to know the best type to use when parsing the header value.

heaths commented 4 days ago

That I like!

analogrelay commented 4 days ago

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.