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

Fix binary request & response openapi #125

Merged
merged 7 commits into from
Jan 5, 2022

Conversation

timotheeguerin
Copy link
Member

@timotheeguerin timotheeguerin commented Dec 10, 2021

resolve openapi3 part of #124

If request or body is bytes and content type is;

  • application/json => format: bytes
  • text/plain => format: bytes
  • others => format: binary

@timotheeguerin timotheeguerin changed the title Fix binary response openapi Fix binary request & response openapi Dec 10, 2021
Copy link
Member

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

I think there is a little too much "magic" (or maybe it is "inconsistent magic") in this design, if I understand it correctly.

My understanding is that we will special case just one return type -- bytes -- and interpret it to mean content-type: application/octet-stream (by default)
and type: string, format: binary (instead of format: bytes).

In contrast, return type of string is not taken to mean content-type: text/plain by default -- the user must specify it explicitly.

I think it would be clearer and simpler to require an explicit content-type whenever it should be different from application/json.

Regarding format: bytes vs format: binary, I recommend a simple rule based on content-type (either explicit or default). Like

  • bytes has format: bytes iff content-type is application/json, else format: binary, or
  • bytes has format: binary iff content-type is application/octet-stream, else format: bytes

This replaces "special cases" with clear and simple rules.

Thoughts?

@timotheeguerin
Copy link
Member Author

timotheeguerin commented Dec 13, 2021

@mikekistler, yes it is a fair point. That might be better to do that to start with.
For the option to go I would do #1 otherwise, you can't really specify binary with image/png or other content types.

@timotheeguerin
Copy link
Member Author

@mikekistler actually that might not work. There could be case where the user would want to have application/json be sent as bytes (a raw json file with no schema) if we keep defaulting application/json to bytes that would not work.

But we do have to be able to keep representing type string, format: bytes, so might need more design

@mikekistler
Copy link
Member

@timotheeguerin I think you are right that Option 1 is probably best. As for sending json data as bytes (raw), I think that is probably best described as content-type = "application/octet-stream". I have always taken this to mean "the application will interpret the data", which may not be the precise definition but I think is how it is commonly used.

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.

I think we need to update the language tuturoail and add in an actual swagger example with both binary and json schema types, but this can be done in later PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants