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

Support custom HandlerMethodArgumentResolver #603

Closed
bystam opened this issue Feb 2, 2023 · 9 comments
Closed

Support custom HandlerMethodArgumentResolver #603

bystam opened this issue Feb 2, 2023 · 9 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@bystam
Copy link

bystam commented Feb 2, 2023

Spring GraphQL has a strategy interface for dynamically resolving values in schema mapping signatures called org.springframework.graphql.data.method.HandlerMethodArgumentResolver.

The multiple built-in instances of this are set up in the AnnotatedControllerConfigurer which is set up with the auto-configuration. However, this doesn't scan for custom instances of it, which means one can't inject custom argument resolvers.

This makes it differ from the spring-mvc counterpart with the same name: org.springframework.web.method.support.HandlerMethodArgumentResolver. If I read the source code correctly these are set up in here: https://github.com/spring-projects/spring-framework/blob/1e98fb607a274fc3cc49c24902d7c269f2946514/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerAdapter.java#L669

Since they have the exact same name, I would have assumed one could also create custom ones in the Spring GraphQL like in web-mvc.

@bystam
Copy link
Author

bystam commented Feb 2, 2023

I could look into implementing this myself if you think it is a valid idea! :)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 2, 2023
@rstoyanchev
Copy link
Contributor

We have a pull request for this #325, but there hasn't been a very clear case for it yet.

@mladenjotanovichtec
Copy link

Hello @rstoyanchev, it would be extremely useful to be able to register custom method argument resolver, as an example
we can consider Pageable interface from Spring Data and resolver from spring-data-commons , where we can use SortDefaults for instance.

Is there an alternative to override of AnnotatedControllerConfigurer to support this?

@rstoyanchev
Copy link
Contributor

Thanks for the comment, @mladenjotanovichtec. I am aware of Pageable as one major case that we don't currently support. We're going to add support for it and that should be in main fairly soon. I'm interested in your thoughts for sorting and @SortDefaults as it relates to this. Would you mind if you comment under #620 with some detail on what you would expect?

@bystam
Copy link
Author

bystam commented Mar 3, 2023

Thanks for the feedback!

Our use case is basically that we have leveraged the spring-mvc one in order to produce some neatly injectable data types that wrap more low level spring/http concepts such as:

  • Turning a set of headers into some Client object that can be injected
  • A more high level "User" object built on top of spring security to avoid the repetitive @AuthenticationPrincipal injection

These can all be rebuilt by using @Context argument injection, but for us that came with 2 drawbacks:

  • @Context is tied to a string key rather than the type, which means one has to make sure the input argument name matches what was injected in the context
  • @Context is specific to GraphQL, which means we can't leverage the same injection pattern for regular @RestController endpoints and graphql schema mappings (we have both for different clients)

So it's not really a blocker for us, it just mostly feels like a discrepancy. The fact that both spring grapqhl and spring mvc use @Controller annotations gives the impression that you would be able to build things similarly, in my mind.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Mar 8, 2023

hi @bystam, thanks for the additional context.

I expect the logic to create such a Client object wouldn't be down at the level of data fetching, but higher at the transport layer. I imagine you have a WebGraphQlInterceptor rather that checks HTTP request details, and inserts the Client object into the GraphQLContext. So if I'm not mistaken, such a HandlerMethodArgumentResolver then would be mainly to access the object from the context.

I'm wondering about an enhancement in the ContextValueArgumentResolver such that if it the name is not explicitly specified, and there is no match based on method parameter name, and the method parameter is not a simple type, then look through the context for a single match by type.

This is of course not mutually exclusive with support for custom resolvers, but it could make a nice improvement, and also makes it more clear the object comes from the context and was prepared earlier.

@bystam
Copy link
Author

bystam commented Mar 8, 2023

That could be a nice improvement indeed, if it doesn't create any confusion. I think being able to wire it by type would align well with how many similar things work in spring. I also think there is a precedence in how spring-graphql handles data loaders. Graphql-java has the loaders registered by a String name, but spring helps tucking that away and focused on what types one wishes to load.

You're right that these things are indeed "execution global" and not really specific to any one data fetcher. I think my initial question comes from wanting spring graphql to be "as similar to web-mvc as possible" for the sake of onboarding people who are used to classic mvc but now get to learn graphql.

@rstoyanchev
Copy link
Contributor

On further thought, this could create confusion with collections, maps, container types with generics, and possibly others. Unlike data loaders where registrations are unique by type name, this is much more wide open.

I'll go ahead and schedule this for 1.2. Thanks for the feedback.

@rstoyanchev rstoyanchev changed the title Support for custom HandlerMethodArgumentResolver Support custom HandlerMethodArgumentResolver Mar 8, 2023
@rstoyanchev rstoyanchev self-assigned this Mar 8, 2023
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 8, 2023
@rstoyanchev rstoyanchev added this to the 1.2.0-M1 milestone Mar 8, 2023
@GoncaloPT
Copy link

Hi.
Although this issue is completed, i'm still unsure on how to register a custom HandlerMethodArgumentResolver.
Could anyone give an example on how to bootstrap this?

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

No branches or pull requests

5 participants