Skip to content
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

Update to Body consistency in http request #2945

Merged
merged 35 commits into from
Apr 17, 2024

Conversation

timotheeguerin
Copy link
Member

@timotheeguerin timotheeguerin commented Feb 21, 2024

fix #2868

  • Change meaning of @body to mean this is the body and nothing will be excluded(show warning on containing metadata)
  • Add new @bodyRoot which has the same purpose as the old @body( Allows changing where the body is resolved from but allows mixing with metadata.
  • Add new @bodyIgnore which allows a property to be ignored from the body
  • Provide a new body resolution common function for request and response

also fix #2075

Examples from original issue

  1. Inconsitency between request and response
  2. Inconsitency between different ways

Breaking changes

Azure spec PR showing scale of breaking changes Azure/azure-rest-api-specs#27897

@body means this is the body

This change makes it that using @body will mean exactly this is the body and everything underneath will be included, including metadata properties. It will log a warning explaining that.

op a1(): {@body _: {@header foo: string, other: string} };
                ^ warning header in a body, it will not be included as a header.

Solution use @bodyRoot as the goal is only to change where to resolve the body from.

op a1(): {@bodyRoot _: {@header foo: string, other: string} };

Empty model after removing metadata and visibility property result in void always

This means the following case have changed from returning {} to no body

op b1(): {};
op b2(): {@visibility("none") prop: string};
op b3(): {@added(Versions.v2) prop: string};

Workaround: Use explicit @body

op b1(): {@body _: {}};
op b2(): {@body _: {@visibility("none") prop: string}};
op b3(): {@body _: {@added(Versions.v2) prop: string}};

Status code always 200 except if response is explicitly void

op c1(): {@header foo: string}; // status code 200 (used to be 204)

Solution: Add explicit @statusCode

op c1(): {@header foo: string, @statusCode _: 204};
op c1(): {@header foo: string, ...NoContent}; // or spread common model

Properties are not automatically omitted if everything was removed from metadata or visibility

op d1(): {headers: {@header foo: string}}; // body will be {headers: {}}

Solution: use @bodyIgnore

op d1(): {@bodyIgnore headers: {@header foo: string}}; // body will be {headers: {}}

@timotheeguerin timotheeguerin added the breaking-change A change that might cause specs or code to break label Feb 21, 2024
@azure-sdk
Copy link
Collaborator

azure-sdk commented Feb 21, 2024

All changed packages have been documented.

  • @typespec/http
  • @typespec/openapi3
  • @typespec/rest
Show changes

@typespec/http - breaking ✏️

Empty model after removing metadata and visibility property result in void always,> This means the following case have changed from returning {} to no body,> ,> tsp,> op b1(): {};,> op b2(): {@visibility("none") prop: string};,> op b3(): {@added(Versions.v2) prop: string};,> ,> ,> Workaround: Use explicit @body,> ,> tsp,> op b1(): {@body _: {}};,> op b2(): {@body _: {@visibility("none") prop: string}};,> op b3(): {@body _: {@added(Versions.v2) prop: string}};,>

@typespec/http - breaking ✏️

Implicit status code always 200 except if response is explicitly void,> ,> tsp,> op c1(): {@header foo: string}; // status code 200 (used to be 204),> ,> ,> Solution: Add explicit @statusCode,> tsp,> op c1(): {@header foo: string, @statusCode _: 204};,> op c1(): {@header foo: string, ...NoContent}; // or spread common model,>

@typespec/http - breaking ✏️

@body means this is the body,> ,> This change makes it that using @body will mean exactly this is the body and everything underneath will be included, including metadata properties. It will log a warning explaining that.,> ,> tsp,> op a1(): {@body _: {@header foo: string, other: string} };,> ^ warning header in a body, it will not be included as a header.,> ,> ,> Solution use @bodyRoot as the goal is only to change where to resolve the body from.,> ,> tsp,> op a1(): {@bodyRoot _: {@header foo: string, other: string} };,>

@typespec/openapi3 - feature ✏️

Add supoort for new @bodyRoot and @body distinction

@typespec/rest - feature ✏️

Add supoort for new @bodyRoot and @body distinction

@typespec/http - breaking ✏️

Properties are not automatically omitted if everything was removed from metadata or visibility,> ,> tsp,> op d1(): {headers: {@header foo: string}}; // body will be {headers: {}},> ,> ,> Solution: use @bodyIgnore,> ,> tsp,> op d1(): {@bodyIgnore headers: {@header foo: string}}; // body will be {headers: {}},>

@azure-sdk
Copy link
Collaborator

You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/2945/

Check the website changes at https://tspwebsitepr.z22.web.core.windows.net/prs/2945/

Copy link
Contributor

@markcowl markcowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor nit

packages/http/src/metadata.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A change that might cause specs or code to break
Projects
None yet
3 participants