Replace `Model` trait with `Format` trait (applied to `Response` instead) #2559

analogrelay · 2025-05-03T14:53:20Z

Early draft of #1831. It should have the necessary changes in typespec_client_core, but I haven't gone through other crates and updated them. I'll continue doing that to see if other surprising issues appear, but wanted to give folks (particularly @heaths) a chance to take a look at how Response<T, F> looks and how I implemented it.

I did things a little different than I originally planned because I wanted to retain the ability to take a default "bare" Response type (using no explicit type parameters, and thus just the defaults) and treat it like a raw response with no deserialization. So, here's how it works:

The Response<T, F> type expects two type parameters: T is the return type for into_body() calls, the type of the model, and F is a marker type, implementing a marker trait Format, that specifies how T is "formatted" (i.e. JSON, XML, etc.).

There are two formats in typespec_client_core: DefaultFormat is the "default" format. It deserializes T using some default rules that prefer JSON formatting (see below), XmlFormat is available when feature xml is enabled, it deserializes T using XML.

The Format trait used by F doesn't actually provide any methods. Instead, there is a separate trait, DeserializeWith<F: Format> that is applied to T. This allows us to provide implementations that vary based on BOTH the target type T and the desired format F. We provide the following implementations in typespec_client_core:

impl<F> DeserializeWith<F> for ResponseBody which "deserializes" a ResponseBody from a ResponseBody by just returning it. This allows Response<ResponseBody, F>::into_body() return the raw ResponseBody. This happens no matter what format is specified, so Response<ResponseBody, XmlFormat>::into_body() still returns a raw unparsed XML buffer.
impl<T: DeserializeOwned> DeserializeWith<DefaultFormat> for T which deserializes a T: DeserializeOwned using serde_json, IF the response was marked with DefaultFormat. This impl is only present if feature = "json". The DefaultFormat type exists unconditionally though, because it is the default.
impl<T: DeserializeOwned> DeserializeWith<XmlFormat> for T which deserializes a T: DeserializeOwned using our XML parsing library. This impl, like XmlFormat itself, is only present if feature = "xml".

Future serialization formats can be implemented with new marker types and new blanket DeserializeWith<SomeOtherFormat> impls.

In order to "set" these marker type parameters (T, F) we have a few other changes:

Response<ResponseBody, F> (for any F) has an into_typed::<T, F2>() method that transforms the response from a raw "untyped" response into a fully typed Response<T, F>, using the type and format specified (usually via inference).
Transport/Policy send methods are parameterized with T: DeserializeWith<F>, F: Format and call into_typed automatically. Calling Response<ResponseBody, DefaultFormat>::into_typed::<ResponseBody, DefaultFormat>() (which would happen when a service client intends to return a bare response) is essentially a no-op. I'll do some investigation to see how well the compiler optimizes this out. Since the only place we use T and F are in PhantomData, my hope is it optimizes quite well.

These changes may mean that the emitter doesn't have to change, for service client methods that are satisfied with the default format. I'm not quite sure about that yet.

I expect this PR to fail CI because I haven't updated anything outside typespec_client_core. The main purpose here is to give folks a chance to review the APIs above and make comments. Please feel free to do so!

analogrelay · 2025-05-05T17:51:21Z

I've now updated the hand-written crates in this repo, but need to consider the best way to update generated crates. We'll need to update the emitter, but I could also update the generated code here right away. I'm going to do that in a separate commit so that I can confirm it works and then we can decide whether to keep it or not. That commit will also serve as a clear guide for how we need to update the emitter.

analogrelay · 2025-05-05T18:01:36Z

Calling Response<ResponseBody, DefaultFormat>::into_typed::<ResponseBody, DefaultFormat>() (which would happen when a service client intends to return a bare response) is essentially a no-op

