Skip to content
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

Change default behavior of RegisterForReflection ignoreNested attribute to false #38534

Merged
merged 1 commit into from
Feb 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. If we change the ignoreNested default sometime in the future it will fail again.
  2. If we change any other default from true to false it will fail again.

I will prepare a PR to fix this independently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's keep them separate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boolean serialization = i.value("serialization") != null && i.value("serialization").asBoolean();
boolean unsafeAllocated = i.value("unsafeAllocated") != null && i.value("unsafeAllocated").asBoolean();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@

/**
* Annotation that can be used to force a class to be registered for reflection in native image mode.
* Note that by default nested classes and interfaces are not registered, unless {@link #ignoreNested()} is set to false.
* Similarly, by default only the class itself is registered, not the full class hierarchy. This can be changed by setting
* {@link #registerFullHierarchy()} to true.
* Note that by default the class itself is registered including nested classes and interfaces,
* but not the full class hierarchy. This can be changed by setting:
* <ul>
* <li>{@link #ignoreNested()} to true, to ignore nested classes.</li>
* <li>{@link #registerFullHierarchy()} to true, to register the full hierarchy.</li>
* </ul>
*/
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
Expand All @@ -26,11 +29,10 @@
boolean fields() default true;

/**
* If nested classes/interfaces should be ignored/registered
*
* This is useful when it's necessary to register inner (especially private) classes for Reflection.
* If nested classes/interfaces should be ignored.
* By default, nested classes are registered. To ignore them set it to true.
*/
boolean ignoreNested() default true;
boolean ignoreNested() default false;

/**
* Alternative classes that should actually be registered for reflection instead of the current class.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,6 @@
org.eclipse.aether.internal.impl.SimpleLocalRepositoryManagerFactory.class,
org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.class,
org.eclipse.aether.transport.wagon.WagonTransporterFactory.class
})
}, ignoreNested = true)
public class Reflections {
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@
io.quarkus.registry.json.JsonArtifactCoordsSerializer.class,
io.quarkus.registry.json.JsonBooleanTrueFilter.class,
io.quarkus.registry.json.JsonEntityWithAnySupport.class,
})
}, ignoreNested = true)
public class Reflections {
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import io.quarkus.hibernate.orm.panache.common.NestedProjectedClass;
import io.quarkus.runtime.annotations.RegisterForReflection;

@RegisterForReflection(ignoreNested = false)
@RegisterForReflection
zakkak marked this conversation as resolved.
Show resolved Hide resolved
public class DogDto2 {
public String name;
public PersonDto2 owner;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import io.quarkus.hibernate.orm.panache.common.ProjectedFieldName;
import io.quarkus.runtime.annotations.RegisterForReflection;

@RegisterForReflection(ignoreNested = false)
@RegisterForReflection
public class PersonDTO extends PersonName {

public final AddressDTO address;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import io.quarkus.hibernate.reactive.panache.common.NestedProjectedClass;
import io.quarkus.runtime.annotations.RegisterForReflection;

@RegisterForReflection(ignoreNested = false)
@RegisterForReflection
public class DogDto {
public String name;
public PersonDto owner;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import io.quarkus.hibernate.reactive.panache.common.ProjectedFieldName;
import io.quarkus.runtime.annotations.RegisterForReflection;

@RegisterForReflection(ignoreNested = false)
@RegisterForReflection
public class PersonDTO extends PersonName {
public final AddressDTO address;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

import io.quarkus.runtime.annotations.RegisterForReflection;

@RegisterForReflection
@RegisterForReflection(ignoreNested = true)
@JsonIgnoreProperties(ignoreUnknown = true)
public class ModelWithSerializerAndDeserializerOnField {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

import io.quarkus.runtime.annotations.RegisterForReflection;

@RegisterForReflection
@RegisterForReflection(ignoreNested = true)
public class ModelWithSerializerAndDeserializerOnField {

private String name;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
/**
* Only Parent Class will be registered and none of the inner classes will be registered for reflection.
*/
@RegisterForReflection
@RegisterForReflection(ignoreNested = true)
class ResourceA {

private class InnerClassOfA {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
/**
* Both Parent Class and all nested classed will be registered for reflection
*/
@RegisterForReflection(ignoreNested = false)
@RegisterForReflection
public class ResourceB {

private class InnerClassOfB {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* This class is registering targets = ResourceD.StaticClassOfD
* The class itself won't be registered by this, only target will be registered without registering nested classes
*/
@RegisterForReflection(targets = ResourceD.StaticClassOfD.class)
@RegisterForReflection(targets = ResourceD.StaticClassOfD.class, ignoreNested = true)
public class ResourceC {

private class InaccessibleClassOfC {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* This class is registering targets = ResourceC$InaccessibleClassOfC
* The class itself won't be registered by this, only target will be registered including target's nested classes
*/
@RegisterForReflection(classNames = "io.quarkus.it.rest.ResourceC$InaccessibleClassOfC", ignoreNested = false)
@RegisterForReflection(classNames = "io.quarkus.it.rest.ResourceC$InaccessibleClassOfC")
public class ResourceD {

// Parent class won't be registered, only the below private class will be registered without nested classes
Expand Down
Loading