microsoft / typespec

https://typespec.io/
MIT License
4.56k stars 223 forks source link

[Bug]: contentType header is missing in response headers #3643

Closed qiaozha closed 5 months ago

qiaozha commented 5 months ago

Describe the bug

content type header is missng in response headers. And I didn't see anything mentioned this kind of removal in the release notes of 0.57.0

here's what the route.response shows in version 0.56.0

image

here's what the route.response shows in version 0.57.0

image

From what TypeGraph shows, the returnType of operation getSchemaById has this contentType header, but it's missing from getHttpOperation.

Reproduction

Playground link

Checklist

lirenhe commented 5 months ago

For languages that would include headers in the response model like JS & GO, this is likely to be a breaking change for the SDK. @allenjzhang, @markcowl, could you help to confirm whether this behavior is by design? This would impact our emitters to move to TypeSpec 0.57. cc @lmazuel for awareness.

weidongxu-microsoft commented 5 months ago

No impact for Java emitter, so far. The response in Java is Response<T> where headers is a dict (so it is runtime, and value from server live response).

It could impact some Javadoc about what user can expect from response. But that definitely not in method signature.

allenjzhang commented 5 months ago

This is by design. Response content-type is express via Produce in Swagger and content in OAS 3.0. See respective linked specs.

This is correctly expressed in swagger playground and OAS3.0 playground.

If you do need to access the contentType values, you can get it via body.contentTypes in string array or their source ModelProperty via contentTypeProperty.

allenjzhang commented 5 months ago

Sorry, the above was commented as for the autorest/OAS3 emitter and not exactly you are referring to. For the getHttpOperation function result , there was some refactoring around httpProperty . This actually fixed a casing bug originally existed.

Now that is fixed, please switch the logic to access the body.contentTypes. If you still want it as part of headers, additional logic will be needed to filter them out in autorest and openapi3 emitters, as I mentioned above. So that would be far less ideal.

bterlson commented 5 months ago

Worth noting explicitly that the presence of "Content-Type" in the headers list is a bug, if it were called "content-type" instead it would have been excluded properly, so the previous code relying on this was still incorrect. This playground shows the bug that existed previously.

FWIW I definitely prefer to just fix the bug and change how emitters are accessing content type. Adding the content type headers to the header collection seems likely to have a lot of downstream impact beyond our own openapi3 and autorest emitters.

qiaozha commented 5 months ago

Thanks @allenjzhang and @bterlson for the investigation and suggestions, I will use body.contentTypeProperty to make it work.

lirenhe commented 5 months ago

Looks we need to apply the same logic for Go cc @tadelesh @jhendrixMSFT