-
Notifications
You must be signed in to change notification settings - Fork 34
Feature/support list element validation for all constraints #48
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
Feature/support list element validation for all constraints #48
Conversation
@bbakerman if you want to have a look at this. Sorry about it, it's quite large, but I think this is a good addition 😉. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prove to me why the null value hanlding code (which returns true) is sensible to remove.
You have removed it all over the place. Why is that ok?
src/main/java/graphql/validation/constraints/standard/AbstractDecimalMinMaxConstraint.java
Show resolved
Hide resolved
src/main/java/graphql/validation/constraints/standard/AbstractMinMaxConstraint.java
Show resolved
Hide resolved
src/main/java/graphql/validation/constraints/standard/PatternConstraint.java
Show resolved
Hide resolved
I thank you for this PR - its extensive to say the least. Smaller PRs usually have more success in open source terms. I am musing on the breaking change here where @SiZe no longer does lists. I still need to be convinced this is a good idea. Also the null handling as outlined above |
I agree, I rarely do big PRs in open source. I tried to do too much at once. If it's better for you, I can probably split it in 2 or 3 PRs. Regarding the change of size, here's the issue I wanted to fix. When you have a schema like this. field( arg : [ String ] @Size(min: 2)) : ID It would have been possible to implement Now, what I proposed is field( arg : [ String ] @Size(min: 2) @ContainerSize(min : 5)) : ID Which would say Now maybe this change was not required. It may be an edge case, but it felt important to handle something like this. Or maybe there is another way I haven't seen? |
@bbakerman to come back on this, what to do you think about my justification? If you don't like it, what do you propose? And do you wish me to split this PR in smaller chunks? |
I have had no update with this for a while... I'm currently facing the issue that I have to validate both the container size and the string size. I created another constraint just for that as a work-around, but it would be really great is something like this could be integrated in the library... Does anyone have a better solution if the proposed one is not okay with you? |
Sorry for the delay on this PR - I have not been able to put the time towards this repo that it deserves. I am happy to accept this PR and have the change in behavior Thank you for such an extensive PR with great quality |
Thanks a lot! |
Basically, every directive now has to specify if it supports list elements. If yes, instead of receiving the list of elements as an arguments, it receives each element individually. Which makes the validations easier to write, and gives better errors (specific index instead of global list error).
The only thing with that solution is that two constraints didn't work well like that.
@NotEmpty
and@Size
could work for both strings and lists. In order to avoid confusion and make it possible to support both constraints on an input, I added two new constraints.@ContainerSize
@ContainerNotEmpty
which will only perform the validations on list inputs, while
@Size
@NotEmpty
were modified to work only with strings, and IDs
I also simplified a little bit the directive code to reduce dupplication.
Resolves: #31