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

feat(spec): codify Encoding in specific requestBody mediatypes #3837

Conversation

jeremyfiel
Copy link
Contributor

the specification indicates

The encoding object SHALL only apply to requestBody objects when the media type is multipart or application/x-www-form-urlencoded.

This PR codifies that behavior in the JSON Schema to enable validation.

Per the Development.md, this change was already merged to the Specification on this PR

the specification indicates
> The encoding object SHALL only apply to requestBody objects when the media type is multipart or application/x-www-form-urlencoded.

Co-authored-by: Fiel, Jeremy <jeremy.fiel@ADP.com>
@handrews
Copy link
Member

Also paging @karenetheridge

Copy link
Member

@handrews handrews left a comment

Choose a reason for hiding this comment

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

I've suggested a change although if you feel strongly about it it's not a hill I care to die on :-)

schemas/v3.1/schema.yaml Show resolved Hide resolved
Comment on lines 786 to 788
patternProperties:
'^application/x-www-form-urlencoded$|^multipart\/.+$':
$ref: '#/definitions/MediaTypeWithEncoding'
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@jeremyfiel jeremyfiel requested a review from handrews May 22, 2024 19:35
@darrelmiller
Copy link
Member

Minor concern that this would prevent */* as a media type. Generally supportive of this addition.

@handrews
Copy link
Member

@OAI/tsc I think what is needed here is a policy on whether field combinations that are said to be ignored or to not apply can be forbidden by the schema or not. That would help us decide a lot of schema design issues, rather than having to re-think each proposed restriction. There are quite a few places in the spec with this kind of language, and we should treat them consistently in the schemas.

I'd be fine with either way the TSC wants to decide this - if it is decided that ignored/non-applicable things can be forbidden in the schema, then I'm fine with this PR being merged.

@@ -763,6 +783,9 @@ definitions:
type: object
additionalProperties:
$ref: '#/definitions/MediaType'
patternProperties:
'\*\/\*|^application/x-www-form-urlencoded$|^multipart\/.+$':
Copy link
Contributor

Choose a reason for hiding this comment

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

Should */* have ^/$ anchors?
Should application/* be matched, as that would apply to application/x-www-form-urlencoded?

Copy link
Contributor Author

@jeremyfiel jeremyfiel Jun 10, 2024

Choose a reason for hiding this comment

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

what is the purpose of the anchors? Why would it be needed?

I see your point on the above. I'll update that

application/* would match any application media type and that's not what we want here., although that would enable quite a few different media types to allow encoding schema. Matching */* was only for backwards compatibility per Darrell's comment.

@jeremyfiel jeremyfiel requested a review from notEthan June 10, 2024 17:16
@mikekistler
Copy link
Contributor

@handrews on this point:

@OAI/tsc I think what is needed here is a policy on whether field combinations that are said to be ignored or to not apply can be forbidden by the schema or not.

My opinion is that if the spec says a field combination must be ignored or not apply, then we can make these forbidden by the schema.

@mikekistler
Copy link
Contributor

Following up here ... what I see in the v3.0.3 spec is:

The encoding object SHALL only apply to requestBody objects when the media type is multipart or application/x-www-form-urlencoded.

Since the spec uses "SHALL" and not "MUST", and my prior comment, I think my view is that we should NOT make this change to the schema, at least for v3.0. Do you agree @jeremyfiel ?

@ralfhandl
Copy link
Contributor

Since the spec uses "SHALL" and not "MUST"

Note that RFC2119 does not differentiate between these two words:

  1. MUST This word, or the terms "REQUIRED" or "SHALL", mean that the
    definition is an absolute requirement of the specification.

We cannot base any decisions on this stylistic choice.

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

@jeremyfiel
Copy link
Contributor Author

Thanks Ralf and Mike for reviewing and your comments.

@handrews
Copy link
Member

@mikekistler MUST be ignored is not the same as forbidden.

I don't think we can forbid anything in the schema on the grounds that it is to be ignored. Those are two different things.

Again, if we want to supply two schemas, one that only constrains what is required by the OAS, and one that has additional constraints that help avoid pitfalls, I think that would be a great idea.

But I think it is incorrect to over-constrain the schema if it is the only schema. (I realize this is a bit different from what I said earlier, but there are a whole bunch of "MUST be ignored" in the OAS, and if we start changing the schema based on that, that is a huge amount of change, and I think an incorrect change).

Copy link
Member

@handrews handrews left a comment

Choose a reason for hiding this comment

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

Regrettably, I don't think we can forbid things that are mandated to be ignored. Not if we only have one schema. I would support a more "helpful" schema that added more constraints, though.

@ralfhandl ralfhandl marked this pull request as draft August 27, 2024 11:58
@handrews
Copy link
Member

handrews commented Sep 5, 2024

Based on today's TDC call, I'm going to close this for the same reasons as #4069 and #4070. Technically, the encoding field is allowed but ignored in these circumstances, so it cannot be forbidden in the validation schema. But as @lornajane noted when closing those PRs, there is likely to be a home for this in some sort of separate linting schema or schemas:

Please keep an eye on the project for more discussion about linting schemas in addition to our validation schemas.

@handrews handrews closed this Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants