search

microsoft / typespec

https://typespec.io/
MIT License
4.13k stars 199 forks source link

Operation BodyMediaType is not set correctly. #4208

Open m-nash opened 1 month ago

m-nash commented 1 month ago

For this cadl ranch spec we can see that operation sendAsJson is marked with application/json yet the RequestBodyMediaType comes out as Text instead of Json.

m-nash commented 1 month ago

from @archerzz

Looks like it's a legacy problem. I search the codes, and we have two properties regarding media types in InputOperation :

RequestBodyMediaType: It's determined by the request body parameter type https://github.com/microsoft/typespec/blob/2d4f55f92160c22af19004e9e1aa813caeec46c6/packages/http-client-csharp/emitter/src/lib/operation-converter.ts#L92 It's not used in MGC. In autorest.csharp, it's used by CmcRestClientBuilder and RestClientBuilder RequestMediaTypes: it's determined by scanning the operation, which I think is more accurate https://github.com/microsoft/typespec/blob/2d4f55f92160c22af19004e9e1aa813caeec46c6/packages/http-client-csharp/emitter/src/lib/operation-converter.ts#L96 It's not used in MGC either (probably MGC hasn't implement the various media type support?) In autoest.csharp: it's used by OperationMethodChainBuilder when setting the request Content-Type header: https://github.com/Azure/autorest.csharp/blob/a19bafb7ff7418930deb75b1df40ccbf55825228/src/AutoRest.CSharp/LowLevel/Output/OperationMethodChainBuilder.cs#L557 it's used by ConvenienceMethod when determine how to serialize the input body: https://github.com/Azure/autorest.csharp/blob/a19bafb7ff7418930deb75b1df40ccbf55825228/src/AutoRest.CSharp/LowLevel/Output/ConvenienceMethod.cs#L36 So that's why DPG is not impacted. It correctly consumes only RequestMediaTypes. All others are using RequestBodyMediaType.

m-nash commented 1 month ago

https://github.com/microsoft/typespec/issues/4215 follow up to remove dupe

m-nash commented 1 month ago

@archerzz this fixed it partially, but for this operation, the RequestMediaTypes is null so we fail that check that looks for text/plain.

You can repro by running this test https://github.com/Azure/cadl-ranch/blob/main/packages/cadl-ranch-specs/http/payload/media-type/main.tsp#L32-L35

archerzz commented 1 month ago

@archerzz this fixed it partially, but for this operation, the RequestMediaTypes is null so we fail that check that looks for text/plain.

You can repro by running this test Azure/cadl-ranch@main/packages/cadl-ranch-specs/http/payload/media-type/main.tsp#L32-L35

@m-nash That's another legacy issue. We always hard-code the response media type to json, see https://github.com/microsoft/typespec/blob/d9faa2ac04b571e5701888ac60e4fd10e6af33f9/packages/http-client-csharp/emitter/src/lib/operation-converter.ts#L255 I traced back to the original autorest.csharp emitter, and I think the hard-coded value has existed for quite a long time: https://github.com/Azure/autorest.csharp/blob/0fb10e0dec33253e9db8b7fae05d311abf9f76ed/src/TypeSpec.Extension/Emitter.Csharp/src/lib/operation.ts#L390

So, I think we need to remove not only InputOperation.RequestBodyMediaType, but the BodyMediaType enum and corresponding reference as well.

I filed https://github.com/microsoft/typespec/issues/4225 to track that.