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

Provide more declarative control over reflection hint registration #29194

Closed
wilkinsona opened this issue Sep 23, 2022 · 14 comments
Closed

Provide more declarative control over reflection hint registration #29194

wilkinsona opened this issue Sep 23, 2022 · 14 comments
Assignees
Labels
theme: aot An issue related to Ahead-of-time processing type: enhancement A general enhancement
Milestone

Comments

@wilkinsona
Copy link
Member

wilkinsona commented Sep 23, 2022

Affects: 6.0.0-M6

For declarative registration of reflection hints, we currently offer @Reflective and @RegisterReflectionForBinding. These two options, particularly when used at the type level, sit at two opposite ends of the spectrum. @Reflective on a type will register a hint for the type and nothing more. @RegisterReflectionForBinding on a type will register hints for the type, its constructors, fields, properties, and record components for the whole type hierarchy. Sometimes we want something in between. That means you either write your own ReflectiveProcessor or you accept that @RegisterReflectionForBinding will do the job at the cost of some unnecessary hints.

A concrete example of the above can be found in some of Spring Boot's actuator endpoints. We have types that need to be serialised to JSON that can't be discovered by @Reflective at the method level. This can be because a method returns something quite loosely typed like Map<String, Object> or Map<String, Thing> where there are multiple different Thing sub-classes. I'd like to be able to annotate the endpoint type with something that registers additional types for a certain kind of reflection:

@NewReflectionAnnotation(classes = { FirstConcreteThing.class, SecondConcreteThing.class }, memberCategories = INVOKE_PUBLIC_METHODS)

We could take some inspiration from @RegisterForReflection in Quarkus.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 23, 2022
@sbrannen sbrannen added type: enhancement A general enhancement theme: aot An issue related to Ahead-of-time processing labels Sep 24, 2022
@sbrannen sbrannen added this to the Triage Queue milestone Sep 24, 2022
@sdeleuze sdeleuze self-assigned this Oct 4, 2022
@sdeleuze sdeleuze removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 4, 2022
@sdeleuze sdeleuze modified the milestones: Triage Queue, 6.0.0-RC1 Oct 4, 2022
@sdeleuze
Copy link
Contributor

sdeleuze commented Oct 7, 2022

It may not looks like, but this issue is IMO more complex than it looks like, so let's have a deep dive.

If we want to start simple, having something like @RegisterReflection(classes = { FirstConcreteThing.class, SecondConcreteThing.class }, memberCategories = { INVOKE_PUBLIC_CONSTRUCTORS, INVOKE_PUBLIC_METHODS } ) looks appealing to me. It leverages the existing MemberCategory class and allows to easily add related hints without having to leverage the more involved RuntimeHintsRegistrar programmatic approach. Notice the need will IMO quickly arise to have repeatable annotations to be able to specify different MemberCategory for each classes.

It could be tempting to unify the model and describe as suggested by @wilkinsona @RegisterReflectionForBinding by meta annotating it with a more flexible @RegisterReflection annotations, and that's where things become tricky. @RegisterReflectionForBinding({ Foo.class, Bar.class }) is designed to provide not too much but enough hints for reflection based binding:

  • Declared field reflection is registered to be able to take in account field level annotations (used in Spring Data or Jackson for example) to annotate a property at a single place
  • Declared constructors are registered to allow instantiation
  • Getters, setters and record components are registered by taking in account type transitively used (including generics)
  • Some types like Object or primitive types are skipped
  • Members are not registered for Java types
  • Special processing happen for enums
    The fact that @RegisterReflectionForBinding deals with members with different scopes than MemberCategory, the transitive discovery of types applied just to properties and record components, and the type/member exclusion rules seems to indicate it will be very challenging to have something we would be happy with. So I have the feeling that trying to unify @RegisterReflection and @RegisterReflectionForBinding is not necessarily the target, and that if we introduce something like @RegisterReflection it should try to not be too clever and basically expose a small subset of what is possible with RuntimeHintsRegistrar, at least that's my current feeling. Something we could do is to make @RegisterReflectionForBinding customizable by make it possible to specify classes to skip for example, and update RuntimeHintsRegistrar accordingly.

