-
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 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!
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!
@@ -1661,6 +1686,8 @@ Determining how to handle a `type` value of `null` depends on how `null` values | |||
If `null` values are entirely omitted, then the `contentType` is irrelevant. | |||
See [Appendix B](#appendix-b-data-type-conversion) for a discussion of data type conversion options. | |||
|
|||
It is not currently possible to model nested `multipart` media types. |
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.
Pity, my only multipart use case is that the multipart request consists of parts that are either of
- media type
application/http
, or - media type
multipart/mixed
with parts of media typeapplication/http
,
I had assumed I can model that with an itemSchema
that is oneOf
an array of HTTP requests or a single HTTP request, but the corresponding Encoding Object may be difficult.
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.
@ralfhandl It's telling that the two people to give the most comprehensive reviews of this PR to date both need nested multipart
. I do have thoughts on how to do this, and would be more than happy to consider it part of this release-blocking functionality for 3.2. It would involve adding encoding
, prefixEncoding
, and itemEncoding
to the Encoding Object, but there are challenging edge cases and I don't want to drag this PR down with them. I'd rather get this merged, submit a follow-on PR (which is impractical without merging this first), and then we can decide if the follow-on works.
Regarding application/http
, the problem (to me) is not "how to use an Encoding Object with application/http
" but "how to model application/http
at all." Can you point me to how you would model an application/http
payload if it were not in a multipart
part? I realize that would be a rather strange payload outside of multipart
, but please humor me- feel free to open a new discussion on it if there's no clear answer. If we know how to model it in general, we can look at how to model it as a multipart
part. Although that might have to wait for 3.3.
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.
@ralfhandl is my understanding you have either a single application/http
payload or a multipart/mixed
payload? But the multipart/mixed does not have a nested multipart document?
GET /pages HTTP/1.1
accept: application/http
GET /page/{page-id} HTTP/1.1
accept: application/json
{
"pages": [{}, {}]
}
POST /pages HTTP/1.1
content-type: multipart/mixed; boundary='my-boundary'
--my-boundary
content-type: application/http
GET /page/{page-id} HTTP/1.1
accept: application/json
{
"pages": {{}, {}]
}
--my-boundary
content-type: application/http
PATCH /pages/{page-id} HTTP/1.1
content-type: application/json
{
"pageTitle": "a new title"
}
--my-boundary
content-type: text/plain
this is a text file describing the two different http messages included in this request
--my-boundary--
if the multipart example is true, you can define it with OAS 3.2.x like this:
openapi: 3.2.0
info:
title: multipart application/http
version: 0.0.1
paths:
'/pages':
summary: a list of http pages
requestBody:
content:
'multipart/mixed':
schema:
prefixItems: // these can be in any order you define
- type: string
- type: string
- type: string
prefixEncoding: // these can be in any order you define
- contentType: application/http
- contentType: application/http
- contentType: text/plain
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.
my example shows mixed GET and PATCH, this appears to be forbidden by RFC9112
but the OAS definition still applies.
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.
@jeremyfiel I think @ralfhandl is talking about OData's multipart
batch format.
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.
ah ok. i see it has two distinct $batch options, which he eluded to.
the second is indeed a nested multipart. That's a bummer
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.
@jeremyfiel I think I have figured out how to support nested multipart
... we'll go through this PR on Thursday's call, and if there's time I can introduce the nested proposal.
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.
[EDIT: This is not as bad as I thought, see next comment]
@jeremyfiel @ralfhandl I'm realizing that there is a substantial limitation with ordered multipart
where something like the OData batch format (ignoring the nested-ness for now) can have arbitrarily many parts of a few types, where the types can appear in any order. Or even in a fixed order but not a fixed number of each media type.
Another example would be an RFC2557 web page archive, which might want to allow any number of HTML, CSS, JavaScript and/or image parts. What we could do is allow contentMediaType
to be used to specify this. We say that contentMediaType
SHALL be ignored if it conflicts with contentType
(even the implicit default value of contentType
), but we do not say anything about contentMediaType
narrowing the media type from contentType
. Which, if we set to */*
, would effectively delegate to contentMediaType
. It would look like this:
multipart/related:
schema:
items:
anyOf:
- type: string
contentMediaType: text/html
- type: string
contentMediaType: text/css
- type: string
contentMediaType: text/javascript
- contentMediaType: image/*
itemEncoding:
contentType: */*
headers:
ContentType:
$ref: '#/components/headers/requiredContentType'
The only wrinkle here is that now implementations would have to detect patterns like this anyOf
, at least if (as with the type
detection rules included in this PR) they are reasonably find-able from the relevant Schema Object.
This seems like a big limitation that we ought to address, and a plausible way to address it. But how reasonable is it? Could we include it and put some boundaries around just how much tools are expected to handle? Should we instead add another keyword or mechanism of some sort? Or just let it be for now?
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.
Wait... maybe this isn't as bad as I thought, because contentType
takes a comma-separated list. So you could do:
multipart/related:
schema:
items:
anyOf:
- type: string
- $comment: Must allow anything to allow binary
itemEncoding:
contentType: text/html, text/css, text/javascript, image/*
headers:
ContentType:
$ref: '#/components/headers/requiredContentType'
@ralfhandl I have fixed the sentence ordering, and also added a new section, Encoding and This is not realistic, so I think that setting some boundaries on whatis expected is required. I stuck with requiring (MUST) only the most unambiguous scenario, although I considered including the search-order support for multi-valued This, btw, is a prerequisite for supporitng nested |
@ralfhandl oops, push had failed and I hadn't noticed. The section mentioned in my last comment is now actually added, sorry about that. |
@ralfhandl added more tests, merged your suggestions (and yeah "falling back" is probably too idiomatic to be used here) |
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`
Thanks to @thecheatah for catching this.
Co-authored-by: Jeremy Fiel <[email protected]>
Co-authored-by: Jeremy Fiel <[email protected]>
Co-authored-by: Ralf Handl <[email protected]>
Co-authored-by: Ralf Handl <[email protected]>
Also rebased to pick up the test-running changes. No modifications in the rebase force-push |
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