RegisterForReflection#classNames does not register full hierarchy of the class

[igorregis]

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]

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

[radcortez]

@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]

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]

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]

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]

Great. Thanks!

[igorregis]

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:

https://github.com/quarkusio/quarkus/blob/57f6a9d9cb288cce2b558d793948b8d0e56ea806/core/deployment/src/main/java/io/quarkus/deployment/steps/ReflectiveHierarchyStep.java#L221-L223

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]

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]

Sure go ahead.

[igorregis]

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]

Thanks. Lets see if the CI is happy.

[quarkus-bot[bot]]

---

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

---

Failing Jobs - Building 3933c661215b9f0ef830a67740b0b5a43d45bd4f

Status	Name	Step	Failures	Logs	Raw logs
✖	Native Tests - Data3	Build	Failures	Logs	Raw logs
✖	Native Tests - Data6	Build	Failures	Logs	Raw logs
✖	Native Tests - HTTP	Build	Failures	Logs	Raw logs
✖	Native Tests - Main	Build	Failures	Logs	Raw logs
:hourglass:	Native Tests - Messaging1	Build	:warning: Check →	Logs	Raw logs
✖	Native Tests - Misc1	Build	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

:gear: Native Tests - Data3 #

- Failing: integration-tests/hibernate-orm-panache 

:package: integration-tests/hibernate-orm-panache

✖ io.quarkus.it.panache.PanacheFunctionalityInGraalITCase.testPanacheFunctionality - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Response body doesn't match expectation.

---

:gear: Native Tests - Data6 #

- Failing: integration-tests/hibernate-reactive-panache 

:package: integration-tests/hibernate-reactive-panache

✖ io.quarkus.it.panache.reactive.PanacheFunctionalityInGraalITCase.testPanacheFunctionality - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Response body doesn't match expectation.

---

:gear: Native Tests - HTTP #

- Failing: integration-tests/vertx 

:package: integration-tests/vertx

✖ io.quarkus.it.vertx.JsonReaderIT.testJsonMappingUsingJackson - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
JSON path firstName doesn't match.

---

:gear: Native Tests - Main #

- Failing: integration-tests/main 

:package: integration-tests/main

✖ io.quarkus.it.main.CoreSerializationInGraalITCase.testEntitySerializationFromServlet line 16 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Response body doesn't match expectation.

✖ io.quarkus.it.main.RegisterForReflectionITCase.testSelfWithNested line 33 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Response body doesn't match expectation.

✖ io.quarkus.it.main.RegisterForReflectionITCase.testSelfWithoutNested line 23 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Response body doesn't match expectation.

✖ io.quarkus.it.main.WebsocketITCase.addedWebSocketTest - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <DYNAMIC> but was: <null>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)

---

:gear: Native Tests - Misc1 #

- Failing: integration-tests/jackson integration-tests/jsonb integration-tests/kotlin-serialization and 1 more

:package: integration-tests/jackson

✖ io.quarkus.it.jackson.ModelWithJsonNamingStrategyResourceIT.testStrategy - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Response body doesn't match expectation.

✖ io.quarkus.it.jackson.ModelWithSerializerAndDeserializerOnFieldResourceIT.testSerializer - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
JSON path name doesn't match.

✖ io.quarkus.it.jackson.ModelWithSerializerAndDeserializerOnFieldResourceIT.testDeserializer - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

✖ io.quarkus.it.jackson.RegisteredPojoModelResourceIT.testSimplePojoModel - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <201> but was <500>.

:package: integration-tests/jsonb

✖ io.quarkus.it.jsonb.ModelWithSerializerAndDeserializerOnFieldResourceIT.testSerializer - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
JSON path name doesn't match.

✖ io.quarkus.it.jsonb.ModelWithSerializerAndDeserializerOnFieldResourceIT.testDeserializer - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

:package: integration-tests/kotlin-serialization

✖ io.quarkus.it.kotser.ResourceIT.testCreateAndFetch - More details - Source on GitHub

java.lang.AssertionError: 
2 expectations failed.
Expected status code <200> but was <500>.

:package: integration-tests/quartz

✖ io.quarkus.it.quartz.QuartzITCase.testCount - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)

[igorregis]

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]

Thanks!

[igorregis]

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.

[igorregis]

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.

[igorregis]

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]

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]

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]

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]

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]

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 https://github.com/quarkusio/quarkus/pull/38496

Regarding the whys, what @igorregis mentions in https://github.com/quarkusio/quarkus/pull/24474#issuecomment-1915516128 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