Now, I would like to understand a little bit more the use case described.

A concrete example of the above can be found in some of Spring Boot's actuator endpoints. We have types that need to be serialised to JSON that can't be discovered by @Reflective at the method level. This can be because a method returns something quite loosely typed like Map<String, Object> or Map<String, Thing> where there are multiple different Thing sub-classes. I'd like to be able to annotate the endpoint type with something that registers additional types for a certain kind of reflection:

My 2 questions related to that use case are:

  • If the use case is reflection based binding, what currently prevents using @RegisterReflectionForBinding({ FirstConcreteThing.class, SecondConcreteThing.class })? If that's unreasonable bloat of unused reflection hints, could you detail which ones you would like to avoid?
  • Would @RegisterReflection(memberCategories = { ..., ...})` be sufficient for your use case?

@wilkinsona
Copy link
Member Author

wilkinsona commented Oct 7, 2022

For the types returned by Boot's actuator endpoints we only need reflective access to the getter methods that are used by Jackson to serialise them to json. As such, I expect that @RegisterReflection with PUBLIC_METHODS would be sufficient.

This is just one use case of course. My hope was that we could provide a single, easy to use mechanism for registering reflection hints. If we offer both RegisterReflectionForBinding and RegisterReflection, users now have two different ways of doing the "same" thing. If we identify another scenario that falls somewhere in between RegisterReflectionForBinding and RegisterReflection we'll need to add a third way of doing things. It feels like a bit of a slippery slope and one that I hoped we may be able to avoid by introducing a single, more configurable mechanism that can cover the full range of scenarios all the way from what the actuator endpoints need (just public getters) right through to the far more complex binding that RegisterReflectionForBinding covers.

@sdeleuze
Copy link
Contributor

sdeleuze commented Oct 10, 2022

For @RegisterReflectionForBinding, that may be counter intuitive, but I am not sure in practice it will add more than a potential @RegisterReflection on PUBLIC_METHODS. The field reflection entry adds almost no overhead and the additional reflection entry on the constructor will be likely more than compensated by the inherited public methods not included like toString(), hashcode(), equals(), etc.

From Spring Native days, one lesson learnt is that binding and reflection based serialization is almost always much more tricky than expected because:

  • You have to register not only the returned type, but also the types returned by the properties/record components
  • You have to take in account types referenced in generics.
  • You may miss the annotations on the field
  • You may forget to add the constructor for deserialization
  • You may be trapped by the fact that PUBLIC_METHODS include inherited methods while DECLARED_METHODS does not
  • There was no reuse of this non trivial feature across the portfolio which led to inconsistencies, suboptimal footprint and bugs

If you prefer something laser focused on exactly what you need like registering with deep control, you can just implement it in a programmatic RuntimeHintsRegistrar way and potentially enable it in a declarative way via @ImportRuntimeHints.

Now, should we introduce more declarative hint registration capabilities? Potentially, but I am not sure we have enough time and strong and various enough use cases yet to take the right decisions:

  • Should we just limit it to @RegisterReflection(memberCategories = { ..., ...})?
  • Should we go as far as Spring Native @TypeHint (sources) and provide a declarative counterpart to ReflectionHints via an annotation?
  • Why should we limit to declarative reflection hints? Declarative resource, proxy or serialization hints like done in Spring Native will also be popular use cases. But that can quickly lead to this kind of giant annotation which could be considered an annotation abuse so not sure we want to replicate.
  • As explained in my previous comment, I am not sure expressing @RegisterReflectionForBinding with @RegisterReflection meta annotations is doable and what we want.

So today we have 2 strong use cases with @RegisterReflectionForBinding which provides a strong added value and will likely need to evolve to be more flexible, and RuntimeHintsRegistrar which is much more low level and can provide all the flexibility you need.

RC1 freeze is tomorrow and RC2 is already packed with challenging issues like the GraalVM 22.3 upgrade so fixing this issue for GA sounds like not possible to me (apology for not having processed this issue earlier but I was on other topics). I am not opposed to introduce something in one of the 6.0.x patch releases if the need is very strong (I am sure a lot of users will ask that) and we agree on the path to take. In the meantime, I would suggest to move forward on spring-projects/spring-boot#32486 with either @RegisterReflectionForBinding or RuntimeHintsRegistrar.

@sdeleuze sdeleuze modified the milestones: 6.0.0-RC1, 6.0.x Oct 10, 2022
@snicoll snicoll self-assigned this Oct 11, 2022
@snicoll
Copy link
Member

snicoll commented Oct 11, 2022

I'd like that we take an hour to brainstorm together about what can be done. IMO it would be a shame to lose the opportunity to do something before 6.0 GA.

@sdeleuze
Copy link
Contributor

sdeleuze commented Oct 17, 2022

We could maybe support something like:

@RegisterReflectionHint(Foo.class, memberCategories = { ..., ...})
@RegisterReflectionHint(Foo.class, binding = true)
@RegisterReflectionHint(classNames = "com.example.Foo", memberCategories = { ..., ...})
@RegisterResourcesHint("META-INF/resources/webjars/*")
@RegisterSerializationHint(Foo.class)

@fsomme2s
Copy link

fsomme2s commented Feb 13, 2023

Just my 2 cts (view it as a "think aloud test" protocol of the humble spring boot developer):

I'd like to have the Effect of @RegisterReflectionForBinding but with an Annotation that I can use on the acutal target class, like with @Reflective. Maybe this already exists and I'm just blind - then I'm sorry (but it would be helpful to link this thing in the jdocs of the other annotations then).

Background Story:
Entities (for persistence mappers) and Dtos (for Jackson) are responsible for 99% of my "constructor / getter / property / whatever not found at runtime" problems in my own code.

Working with Quarkus I am used to just pin an "@RegisterForReflection" on top of those Entities or Dtos - done. Any Constructor and Getter and field etc. is discovered.

I expected "@Reflective" to work the same - but: "Class '...' appears to have no default constructor... caused by NoSuchMethodException ... "

It seems to work with @RegisterReflectionForBinding({MyEntity1.class, MyDto1.class, ... MyEntity26.class, MyDto42.class})
on a @Configuration class.
But I really don't like the idea, that I have to list all my dtos and entities on a central class, feels like in the dark ages before all those auto-discovery mechanisms that we are used to now.

@syedyusufh
Copy link

@RegisterReflectionForBinding is great, however in reality it is really a difficult way of getting around when you have 100+ Spring Boot applications all with complete reactive stack.

Please consider auto reachability of Mono<?> or Flux<?> like how we could unwrap Mono or Flux in springdoc api-docs generation. Thanks.

@bclozel
Copy link
Member

bclozel commented Nov 25, 2023

If I'm not mistaken we already support reflection hints on Flux and Mono types on controllers to some extent.

Please consider auto reachability of Mono or Flux like how we could unwrap Mono or Flux in springdoc api-docs generation.

What do you mean by springdoc api docs generation? Are you referring to this project? If so, this is not supported by the Spring team and if native support has issues with it then you should report that problem directly to the maintainers.

@syedyusufh
Copy link

syedyusufh commented Nov 25, 2023

@bclozel in the below example unless we add @RegisterReflectionForBinding on top of the method that returns Mono<SomeClass>, reachability-metadata would not include the reflection hint for SomeClass. Requesting to have a feature that would auto-wrap a Mono or Flux and discover / include SomeClass from Mono<SomeClass>

public Mono<SomeClass> someServiceMethod() {
...
}

We are having real difficulty adding @RegisterReflectionForBinding in 100+ applications which are all full reactive stack

@bclozel
Copy link
Member

bclozel commented Nov 25, 2023

@syedyusufh I'm not sure I understand - why would you need reflection on return types from a Service method? This is only necessary if the returned value needs JSON serialization, for example. We do this automatically for all Controller methods (including parameterized types); you can see it in action in our smoke tests. This is applied automatically by our ControllerMappingReflectiveProcessor.

This issue is about providing refined mechanisms in case it is not easy to detect the types at the method level, or if the required hints are different from what's provided by @RegisterReflectionForBinding.

In your case, maybe you shouldn't have to add all those annotations because:

  • you might not need these hints after all
  • you could get all of them done with a custom ReflectiveProcessor or another mean

Could you answer the following?

  1. what kind of reflection hint is required for all those types and what error are you getting at runtime if those hints aren't here?
  2. what's the link with the springdoc project as mentioned in your original issue?
  3. could you tell us more about the use case? A "service method" is a bit generic: is this about JSON serialization, java introspection, something else?

@syedyusufh
Copy link

I did two exercises following the below sequential steps,

Sequence-A:

  1. Built the application fat jar normally
  2. Started the application with GraalVM tracing agent to generate the metadata. Note: Here, I did not do any application API call after it has started, simply ran the traceAgent, then stopped it. Now I have got the reachability metadata generated under /src/main/resources/META-INF/native-image. Let us call this generated native-image hints as 'PLAIN'

Sequence-B:

  1. Built the application fat jar normally
  2. Started the application with GraalVM tracing agent to generate the metadata. Once the application has started, I did test all the controller endpoints to ensure that my entire application functionality is tested and the reachability metadata is full & completed. Now I have got the reachability metadata generated under /src/main/resources/META-INF/native-image. Let us call this generated native-image hints as 'TESTED'

If I compare the refelect-config.json from Sequence-A with Sequence-B, I could see all my DTO classes missing in Sequence A generated 'PLAIN' native-image.

The above missing DTO are all for JSON serialization. Having to register and getting the DTO added to the relect-config.json via a processor for 100+ applications is a BIG task. Hence, requesting if auto-discovery of DTOs for JSON serialization can be done when the tracingAgent is run.

Regarding the springdoc api-docs, it was just a reference where we use springdoc-openapi-starter-webmvc-ui to autogenerate the Swagger API spec on our WebMVC + WebFlux application. In which, we had to configure a unwrap() so that the Mono & Flux are unwrapped and the actual DTO would be documented in the OpenAPI spec.

If I could ask in one liner, is there a plan or can it be considered as a feature request to autodetect and register all the DTOs without any custom processor?

Thanks

@bclozel
Copy link
Member

bclozel commented Nov 25, 2023

@syedyusufh as mentioned in my previous comment this is already supported. We do not use the tracing agent and rely on our own AOT process.

If you can reproduce an issue where a DTO cannot be serialized from a controller endpoint (even a reactive one), please create a new issue here and provide a minimal sample application that we can run.

@syedyusufh

This comment was marked as off-topic.

@sdeleuze sdeleuze modified the milestones: 6.1.x, 6.2.x Dec 7, 2023
@snicoll snicoll self-assigned this May 2, 2024
snicoll added a commit to snicoll/spring-framework that referenced this issue Jul 4, 2024
This commit introduces a declarative way of registering reflection
information for arbitrary types. Types can be specified as a class,
a class name, or by annotating the type itself.

This existing RegisterReflectionForBinding becomes a specialized
version of the new annotation, registering the necessary hints for
data binding.

Closes spring-projectsgh-29194
@snicoll snicoll closed this as completed in f4607da Jul 4, 2024
@snicoll snicoll modified the milestones: 6.2.x, 6.2.0-M5 Jul 4, 2024
@snicoll
Copy link
Member

snicoll commented Jul 4, 2024

We've introduced a RegisterReflection where types can be specified alongside member categories. It can also be put on a type and we'll register hints for that type.

After a bit of a back and forth the existing RegisterReflectionForBinding is a specialization of the new annotation and delegates most of the code to it.

The next piece of the puzzle is to ease the inclusion of non-beans as target of this annotation. See #33132.

snicoll added a commit that referenced this issue Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: aot An issue related to Ahead-of-time processing type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

8 participants