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

Auto-configure HandlerMethodArgumentResolvers on AnnotatedControllerConfigurer #40393

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

maxhov
Copy link

@maxhov maxhov commented Apr 17, 2024

Add an AnnotatedControllerConfigurerCustomizer to provide a friendly API to modify the autoconfiguredAnnotatedControllerConfigurer. End users could provide HandlerMethodArgumentResolvers without having to override the entire AnnotatedControllerConfigurer bean.

This has been discussed in (amongst others) spring-projects/spring-graphql#603, but there seems no way to customize the AnnotatedControllerConfigurer. This PR aims to improve the API for end users.

… to modify the AnnotatedControllerConfigurer. End users could provide HandlerMethodArgumentResolvers without having to override the entire AnnotatedControllerConfigurer bean.
@maxhov maxhov changed the title Add AnnotatedControllerConfigurerCustomizer modify the AnnotatedControllerConfigurer Add AnnotatedControllerConfigurerCustomizer to modify the AnnotatedControllerConfigurer Apr 17, 2024
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 17, 2024
@maxhov
Copy link
Author

maxhov commented Apr 19, 2024

I see that the build fails on a missing @since in the Javadoc. Which version should I use here? Is it probable that it will make it into 3.3.0 still?

@philwebb
Copy link
Member

@maxhov I'm afraid we'll probably not get this into 3.3 since we just cut RC1, but feel free to use that as the @Since tag. We can fix it up later.

@philwebb philwebb requested a review from bclozel April 19, 2024 18:30
@bclozel
Copy link
Member

bclozel commented Apr 20, 2024

Why not auto-configure HandlerMethodArgumentResolver beans on that component in the existing configuration? To me customizers are useful when lots of options exist and when applications want to contribute several.

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Apr 20, 2024
@maxhov
Copy link
Author

maxhov commented Apr 20, 2024

@bclozel I chose this approach because it is also used for GraphQlSourceBuilder via GraphQlSourceBuilderCustomizer and it seemed sensible to take the same approach.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 20, 2024
@bclozel bclozel changed the title Add AnnotatedControllerConfigurerCustomizer to modify the AnnotatedControllerConfigurer Auto-configure HandlerMethodArgumentResolvers on AnnotatedControllerConfigurer Apr 21, 2024
@bclozel
Copy link
Member

bclozel commented Apr 21, 2024

The org.springframework.graphql.execution.GraphQlSource.Builder not only configures many things, but also is the gateway to configuring the GraphQL engine itself. The AnnotatedControllerConfigurer has less options and in this case, the HandlerMethodArgumentResolver are ordered, which means AnnotatedControllerConfigurerCustomizer should be ordered themselves or a single AnnotatedControllerConfigurerCustomizer should add all argument resolvers.

I think that as a first step, we should consider HandlerMethodArgumentResolver beans and set them on the configurer in an ordered fashion. I think we're too late in the cycle to consider this feature now.

@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Apr 21, 2024
@bclozel bclozel modified the milestones: General Backlog, 3.x Apr 21, 2024
@maxhov
Copy link
Author

maxhov commented Apr 22, 2024

@bclozel I am not sure I understand. Registering either HandlerMethodArgumentResolver or AnnotatedControllerConfigurerCustomizer beans both need to be explicitely ordered via @Order/Ordered at the bean declaration, so which bean is defined would not really matter for that purpose?

Maybe exposing more methods on the AnnotatedControllerConfigurer API is not desirable at this moment, I can change the implementation to accepting HandlerMethodArgumentResolver beans and registering them. Let me know if you are open to that, then I will change the implementation as such. I understand that for 3.3 this may be too late.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants