RegisterForReflection#classNames does not register full hierarchy of the class #24474

igorregis · 2022-03-22T13:00:16Z

fix #21985

First time PR
Didn't find any specific tests examples for these classes, open to suggestions
Added new javadoc comment for public field created on RegisterForReflection annotation
Didn't feel the need to add new comments, since the changes haven't added nothing much different from previous code state

geoand · 2022-03-29T06:37:10Z

Thanks for this! Let's see what @radcortez thinks (who opened the original issue)

radcortez

Can we also add a test? I recommend looking into https://github.com/quarkusio/quarkus/blob/57f6a9d9cb288cce2b558d793948b8d0e56ea806/integration-tests/opentelemetry-vertx/src/main/java/io/quarkus/it/opentelemetry/vertx/SpanData.java, which was the trigger for the original issue. Ideally, io.opentelemetry.sdk.trace.data.SpanData should be the only class required to be registered.

core/runtime/src/main/java/io/quarkus/runtime/annotations/RegisterForReflection.java

core/deployment/src/main/java/io/quarkus/deployment/steps/RegisterForReflectionBuildStep.java

radcortez · 2022-03-29T10:40:55Z

@igorregis Thank you for your time and the PR. It is very welcomed. I've left a couple of comments to discuss but nothing major and we do need a test for this.

igorregis · 2022-03-31T01:06:56Z

Can we also add a test? I recommend looking into https://github.com/quarkusio/quarkus/blob/57f6a9d9cb288cce2b558d793948b8d0e56ea806/integration-tests/opentelemetry-vertx/src/main/java/io/quarkus/it/opentelemetry/vertx/SpanData.java, which was the trigger for the original issue. Ideally, io.opentelemetry.sdk.trace.data.SpanData should be the only class required to be registered.

Thanks for pointing me this integration test @radcortez! I've tried to run it using mvn install -Dnative with no success. Do you know any further documentation reading or should I dig the code?

The error that I've got is this (in io.quarkus.it.opentelemetry.vertx.HelloRouterTest):
Caused by: java.lang.IllegalStateException: No config found for class io.quarkus.deployment.dev.devservices.GlobalDevServicesConfig

radcortez · 2022-03-31T10:14:31Z

Strange. If there was an issue with the test, the CI would have reported it. I did try to run it locally and it works fine for me. Can you try to rebuild the entire project from the root folder to make sure you don't have any outdated dependencies? I recommend mvn install -T 6 -Dquickly -DskipTests=true from the root folder, and then run the test again.

igorregis · 2022-04-01T12:18:44Z

Strange. If there was an issue with the test, the CI would have reported it. I did try to run it locally and it works fine for me. Can you try to rebuild the entire project from the root folder to make sure you don't have any outdated dependencies? I recommend mvn install -T 6 -Dquickly -DskipTests=true from the root folder, and then run the test again.

You were right, I did a git pull and now everything is ok. Thank you for the tip. I'm gonna work on the tests this weekend.

radcortez · 2022-04-01T14:44:07Z

Great. Thanks!

igorregis · 2022-04-04T12:48:16Z

Can we also add a test? I recommend looking into https://github.com/quarkusio/quarkus/blob/57f6a9d9cb288cce2b558d793948b8d0e56ea806/integration-tests/opentelemetry-vertx/src/main/java/io/quarkus/it/opentelemetry/vertx/SpanData.java, which was the trigger for the original issue. Ideally, io.opentelemetry.sdk.trace.data.SpanData should be the only class required to be registered.

Thank you for the reference for this integration test. It's validating quite well the implementation. But I've found that the ReflectiveHierarchStep class requires that all class into the hierarchy must be previously loaded on Jandex index, otherwise it will be skip at this line:

quarkus/core/deployment/src/main/java/io/quarkus/deployment/steps/ReflectiveHierarchyStep.java

Lines 221 to 223 in 57f6a9d

    
           if (info == null) { 
        
               return; 
        
           }

Even as being detected as required with all flags turned on.

I'm going to implement that loading at the RegisterForReflectionBuildStep, but that will require a code set very similar to ReflectiveHierarchyStep

igorregis · 2022-04-18T23:12:34Z

Can we also add a test? I recommend looking into https://github.com/quarkusio/quarkus/blob/57f6a9d9cb288cce2b558d793948b8d0e56ea806/integration-tests/opentelemetry-vertx/src/main/java/io/quarkus/it/opentelemetry/vertx/SpanData.java, which was the trigger for the original issue. Ideally, io.opentelemetry.sdk.trace.data.SpanData should be the only class required to be registered.
.....
I'm going to implement that loading at the RegisterForReflectionBuildStep, but that will require a code set very similar to ReflectiveHierarchyStep

@radcortez I guess I have made all needed changes now. Regarding to tests, all I did was to change it to run as the intended behavior as you wrote before:

Ideally, io.opentelemetry.sdk.trace.data.SpanData should be the only class required to be registered.

Reading the final code, it feels like it really should be in another buildStep to say at least, because it's clear that the RegisterForReflectionBuildStep seems to have two different responsibilities new.

I can, at first, separate the BuildStep logic into another class and create a dedicated new annotation, like RegisterForSerialization for evaluation here. So we can achieve something near to what @Sanne pointed out.

What do you think?

radcortez · 2022-04-19T13:22:22Z

Sure go ahead.

igorregis · 2022-04-21T19:05:43Z

Sure go ahead.

Hi @radcortez, I appreciate your assistance on this first time endeavor. I'm sure it's done now. The SpanData have been changed so now it uses the new Annotation and it's passing clearly.

We have a new BuildStep class and a New explicit annotation that makes clear the serialization intent. I didn't feel the need for a new BuildItem. The ReflectiveHierarchyBuildItem did the trick and it keeps the semantics since we are dealing with a kind of Reflective Hierarchy

If you think it's OK to go ahead, I need some help on cleaning my commits, that seems to be messy :-) Just point me the direction.

Commits should be atomic and semantic. Please properly squash your pull requests before submitting them. Fixup commits can be used temporarily during the review process but things should be squashed at the end to have meaningful commits. We use merge commits so the GitHub Merge button cannot do that for us. If you don't know how to do that, just ask in your pull request, we will be happy to help!

That's my case, I don't have a clue of how to do that.

radcortez · 2022-05-03T16:17:59Z

Thanks. Lets see if the CI is happy.

igorregis · 2022-05-04T00:03:10Z

Thanks. Lets see if the CI is happy.

It seems it didn't like it :-p.

I'm going to work on it next free time that I have.

radcortez · 2022-05-04T15:52:13Z

Thanks!

gsmet

Added a few comments. As Roberto mentioned, this will need a big squash at the end.

core/deployment/src/main/java/io/quarkus/deployment/steps/RegisterForReflectionBuildStep.java

.../deployment/src/main/java/io/quarkus/deployment/steps/RegisterForSerializationBuildStep.java

zakkak

@igorregis welcome to Quarkus and thank you for opening this PR :)

The way I understand it, the intention of this PR was to give Quarkus applications the ability to register the whole class hierarchy of a class (or a set of classes) with a single annotation.

IMO this should be implemented through an optional parameter being passed to RegisterForReflection, e.g. a boolean called includeHierarchy and not in a separate annotation. But even if a separate annotation is dimmed necessary it shouldn't be called RegisterForSerialization as such an annotation is expected to do something else.

