Conversation
timovv
left a comment
There was a problem hiding this comment.
Looks good! A couple of questions but nothing major
| export interface User { | ||
| name: string; | ||
| email: string; | ||
| userId?: string; |
There was a problem hiding this comment.
Might be missing something important here -- but what makes headers different from properties in the request body to motivate making them optional in the response model? I get that headers might not always be present if the service is not following the spec, but the same argument could be made for any field on the request body too, and those fields are still generated as required.
| ``` | ||
|
|
||
| ```ts models function userDeserializer | ||
| export function userDeserializer(item: any, headers?: any): User { |
There was a problem hiding this comment.
Looks like headers comes straight from the PathUncheckedResponse when _getUserDeserialize calls this. Will it ever be undefined?
| const headerProps: SdkModelPropertyType[] = []; | ||
| const addHeaderProps = (model: SdkModelType) => { | ||
| model.properties?.forEach((p) => { | ||
| if (isHeader(context.program, p.__raw!)) { |
There was a problem hiding this comment.
Nit, could TCGC help here? I see there is a SdkServiceResponseHeader type that looks promising but couldn't tell at a glance whether we have access to that info here
There was a problem hiding this comment.
As I mentioned, TCGC isn't fully ready to support this case. So currently the runtime in TCGC has some limitation that we need to touch the raw data I think.
maorleger
left a comment
There was a problem hiding this comment.
Not too well-versed with the implementation details; however, I did spot one thing I had a question about. Overall though, the approach seems ok to me.
It aligns with our guidelines:
The logical entity is a protocol neutral representation of a response. For HTTP, the logical entity may combine data from headers, body and the status line. A common example is exposing an ETag header as a property on the logical entity in addition to any deserialized content from the body.
and with Python's implementation as far as I can tell.
Do you know how it'll deserialize x-ms headers? Just curious
| @@ -271,7 +277,7 @@ export function getDeserializePrivateFunction( | |||
| if (${isXmlContentTypeRef}(responseContentType)) { | |||
| return ${xmlDeserializerName}(${deserializedRoot}); | |||
There was a problem hiding this comment.
would this need changing as well? Wouldn't XML deserializer be called without the header parameter then?
2407f36 to
7861ae4
Compare
|
@MaryGao are you following up with this? Is this the accepted approach for response model related breakings? |
We have some offline discussions and here are the agreements with Maor and Jeff. In short:
Overall the decision looks good to me and I will review this PR for the adoption and verify the impact in our side. |
| name: (restResponse as any).name ?? "", | ||
| type: getTypeExpression(context, restResponse.type!) | ||
| }; | ||
| } else if (hasHeaderOnlyResponse) { |
There was a problem hiding this comment.
Any reason on we specially handle header-only responses?
This would introduce some changes in ARM SDKs where the return type is void.
| } | ||
| ``` | ||
|
|
||
| # [void] Header properties included in the response model when there is no response body |
There was a problem hiding this comment.
As I mentioned this is the case I have concerns. I wonder if we have real case in storeage yet. If no, I prefer we figure out how to express following two scenarios in TypeSpec side first.
- headers are included in response return type
- headers are not included in return type by intention
Currently we would include all headers in responses and this is common that ARM SDKs would introduce new changes especially DELETE with void return type.
If we have changed the design in future, this would be a breaking for ARM libraries. Or maybe we could only enable this for DPG, right now?
| @@ -0,0 +1,185 @@ | |||
| # Header properties included in the response model interface by default | |||
There was a problem hiding this comment.
Have we implemented other cases in gist?
I may wonder if the bodyRoot case is supported: https://gist.github.com/joheredi/d1e310b6d1464ae89fcaefb00601693d#file-3_bodyroot_decorator-md.
model User {
name: string;
email: string;
@header("x-user-id")
userId?: string;
@header
@encode("rfc7231")
createdAt?: utcDateTime;
}
op getUser(): { @bodyRoot user: User} & { @header fooHeader: string};| requestId: result.headers?.["x-ms-request-id"] | ||
| } as GetAccountInfoResponse; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
I looked at the recent storage spec with below example in which the relevant headers would NOT generate in responses(which may not meet our expectation). But the behavior meets my expectation around this PR. My assumption for this PR would be
- if these headers are explicilty added into bodyRoot response type, it will be included in our response type;
- if these headers are not added into bodyRoot or body response type, it will be not be there
Please note some cases, TypeSpec HTTP lib would have an implicit body resolution: https://typespec.io/docs/libraries/http/operations/#implicit-body-resolution.
getAccessPolicy is StorageOperation<
{
...TimeoutParameter;
...LeaseIdOptionalParameter;
},
{
/** Signed identifiers */
@body body: SignedIdentifiers;
...BlobPublicAccess;
...EtagResponseHeader;
...LastModifiedResponseHeader;
}
>;|
Overall the PR looks good to me and I left some comments for clarification. |
| requestId: result.headers?.["x-ms-request-id"] | ||
| } as GetAccountInfoResponse; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Have we handled request headers also?
model User {
id: string;
name: string;
@header("x-user-version")
version: string;
@header("x-request-id")
requestId: string;
}
op getUser(user: User ): User;
Executive Summary
This design document outlines a feature enhancement to the TypeScript SDK emitter that enables automatic deserialization of HTTP response headers into strongly-typed model interfaces. This aligns SDK behavior with HTTP semantics where response headers carry important metadata alongside the response body, and provides developers with a unified, type-safe API for accessing all response data.
Problem Statement
Current State
In HTTP, responses consist of two distinct parts:
x-user-id,content-type,etag)TypeScript SDK-generated clients currently deserialize only the response body into a model type, leaving headers as untyped data available only through raw
result.headersaccess. This creates several issues:result.headersfor critical valuesUser Perspective
When a service contract specifies that a response includes both body data AND header metadata (e.g., a versioning header, request ID, or custom business data):
Users expect to access all response data through a single, typed interface:
Desired behavior: Headers should be seamlessly integrated into the response model, just like body properties.
Solution Overview
Design Approach
Transform the deserialization pipeline to:
Detailed Design
1. Public API Changes: Response Model Interfaces
Change 1.1: Include Header Properties in Model Interfaces
Before:
After:
Rationale:
?) reflects HTTP reality: headers may not always be presentChange 1.2: Header Properties Always Optional
Header properties are always marked as optional (with
?modifier) regardless of their TypeSpec definition because:2. Deserializer Changes: Response Object Construction
Change 2.1: Deserializer Signature Enhancement
Before:
After:
Key Points:
headersparameter is optional (has?modifier), maintaining backward compatibility?.) when accessing headers to safely handleundefinedvalues"x-user-version")Change 2.2: Selective Header Parameter Passing
To avoid unnecessary parameter passing and maintain clean signatures:
Only pass headers when the model has header properties:
Benefits:
Change 2.3: Handling Inheritance
For models with inheritance, header properties from base models are also extracted:
3. Operation Deserialization Integration
Operations that return models with headers automatically pass headers during deserialization:
HTTP Alignment
This design aligns with HTTP specifications and best practices:
RFC 7230/7231: HTTP Semantics and Header Fields
REST API Design Principles
Backward Compatibility
✅ Fully Backward Compatible
undefinedheadersparameterDeveloper Experience Benefits
1. Type Safety
2. Unified Response Access
3. IntelliSense Support
4. Documentation
Types serve as inline documentation:
Implementation Details
Scope of Changes
emitModels.ts): Include header properties, mark as optionalbuildDeserializerFunction.ts): Add optionalheadersparameter, extract header valuesoperationHelpers.ts): Conditionally pass headers based on model compositionDetection Logic
@headerTypeSpec decorator to identify header properties