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

Added support for inheritance hierarchies validation #369

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ArielBerkovich
Copy link
Contributor

@ArielBerkovich ArielBerkovich commented Mar 31, 2024

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:

    ValidatorBuilder.<Parent>of()
                // constraints for parent
                .constraint(...)
                .constraintOnClass(Child.class), b -> b
                            // constraints for child
                            .constraint(...)
                            .constraint(...)
                            .constraint(...)
                    )
                )

@ArielBerkovich ArielBerkovich force-pushed the arielber/add-constraint-on-class-api branch 2 times, most recently from b7d42de to 15f8692 Compare March 31, 2024 09:46
@ArielBerkovich ArielBerkovich force-pushed the arielber/add-constraint-on-class-api branch from 15f8692 to b19615d Compare March 31, 2024 10:07
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();
Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Owner

@making making Apr 2, 2024

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.

Copy link
Contributor Author

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
Copy link
Owner

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, no problem.

@making making added enhancement New feature or request feedback required labels Mar 31, 2024

/**
* @since 0.14.0
*/
Copy link
Owner

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?

Copy link
Contributor Author

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();
Copy link
Owner

@making making Apr 1, 2024

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).

@ArielBerkovich ArielBerkovich force-pushed the arielber/add-constraint-on-class-api branch from 1390541 to 63fc402 Compare April 1, 2024 22:18
@ArielBerkovich ArielBerkovich force-pushed the arielber/add-constraint-on-class-api branch from 63fc402 to 4c5ccff Compare April 2, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feedback required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants