-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix JAXB unmarshalling error when compiling to native #37643
Conversation
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.
Thanks for having a look at this. I think we need to be more careful about when we use the hierachy feature: it should only be used on domain classes that will be serialized with JAXB, not on everything that was declared for reflection.
That's why you end up with all these warnings.
You can have a closer look if you want or I can have a look if you prefer, let me know.
In any case, it's nice to have a test case as we will be able to test that everything is fine if we make adjustments.
String className = xmlJavaTypeAdapterInstance.value().asClass().name().toString(); | ||
Type jandexType = Type.create(DotName.createSimple(className), Type.Kind.CLASS); | ||
reflectiveClass.produce(new ReflectiveHierarchyBuildItem.Builder() | ||
.type(jandexType) | ||
.index(index) | ||
.source(getClass().getSimpleName() + " > " + jandexType.name().toString()) | ||
.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.
My guess is that this change should be reverted and that's probably what causes the warnings.
@@ -274,7 +288,7 @@ void ignoreWarnings(BuildProducer<ReflectiveHierarchyIgnoreWarningBuildItem> ign | |||
void registerClasses( | |||
BuildProducer<NativeImageSystemPropertyBuildItem> nativeImageProps, | |||
BuildProducer<ServiceProviderBuildItem> providerItem, | |||
BuildProducer<ReflectiveClassBuildItem> reflectiveClass, | |||
BuildProducer<ReflectiveHierarchyBuildItem> reflectiveClass, |
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.
My guess is that this change should be reverted and that's probably what causes the warnings.
private void addReflectiveClass(BuildProducer<ReflectiveClassBuildItem> reflectiveClass, boolean methods, boolean fields, | ||
String... className) { | ||
reflectiveClass.produce(new ReflectiveClassBuildItem(methods, fields, className)); | ||
private void addReflectiveClass(BuildProducer<ReflectiveHierarchyBuildItem> reflectiveClass, boolean methods, |
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 call sites of this method would have to checked. I'm really not sure it makes sense to replace it in all cases (and might not even be necessary depending on when it's called).
extensions/jaxb/runtime/pom.xml
Outdated
<!-- The entity classes needs to be indexed --> | ||
<plugin> | ||
<groupId>io.smallrye</groupId> | ||
<artifactId>jandex-maven-plugin</artifactId> | ||
<executions> | ||
<execution> | ||
<id>make-index</id> | ||
<goals> | ||
<goal>jandex</goal> | ||
</goals> | ||
</execution> | ||
</executions> | ||
</plugin> |
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.
Why is this needed? The entity classes should be in the app, not in the extension, shouldn't they?
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.
That was an attempt to resolve the jandex issue but I think that's not the way to go indeed.
53c1424
to
9266f7f
Compare
Thanks for your review! I did some changes according to your inputs but I still got the warning, even if now there are only jaxb classes involved. If you don't mind I would not say no to a little help to troubleshoot this ... :D Thanks a lot again! EDIT : Found the issue :) I added a logic to filter out the JAXB classes themselves when looping over annotated classes. This is done so we only use the hierarchybuilditem on domain classes only, excluding JAXB's ones. |
121b1f7
to
3b44b58
Compare
I will have a look later this week. Thanks for pursuing this. |
652785f
to
e8cb980
Compare
@gsmet I think I resolved the issue with the Jandex warning ... Build seems ok now (No specific warnings or errors) so maybe if you have some time could you check on this please ? 😄 The integration test seems to do its job too as it failed (throwing the original issue) when using the main branch codebase and succeed when I use the fix. Edit : Any chance to check this @gsmet ? (happy new year too 😄) |
d982b89
to
239859d
Compare
dff06b9
to
6103261
Compare
39e0f3e
to
cc9c530
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.
@rysurd sorry for the delay, your latest changes look good.
I made some very small changes to make it more consistent with code we already had in the tree but these were really micro changes.
Let's see what CI has to say and merge!
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. You can consult the Develocity build scans. |
Merged, thanks for your contribution! |
Thanks ! |
This change breaks both Quarkus CXF and Camel Quarkus - see apache/camel-quarkus#5650 |
@ppalaga I created https://github.com/quarkusio/quarkus/pull/38217/files that, I think, should mitigate the issues you're having. Let me know how it goes. |
Ah, sorry I should have catched that the extent of these changes would reach also into other libs ... |
#36479 complained about ancestors of a JAXB class not being registered for reflection. The current change does much more, because ReflectiveHierarchyBuildItem does much more: it registers not only ancestors, but also all field types, type parameters, etc. |
Here is my proposal: #38221 |
@ppalaga Seems to work 👍 |
Fix #36479
The root cause was that not all classes annotated with Jaxb annotations were marked for reflection when using native compilation, in case there we have fields that are declared in parent classes. The fix made was to replace the ReflectiveClassBuildItem from JaxbProcessor with ReflectiveHierarchyBuildItem in order to mark inherited fields for native compilation.
There is a warning throw stating that some classes are not indexed via Jandex. If anyone know how to fix this don't hesitate to tell me ! I have added the jandex maven plugins as documented in the Quarkus doc but it changes nothing. For instance :