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: support default value binding with allOfSchema #885

Merged
merged 5 commits into from
Dec 9, 2023

Conversation

podhmo
Copy link
Contributor

@podhmo podhmo commented Dec 8, 2023

📝 nested complex structure is not supported with this PR

@podhmo
Copy link
Contributor Author

podhmo commented Dec 8, 2023

need tests?

Copy link
Collaborator

@fenollp fenollp left a comment

Choose a reason for hiding this comment

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

need tests?

Well, yeah. Please think proactively about this free software thing.
Here it's clear you should at the very least add a test for the behavior you're changing, so add a func TestIssue884 in openapi3filter/issue884_test.go that makes sure a default can be extracted from the allof schema you mention elsewhere.

I'd also add a test that shows the break behavior, i.e.: the first non-nil default of allofs is the one picked.

Thanks for the patch :)

openapi3filter/validate_request.go Outdated Show resolved Hide resolved
@podhmo
Copy link
Contributor Author

podhmo commented Dec 9, 2023

I added tests

- in: query
name: withManyDefaults
schema:
allOf:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this definition should be schema validation error ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unable to find in the spec à justification to "forbid multiple schema defaults". But if you find one then yes let's impl this validation pass.

Otherwise let's continue to not return a validation error here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is not required according to the specifications.

@fenollp
Copy link
Collaborator

fenollp commented Dec 9, 2023

LGTM. If you feel this in a good state as is I'll merge.

@podhmo
Copy link
Contributor Author

podhmo commented Dec 9, 2023

Yes, it's ready 🚀

@fenollp fenollp merged commit 62bf0f7 into getkin:master Dec 9, 2023
7 checks passed
@podhmo podhmo deleted the fix-884 branch December 12, 2023 03:49
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.

binding parameter default value with allOf schema
2 participants