-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Ignore GraalVM features in HibernateValidatorProcessor #47152
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
Conversation
if (Modifier.isAbstract(implementor.flags())) { | ||
// we can avoid adding the abstract classes here: either they are parent classes | ||
// and they will be dealt with by Hibernate Validator or they are child classes | ||
// without any proper implementation and we can ignore them. | ||
continue; | ||
} | ||
if (isInGraalVMFeature(indexView, implementor)) { |
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 was thinking whether we could say something like:
if (hasAnyConstraints(indexView, implementor)) {
If there are no constraints, there's nothing to cascade into, right? But I don't remember if we need to keep the classes without constraints.. will try to take a look later
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 think:
- if the class is a leaf class
- it doesn't have any constraints
we can skip it.
So I think we could go with your approach... but... I would still try to exclude the Feature
classes as you never know if someone adds a @NotNull
by mistake.
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.
yeah +1 just to be safe
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 let you check that excluding the leaf classes without constraints would be fine?
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.
yes 👍🏻 🙂
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 checked aand that won't fly 🙈:
Validator validator = validator( Set.of( A.class ) );
assertThat( validator.validate( new A() ) ).containsOnlyViolations(
violationOf( NotNull.class ).withProperty( "name" )
);
// fails as we get the `UninitializedBeanMetaData` and hence no constraints ...
assertThat( validator.validate( new B() ) ).containsOnlyViolations(
violationOf( NotNull.class ).withProperty( "name" )
);
If we skip the class, even if it's a leaf with no constraints (or if it's anywhere in the hierarchy for that matter), we will end up having the UninitializedBeanMetaData
for it once someone wants to validate an instance of it and it simply has 0 constraints (the ones from the super classes are there).
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 think we should have started with: WTH is this class in the index? I wouldn't expect it to be.
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 so the problem comes from CXF, which aggressively indexes several jars including: https://github.com/quarkiverse/quarkus-cxf/blob/main/extensions/core/deployment/src/main/java/io/quarkiverse/cxf/deployment/JakartaActivationProcessor.java
I pinged Peter here: https://quarkusio.zulipchat.com/#narrow/channel/187038-dev/topic/CXF.20and.20indexing/with/510702132 .
I will pursue this anyway.
9d8705d
to
2925889
Compare
@marko-bekhta I just pushed an updated version on top of @Ladicek 's work in #47213 . I will rebase and get it out of draft once the Jandex update PR is merged. |
2925889
to
9fe45f2
Compare
This comment has been minimized.
This comment has been minimized.
9fe45f2
to
27b475b
Compare
Status for workflow
|
@marko-bekhta I'm interested in your review! |
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.
looks nice! 🎉
Fixes #47033