Change default behavior of RegisterForReflection ignoreNested attribute to false #38534

humcqc · 2024-02-01T15:50:07Z

As discussed in #38496 (comment).

@zakkak, @geoand , @loicmathieu

quarkus-bot · 2024-02-01T15:50:27Z

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

title should not end up with dot

_{This message is automatically generated by a bot.}

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

integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/DogDto2.java

zakkak · 2024-02-06T14:50:16Z

Thank you @humcqc , please see my two comments.

humcqc · 2024-02-06T23:24:46Z

Thank you @humcqc , please see my two comments.

Done, do you know why the CI Sanity Check Expected — Waiting for status to be reported is always pending ?

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

zakkak · 2024-02-07T09:22:30Z

Done, do you know why the CI Sanity Check Expected — Waiting for status to be reported is always pending ?

Yes, that's a safety guard to prevent first time contributors from running workflows. I approved the run :)

zakkak · 2024-02-07T09:58:48Z

CI fails with:

Error:  Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.23.0:validate (default) on project quarkus-core: File '/home/runner/work/quarkus/quarkus/core/runtime/src/main/java/io/quarkus/runtime/annotations/RegisterForReflection.java' has not been previously formatted. Please format file (for example by invoking `mvn -f core/runtime net.revelc.code.formatter:formatter-maven-plugin:2.23.0:format`) and commit before running validation! -> [Help 1]

@humcqc please run ./mvnw process-sources and amend your commit with the formatting changes.

humcqc · 2024-02-07T13:18:15Z

Hi @zakkak , any idea for this : Error: Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.23.0:validate (default) on project quarkus-core: File '/home/runner/work/quarkus/quarkus/core/runtime/src/main/java/io/quarkus/runtime/annotations/RegisterForReflection.java' has not been previously formatted. Please format file (for example by invoking mvn -f core/runtime net.revelc.code.formatter:formatter-maven-plugin:2.23.0:format) and commit before running validation! -> [Help 1]

Seems link to the javadoc changes ?
--> The direct commit though github interface introduce some spaces not ok with quarkus code style. I fixed it.

zakkak

Thanks @humcqc . Let's see if the CI is OK with it and merge it :)

humcqc · 2024-02-07T14:08:00Z

Thanks @humcqc . Let's see if the CI is OK with it and merge it :)

I can squash the commits before merge if you want ?

zakkak · 2024-02-07T14:16:36Z

I can squash the commits before merge if you want ?

Yes, please do so.

humcqc · 2024-02-07T18:07:29Z

Seems the last change on class with targets make some test failing:
https://github.com/quarkusio/quarkus/actions/runs/7815481419/job/21322155888?pr=38534

@zakkak, what do you think ?

zakkak · 2024-02-08T08:02:23Z

Main IT

Fails with:

Response body doesn't match expectation.
Expected: is "OtherInaccessibleClassOfC"
  Actual: FAILED

and

Response body doesn't match expectation.
Expected: is "InnerClassOfB"
  Actual: FAILED

Which indicates that the nested InnerClassOfB class was not properly registered, despite ResourceB being annotated without setting ignoreNested to true. So in that case it looks like the default is either altered by somehow, or that the default doesn't actually work.

Data3 IT tests - Hibernate ORM with Panache

Fails with:

ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (executor-thread-1) HTTP Request to /test/projection-nested failed, error id: d2d25807-7c24-4361-843f-487351bc4049-1: org.jboss.resteasy.spi.UnhandledException: java.lang.IllegalStateException: Cannot instantiate class 'io.quarkus.it.panache.PersonDTO$AddressDTO' (it has no constructor with signature [java.lang.String], and not every argument has an alias)

Which indicates that the nested AddressDTO class was not properly registered, despite PersonDTO being annotated without setting ignoreNested to true. So in that case it looks like the default is either altered by somehow, or that the default doesn't actually work.

Data 7 IT - Hibernate Reactive with Panache

Fails with:

ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (vert.x-eventloop-thread-1) HTTP Request to /test/projection-nested failed, error id: fe3bdc55-d68d-41d6-9b16-9afe5812889e-1: java.lang.ArrayIndexOutOfBoundsException: Index 0 out of bounds for length 0

Which is not clear why is happening but I suspect is related to the PersonDTO registration above.

