spring-projects / spring-graphql

Spring Integration for GraphQL
https://spring.io/projects/spring-graphql
Apache License 2.0
1.54k stars 310 forks source link

Improve support for TypeResolver registrations for interfaces and unions #154

Closed ptrbojko closed 3 years ago

ptrbojko commented 3 years ago

Hi, currently there is no mapping for interfaces and unions and RuntimeWiringConfigurer must be used to instrument how returned objects map to grapqhql schema.

We have tinkered with following solution:

@Target({ElementType.TYPE})
@Retention(RetentionPolicy.RUNTIME)
public @interface SchemaType {
    String value();
}

Configuration:

   @Bean
    public RuntimeWiringConfigurer configurer() {
        ClassPathScanningCandidateComponentProvider scanner = new ClassPathScanningCandidateComponentProvider(false);
        scanner.addIncludeFilter(new AnnotationTypeFilter(SchemaType.class));
        Set<BeanDefinition> candidates = scanner.findCandidateComponents("com.codedoers.feedbackboards");
        Map<Class, String> collect = candidates.stream()
                .map(BeanDefinition::getBeanClassName)
                .map(this::loadClass)
                .collect(Collectors.toMap(
                        Function.identity(),
                        c -> c.getAnnotation(SchemaType.class).value()
                ));
        return builder -> {
            builder.type("Node", typeBuilder -> {
                typeBuilder.typeResolver(typeResolutionEnvironment -> {
                    Object object = typeResolutionEnvironment.getObject();
                    String s = collect.get(object.getClass());
                    if (s == null) {
                        Optional<String> first = collect.entrySet().stream().filter(entry -> entry.getKey().isAssignableFrom(object.getClass())).map(Map.Entry::getValue).findFirst();
                        if (first.isPresent())
                            s = first.get();
                    }
                    if (s == null)
                        return null;
                    return typeResolutionEnvironment.getSchema().getObjectType(s);
                });
                return typeBuilder;
            });
        };
    }

usage

@SchemaType("Board")
public class Board implements Node { //Node like Relay like Node
    @Id
    private String oid = null;
    @NotBlank
    private String key;
    @NotBlank
    private String name;
    @NotNull
    private AccessType accessType;

    public String getKey() {
        return key;
    }

    public void setKey(String key) {
        this.key = key;
    }

    public AccessType getAccessType() {
        return accessType;
    }

    public void setAccessType(AccessType accessType) {
        this.accessType = accessType;
    }

 // ...
}

Just wondering if similar approach is planned in spring graphql

rstoyanchev commented 3 years ago

We typically scan for annotations on objects declared in Spring configuration, and not on domain types. So short answer is no plans to do something like this.

We could perhaps register a default TypeResolver for types that don't have one. It would try to resolve based on the simple name of the class, and it could also walk up the super type hierarchy to look for a match. That could reduce the need to register a TypeResolver when the class name does not differ from the GraphQL type name.

ptrbojko commented 3 years ago

Ok, does this possible default TypeResolver would look like following?

    @Bean
    public RuntimeWiringConfigurer configurer() {
        return builder -> {
            builder.type("Node", typeBuilder -> {
                typeBuilder.typeResolver(typeResolutionEnvironment -> {
                    Class cl = typeResolutionEnvironment.getObject().getClass();
                    Optional<String> name = findByMatchingClassName(cl, typeResolutionEnvironment.getSchema()::containsType);
                    return typeResolutionEnvironment.getSchema().getObjectType(
                            name.orElseThrow(() -> createCannotResoleSchemaTypeException(cl)));
                });
                return typeBuilder;
            });
        };
    }

    private RuntimeException createCannotResoleSchemaTypeException(Class cl) {
        return new RuntimeException("Cannot resolve " + cl.getName() + " within grapqhl schema");
    }

    private Optional<String> findByMatchingClassName(Class cl, Predicate<String> containsType) {
        return Stream.iterate(cl, Objects::isNull, c -> c.getSuperclass())
                .map(Class::getSimpleName)
                .filter(containsType)
                .findFirst();
    }

It is ok for me, as far as such resolver could be overridden somehow or takes last precedence after users custom resolvers. Some folks models domain tied rather to data layer and then expose it as DTOs.

koenpunt commented 3 years ago

when the class name does not differ from the GraphQL type name.

In addition to this, maybe an annotation like @GraphQLName could be introduced, which defines what the typename should be (this is what I currently have implemented in user-space).

@GraphQLName("Merchant")
class MerchantType : Node {
 // ...
}

With a trivial TypeResolver implementation;

