-
Notifications
You must be signed in to change notification settings - Fork 76
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
Upgrade typespec dep version to v0.56 in May #2503
Conversation
packages/typespec-test/test/contoso/generated/typespec-ts/src/outputModels.ts
Outdated
Show resolved
Hide resolved
packages/typespec-test/test/anomalyDetector/generated/typespec-ts/src/rest/responses.ts
Show resolved
Hide resolved
packages/typespec-test/test/eventgrid_modular/generated/typespec-ts/review/eventgrid.api.md
Show resolved
Hide resolved
…/autorest.typescript into upgrade-typespec-dep-in-may
packages/typespec-test/test/authoring/generated/openapi/2022-05-15-preview/openapi.json
Show resolved
Hide resolved
packages/typespec-test/test/eventgrid_modular/generated/typespec-ts/review/eventgrid.api.md
Show resolved
Hide resolved
packages/typespec-test/test/openai/generated/openapi/2023-07-01-preview/openapi.json
Outdated
Show resolved
Hide resolved
@@ -28,9 +28,9 @@ model ReturnResponse { | |||
namespace NicepayPlatform { | |||
@post | |||
@route("/request-union-body") | |||
op requestUnionBody(@body request: RequestRegisterCC | RequestRegisterVA): {}; | |||
op requestUnionBody(@body request: RequestRegisterCC | RequestRegisterVA): {@bodyRoot _ : {};}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does the original {} gives us? void?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void, see more microsoft/typespec#2945.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shoudl be @body
here if you want actually {}
, that is actually I think a bug, @bodyRoot
should just move the body resolution down to that property instead of the root of the response type and not change the meaning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filed microsoft/typespec#3372
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use {@bodyRoot _: void }
to express void body? I feel like it's strange if both {@bodyRoot _: void }
and {@bodyRoot _: {} }
means response body void?
Also, I am curious if we can use bodyRoot to express non-object and non-void type body, like string, boolean etc with {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is as follow
@bodyRoot
should just move the implicit body resolution from the root of the request/response to the property you have it. So it should be exactly the same handling to haveop foo(): X
vsop foo(): {@bodyRoot _: X}
as the over the write meaning.- Now the reason why
{}
means no body is because{@header foo: string}
means no body but{}
didn't use to. If a property was removed with visibility it wasn't meaning empty body either unless it was marked with@header
as well. So it was very inconsitent. So to simplify the logic is: after removing the non applicable properties if there is none left it means we have no body. In the rare case where you actually want to send{}
you can use@body
which will expect it to be the exact body(ignore any metadata attributes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do understand the intention for bodyRoot as I read about this before when I was working on visibility related design spec, I was just feel like since void can't be used in property type, maybe we should not allow {@bodyRoot _: void }
too?
@@ -178,7 +178,7 @@ namespace Azure.Messaging.EventGrid { | |||
@doc("Single Cloud Event being published.") | |||
@body | |||
event: {event: CloudEvent}; | |||
}, PublishResult>; | |||
}, { @body _: PublishResult; }>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timotheeguerin I believe here we should use @body
not @bodyRoot
because the PublishResult is an empty model.
But in spec repo it is bodyRoot yet: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/eventgrid/Azure.Messaging.EventGrid/main.tsp#L250
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll fix it in the spec repo when the issue I linked above is resolved(or earlier).
FIxing the issue will create a diff in the autorest output so it won't go unoticed and we'll have to change
fixes #2515