-
Notifications
You must be signed in to change notification settings - Fork 61
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
Added support for inheritance hierarchies validation #369
base: develop
Are you sure you want to change the base?
Added support for inheritance hierarchies validation #369
Conversation
b7d42de
to
15f8692
Compare
15f8692
to
b19615d
Compare
public <C extends T> ValidatorBuilder<T> constraintOnClass(Class<C> clazz, | ||
Validator<C> cValidator) { | ||
Validator<T> TValidator = new ValidatorBuilder<T>() | ||
.nest(clazz::cast, clazz.getName(), cValidator).build(); |
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.
target name cannot be determined automatically.
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.
I'm a bit puzzled, actually, since I cant think of a target name that makes sense other than the class name.
that is because unlike regular .nest() usage, I am not validating a nested field, but rather the whole child class.
perhaps using nest() is some sort of an abuse here, given that I use it as work around to cast my entity, not access a nested field.
I'll see if I can come up with a different approach.
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.
ended up creating a new Validatable class dedicated to this case called InheritanceValidator, would love to hear your thoughts
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.
I meant that the target name should be a parameter.
I don't think InheritanceValidator
is needed.
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.
Understood, I appreciate your review by the way. I find Yavi's design and philosophy thoroughly engaging.
I agree that the target name could have been a parameter, my concern arises from utilizing nest() for non-nested fields, and determining the appropriate target name for a child class.
what do you think?
Function<CharSequenceConstraint<Admin, String>, CharSequenceConstraint<Admin, String>> startsWithAdmin = ( | ||
CharSequenceConstraint<Admin, String> c) -> c.startsWith("yavi"); | ||
|
||
return Stream |
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.
The test case is difficult to understand.
Please add the simple example in the pull request to the test case.
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.
sure, no problem.
|
||
/** | ||
* @since 0.14.0 | ||
*/ |
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.
These methods are quite advanced. Can you please add to the JavaDoc what use cases it is useful for and some sample code?
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.
absolutely
User validUser = new User("Rawad", "rawad@gmail", 25); | ||
User invalidUser = new User("Almog", "almog@gmail", 19); | ||
|
||
assertThat(validator.validate(validUser).isValid()).isTrue(); |
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.
It is not enough to simply verify the results of validation. Please also verify the contents of the constraints (name and messageKey).
1390541
to
63fc402
Compare
63fc402
to
4c5ccff
Compare
added constraintOnClass api to resolve issue: #278
this provides convenient API to create a parent class validator that can also validate different inherited classes extending it.
example:
given a class called Child that extends Parent, one could utilize constraintOnClass in the following way: