-
Notifications
You must be signed in to change notification settings - Fork 9.1k
v3.2: Support ordered multipart including streaming #4589
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
base: v3.2-dev
Are you sure you want to change the base?
Conversation
Thanks @handrews for taking this on. I'm really happy to see it coming to fruition and hopefully the tooling catches up with it sooner than later. I couldn't immediately make out if this would support nested multipart.
multipart/mixed:
schema:
prefixItems:
- type: object
properties:
data:
type: string
- prefixItems:
- type: object
properties:
more_data: ""
- {}
- {}
- {}
prefixEncoding:
- {}
- contentType: multipart/mixed
# not sure how to further document a nested structure here. |
@jeremyfiel aww... I was hoping no one would bring up nested multipart... 😵💫 I think it would be hard to do that, because there isn't anywhere to put the nested Encoding Object. I think we'd have to add Alternatively, we could recommend trying that as an extension given that it adds significant complexity and is a rare case that is deprecated by the current RFC (I know that's small consolation when you're the "rare case" and built things in good faith using older RFCs when they were current). The complexity is not just the recursive structure, but also that you are now correlating two separate trees of structure. |
I'm not entirely sure this is a correct statement to include
|
[EDIT: This goes with the nested multipart discussion] @jeremyfiel the problem is that instead of just re-using the Media Type Object, we came up with the |
I totally understand the complexity, just trying to confirm my initial impression. |
@jeremyfiel That statement only says that some of the listed types are not registered. I decided not to get into the preamble and postamble of |
@jeremyfiel I added some clarifications about the envelope/preamble/epilogue and the lack of nesting support. |
This adds support for all `multipart` media types that do not have named parts, including support for streaming such media types. Note that `multipart/mixed` defines the basic processing rules for all `multipart` types, and implementations that encounter unrecognized `multipart` subtypes are required to process them as `multipart/mixed`. Therefore support for `multipart/mixed` addresses all other subtypes to some degree. This builds on the recent support for sequential media types: * `multipart/mixed` and similar meet the definition for a sequential media type, requiring it to be modeled as an array. This does use an expansive definition of "repeating the same structure", where the structure is literally any content with a media type. * As a sequential media type, it also supports `itemSchema` * Adding a parallel `itemEncoding` is the obvious solution to `multipart/mixed` streams requiring an Encoding Object * We have regularly received requests to support truly mixed `multipart/mixed` payloads, and previously claimed such support from 3.0.0 onwards, without actually supporting it. Adding `prefixEncoding` along with `itemEncoding` supports this use case with a clear parallel to `prefixItems`, which is the schema construct needed to support this case. * There is no need for a `prefixSchema` field because the streaming use case requires a repetition of the same schema for each item. Therefore all mixed use cases can use `schema` and `prefixItems`
This force-push was just a plain re-base with no conflicts or other changes. Exactly the same commits applied, I just wanted to make sure the other big PRs wouldn't cause merge issues. @jeremyfiel GitHub won't let me request a review from you, but if you could provide an approval when you are satisfied with the PR it would be much appreciated as you probably have more expertise with this than just about anyone else. @thecheatah if you are able to review, even just for the streaming support part, that would also be greatly appreciated. I did not use |
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.
Looks good. This change allows us to describe the multipart/mixed streaming use case. Thanks!
src/oas.md
Outdated
The fourth repeats `application/geo+json`-structured values, while the last repeats a custom text format related to Server-Sent Events. | ||
The fourth repeats `application/geo+json`-structured values, while `text/event-stream` repeats a custom text format related to Server-Sent Events. | ||
The final media type listed above, `multipart/mixed`, provides an ordered list of documents of any media type, and is sometimes streamed. | ||
Note that while `multipart` formats technically allow a preamble and an epilogue, the RFC directs that they are to be ignored, making the effectively comments, and this specification does not model them. |
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.
making them effectively
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.
@thecheatah thanks, just added a commit to fix this.
Thanks to @thecheatah for catching this.
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.
LGTM!
Co-authored-by: Jeremy Fiel <[email protected]>
Fixes:
multipart/mixed
(and possibly bettermultipart/*
support in general) #3721 (multipart/mixed
in general)multipart/byteranges
for 206 responses #3725 (multipart/byteranges
)application/json
withmultipart/mixed
)This adds support for all
multipart
media types that do not have named parts, including support for streaming such media types. Note thatmultipart/mixed
defines the basic processing rules for allmultipart
types, and implementations that encounter unrecognizedmultipart
subtypes are required to process them asmultipart/mixed
. Therefore support formultipart/mixed
addresses all other subtypes to some degree.This builds on the recent support for sequential media types:
multipart/mixed
and similar meet the definition for a sequential media type, requiring it to be modeled as an array. This does use an expansive definition of "repeating the same structure", where the structure is literally any content with a media type.itemSchema
itemEncoding
is the obvious solution tomultipart/mixed
streams requiring an Encoding Objectmultipart/mixed
payloads, and previously claimed such support from 3.0.0 onwards, without actually supporting it. AddingprefixEncoding
along withitemEncoding
supports this use case with a clear parallel toprefixItems
, which is the schema construct needed to support this case.prefixSchema
field because the streaming use case requires a repetition of the same schema for each item. Therefore all mixed use cases can useschema
andprefixItems
We do not seem to run tests on the 3.2 schemas, and I couldn't quickly figure out how to add that, so we should do that separately and include coverage for this and other new fields.
Also paging @thecheatah, @jeremyfiel