spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.25k stars 37.98k forks source link

Provide more declarative control over reflection hint registration #29194

Closed wilkinsona closed 2 months ago

wilkinsona commented 1 year ago

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.

sdeleuze commented 1 year ago

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:

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:

wilkinsona commented 1 year ago

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 commented 1 year ago

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:

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:

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 https://github.com/spring-projects/spring-boot/issues/32486 with either @RegisterReflectionForBinding or RuntimeHintsRegistrar.

snicoll commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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

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 commented 10 months ago

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 commented 10 months ago

@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 commented 9 months ago

Thanks @bclozel makes sense.

Tried with Spring AOT build, however the paketo build pack latest has many vulnerabilities, which is NOT an option to proceed. Can you please suggest a way to go ahead with a typical OS Image and Graal runtime which we already have vulnerability free

snicoll commented 2 months ago

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.