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

fix: Validate intersection with multiple unions #1501

Merged

Conversation

connor-g-swyftx
Copy link
Contributor

@connor-g-swyftx connor-g-swyftx commented Oct 26, 2023

This fixes a bug in validation of intersections with multiple unions. If the intersection includes at least three unions, only the first schema per union is accepted. One of the unions must have an overlapping required property to hit this bug.

I have written a failing test in f453161, and fixed it in f5f5fdb.

There has been similar work in the past on intersection validation in #888.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you written unit tests?
  • Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
  • This PR is associated with an existing issue?

Closing issues

Put closes #XXXX (where XXXX is the issue number) in your comment to auto-close the issue that your PR fixes.

Test plan

I've written parameterized tests to cover all valid schema combinations, and parameterized tests for invalid inputs covering invalid dataypes, missing required properties, and excess properties.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there connor-g-swyftx 👋

Thank you and congrats 🎉 for opening your first PR on this project.✨

We will review the following PR soon! 👀

Comment on lines -637 to -639
let currentCollector = { ...modelSchemass[outerIndex][0] };
for (let innerIndex = outerIndex + 1; innerIndex < modelSchemass.length; innerIndex++) {
currentCollector = { ...this.intersectRefObjectModelSchemas([currentCollector], modelSchemass[innerIndex])[0] };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bug was caused on lines 637 and 639, using the first subschema from each schema of the intersection

@WoH
Copy link
Collaborator

WoH commented Oct 30, 2023

Can you slap a return type onto buildContext (any/unknown is fine) to get the CI result?
LGTM, but I want the CI to confirm

@connor-g-swyftx
Copy link
Contributor Author

@WoH done, thanks

@WoH WoH merged commit 26e88bd into lukeautry:master Nov 2, 2023
12 checks passed
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.

None yet

2 participants