Fix reflections registration of constructors used in serialization

[zakkak]

Supersedes #19942

Closes: #19711

[vsevel]

LGTM
should we have also the following code in RegisterForReflectionBuildStep, as suggested in this comment:

reflectiveClass.produce(serialization ? ReflectiveClassBuildItem.serializationClass(methods, fields, className) : new ReflectiveClassBuildItem(methods, fields, className));

[zakkak]

should we have also the following code in RegisterForReflectionBuildStep, as suggested in this comment:

reflectiveClass.produce(serialization ? ReflectiveClassBuildItem.serializationClass(methods, fields, className) : new ReflectiveClassBuildItem(methods, fields, className));

That's a good question! I think that really depends on whether serialization = true in @RegisterForReflection implies that the corresponding class should be registered solely for serialization or both for serialization and reflection. Unfortunately the option appears to be undocumented leaving this open to interpretation.

As far as this PR is concerned I believe this question is orthogonal, however, and should be tackled in a separate issue/PR.

For the record, I would personally prefer to be able to register a class either for:

1. reflection,
2. serialization, or
3. both reflection and serialization.

instead of either "1 or 2" or "1 or 3".

"1 or 2" implies that there is no way for me to register a class for both reflection and serialization with a single annotation. I wonder if

@RegisterForReflection(targets = String.class)
public class ReflectionConfig {

}

@RegisterForReflection(targets = String.class, serialization = true)
public class SerializationConfig {

}

would do the trick in such cases.

On the other hand "1 or 3" implies that there is no way to avoid registering for reflection a class that I want to be able to serialization, making registrations for serialization more expensive than necessary.

[vsevel]

I was actually surprised that RegisterForReflection was used for reflection and serialization, when graalvm clearly separates both use cases using the new serialization-config.json config file.
Overloading RegisterForReflection is misleading. it almost looks liked we piggybacked on an existing mechanism.
my first choice would have been to make 2 annotations with an additional RegisterForSerialization. especially if the serialization use case is self sufficient (meaning that if serialization needs some reflection, quarkus will do it for you - e.g. through addReflections)

@RegisterForReflection
@RegisterForSerialization
public class MyClass {
}

I understand the argument for 1 annotation. but then I would honor the different attributes methods and fields that are true by default. I am surprised also that constructors does not appear on the annotation.
but I am not a huge fan. At the minimum the doc should be improved to express what it will and won't do when we use serialization = true.

as you said this may be a different issue covered by a different PR.
your PR fixes an immediate problem, so I would go ahead with it.

[vsevel]

could you provide some guidance here @stuartwdouglas and/or @gsmet?

[geoand]

Yeah, it's a little weird to mix the two concerns, but it should probably be addressed in another PR.

[vsevel]

thanks for taking a look @geoand
thanks for the fix @zakkak this will help going forward with our native tests