In all cases I see that the classes were originally annotated with @RegisterForReflection(ignoreNested = false) and you now removed the ignoreNested = false part as it's supposed to be the new default, but Quarkus is still treating them like it did before.

humcqc · 2024-02-08T14:22:38Z

ok certainly the current condition test does not handle the reverse one, I will check.

…te to false.

zakkak · 2024-02-09T08:18:18Z

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

@@ -50,7 +50,7 @@ public void build(CombinedIndexBuildItem combinedIndexBuildItem, Capabilities ca

            boolean methods = getBooleanValue(i, "methods");
            boolean fields = getBooleanValue(i, "fields");
-            boolean ignoreNested = getBooleanValue(i, "ignoreNested");
+            boolean ignoreNested = i.value("ignoreNested") != null && i.value("ignoreNested").asBoolean();


The original definition of getBooleanValue is:

private static boolean getBooleanValue(AnnotationInstance i, String name) { return i.value(name) == null || i.value(name).asBoolean(); }

which means that it returns true if the value is not explicitly set, no matter what the default value is set to in the corresponding annotation, which seems wrong to me.

With the change you propose you revert this and set the default value to false, but it again doesn't seem right to me because:

If we change the ignoreNested default sometime in the future it will fail again.

If we change any other default from true to false it will fail again.

I will prepare a PR to fix this independently.

yes, I was looking to a more robust change, to handle properly the default value of the annotation. This commit was to trigger the CI , I have some limitation to test the native version locally. We can do this in a second PR if you prefer ?

Yes, let's keep them separate.

One thing if we want to do this properly we need to change the getBooleanValue method and it will impact a lot of more test as it will impact the other annotation attributes. Not sure I have sufficient knowledge on the impact of those attributes.

@humcqc no worries, I will try to fix this (I already have a WIP branch but hit some issue I am not sure how to resolve yet).

@humcqc FYI #38701

quarkus-bot · 2024-02-09T12:21:36Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit beec214.

✅ 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.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

Assertion condition defined as a io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

Assertion condition defined as a io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

Assertion condition Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

zakkak · 2024-02-09T12:27:02Z

The CI failures look unrelated to me.

humcqc · 2024-02-09T12:28:56Z

The CI failures look unrelated to me.

I think so. Do you think it could be merged ?

zakkak · 2024-02-09T12:52:30Z

Yes, the failures show up in other PRs as well:

Refactor RESTEasy Classic default JAX-RS security to make endpoint detection more robust #38622
Initial Observability extension - devservices, devresources, LGTM #38448

humcqc · 2024-02-09T13:08:10Z

Thanks for your help, do you know when the 3.9 release is planed ?

geoand · 2024-02-09T13:13:38Z

It has not been planned yet.

humcqc · 2024-02-09T13:19:12Z

It has not been planned yet.

@geoand Could we backport this one and #24474 in 3.7.x ?

geoand · 2024-02-09T13:56:17Z

We are trying to be super conservative for 3.7/3.8, so I would prefer not

humcqc · 2024-02-09T14:02:49Z

We are trying to be super conservative for 3.7/3.8, so I would prefer not

:-( ok, i still have few issues to contribute. Any estimate for 3.9 ?

geoand · 2024-02-09T14:15:41Z

Not yet

quarkus-bot bot added area/core area/panache labels Feb 1, 2024

humcqc changed the title ~~Change default behavior of RegisterForReflection ignoreNested attribute to false.~~ Change default behavior of RegisterForReflection ignoreNested attribute to false Feb 1, 2024

humcqc force-pushed the main branch 4 times, most recently from 637210f to 3a29c75 Compare February 3, 2024 14:36

zakkak requested changes Feb 6, 2024

View reviewed changes

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

integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/DogDto2.java Show resolved Hide resolved

humcqc force-pushed the main branch from 3a29c75 to 16ae828 Compare February 6, 2024 20:53

quarkus-bot bot added the area/jackson Issues related to Jackson (JSON library) label Feb 6, 2024

zakkak requested changes Feb 7, 2024

View reviewed changes

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

This comment has been minimized.

Sign in to view

humcqc force-pushed the main branch from f4ebf3d to b9c6e11 Compare February 7, 2024 13:18

quarkus-bot bot added area/cli Related to quarkus cli (not maven/gradle/etc.) area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels Feb 7, 2024

zakkak approved these changes Feb 7, 2024

View reviewed changes

zakkak added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 7, 2024

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label Feb 7, 2024

humcqc force-pushed the main branch 2 times, most recently from b22f9f5 to d2022c2 Compare February 9, 2024 07:55

Change default behavior of RegisterForReflection ignoreNested attribu…

beec214

…te to false.

humcqc force-pushed the main branch from d2022c2 to beec214 Compare February 9, 2024 08:03

zakkak reviewed Feb 9, 2024

View reviewed changes

zakkak merged commit 48ddf16 into quarkusio:main Feb 9, 2024
49 checks passed

quarkus-bot bot added this to the 3.9 - main milestone Feb 9, 2024

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default behavior of RegisterForReflection ignoreNested attribute to false #38534

Change default behavior of RegisterForReflection ignoreNested attribute to false #38534

humcqc commented Feb 1, 2024

quarkus-bot bot commented Feb 1, 2024

zakkak commented Feb 6, 2024

humcqc commented Feb 6, 2024

zakkak commented Feb 7, 2024

zakkak commented Feb 7, 2024

This comment has been minimized.

humcqc commented Feb 7, 2024 •

edited

Loading

zakkak left a comment

humcqc commented Feb 7, 2024

zakkak commented Feb 7, 2024

humcqc commented Feb 7, 2024

This comment has been minimized.

zakkak commented Feb 8, 2024

humcqc commented Feb 8, 2024

zakkak Feb 9, 2024

humcqc Feb 9, 2024

zakkak Feb 9, 2024

humcqc Feb 9, 2024

zakkak Feb 9, 2024

zakkak Feb 9, 2024

quarkus-bot bot commented Feb 9, 2024

zakkak commented Feb 9, 2024

humcqc commented Feb 9, 2024

zakkak commented Feb 9, 2024

humcqc commented Feb 9, 2024

geoand commented Feb 9, 2024

humcqc commented Feb 9, 2024 •

edited

Loading

geoand commented Feb 9, 2024 •

edited

Loading

humcqc commented Feb 9, 2024

geoand commented Feb 9, 2024

Change default behavior of RegisterForReflection ignoreNested attribute to false #38534

Change default behavior of RegisterForReflection ignoreNested attribute to false #38534

Conversation

humcqc commented Feb 1, 2024

quarkus-bot bot commented Feb 1, 2024

zakkak commented Feb 6, 2024

humcqc commented Feb 6, 2024

zakkak commented Feb 7, 2024

zakkak commented Feb 7, 2024

This comment has been minimized.

humcqc commented Feb 7, 2024 • edited Loading

zakkak left a comment

Choose a reason for hiding this comment

humcqc commented Feb 7, 2024

zakkak commented Feb 7, 2024

humcqc commented Feb 7, 2024

This comment has been minimized.

zakkak commented Feb 8, 2024

Main IT

Data3 IT tests - Hibernate ORM with Panache

Data 7 IT - Hibernate Reactive with Panache

humcqc commented Feb 8, 2024

zakkak Feb 9, 2024

Choose a reason for hiding this comment

humcqc Feb 9, 2024

Choose a reason for hiding this comment

zakkak Feb 9, 2024

Choose a reason for hiding this comment

humcqc Feb 9, 2024

Choose a reason for hiding this comment

zakkak Feb 9, 2024

Choose a reason for hiding this comment

zakkak Feb 9, 2024

Choose a reason for hiding this comment

quarkus-bot bot commented Feb 9, 2024

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging-kafka/deployment

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

zakkak commented Feb 9, 2024

humcqc commented Feb 9, 2024

zakkak commented Feb 9, 2024

humcqc commented Feb 9, 2024

geoand commented Feb 9, 2024

humcqc commented Feb 9, 2024 • edited Loading

geoand commented Feb 9, 2024 • edited Loading

humcqc commented Feb 9, 2024

geoand commented Feb 9, 2024

humcqc commented Feb 7, 2024 •

edited

Loading

Status for workflow `Quarkus CI`

humcqc commented Feb 9, 2024 •

edited

Loading

geoand commented Feb 9, 2024 •

edited

Loading