Skip to content

Conversation

notEthan
Copy link
Contributor

#4590 introduces parameter object with in: querystring, which may not have properties schema, style, explode, or allowReserved. The accompanying schema change only forbids an object with all of those four properties, though. This fixes to forbid any of them.

  • schema changes are included in this pull request
  • schema changes are needed for this pull request but not done yet
  • no schema changes are needed for this pull request

@notEthan notEthan requested review from a team as code owners August 28, 2025 06:13
Copy link
Contributor

@ralfhandl ralfhandl 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 this change is unnecessary. Adding this fail test case with the current schema fails as expected:

openapi: 3.2.0
info:
  title: API
  version: 1.0.0
components:
  parameters:
    querystring-with-schema-not-allowed:
      in: querystring
      name: json
      content:
        application/json:
          schema:
            type: object
      explode: true

It also fails with a single one of the other three keywords not allowed with content.

Side note: schema is not necessary in the not-required list because having both content and schema is already forbidden by the oneOf-required construct in lines 367-371.

I prefer the current style as it is shorter.

Independent of this PR I think we should add a "fail" test case for this exclusion:

@handrews
Copy link
Member

@notEthan @ralfhandl actually the entire not: clause should be removed, and only required: [content] should be kept. Other parts of the schema handle the mutual exclusion of content and the other fields, which is why the test case passes. The not: here is superfluous, so the fact that it is (as @notEthan notes) incorrect doesn't matter.

@notEthan
Copy link
Contributor Author

That example fails due to unevaluatedProperties: false in /$defs/parameter applying to explode. Which is fine and may be sufficient - if so the not+required subschema this PR changes could instead be removed entirely. But as it is, that part is not doing what it was intended to do, assuming it's intended to forbid any of those four properties. required will pass only if all its listed properties are present, and not+required will only fail if all listed properties are present.

@notEthan
Copy link
Contributor Author

oh, @handrews beat me to it while I was typing

… subschema

This subschema would only forbid the presence of all four properties, rather than any of them, but it is superfluous as these properties are caught by `unevaluatedProperties: false`.
@notEthan notEthan force-pushed the oad-schema-querystring-forbid-siblings branch from 40a64d6 to 7bb5630 Compare August 28, 2025 19:31
@notEthan
Copy link
Contributor Author

updated to remove this subschema

@ralfhandl
Copy link
Contributor

ralfhandl commented Aug 28, 2025

@notEthan Please include the test cases from

or equivalents to prove that removing these lines does not break the mutual exclusion.

Or let's wait with this one until #4910 is merged.

Thanks in advance!

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.

3 participants