Some confirmation here: I ran a simple sample through the Playground that checked the assembly of a function that consumed its argument, copied all the non-zero-size fields to a new version of the struct, and changed zero-sized values (PhantomData). If you force that function to be non-inlined, it's a complete no-op function consisting only of the prologue/epilogue. If you allow it to be inlined, it disappears completely.

analogrelay · 2025-05-05T20:48:25Z

sdk/typespec/typespec_client_core/src/http/pipeline.rs

+            .map(|r| r.with_model_type())
+    }
+
+    pub async fn send_format<T, F>(


I added this because most cases will want to use the DefaultFormat. However, when a custom format is needed, the syntax gets clunky, because you'll have to call .with_format() on the result, but you can't do that until after you await. This results in having to write Ok(self.pipeline.send(...).await?.with_format()) which felt clunky, because with_format is infallible but send is fallible. It works just fine, but if I could bundle it up into a single send_format call, then methods that create XML-formatted responses would follow a similar pattern as those that create JSON-formatted ones

Keep in mind, though, that with #2073, most clients will do this...which are generated. So that should be okay.

Maybe send_with_format. Makes a little more sense, IMO, and follows with our "with" naming conventions.

The naming is messy, and I might just drop the facade here and have a single send call. send_with_format was the first name I used but I felt it implied it was talking about the Request format. I don't like send_format either though...

analogrelay · 2025-05-05T20:53:10Z

sdk/typespec/typespec_client_core/src/http/pipeline.rs

@@ -82,11 +84,22 @@ impl Pipeline {
        &self,
        ctx: &Context<'_>,
        request: &mut Request,
-    ) -> crate::Result<Response<T>> {
+    ) -> crate::Result<Response<T, DefaultFormat>> {


You might hope we could write this method as pub async fn send<T, F = DefaultFormat>(...) -> Result<Response<T, F>>, which would allow the user to optionally specify a format. Alas, type parameters in functions cannot have defaults, because it messes with type inference (details in rust-lang/rust#36887)

The reason this "hard-codes" DefaultFormat is that if we don't specify a format, you can't write code like this:

pub async fn something(&self) -> azure_core::Result<SomeModelType> { //... prep the request self.pipeline.send(&ctx, &mut request).await?.into_body().await }

Because you never name the Response type, the format goes un-inferred and you get an error. Instead, you'd have to write this:

pub async fn something(&self) -> azure_core::Result<SomeModelType> { //... prep the request self.pipeline.send::<SomeModelType, DefaultFormat>(&ctx, &mut request).await?.into_body().await }

Now, if we're OK with that, I'm open to going back to one send method. I figured it was safe for send to default to the default format since the format of a response can be changed at zero cost using Response::with_format. I also included the below send_format short-cut method to do that in one call.

What if we move the format to the client, since enums and structs can have default typeparam values? The vast majority of clients will only deal with one format anyway - and almost always JSON. The worst case scenario is that if a caller is wanting JSON and XML both from a service, they have to use separate clients. Doesn't seem all that bad given we can share (via clone) options and all that.

Relatedly, rather than a "DefaultFormat", can we just be explicit? No guesswork. The "default" for 99% of our clients would be JsonFormat.

That's an interesting idea. So, we'd make Pipeline take a type parameter F: Format? I think that would be OK. The way I see it, there are three scenarios::

All service client methods can be deserialized using the same format - They create a Pipeline<JsonFormat> or Pipeline<XmlFormat> and go from there.

Most service client methods can be deserialized using a single format, but some need a separate format - They can still create a Pipeline for their most common format and use Response::with_format to change the format of their responses.

A service client has basically an even split of methods each using different formats - They can create several Pipeline instances. Clunky, but works fine.

Of course, scenario 1 is by far the most common IMO, especially since both formats will handle Response<ResponseBody> (raw bytes) and Response<()> (empty content) correctly.

I think I like it, I'll work that up.

It works, but has a few quirks that are worth pointing out (I'll push my commit too so you can take a look).

We have to define both typespec_client_core::http::Pipeline and azure_core::http::Pipeline as generic, but that's fine since users should still only interact with the azure_core one.

Setting a default does work: pub struct Pipeline<F: Format = Json>. However, it's a little quirky.

Default type parameters and constructors don't quite work well together. While a field labelled pipeline: Pipeline does get treated by the compiler as pipeline: Pipeline<JsonFormat> that is not true if the type is used in method call position: let pipeline = Pipeline::new(...) (with no specified type parameters) yields an error saying a type annotation is needed ¹

Footnotes

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=59c5f013ad5856ba13de8dfcc102a1ff ↩

Then the only other thing is that every Response said pipeline produces, even those with () or ResponseBody as the payload type, requires spelling out the whole thing, including the format type: Response<(), XmlFormat> or Response<ResponseBody, XmlFormat>

Does this mean that a client can support JSON or XML but not both?

In this approach, a Pipeline can only support one format, but nothing prevents a client from holding multiple Pipeline<F> instances with different Fs. The policies are held in an Arc so they can be safely shared.

analogrelay · 2025-05-05T23:36:43Z

Opening this up for review. The build failure appears to be issues with APIView

heaths

I'm liking this. There's a few discussion points, but I think it's really close.

sdk/typespec/typespec_client_core/src/http/format.rs

heaths · 2025-05-07T18:54:26Z

sdk/typespec/typespec_client_core/src/http/pipeline.rs

+            .map(|r| r.with_model_type())
+    }
+
+    pub async fn send_format<T, F>(


Maybe send_with_format. Makes a little more sense, IMO, and follows with our "with" naming conventions.

sdk/typespec/typespec_client_core/src/http/pipeline.rs

sdk/typespec/typespec_client_core/src/http/response.rs

Pilchie

Generally LGTM, but I'll defer to @heaths .

heaths

Barring my one question, this LGTM but the "request changes" is more about putting this on a feature branch. You'll need to rebase anyway, so if you want to do it, that might be faster/easier. Just preface the branch name with "feature/" and make sure to push it to the upstream remote so you can retarget this PR and we keep all this great context!

sdk/core/azure_core/src/http/pipeline.rs

heaths · 2025-06-03T20:55:48Z

sdk/typespec/typespec_client_core/src/http/pipeline.rs

@@ -30,13 +32,17 @@ use std::sync::Arc;
 /// cannot be enforced by code). All policies except Transport policy can assume there is another following policy (so
 /// `self.pipeline[0]` is always valid).
 #[derive(Debug, Clone)]
-pub struct Pipeline {
+pub struct Pipeline<F: Format> {


After our design discussion, I believe we agreed we'd keep Pipeline returning a ResponseBody or something, and then each client method would have to deserialize that (async) into a Response<T, F>, right?

…boilerplate

analogrelay · 2025-06-03T22:11:04Z

Updated per the offline design discussion. This will still fail to build as the generated code needs to be updated, but I've updated Cosmos code (though I'm having some unrelated difficulty with testing against the emulator so I'll keep working on that, it compiles though so the API changes are reviewable).

Split Response into two types: RawResponse which holds the data and Response<T, F> which holds a RawResponse and a PhantomData<(T, F)> and implements the deserialization logic
Add From/Into conversions between RawResponse and Response<T, F> (which should be "free" because the only difference between the two is a zero-sized PhantomData).
Updated Cosmos SDK to return Response<()> for empty bodies and RawResponse for "raw" bodies.
Change Pipeline to be fully based on RawResponse (and thus non-generic) and leave it up to the caller to convert the response using .into(). This leads to some extra boilerplate but most of our code is generated, and the code that isn't (Cosmos, primarily) can easily use it's own helper to smooth out the boilerplate.

analogrelay · 2025-06-04T16:16:52Z

So, the main emitter changes here, as far as I can tell, are:

When returning raw bytes from an endpoint, return RawResponse instead of Response.
Remove all references to azure_core::Model and the #[typespec(...)] attributes, they're no longer needed 🎉
To create a Response<T, F> from raw bytes, you need to use RawResponse::from_bytes(...).into() (to keep things minimal and simple there's no direct constructor for Response<T, F> now, instead you create the RawResponse first and wrap it)
impls of ...Headers traits need to EITHER be generic on F (which should work for every client), or specify the correct format based on what the API returns (which is more "correct").
In service client methods that return a Response<T, F> or Pager<T> (which is now Pager<T, F>, we need to make some adjustments
- Depending on the output format, set the F to either JsonFormat or XmlFormat (if it's JsonFormat, you could omit the F parameter entirely, but I don't think it's necessary for the generator to do that)
- Call .into() on the RawResponse returned the by the pipeline. As long as it's the final expression in the function and the return type is specified, no type annotations will be needed. For example:

pub fn get_secret(...) -> azure_core::Result<Response<GetSecretResponse, JsonFormat>> {
    // ...
    Ok(self.pipeline.send(...).await?.into())
}

Alternatively, using Result::map instead of ?:

pub fn get_secret(...) -> azure_core::Result<Response<GetSecretResponse, JsonFormat>> {
    // ...
    self.pipeline.send(...).await.map(|r| r.into())
}

Service client methods that return RawResponse (things like download blob) wouldn't need to do this and can just call self.pipeline.send(...).await, because that will return the Result<RawResponse> they need.

I'm going to see if I can quickly update the generated code here to illustrate, but obviously we need to fix the emitter and go from that.

analogrelay · 2025-06-04T17:01:26Z

Ok, @jhendrixMSFT @heaths , I think I was able to update the generated code in f6d9786 which should provide an illustration of emitter changes needed. Happy to sync up offline on these as well! I detailed most (if not all, I think I was fairly complete) of the individual changes I had to make in the comment above.

heaths

Just a couple thoughts based on the commits since my last review. I'm going to look at the PR as new (all commits) since these last 3 or so seemed more like reversions, but looking really good otherwise.

.github/copilot-instructions.md

heaths

Just a few nits, but otherwise looks great to me! I mean, seriously, this cleans up the code so nicely! 🥰

sdk/typespec/typespec_client_core/src/http/response.rs

sdk/core/azure_core/src/http/pager.rs

…ead) (#2559) * start building Format/DeserializeWith trait * remove Model and replace with Format type parameter * fix issues in azure_core, azure_identity, and azure_data_cosmos * fix tests in hand-written crates * updates to generated crates * make send_format more clearly a short-cut method * fix async_trait on wasm32 * fix tyop * initial pr feedback * rename DefaultFormat to JsonFormat * paramerize pipeline on format * undo all generated code changes * revert keyvault generated changes * some updates to copilot instructions * split Response into Response and RawResponse * remove extraneous helpers, let's focus on simple APIs and be ok with boilerplate * api doc updates * remove some extraneous copilot instructions * refactor cosmos for new Response types * fix typespec client examples * fix issues in non-generated clients * fix issues in generated clients * a few other hand-written client fixes * update changelog * pr feedback * refmt * fix doctest * fix more doctests * add `F` parameter to `Pager`

github-actions bot added the Azure.Core The azure_core crate label May 3, 2025

analogrelay changed the title ~~Ashleyst/1831 model format trait~~ Replace Model trait with Format trait (applied to Response instead) May 5, 2025

analogrelay commented May 5, 2025

View reviewed changes

analogrelay marked this pull request as ready for review May 5, 2025 23:36

analogrelay requested review from Pilchie, kirankumarkolli, tvaron3, FabianMeiswinkel, kundadebdatta, heaths, RickWinter, LarryOsterman, vincenttran-msft, jalauzon-msft, chlowell and ronniegeraghty as code owners May 5, 2025 23:36

heaths requested changes May 7, 2025

View reviewed changes

analogrelay force-pushed the ashleyst/1831-model-format-trait branch from 97373fa to c29e3f7 Compare May 8, 2025 21:21

Pilchie approved these changes May 9, 2025

View reviewed changes

heaths linked an issue May 13, 2025 that may be closed by this pull request

azure_core::Model derive macro doesn't infer Deserialize bounds in impl #1803

Open

heaths requested changes May 13, 2025

View reviewed changes

sdk/core/azure_core/src/http/pipeline.rs Outdated Show resolved Hide resolved

analogrelay added 5 commits June 3, 2025 16:52

start building Format/DeserializeWith trait

9188e3a

remove Model and replace with Format type parameter

db21fd2

fix issues in azure_core, azure_identity, and azure_data_cosmos

c05ba35

fix tests in hand-written crates

ebb3ac6

updates to generated crates

1c7cf3f

analogrelay force-pushed the ashleyst/1831-model-format-trait branch from c29e3f7 to 2409a7d Compare June 3, 2025 16:57

analogrelay changed the base branch from main to feature/replace-model-with-format June 3, 2025 16:58

analogrelay added 3 commits June 3, 2025 17:01

revert keyvault generated changes

c9fab4e

some updates to copilot instructions

2234aa1

split Response into Response and RawResponse

857daf6

heaths reviewed Jun 3, 2025

View reviewed changes

analogrelay added 5 commits June 3, 2025 21:46

remove extraneous helpers, let's focus on simple APIs and be ok with …

1a068d0

…boilerplate

api doc updates

c342891

remove some extraneous copilot instructions

27fe398

refactor cosmos for new Response types

9201af2

fix typespec client examples

5f2f873

heaths mentioned this pull request Jun 4, 2025

Iterate over items asynchronously from Pager #2659

Merged

analogrelay added 2 commits June 4, 2025 17:00

fix issues in non-generated clients

a709ff4

fix issues in generated clients

f6d9786

analogrelay added 2 commits June 4, 2025 17:44

a few other hand-written client fixes

f14733b

update changelog

955d60d

heaths reviewed Jun 4, 2025

View reviewed changes

.github/copilot-instructions.md Outdated Show resolved Hide resolved

.github/copilot-instructions.md Outdated Show resolved Hide resolved

heaths approved these changes Jun 4, 2025

View reviewed changes

sdk/typespec/typespec_client_core/src/http/response.rs Outdated Show resolved Hide resolved

sdk/core/azure_core/src/http/pager.rs Outdated Show resolved Hide resolved

analogrelay added 5 commits June 4, 2025 20:56

pr feedback

99cc69f

refmt

03489f5

fix doctest

a144fd5

fix more doctests

452935b

add F parameter to Pager

c15133a

heaths approved these changes Jun 4, 2025

View reviewed changes

heaths merged commit cc19f5d into Azure:feature/replace-model-with-format Jun 4, 2025
17 checks passed

Replace Model trait with Format trait (applied to Response instead) #2559

Replace Model trait with Format trait (applied to Response instead) #2559

Uh oh!

Conversation

analogrelay commented May 3, 2025

Uh oh!

analogrelay commented May 5, 2025

Uh oh!

analogrelay commented May 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

analogrelay May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

analogrelay commented May 5, 2025

Uh oh!

heaths left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Pilchie left a comment

Choose a reason for hiding this comment

Uh oh!

heaths left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

analogrelay commented Jun 3, 2025

Uh oh!

analogrelay commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analogrelay commented Jun 4, 2025

Uh oh!

heaths left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

heaths left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Replace `Model` trait with `Format` trait (applied to `Response` instead) #2559

Replace `Model` trait with `Format` trait (applied to `Response` instead) #2559

analogrelay May 13, 2025 •

edited

Loading

analogrelay commented Jun 4, 2025 •

edited

Loading