-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
Feat/jooby hibernate validator #3508
Feat/jooby hibernate validator #3508
Conversation
# Conflicts: # modules/jooby-hbv/pom.xml # modules/jooby-hbv/src/main/java/io/jooby/hbv/HbvModule.java
I'm not sure I like the name of jooby-validation if it is going to have a hard dependency on jakarta.validation. Is the plan to make the validation pluggable in that module and it is just jakarta for now? Otherwise I think it should be jooby-jakarta-validation. Also the jooby-apt module should not have any dependency on basically anything ideally. I haven't code dived to see why you made it that way. (you don't need the actual classes you depend on with generated code) EDIT actually maybe you changed it. When you get a chance @kliushnichenko maybe squash the commits to one or two? |
cleanup cleanup cleanup
5f1dd9b
to
646f355
Compare
The way I've done it with other libs I've worked was to have an extensible Validator interface which clients can extend to provide custom validation. Example hibernate impl and avaje validation impl . Perhaps you guys could try something similar if you want to eliminate the dependencies |
@SentryMan thank you. I feel here isn't necessary to add another abstraction over the jakarta one. Leaving jakarta-validator outside of core is enough. |
Updated. Pls verify the changes and I'll proceed with the documentation |
@SentryMan could you pls shed some light on Is it a trade-off of a reflection-free solution or maybe you can provide us jakarta-compliant entry point in the future release? |
Jakarta validation is mainly reflection-based so we couldn't fully implement its interfaces. Hence why we took a page from spring in our http solution by defining another abstraction so we could support both jakarta and avaje validation |
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.
looking good
...s/jooby-jakarta-validation/src/main/java/io/jooby/validation/ConstraintViolationHandler.java
Outdated
Show resolved
Hide resolved
public class PasswordsShouldMatchValidator implements ConstraintValidator<PasswordsShouldMatch, NewAccountRequest> { | ||
|
||
@Override | ||
public boolean isValid(NewAccountRequest request, ConstraintValidatorContext constraintContext) { |
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.
Is there a way to access to jooby registry from ConstraintValidatorContext
?????
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 don't see how we can put it into ConstraintValidatorContext
, but there are other options for accessing the registry. It is possible by implementing a custom ConstraintValidatorFactory
.
I don't think we need to support it out of the box bc it most likely will require introducing another custom abstraction.
I can add an example in the docs of how to do this. Maybe it is good enough for now?
Additionally, if the project utilizes some DI it is not a problem to inject any service/repo/etc. into custom validator
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.
Additionally, if the project utilizes some DI it is not a problem to inject any service/repo/etc. into custom validator
An example of that on docs will be nice.
Getting back to As SentryMan clarified, there is no way to smoothly support both (hbv/avaje) relying solely on Options that are running through my mind in this regard:
@jknack what's your take on it? maybe some other options? |
Seems 3 it is OK. jooby-apt should call io.jooby.validation.BeanValidator and extra logic goes here (jooby-jakarta-validation) and probably on the jooby-avaje-validator module. So jooby-apt will do what is doing here, wrap the call around BeanValidator |
Updated. Please note the following:
Naming is getting tough with a lot of |
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.
few minor comments
"Unable to load 'BeanValidator' class. " + | ||
"Bean validation usage (@Valid) was detected, but the appropriate dependency is missing. " + | ||
"Please ensure that you have added the corresponding validation dependency " + | ||
"(e.g., jooby-hbv)."); |
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.
typo: jooby-hibernate-validator
@@ -0,0 +1,6 @@ | |||
package io.jooby.validation; | |||
|
|||
public interface MvcValidator<E extends Throwable> { |
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.
probably make E RuntimeException
|
||
public interface MvcValidator<E extends Throwable> { | ||
|
||
void validate(Object bean) throws E; |
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.
well not sure if we need a generic for exception, RuntimeExceptoion or Exception will be OK
minor improvements, javadoc, asciidoc has been added. Ready to final review/merge. |
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.
@kliushnichenko love what you did! Let me know if we can merge.
Yes, we can. Just a minor thing, master and my branch are under |
oh yea, let me push the version bump |
@kliushnichenko done |
@jknack updated, feel free to merge |
Still need some docs, tests, cleanup, but the basic logic seems done