-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Integrate Hibernate ORM with Hibernate Validator #2753
Conversation
1fb7eca
to
27f4704
Compare
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.
@machi1990 thanks!
I added an important question inline. Could you explain your rationale?
...-orm/deployment/src/test/java/io/quarkus/hibernate/orm/validation/JPAValidationTestCase.java
Outdated
Show resolved
Hide resolved
.initializeBeanMetaData(classesToBeValidated) | ||
.initializeLocales(localesToInitialize) | ||
.beanMetaDataClassNormalizer(new ArcProxyBeanMetaDataClassNormalizer()); | ||
|
||
ValidatorHolder.initialize(configuration.buildValidatorFactory()); | ||
ValidatorHolder.initializeHibernateValidator(hibernateValidatorConfiguration.buildValidatorFactory()); | ||
ValidatorHolder.initializePredefinedScopeValidator(predefinedScopeValidatorConfiguration.buildValidatorFactory()); |
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 understand why you define 2 different validators?
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 because I was getting this
java.lang.IllegalStateException: Defining a Validator-specific traversable resolver is not supported by the predefined scope ValidatorFactory.
when trying to use PredefinedScopeHibernateValidatorFactory
.
Any ideas?
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.
Hmmm, right. So that's something we need to fix in HV. I was a bit too conservative on what would be accepted.
TBH, I didn't think the traversable resolver would be set for each JPA validation call.
Could you change your code to use the initial ValidatorFactory
(well, it will mostly be a matter of reverting changes)? I'll push a PR to HV so that you can test if it works OK for you.
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.
@machi1990 can you check with this branch of Hibernate Validator: hibernate/hibernate-validator#1039 ?
It should solve the issue and you should be able to have only one VF.
I'll wait for your feedback before merging and releasing a new alpha.
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.
@machi1990 can you check with this branch of Hibernate Validator: hibernate/hibernate-validator#1039 ?
It should solve the issue and you should be able to have only one VF.
I'll wait for your feedback before merging and releasing a new alpha.
@gsmet thanks for the quick response. It solves the problem.
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.
@gsmet Hi, any news on the HV release?
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.
Sorry, still in the pipeline. I'm preparing a talk and didn't find the time to do it.
I think I should be able to push the release button this afternoon.
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.
Cool :-)
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.
OK, release done. Now, we have to wait for it to be synced to Central. Hopefully, it will be synced today and I'll push an upgrade PR to fix another issue and prepare for this one.
@gsmet finally, we are ready for a full review :-) |
@machi1990 just a status: I worked on this one yesterday, trying to get the bean manager working. It's a bit more complicated than expected to get things working. I hope I'll get something working before the release. |
Thanks for the update. Hope we get it working before the release.And then I'll be interested in finding out how you did it. |
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 won't have the time to finish the bean manager part before the release and this is already a big improvement so let's merge it.
Thanks @machi1990 ! I'll ping you when I create the bean manager PR. |
@gsmet this follows up our discussion on Zulip chat on how to go about #1889.
Your feedbacks are welcomed as usual :-)
Fixes #1889