Interestingly though, RegisterForSerialization is an annotation we have been wanting to implement for some time now (see #20300), so I would suggest keeping it as well (addressing the issues I mention in my review).

So to sum up, I believe that this PR should:

Extend RegisterForReflection to allow it to register the whole hierarchy
Deprecate RegisterForReflection's serialization parameter
Introduce RegisterForSerialization (as it's already there) which should register the class and its hierarchy for reflection including all fields (ideally excluding transient ones) and ideally excluding all methods, except for writeObject, readObject and readObjectNoData, as they are not needed for serialization.

core/runtime/src/main/java/io/quarkus/runtime/annotations/RegisterForSerialization.java

...tion-tests/opentelemetry-vertx/src/main/java/io/quarkus/it/opentelemetry/vertx/SpanData.java

igorregis · 2022-05-15T22:23:01Z

Added a few comments. As Roberto mentioned, this will need a big squash at the end.

Actually here I was quoting the quarkus documentation :-) since I surely need some orientation on how to perform the squash. Any link to any documentation or tutorial is suffice.

...tion-tests/opentelemetry-vertx/src/main/java/io/quarkus/it/opentelemetry/vertx/SpanData.java

zakkak · 2022-07-04T06:25:29Z

core/deployment/src/main/java/io/quarkus/deployment/steps/RegisterForReflectionBuildStep.java

+    private void addMethodsForSerialization(BuildProducer<ReflectiveHierarchyBuildItem> reflectiveClassHierarchy,
+            ClassLoader classLoader, Set<DotName> processedReflectiveHierarchies, IndexView indexView, DotName initialName,
+            ReflectiveHierarchyBuildItem.Builder builder, ClassInfo classInfo, boolean methods) {
+        List<MethodInfo> methodList = classInfo.methods();
+        for (MethodInfo methodInfo : methodList) {
+            // we will only consider potential getters
+            if (methodInfo.parameters().size() > 0 ||
+                    Modifier.isStatic(methodInfo.flags()) ||
+                    methodInfo.returnType().kind() == Kind.VOID || methodInfo.returnType().kind() == Kind.PRIMITIVE) {
+                continue;
+            }
+            registerType(reflectiveClassHierarchy, classLoader, processedReflectiveHierarchies,
+                    methods, builder,
+                    getMethodReturnType(indexView, initialName, classInfo, methodInfo));
+        }
+    }


Non-blocker: I still don't see why serialization/deserialization would use the getters instead of reflection to access the fields. If you could provide an example where getters are accessed/used I would appreciate it :)

Sure. We are not using the getters per se, but only to guess what field should we register the type from. That way we are registering types returned by public methods, since I'm guessing that its state is needed to be transferred or stored.

If I stop doing that, the opentelemetry vertex integration test break due to lack of type when you run it on native.

Why not register the types of all the fields though? Wouldn't that be safer?

That crossed my mind, but I've noticed that many types returned on opentelemetry vertex integration test are just interfaces, so they have no fields to be registered and we have no access (the developer may not know and not asking them to be registered) to the real objects behind those interfaces.

zakkak

Thanks for the changes @igorregis . Please see a couple of comments, only the one with the try catch is a blocker for me.

core/deployment/src/main/java/io/quarkus/deployment/steps/RegisterForReflectionBuildStep.java

igorregis · 2022-07-14T12:20:51Z

Thanks for the changes @igorregis . Please see a couple of comments, only the one with the try catch is a blocker for me.

Ok, I m on a vacation trip. So I'm going to be able to do this changes in 2 weeks.

zakkak

LGTM, thanks for addressing all the blocking issues @igorregis

igorregis · 2022-08-21T23:17:05Z

My fork was too behind so I discarded all changes on my main. I had no idea it would close the PR, sorry for that. I still have all history on my fork's feature branch.
What's the preferred option now? Should I simply throw away all change history (IMHO it doesn't seems to be important anyway) and do a fresh PR with only one commit?

zakkak · 2022-08-23T20:29:16Z

My fork was too behind so I discarded all changes on my main.

FYI: In such cases git rebase often does the trick without any hustle if there are not conflicts.

I had no idea it would close the PR, sorry for that.

That's OK.

