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: allow nullable input objects and arrays #67

Merged
merged 3 commits into from
Oct 12, 2024

Conversation

peterc1731
Copy link
Contributor

@peterc1731 peterc1731 commented Mar 2, 2023

Fixes #57

When inferring types from the schema, the library does not consider nullability for objects or arrays. The test that validates nullable input types ("should support nullable input type arguments") was only validating string type inputs, when adding validation for a few more input types as I've done in this PR, that test fails.

I've applied a fix for the issue as well as updating the tests, I'm not sure if the approach is the best one, but it works and its a simple change with low impact on the rest of the behaviour.

@jonnydgreen
Copy link
Collaborator

jonnydgreen commented Mar 2, 2023

Thanks for submitting - I'll take a look tonight :)

@jonnydgreen jonnydgreen added the bug Something isn't working label Mar 2, 2023
Copy link
Collaborator

@jonnydgreen jonnydgreen left a comment

Choose a reason for hiding this comment

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

Thanks - just one comment about the object side of things!

lib/validators/json-schema-validator.js Show resolved Hide resolved
@bglgwyng
Copy link

@peterc1731 Thanks for addressing this fix!
I've seen the significance of this issue, and I've found that this solution works accurately. Is there any reason to hold back from merging this pull request?

@mcollina
Copy link
Contributor

@peterc1731 are you still interested in addressing @jonnydgreen comment?
@bglgwyng maybe would you like to take over?

@peterc1731
Copy link
Contributor Author

@peterc1731 are you still interested in addressing @jonnydgreen comment?

Apologies, completely forgot to get back to this one! I will take a look today.

Copy link
Contributor

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@jonnydgreen jonnydgreen left a comment

Choose a reason for hiding this comment

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

LGTM!

@jonnydgreen
Copy link
Collaborator

@mcollina thinking a semver patch release for this one?

@mcollina
Copy link
Contributor

yes

@jonnydgreen jonnydgreen merged commit 773e505 into mercurius-js:main Oct 12, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array or null validation error
5 participants