Support instrumenting source method call

[trehak1]

We are currently using our custom GraphQL library (https://github.com/agrpdev/graphql) which is very similar to spring-graphql. We would like to switch to spring-graphql for obvious reasons.

Our code depends heavily on aspects provided by custom annotations and their processing that is done on method call, therefore I am very please with PR #325 and looking forward to it.

The only missing part is support for fields with arguments on DTOs:

public class User {  
  
  // `spring-grapqhl` uses PropertyDataFetcher to fetch this field
  public String getId() {
   return id;
  }  

  // this field is ignored by `spring-graphql`
  @SchemaMapping(field = "permissions")
  public List<Permission> getPermissions(final @Argument("authority") String authority) {
     return this.permissions.stream().filter(p -> p.getAuthority().equals("authority")).toList();
  }
}

@Controller
public class UserController {
  @QueryMapping
  public User getUser(@Argument("id") final String id) {
    return userService.getUser(id);
  }
}

I'd like spring-graphql to call DTO method getPermissions using same logic it uses to call controller methods.

Why?

1. Better encapsulation of data in DTOs
2. Less verbose Controllers

[koenpunt]

I don't think an object like that can be called a DTO.

According to Wikipedia

a DTO does not have any behavior except for storage, retrieval, serialization and deserialization of its own data (mutators, accessors, parsers and serializers). In other words, DTOs are simple objects that should not contain any business logic but may contain serialization and deserialization mechanisms for transferring data over the wire.

https://en.wikipedia.org/wiki/Data_transfer_object

[koenpunt]

That said; How does DTO implementation differ from defining an additional controller for your user object?

[trehak1]

Technically, you are right. Those are not pure "DTOs". The use case here is retrofitting existing codebase to use with spring-graphql without the need to rewrite a lot of code. This would help a lot.

The bigger picture here in my opinion and according to issues #636 #599 #521 is to create a spring-graphql capability that would allow instrumenting/intercepting controller and DTO method calls directly. What do you think?

[jrsperry]

I'm new to Graphql in Java and spring graphql but I believe this would allow you to specify arguments in the DataFetcher as you showed in your example is that right? I'm currently on graphql in node and would like to move to spring graphql but I make heavy use of arguments specified in selection sets for added query power. Here's an example schema and query for clarification.

type Query {
    authors(where: AuthorInput): [Author!]!
}

type Book {
    title: String
    publishYear: Int
    Author: Author
}

input BookInput {
    title: String
    publishYear: Int
}

input AuthorInput {
    firstName: String
    lastName: String
    books: BookInput
}

type Author {
    firstName: String
    lastName: String
    books(where: BookInput): [Book!]!
}

And a query

{
  authors{
    firstName
    lastName
    books(where: {publishYear: 1996}){
      title
    }
  }
}

[koenpunt]

@jrsperry spring-graphql is a spec-compliant implementation, so the schema you posted works fine.

It's just that @trehak1 used a convention in their own implementation before, that isn't supported in spring-graphql.

If you follow the documentation, and use controllers for non-trivial fields of your schema it should work all fine.

NB. I'm not a maintainer of the project, but I am a long time user and contributor.

[trehak1]

I've renamed this issue and will come up with a PR to support general source method call instrumentation. To cover issues #636 #599 #521

[bclozel]

I'm not sure we should expand the scope of this with the issues you've linked. Some of them are related to argument binding, which is in my opinion a different matter. I don't think this is about instrumentation either, since this is supported at the graphql-java level directly through the Instrumentation contract.

If I understand correctly, you would like a contract that gives you a chance at inspecting all Java types that correspond to schema types, and possibly register a custom DataFetcher for a field that does not rely on property fetching like PropertyDataFetcher. I think this approach is probably limited, as we cannot consistently visit all the Java types that your application will expose. We recently added a feature that visits as much as possible in order to warn developers if they miss a @SchemaMapping handler (see #386) - there, we noticed that in many cases we cannot reach the actual Java type that will be seen at runtime. This can happen with union types, or when controller methods do not declare the concrete type but a supertype.

I think this could be achieved with a simpler approach, by configuring your own default DataFetcher that:

• looks at custom annotations on the returned Java object type
• delegates to the PropertyDataFetcher accordingly

This would not require any new contract in Spring for GraphQL and would give a lot of freedom for opinions (name of the annotation, heuristics or best practices, etc). Do you think this approach would work @trehak1 ?

[trehak1]

Thanks for constructive comment @bclozel . I am aware of inability to visit all java types in current spring-graphql implementation. To do so would require introducing extra annotations (type hints) as I did in my custom graphql library (which was code first, so to generate schema I had to implement this mechanism). I think that your proposal to use defaultDataFetcher makes perfect sense, that's how I wired stuff up in configuration of spring-graphql fork:

    @Bean
    public RuntimeWiringConfigurer runtimeWiringConfigurer(ApplicationContext ctx, AnnotatedControllerConfigurer configurer) {
        return builder -> builder
                .codeRegistry(GraphQLCodeRegistry.newCodeRegistry()
                        .defaultDataFetcher(new DataFetcherFactory<Object>() {
                            @Override
                            public DataFetcher<Object> get(DataFetcherFactoryEnvironment factoryEnvironment) {
                                return new DataFetcher<Object>() {
                                    @Override
                                    public Object get(DataFetchingEnvironment environment) throws Exception {
                                        final FieldCoordinates coordinates = AnnotatedControllerConfigurer.getCoordinates(environment);
                                        final Object src = environment.getSource();
                                        if (src == null) {
                                            return null;
                                        } else {
                                            final AnnotatedControllerConfigurer.SchemaMappingDataFetcher df = configurer.createDataFetcher(environment);
                                            if (df == null) {
                                                return new PropertyDataFetcher(coordinates.getFieldName()).get(environment);
                                            } else {
                                                return df.get(environment);
                                            }
                                        }
                                    }
                                };
                            }
                        })
                        .build())
                .build();
    }

I've exposed some internals of AnnotatedControllerConfigurer and added method createDataFetcher for the purpose of re-using all the logic contained inside - validation, argument resolving. Some of the classes are private and some methods that would need to be overridden are private too (DataFetcherHandlerMethod should be overridden to use env source instead of getBean).

[nort3x]

i also think DTOs should keep DTOs and non-trivial fields should be resolved from controllers (like for instance most of the time you might want to cache in your permissions to have better performance , could you do that through above retrofit DTO?)
but still i think renaming a field should be supported, otherwise we have boilerplate code that just act as a name mapper which is not ideal