I still have all history on my fork's feature branch. What's the preferred option now? Should I simply throw away all change history (IMHO it doesn't seems to be important anyway) and do a fresh PR with only one commit?

Yes I think opening a new PR that will be referencing this one is OK.

@igorregis igorregis merged commit b5618c6 into quarkusio:main 2 days ago

That got me really confused :)
@gsmet you might want to keep this in mind. I think it will make this PR's title end in the changelog despite no actual code going in.

humcqc · 2024-01-29T14:07:14Z

Hi @zakkak @igorregis , I try to understand in which case we don't want the full hierarchy to be registered for reflection as soon as with register a class ?
Another question:
I've created my own annotation annotated by @RegisterForReflection but classes annotated with my own annotation are not registered for reflection. Do you think it's a bug ? or something that could be improved ?

igorregis · 2024-01-29T20:33:59Z

Hi @humcqc

I try to understand in which case we don't want the full hierarchy to be registered for reflection as soon as with register a class ?

I think that would make sense when you have a performance concern. To registry everything for reflection pose a weight into the build process and influence the final binary size (AFAIK), and I guess it also add some weight into the runtime. But more experienced Quarkus developers should validate what I wrote here.

I've created my own annotation annotated by @RegisterForReflection but classes annotated with my own annotation are not registered for reflection. Do you think it's a bug ? or something that could be improved ?

I guess it's something new to be discussed into another issue, where we can explore the scenarios in witch an annotation is the anchor or trigger to a hierarchy reflection. That seems to be fair to me, but I'm sure we need to explore more this need.

humcqc · 2024-01-30T08:47:39Z

Hi @igorregis , thanks for your quick reply,
For my first point here is my example : when I registered PersonDTO for reflection I was expecting the members of this class to be registered by default. But it was not if I don't add the registerFullHierarchy = true.

For the second point:
Here is my annotation. And here where I use it.
My first thought was that it will register the class annotated by my annotation, but not.
Do you know how I can enhance the registration processor to handle this case if we think it's a good idea ?

zakkak · 2024-01-31T10:01:52Z

Hi @humcqc

For my first point here is my example : when I registered PersonDTO for reflection I was expecting the members of this class to be registered by default. But it was not if I don't add the registerFullHierarchy = true.

We probably need to mention the defaults in the class description to make this clear. I opened #38496

Regarding the whys, what @igorregis mentions in #24474 (comment) is correct.

For the second point:
Here is my annotation. And here where I use it.
My first thought was that it will register the class annotated by my annotation, but not.
Do you know how I can enhance the registration processor to handle this case if we think it's a good idea ?

As suggested by @igorregis please open a new issue for this or try starting a discussion in https://quarkusio.zulipchat.com/#streams/187038/dev or https://github.com/quarkusio/quarkus/discussions

quarkus-bot bot added the area/core label Mar 22, 2022

igorregis marked this pull request as ready for review March 23, 2022 00:49

radcortez requested changes Mar 29, 2022

View reviewed changes

core/runtime/src/main/java/io/quarkus/runtime/annotations/RegisterForReflection.java Outdated Show resolved Hide resolved

core/deployment/src/main/java/io/quarkus/deployment/steps/RegisterForReflectionBuildStep.java Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

gsmet requested changes May 4, 2022

View reviewed changes

zakkak requested changes May 6, 2022

View reviewed changes

zakkak mentioned this pull request May 6, 2022

RegisterForReflection Not Working Where Class Inheritance Hierarchy Spans Subprojects #25404

Closed

radcortez requested changes Jun 13, 2022

View reviewed changes

...tion-tests/opentelemetry-vertx/src/main/java/io/quarkus/it/opentelemetry/vertx/SpanData.java Outdated Show resolved Hide resolved

igorregis requested a review from radcortez July 1, 2022 12:53

zakkak reviewed Jul 4, 2022

View reviewed changes

zakkak requested changes Jul 4, 2022

View reviewed changes

zakkak approved these changes Aug 7, 2022

View reviewed changes

igorregis merged commit b5618c6 into quarkusio:main Aug 21, 2022

igorregis force-pushed the main branch from 53c33a3 to b5618c6 Compare August 21, 2022 21:02

