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

Clarify how to model streaming binary data #3729

Merged
merged 3 commits into from Apr 29, 2024

Conversation

handrews
Copy link
Contributor

Specifically, see #1576 (comment) for the TSC decision documented in this PR.

If this is accepted, I will add analogous information to 3.1.1 (after PR #3727 is resolved) and 3.2.0.

@handrews handrews added clarification requests to clarify, but not change, part of the spec media and encoding Issues regarding media type support and how to encode data (outside of query/path params) labels Apr 20, 2024
@handrews handrews added this to the v3.0.4 milestone Apr 20, 2024
@handrews handrews requested a review from a team April 20, 2024 21:33
versions/3.0.4.md Outdated Show resolved Hide resolved
@mikekistler
Copy link
Contributor

I have some concerns with this proposed change.

  • It seems to address only streams of binary data, but streams (as I understand them) are also application for non-binary data (I think SSE is an example).

  • The current change is being made in the "Data Types" section, which seems like the wrong place (to me) to describe how to represent streaming payloads. I think a better place for this information is in the Request Body and Response Body object, or maybe the Media Type object description.

@handrews
Copy link
Contributor Author

handrews commented Apr 23, 2024

@mikekistler This change is only intended to address the binary case because that's the part that the TSC agreed on in June of 2018 as documented in the linked issue. I'm just trying to resolve old issues and adding a previously-approved TSC decision seemed straightforward. You can see that I'm addressing JSON Streaming in PR #3735, and if you want other types of streaming addressed please feel free to open an issue / PR. But at least with binary and JSON streaming, the concerns are very different.

Adding the binary data streaming data to the data types section fits with adding binary handling to data types in general, which was already approved in PR #3187. If we're having a datatypes section about handling binary data, then I feel that the binary streaming should go with it.

@handrews
Copy link
Contributor Author

handrews commented Apr 24, 2024

@mikekistler while I'd rather not move the section that's already there, if you think this new addition should be separate from it entirely, that would be different. I'm also happy to consider moving the section as a separate issue/pr from whether to add this part. (I realize that might not have been clear from how I'd worded it in my previous (now edited) comment).

iox666999

This comment was marked as spam.

Copy link

@iox666999 iox666999 left a comment

Choose a reason for hiding this comment

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

闡明如何對流式二進位資料進行建模

versions/3.0.4.md Outdated Show resolved Hide resolved
Co-authored-by: Ralf Handl <ralf.handl@sap.com>
@handrews
Copy link
Contributor Author

@ralfhandl @mikekistler I added an explicit note about string data, made the whole thing a MAY, and did some other minor wording improvements.

I did look at the Response Object section, as I'm feeling more persuaded by @mikekistler 's argument about placement the more I think about it. But there's really nowhere obvious to put it there. I think we can revisit this if we want to make a more comprehensive statement on streaming responses.

Copy link
Contributor

@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.

Looks good! 👍

@ralfhandl ralfhandl merged commit cd0d290 into OAI:v3.0.4-dev Apr 29, 2024
1 check passed
@handrews handrews deleted the streaming-binary branch May 1, 2024 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification requests to clarify, but not change, part of the spec media and encoding Issues regarding media type support and how to encode data (outside of query/path params)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants