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

openapi3: fix schema validation for anyOf, allOf, oneOf operations #850

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

AmadeusK525
Copy link
Contributor

Schemas with XOF children that allowed for nullable values had their validation broken, as the null check happened before the XOF check and would fail immediately, without checking if XOF allowed for such nullability

It can be safely assumed that the value is valid if the 'XOF' operations
were successful
Old validation immediately exited if the value passed was nil and the
schema wasn't nullable. This isn't correct, though, as a schema can have
a nullable type in its XOF operations, and that should result in correct
validation if the input is 'nil'
@fenollp fenollp changed the title fix(schema): fix schema validation for XOF operations openapi3: fix schema validation for anyOf, allOf, oneOf operations Oct 20, 2023
@fenollp fenollp merged commit fa7ca86 into getkin:master Oct 20, 2023
7 checks passed
@fenollp
Copy link
Collaborator

fenollp commented Oct 20, 2023

Thanks for this!

@micronull
Copy link
Contributor

micronull commented Dec 28, 2023

@AmadeusK525 Your edits broke our tests. I can't tell you exactly why yet. Something to do with the run flag.

	if err, run = schema.visitXOFOperations(settings, value); err != nil || !run {
		return
	}

@micronull
Copy link
Contributor

You have all the tests run so the execution process doesn't get to validation.
There are not enough negative test cases in the library.

@AmadeusK525
Copy link
Contributor Author

@micronull could you provide an example of a broken test, please?

@micronull
Copy link
Contributor

@AmadeusK525 Yes. I have plans for a separate issue.

@micronull
Copy link
Contributor

@AmadeusK525 #890

@AmadeusK525
Copy link
Contributor Author

Thank you @micronull, this was a mistake on my part for misunderstanding OpenAPI, it's a super easy fix, I'll open up a PR right now

@AmadeusK525
Copy link
Contributor Author

@micronull Check it out #892
Thanks for the help with the issue :)

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