quarkus-bot bot added this to the 2.13 - main milestone Aug 21, 2022

quarkus-bot bot added the kind/enhancement New feature or request label Aug 21, 2022

igorregis mentioned this pull request Aug 28, 2022

RegisterForReflection#classNames does not register full hierarchy of the class #27551

Merged

zakkak mentioned this pull request Jan 31, 2024

Reduce ambiguity around RegisterForReflection's defaults #38496

Merged

humcqc mentioned this pull request Feb 9, 2024

Change default behavior of RegisterForReflection ignoreNested attribute to false #38534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RegisterForReflection#classNames does not register full hierarchy of the class #24474

RegisterForReflection#classNames does not register full hierarchy of the class #24474

igorregis commented Mar 22, 2022

geoand commented Mar 29, 2022

radcortez left a comment

radcortez commented Mar 29, 2022

igorregis commented Mar 31, 2022

radcortez commented Mar 31, 2022

igorregis commented Apr 1, 2022

radcortez commented Apr 1, 2022

igorregis commented Apr 4, 2022 •

edited

igorregis commented Apr 18, 2022 •

edited

radcortez commented Apr 19, 2022

igorregis commented Apr 21, 2022 •

edited

radcortez commented May 3, 2022

This comment has been minimized.

igorregis commented May 4, 2022

radcortez commented May 4, 2022

gsmet left a comment

zakkak left a comment

igorregis commented May 15, 2022

zakkak Jul 4, 2022

igorregis Aug 6, 2022

zakkak Aug 7, 2022

igorregis Aug 8, 2022

zakkak left a comment

igorregis commented Jul 14, 2022

zakkak left a comment

igorregis commented Aug 21, 2022 •

edited

zakkak commented Aug 23, 2022

humcqc commented Jan 29, 2024

igorregis commented Jan 29, 2024

humcqc commented Jan 30, 2024 •

edited

zakkak commented Jan 31, 2024 •

edited

RegisterForReflection#classNames does not register full hierarchy of the class #24474

RegisterForReflection#classNames does not register full hierarchy of the class #24474

Conversation

igorregis commented Mar 22, 2022

geoand commented Mar 29, 2022

radcortez left a comment

Choose a reason for hiding this comment

radcortez commented Mar 29, 2022

igorregis commented Mar 31, 2022

radcortez commented Mar 31, 2022

igorregis commented Apr 1, 2022

radcortez commented Apr 1, 2022

igorregis commented Apr 4, 2022 • edited

igorregis commented Apr 18, 2022 • edited

radcortez commented Apr 19, 2022

igorregis commented Apr 21, 2022 • edited

radcortez commented May 3, 2022

This comment has been minimized.

igorregis commented May 4, 2022

radcortez commented May 4, 2022

gsmet left a comment

Choose a reason for hiding this comment

zakkak left a comment

Choose a reason for hiding this comment

igorregis commented May 15, 2022

zakkak Jul 4, 2022

Choose a reason for hiding this comment

igorregis Aug 6, 2022

Choose a reason for hiding this comment

zakkak Aug 7, 2022

Choose a reason for hiding this comment

igorregis Aug 8, 2022

Choose a reason for hiding this comment

zakkak left a comment

Choose a reason for hiding this comment

igorregis commented Jul 14, 2022

zakkak left a comment

Choose a reason for hiding this comment

igorregis commented Aug 21, 2022 • edited

zakkak commented Aug 23, 2022

humcqc commented Jan 29, 2024

igorregis commented Jan 29, 2024

humcqc commented Jan 30, 2024 • edited

zakkak commented Jan 31, 2024 • edited

igorregis commented Apr 4, 2022 •

edited

igorregis commented Apr 18, 2022 •

edited

igorregis commented Apr 21, 2022 •

edited

igorregis commented Aug 21, 2022 •

edited

humcqc commented Jan 30, 2024 •

edited

zakkak commented Jan 31, 2024 •

edited