class DefaultTypeResolver : TypeResolver {
    override fun getType(env: TypeResolutionEnvironment): GraphQLObjectType {
        val schema = env.schema
        val node = env.getObject<Any>()

        val annotation = node.javaClass.kotlin.findAnnotation<GraphQLName>()

        if (annotation != null) {
            return schema.getTypeAs(annotation.value)
        }

        return schema.getTypeAs(node.javaClass.simpleName)
    }
}

This is different than the @SchemaType proposed by @ptrbojko, because no scanning is necessary, and instead the annotation is checked at runtime by the default TypeResolver.

Edit: I'm unsure about performance implications, but if necessary the result of the typeresolver could be cached in a hashmap.

ptrbojko commented 3 years ago

@koenpunt yeah, I have already rewritten the scanning in favour of checking directly through the objects class and its super class chain

@Configuration
public class GraphQLConfiguration {

    @Bean
    public RuntimeWiringConfigurer configurer() {
        return builder -> {
            builder.type("Node", typeBuilder -> {
                typeBuilder.typeResolver(typeResolutionEnvironment -> {
                    Class cl = typeResolutionEnvironment.getObject().getClass();
                    Optional<String> name = findFromSchemaTypeAnnotation(cl)
                            .or(() -> findByMatchingClassName(cl, typeResolutionEnvironment.getSchema()::containsType));
                    return typeResolutionEnvironment.getSchema().getObjectType(
                            name.orElseThrow(() -> createCannotResoleSchemaTypeException(cl)));
                });
                return typeBuilder;
            });
        };
    }

    private RuntimeException createCannotResoleSchemaTypeException(Class cl) {
        return new RuntimeException("Cannot resolve " + cl.getName() + " within grapqhl schema");
    }

    private Optional<String> findByMatchingClassName(Class cl, Predicate<String> containsType) {
        return Stream.iterate(cl, Objects::nonNull, c -> c.getSuperclass())
                .map(Class::getSimpleName)
                .filter(containsType)
                .findFirst();
    }

    private Optional<String> findFromSchemaTypeAnnotation(Class cl) {
        return Stream.iterate(cl, Objects::nonNull, c -> c.getSuperclass())
                .filter(c -> c.isAnnotationPresent(SchemaType.class))
                .findFirst()
                .map(c -> (SchemaType) c.getAnnotation(SchemaType.class))
                .map(SchemaType::value);
    }
}
rstoyanchev commented 3 years ago

A TypeResolver that checks the object returned at runtime, looks for a type name hint in an annotation, falls back on the simple class name, then continues up the hierarchy if necessary, and caches the result, could work well. We could register one of these for all interfaces and unions through a type visitor, i.e. after the schema is fully parsed.

I think if such an annotation exists, it could also be expected to be checked if present on the source/parent object injected into a @SchemaMapping method, effectively an alternative to @SchemaMapping(typeName="...") on the controller method, and the same for it being present on the entity for an @GraphQlRepository, effectively an alternative to GraphQlRepository(typeName="...").

For the name, @SchemaType sounds a bit like it declares it as a GraphQL type, but the intent is to only to provide a hint for the associated type name, and GraphQLType is already a type in GraphQL Java, and best to avoid such overlap. We could actually broaden the use of @SchemaMapping, so that if it is found on a data type (i.e. source/parent, field value, entity) we would pick up the typeName from it. It would serve a similar purpose as having a class-level @SchemaMapping on a controller as a way of supplying a default parent type name.

rstoyanchev commented 3 years ago

After a team discussion, we'll start with a TypeResolver that compares class names and walks up the hierarchy and we'll leave out the annotation for now. Let's find out see what specific cases remain and we might be able to provide just enough flexibility.

ptrbojko commented 3 years ago

Ok, but please - leave the possibility for override the new resolver in user code.

rstoyanchev commented 3 years ago

@ptrbojko, @koenpunt, I've added defaultTypeResolver on GraphQlSource.Builder. By default, it is set to ClassNameTypeResolver which uses the simple class name of the value or its supertypes, and further exposes options to take over the determination of the simple class name, so for example you could compensate for classname conventions that lead to a mismatch, or to provide additional Class to String mappings.

This should cover a lot of cases. I would be interested to discuss if you have more cases that are not already well covered by this.

koenpunt commented 3 years ago

@rstoyanchev for me the ClassNameTypeResolver is enough for all use cases we currently have 👍

ptrbojko commented 3 years ago

Hello,

It seems that for our needs defaultTypeResolver will get the job done.

Regards,

rstoyanchev commented 3 years ago

Sounds good, thanks for @ptrbojko and @koenpunt for the feedback.