From 052b1f09b7e47b3e51d3731523a49c04d88cfa04 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 10 Mar 2023 16:15:23 +0100 Subject: [PATCH] Refactor schema mapping inspection support Because performing the schema mapping inspection on the `TypeDefinitionRegistry` could be too early in the process, this commit uses instead the `GraphQLSchema` instance directly for the inspection. Other extension points could indeed contribute more to the schema before it's fully built. As for querying for DataFetcher instances, the inspector keeps using the `RuntimeWiring` instance right after Spring for GraphQL contributed to it. After that, data fetchers could wrapped by other extensions, preventing it from getting the type information we need. This also moves the `TypedDataFetcher` interface to a different package to prevent a package tangle, and renamed as `SelfDescribingDataFetcher` to enable further extension and to be more descriptive. This changes the report format to keep it on a single line. Closes gh-386 --- .../AnnotatedControllerConfigurer.java | 6 +- .../data/query/QueryByExampleDataFetcher.java | 18 +- .../data/query/QuerydslDataFetcher.java | 18 +- ...ultSchemaResourceGraphQlSourceBuilder.java | 12 +- .../graphql/execution/SchemaInspector.java | 270 ------------------ .../execution/SchemaMappingInspector.java | 221 ++++++++++++++ .../SelfDescribingDataFetcher.java} | 21 +- ....java => SchemaMappingInspectorTests.java} | 109 ++++--- 8 files changed, 307 insertions(+), 368 deletions(-) delete mode 100644 spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaInspector.java create mode 100644 spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaMappingInspector.java rename spring-graphql/src/main/java/org/springframework/graphql/{data/TypedDataFetcher.java => execution/SelfDescribingDataFetcher.java} (51%) rename spring-graphql/src/test/java/org/springframework/graphql/execution/{SchemaInspectorTests.java => SchemaMappingInspectorTests.java} (78%) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java index 9c40589e..0fbc3e81 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java @@ -62,7 +62,7 @@ import org.springframework.format.FormatterRegistrar; import org.springframework.format.support.DefaultFormattingConversionService; import org.springframework.format.support.FormattingConversionService; import org.springframework.graphql.data.GraphQlArgumentBinder; -import org.springframework.graphql.data.TypedDataFetcher; +import org.springframework.graphql.execution.SelfDescribingDataFetcher; import org.springframework.graphql.data.method.HandlerMethod; import org.springframework.graphql.data.method.HandlerMethodArgumentResolver; import org.springframework.graphql.data.method.HandlerMethodArgumentResolverComposite; @@ -540,7 +540,7 @@ public class AnnotatedControllerConfigurer /** * {@link DataFetcher} that wrap and invokes a {@link HandlerMethod}. */ - static class SchemaMappingDataFetcher implements TypedDataFetcher { + static class SchemaMappingDataFetcher implements SelfDescribingDataFetcher { private final MappingInfo info; @@ -632,7 +632,7 @@ public class AnnotatedControllerConfigurer } @Override - public ResolvableType getDeclaredType() { + public ResolvableType getReturnType() { return ResolvableType.forMethodReturnType(this.info.getHandlerMethod().getMethod()); } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/query/QueryByExampleDataFetcher.java b/spring-graphql/src/main/java/org/springframework/graphql/data/query/QueryByExampleDataFetcher.java index 711142e5..7bbc2126 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/query/QueryByExampleDataFetcher.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/query/QueryByExampleDataFetcher.java @@ -41,7 +41,7 @@ import org.springframework.data.repository.query.ReactiveQueryByExampleExecutor; import org.springframework.data.util.TypeInformation; import org.springframework.graphql.data.GraphQlArgumentBinder; import org.springframework.graphql.data.GraphQlRepository; -import org.springframework.graphql.data.TypedDataFetcher; +import org.springframework.graphql.execution.SelfDescribingDataFetcher; import org.springframework.graphql.execution.RuntimeWiringConfigurer; import org.springframework.util.Assert; import org.springframework.validation.BindException; @@ -442,7 +442,7 @@ public abstract class QueryByExampleDataFetcher { } - private static class SingleEntityFetcher extends QueryByExampleDataFetcher implements TypedDataFetcher { + private static class SingleEntityFetcher extends QueryByExampleDataFetcher implements SelfDescribingDataFetcher { private final QueryByExampleExecutor executor; @@ -482,13 +482,13 @@ public abstract class QueryByExampleDataFetcher { } @Override - public ResolvableType getDeclaredType() { + public ResolvableType getReturnType() { return ResolvableType.forClass(this.resultType); } } - private static class ManyEntityFetcher extends QueryByExampleDataFetcher implements TypedDataFetcher> { + private static class ManyEntityFetcher extends QueryByExampleDataFetcher implements SelfDescribingDataFetcher> { private final QueryByExampleExecutor executor; @@ -528,14 +528,14 @@ public abstract class QueryByExampleDataFetcher { } @Override - public ResolvableType getDeclaredType() { + public ResolvableType getReturnType() { return ResolvableType.forClassWithGenerics(Iterable.class, this.resultType); } } - private static class ReactiveSingleEntityFetcher extends QueryByExampleDataFetcher implements TypedDataFetcher> { + private static class ReactiveSingleEntityFetcher extends QueryByExampleDataFetcher implements SelfDescribingDataFetcher> { private final ReactiveQueryByExampleExecutor executor; @@ -575,14 +575,14 @@ public abstract class QueryByExampleDataFetcher { } @Override - public ResolvableType getDeclaredType() { + public ResolvableType getReturnType() { return ResolvableType.forClassWithGenerics(Mono.class, this.resultType); } } - private static class ReactiveManyEntityFetcher extends QueryByExampleDataFetcher implements TypedDataFetcher> { + private static class ReactiveManyEntityFetcher extends QueryByExampleDataFetcher implements SelfDescribingDataFetcher> { private final ReactiveQueryByExampleExecutor executor; @@ -622,7 +622,7 @@ public abstract class QueryByExampleDataFetcher { } @Override - public ResolvableType getDeclaredType() { + public ResolvableType getReturnType() { return ResolvableType.forClassWithGenerics(Flux.class, this.resultType); } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/query/QuerydslDataFetcher.java b/spring-graphql/src/main/java/org/springframework/graphql/data/query/QuerydslDataFetcher.java index 4f2cdb25..82623660 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/query/QuerydslDataFetcher.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/query/QuerydslDataFetcher.java @@ -47,7 +47,7 @@ import org.springframework.data.repository.query.FluentQuery; import org.springframework.data.repository.query.FluentQuery.FetchableFluentQuery; import org.springframework.data.util.TypeInformation; import org.springframework.graphql.data.GraphQlRepository; -import org.springframework.graphql.data.TypedDataFetcher; +import org.springframework.graphql.execution.SelfDescribingDataFetcher; import org.springframework.graphql.execution.RuntimeWiringConfigurer; import org.springframework.util.Assert; import org.springframework.util.LinkedMultiValueMap; @@ -540,7 +540,7 @@ public abstract class QuerydslDataFetcher { } - private static class SingleEntityFetcher extends QuerydslDataFetcher implements TypedDataFetcher { + private static class SingleEntityFetcher extends QuerydslDataFetcher implements SelfDescribingDataFetcher { private final QuerydslPredicateExecutor executor; @@ -584,13 +584,13 @@ public abstract class QuerydslDataFetcher { } @Override - public ResolvableType getDeclaredType() { + public ResolvableType getReturnType() { return ResolvableType.forClass(this.resultType); } } - private static class ManyEntityFetcher extends QuerydslDataFetcher implements TypedDataFetcher> { + private static class ManyEntityFetcher extends QuerydslDataFetcher implements SelfDescribingDataFetcher> { private final QuerydslPredicateExecutor executor; @@ -632,14 +632,14 @@ public abstract class QuerydslDataFetcher { } @Override - public ResolvableType getDeclaredType() { + public ResolvableType getReturnType() { return ResolvableType.forClassWithGenerics(Iterable.class, this.resultType); } } - private static class ReactiveSingleEntityFetcher extends QuerydslDataFetcher implements TypedDataFetcher> { + private static class ReactiveSingleEntityFetcher extends QuerydslDataFetcher implements SelfDescribingDataFetcher> { private final ReactiveQuerydslPredicateExecutor executor; @@ -682,14 +682,14 @@ public abstract class QuerydslDataFetcher { } @Override - public ResolvableType getDeclaredType() { + public ResolvableType getReturnType() { return ResolvableType.forClassWithGenerics(Mono.class, this.resultType); } } - private static class ReactiveManyEntityFetcher extends QuerydslDataFetcher implements TypedDataFetcher> { + private static class ReactiveManyEntityFetcher extends QuerydslDataFetcher implements SelfDescribingDataFetcher> { private final ReactiveQuerydslPredicateExecutor executor; @@ -732,7 +732,7 @@ public abstract class QuerydslDataFetcher { } @Override - public ResolvableType getDeclaredType() { + public ResolvableType getReturnType() { return ResolvableType.forClassWithGenerics(Flux.class, this.resultType); } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/execution/DefaultSchemaResourceGraphQlSourceBuilder.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/DefaultSchemaResourceGraphQlSourceBuilder.java index 3c96076c..9c67a078 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/execution/DefaultSchemaResourceGraphQlSourceBuilder.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/DefaultSchemaResourceGraphQlSourceBuilder.java @@ -139,11 +139,13 @@ final class DefaultSchemaResourceGraphQlSourceBuilder } }); - SchemaInspector.Report schemaInspectionReport = new SchemaInspector().inspectSchema(registry, runtimeWiring); - if(!schemaInspectionReport.isEmpty()) { - logger.info(schemaInspectionReport.getSummary()); - logger.info(schemaInspectionReport.getDetailedReport()); - } + configureGraphQl(builder -> { + GraphQLSchema schema = builder.build().getGraphQLSchema(); + SchemaMappingInspector.Report schemaInspectionReport = new SchemaMappingInspector().inspectSchemaMappings(schema, runtimeWiring); + if(!schemaInspectionReport.isEmpty()) { + logger.info(schemaInspectionReport.getSummary()); + } + }); return (this.schemaFactory != null ? this.schemaFactory.apply(registry, runtimeWiring) : diff --git a/spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaInspector.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaInspector.java deleted file mode 100644 index 86f3ea4e..00000000 --- a/spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaInspector.java +++ /dev/null @@ -1,270 +0,0 @@ -/* - * Copyright 2020-2023 the original author or authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.springframework.graphql.execution; - -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.function.Consumer; - -import graphql.language.FieldDefinition; -import graphql.language.ImplementingTypeDefinition; -import graphql.language.ListType; -import graphql.language.NonNullType; -import graphql.language.ObjectTypeDefinition; -import graphql.language.ObjectTypeExtensionDefinition; -import graphql.language.SDLExtensionDefinition; -import graphql.language.Type; -import graphql.language.TypeDefinition; -import graphql.language.TypeName; -import graphql.schema.DataFetcher; -import graphql.schema.idl.RuntimeWiring; -import graphql.schema.idl.TypeDefinitionRegistry; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; - -import org.springframework.beans.BeanUtils; -import org.springframework.beans.BeansException; -import org.springframework.core.ReactiveAdapter; -import org.springframework.core.ReactiveAdapterRegistry; -import org.springframework.core.ResolvableType; -import org.springframework.graphql.data.TypedDataFetcher; -import org.springframework.lang.Nullable; -import org.springframework.util.LinkedMultiValueMap; -import org.springframework.util.MultiValueMap; - -/** - * Inspect the GraphQL schema and look for inconsistencies with declared {@code @SchemaMapping} handlers and {@link DataFetcher}. - * The inspector will produce a {@link Report}, its content can be used for logging purposes. - *

This inspection utility will report to developers: - *

    - *
  • {@code Query}, {@code Mutation} and {@code Subscription} fields that have no corresponding {@link DataFetcher} registered - *
  • Fields in other schema types that have no property on the relevant Java type, or no DataFetcher registered - *
- *

This approach has several known limitations; the corresponding Java types are only discovered through registered - * {@code DataFetcher} instances, if they implement the {@link TypedDataFetcher} contract. Union types are not supported, - * even if a common interface is declared by a {@link TypedDataFetcher}. - * - * @author Brian Clozel - * @since 1.2.0 - */ -class SchemaInspector { - - private static final Log logger = LogFactory.getLog(SchemaInspector.class); - - Report inspectSchema(TypeDefinitionRegistry typeDefinitionRegistry, RuntimeWiring runtimeWiring) { - ReportBuilder report = ReportBuilder.create(); - SchemaInspection inspection = new SchemaInspection(typeDefinitionRegistry, runtimeWiring); - inspection.inspectOperation("Query", report); - inspection.inspectOperation("Mutation", report); - inspection.inspectOperation("Subscription", report); - return report.build(); - } - - private static class SchemaInspection { - - private final TypeDefinitionRegistry typeDefinitionRegistry; - - private final RuntimeWiring runtimeWiring; - - private final Set seenTypes = new HashSet<>(); - - SchemaInspection(TypeDefinitionRegistry typeDefinitionRegistry, RuntimeWiring runtimeWiring) { - this.typeDefinitionRegistry = typeDefinitionRegistry; - this.runtimeWiring = runtimeWiring; - } - - @SuppressWarnings("rawtypes") - void inspectOperation(String operationName, ReportBuilder report) { - Map queryFetchers = this.runtimeWiring.getDataFetcherForType(operationName); - this.typeDefinitionRegistry.getType(operationName, ObjectTypeDefinition.class) - .ifPresent(queryType -> inspectOperation(queryType, queryFetchers, report)); - forEachObjectTypeExtension(operationName, objectTypeExtension -> inspectOperation(objectTypeExtension, queryFetchers, report)); - } - - @SuppressWarnings("rawtypes") - private void inspectOperation(ObjectTypeDefinition operationDefinition, Map operationDataFetchers, ReportBuilder report) { - for (FieldDefinition fieldDefinition : operationDefinition.getFieldDefinitions()) { - if (operationDataFetchers.containsKey(fieldDefinition.getName())) { - DataFetcher fieldDataFetcher = operationDataFetchers.get(fieldDefinition.getName()); - if (fieldDataFetcher instanceof TypedDataFetcher typedDataFetcher) { - inspectType(fieldDefinition.getType(), typedDataFetcher.getDeclaredType(), report); - } - } - else { - report.missingOperation(operationDefinition, fieldDefinition); - } - } - } - - private void inspectType(Type fieldType, ResolvableType declaredType, ReportBuilder report) { - if (fieldType instanceof TypeName typeName) { - this.typeDefinitionRegistry.getType(typeName) - .ifPresent(typeDefinition -> inspectTypeDefinition(typeDefinition, declaredType, report)); - forEachObjectTypeExtension(typeName.getName(), - objectTypeExtension -> inspectTypeDefinition(objectTypeExtension, declaredType, report)); - } - else if (fieldType instanceof ListType listType) { - inspectType(listType.getType(), declaredType.getNested(2), report); - } - else if (fieldType instanceof NonNullType nonNullType) { - inspectType(nonNullType.getType(), declaredType, report); - } - } - - private void inspectTypeDefinition(TypeDefinition typeDefinition, ResolvableType declaredType, ReportBuilder report) { - if (typeDefinition instanceof ImplementingTypeDefinition implementingTypeDefinition) { - inspectImplementingType(implementingTypeDefinition, declaredType, report); - } - else if (logger.isDebugEnabled()){ - logger.debug("Cannot inspect type '" + typeDefinition.getName() + "', inspector does not support " - + typeDefinition.getClass().getSimpleName()); - } - } - - @SuppressWarnings("rawtypes") - private void inspectImplementingType(ImplementingTypeDefinition typeDefinition, ResolvableType declaredType, ReportBuilder report) { - if (isTypeAlreadyInspected(typeDefinition)) { - return; - } - Map typeDataFetcher = this.runtimeWiring.getDataFetcherForType(typeDefinition.getName()); - Class declaredClass = unwrapPublisherTypes(declaredType); - for (FieldDefinition field : typeDefinition.getFieldDefinitions()) { - if (typeDataFetcher.containsKey(field.getName())) { - DataFetcher fieldDataFetcher = typeDataFetcher.get(field.getName()); - if (fieldDataFetcher instanceof TypedDataFetcher typedFieldDataFetcher) { - inspectType(field.getType(), typedFieldDataFetcher.getDeclaredType(), report); - } - } - else { - try { - if (declaredClass == null || BeanUtils.getPropertyDescriptor(declaredClass, field.getName()) == null) { - report.missingField(typeDefinition, field); - } - } - catch (BeansException exc) { - logger.debug("Failed while inspecting " + declaredType + " for property " + field.getName() + "", exc); - } - } - } - for (Type interfaceType : typeDefinition.getImplements()) { - inspectType(interfaceType, declaredType, report); - } - } - - private void forEachObjectTypeExtension(String typeName, Consumer extensionsConsumer) { - List objectTypeExtensions = this.typeDefinitionRegistry.objectTypeExtensions().get(typeName); - if (objectTypeExtensions != null) { - objectTypeExtensions.forEach(extensionsConsumer); - } - } - - @Nullable - private Class unwrapPublisherTypes(ResolvableType declaredType) { - Class rawClass = declaredType.getRawClass(); - if (rawClass != null) { - ReactiveAdapter adapter = ReactiveAdapterRegistry.getSharedInstance().getAdapter(declaredType.getRawClass()); - if (adapter != null) { - return declaredType.getNested(2).getRawClass(); - } - } - return rawClass; - } - - private boolean isTypeAlreadyInspected(ImplementingTypeDefinition typeDefinition) { - if (typeDefinition instanceof SDLExtensionDefinition) { - return false; - } - boolean inspectedType = this.seenTypes.contains(typeDefinition.getName()); - if (!inspectedType) { - this.seenTypes.add(typeDefinition.getName()); - } - return inspectedType; - } - - } - - record Report(MultiValueMap missingOperations, MultiValueMap missingFields) { - - String getSummary() { - StringBuilder builder = new StringBuilder("GraphQL schema inspection found "); - if (this.missingOperations.isEmpty()) { - builder.append("no missing mappings for operations"); - } - else { - builder.append("missing mappings for ").append(this.missingOperations.keySet()); - } - if (this.missingFields.isEmpty()) { - builder.append(", no missing data fetchers for inspected types."); - } - else { - builder.append(", missing data fetchers for types ").append(this.missingFields.keySet()).append('.'); - } - return builder.toString(); - } - - String getDetailedReport() { - StringBuilder builder = new StringBuilder(); - this.missingOperations.keySet().forEach(operationName -> { - builder.append(String.format("- on %s: %s", operationName, this.missingOperations.get(operationName))) - .append(System.lineSeparator()); - }); - this.missingFields.keySet().forEach(typeName -> { - builder.append(String.format("- on %s: %s", typeName, this.missingFields.get(typeName))) - .append(System.lineSeparator()); - }); - return builder.toString(); - } - - boolean isEmpty() { - return this.missingOperations.isEmpty() && this.missingFields.isEmpty(); - } - - } - - private static class ReportBuilder { - - private final MultiValueMap missingOperations = new LinkedMultiValueMap<>(); - - private final MultiValueMap missingFields = new LinkedMultiValueMap<>(); - - private ReportBuilder() { - - } - - static ReportBuilder create() { - return new ReportBuilder(); - } - - ReportBuilder missingOperation(ImplementingTypeDefinition operationType, FieldDefinition operationDefinition) { - this.missingOperations.add(operationType.getName(), operationDefinition.getName()); - return this; - } - - ReportBuilder missingField(ImplementingTypeDefinition type, FieldDefinition field) { - this.missingFields.add(type.getName(), field.getName()); - return this; - } - - Report build() { - return new Report(this.missingOperations, this.missingFields); - } - - } - -} diff --git a/spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaMappingInspector.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaMappingInspector.java new file mode 100644 index 00000000..a5e9a82a --- /dev/null +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaMappingInspector.java @@ -0,0 +1,221 @@ +/* + * Copyright 2020-2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.graphql.execution; + +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import graphql.schema.DataFetcher; +import graphql.schema.GraphQLFieldDefinition; +import graphql.schema.GraphQLList; +import graphql.schema.GraphQLNamedType; +import graphql.schema.GraphQLNonNull; +import graphql.schema.GraphQLObjectType; +import graphql.schema.GraphQLSchema; +import graphql.schema.GraphQLType; +import graphql.schema.idl.RuntimeWiring; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +import org.springframework.beans.BeanUtils; +import org.springframework.beans.BeansException; +import org.springframework.core.ReactiveAdapter; +import org.springframework.core.ReactiveAdapterRegistry; +import org.springframework.core.ResolvableType; +import org.springframework.lang.Nullable; +import org.springframework.util.LinkedMultiValueMap; +import org.springframework.util.MultiValueMap; + +/** + * Inspect the GraphQL schema and look for inconsistencies with declared {@code @SchemaMapping} handlers and {@link DataFetcher}. + * The inspector will produce a {@link Report}, its content can be used for logging purposes. + *

This inspection utility will report to developers: + *

    + *
  • {@code Query}, {@code Mutation} and {@code Subscription} fields that have no corresponding {@link DataFetcher} registered + *
  • Fields in other schema types that have no property on the relevant Java type, or no DataFetcher registered + *
+ *

This approach has several known limitations; the corresponding Java types are only discovered through registered + * {@code DataFetcher} instances, if they implement the {@link SelfDescribingDataFetcher} contract. Union types are not supported, + * even if a common interface is declared by a {@link SelfDescribingDataFetcher}. + * + * @author Brian Clozel + * @since 1.2.0 + */ +class SchemaMappingInspector { + + private static final Log logger = LogFactory.getLog(SchemaMappingInspector.class); + + Report inspectSchemaMappings(GraphQLSchema schema, RuntimeWiring runtimeWiring) { + ReportBuilder report = ReportBuilder.create(); + SchemaMappingInspection inspection = new SchemaMappingInspection(runtimeWiring); + inspection.inspectOperation(schema.getQueryType(), report); + inspection.inspectOperation(schema.getMutationType(), report); + inspection.inspectOperation(schema.getSubscriptionType(), report); + return report.build(); + } + + private static class SchemaMappingInspection { + + private final RuntimeWiring runtimeWiring; + + private final Set seenTypes = new HashSet<>(); + + SchemaMappingInspection(RuntimeWiring runtimeWiring) { + this.runtimeWiring = runtimeWiring; + } + + @SuppressWarnings("rawtypes") + void inspectOperation(@Nullable GraphQLObjectType operationType, ReportBuilder report) { + if (operationType != null) { + Map operationDataFetchers = this.runtimeWiring.getDataFetcherForType(operationType.getName()); + for (GraphQLFieldDefinition fieldDefinition : operationType.getFieldDefinitions()) { + if (operationDataFetchers.containsKey(fieldDefinition.getName())) { + DataFetcher fieldDataFetcher = operationDataFetchers.get(fieldDefinition.getName()); + if (fieldDataFetcher instanceof SelfDescribingDataFetcher selfDescribingDataFetcher) { + inspectType(fieldDefinition.getType(), selfDescribingDataFetcher.getReturnType(), report); + } + } + else { + report.missingOperation(operationType, fieldDefinition); + } + } + } + } + + private void inspectType(GraphQLType type, ResolvableType declaredType, ReportBuilder report) { + if (type instanceof GraphQLObjectType objectType) { + inspectObjectType(objectType, declaredType, report); + } + else if (type instanceof GraphQLList listType) { + inspectType(listType.getWrappedType(), declaredType.getNested(2), report); + } + else if (type instanceof GraphQLNonNull nonNullType) { + inspectType(nonNullType.getWrappedType(), declaredType, report); + } + else if (type instanceof GraphQLNamedType namedType && logger.isTraceEnabled()){ + logger.trace("Cannot inspect type '" + namedType.getName() + "', inspector does not support " + + type.getClass().getSimpleName()); + } + } + + @SuppressWarnings("rawtypes") + private void inspectObjectType(GraphQLObjectType objectType, ResolvableType declaredType, ReportBuilder report) { + if (isTypeAlreadyInspected(objectType)) { + return; + } + Map typeDataFetcher = this.runtimeWiring.getDataFetcherForType(objectType.getName()); + Class declaredClass = unwrapPublisherTypes(declaredType); + for (GraphQLFieldDefinition field : objectType.getFieldDefinitions()) { + if (typeDataFetcher.containsKey(field.getName())) { + DataFetcher fieldDataFetcher = typeDataFetcher.get(field.getName()); + if (fieldDataFetcher instanceof SelfDescribingDataFetcher typedFieldDataFetcher) { + inspectType(field.getType(), typedFieldDataFetcher.getReturnType(), report); + } + } + else { + try { + if (declaredClass == null || BeanUtils.getPropertyDescriptor(declaredClass, field.getName()) == null) { + report.missingField(objectType, field); + } + } + catch (BeansException exc) { + logger.debug("Failed while inspecting " + declaredType + " for property " + field.getName() + "", exc); + } + } + } + } + + @Nullable + private Class unwrapPublisherTypes(ResolvableType declaredType) { + Class rawClass = declaredType.getRawClass(); + if (rawClass != null) { + ReactiveAdapter adapter = ReactiveAdapterRegistry.getSharedInstance().getAdapter(declaredType.getRawClass()); + if (adapter != null) { + return declaredType.getNested(2).getRawClass(); + } + } + return rawClass; + } + + private boolean isTypeAlreadyInspected(GraphQLObjectType objectType) { + return !this.seenTypes.add(objectType.getName()); + } + + } + + record Report(MultiValueMap missingOperations, MultiValueMap missingFields) { + + String getSummary() { + StringBuilder builder = new StringBuilder("GraphQL schema inspection found "); + if (this.missingOperations.isEmpty() && this.missingFields.isEmpty()) { + builder.append("no missing mapping."); + } + else { + builder.append(getDetailedReport()); + } + return builder.toString(); + } + + private String getDetailedReport() { + Stream missingOperationsReport = this.missingOperations.keySet().stream() + .map(operationName -> String.format("%s%s", operationName, this.missingOperations.get(operationName))); + Stream missingFieldsReport = this.missingFields.keySet().stream() + .map(typeName -> String.format("%s%s", typeName, this.missingFields.get(typeName))); + return Stream.concat(missingOperationsReport, missingFieldsReport) + .collect(Collectors.joining(", ", "missing mappings for: ", ".")); + } + + boolean isEmpty() { + return this.missingOperations.isEmpty() && this.missingFields.isEmpty(); + } + + } + + private static class ReportBuilder { + + private final MultiValueMap missingOperations = new LinkedMultiValueMap<>(); + + private final MultiValueMap missingFields = new LinkedMultiValueMap<>(); + + private ReportBuilder() { + + } + + static ReportBuilder create() { + return new ReportBuilder(); + } + + ReportBuilder missingOperation(GraphQLObjectType operationType, GraphQLFieldDefinition operationDefinition) { + this.missingOperations.add(operationType.getName(), operationDefinition.getName()); + return this; + } + + ReportBuilder missingField(GraphQLObjectType objectType, GraphQLFieldDefinition field) { + this.missingFields.add(objectType.getName(), field.getName()); + return this; + } + + Report build() { + return new Report(this.missingOperations, this.missingFields); + } + + } + +} diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/TypedDataFetcher.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/SelfDescribingDataFetcher.java similarity index 51% rename from spring-graphql/src/main/java/org/springframework/graphql/data/TypedDataFetcher.java rename to spring-graphql/src/main/java/org/springframework/graphql/execution/SelfDescribingDataFetcher.java index 4ee14891..1252f2a1 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/TypedDataFetcher.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/SelfDescribingDataFetcher.java @@ -14,28 +14,27 @@ * limitations under the License. */ -package org.springframework.graphql.data; +package org.springframework.graphql.execution; import graphql.schema.DataFetcher; -import graphql.schema.DataFetchingEnvironment; import org.springframework.core.ResolvableType; /** - * Specialized {@link DataFetcher} that can provide {@link ResolvableType type information} - * about the {@link #get(DataFetchingEnvironment) instances returned}. - *

Such {@code DataFetchers} are often backed by actual Java methods with declared return types. - * Declared types might not reflect the concrete type of the returned instance. + * Specialized {@link DataFetcher} that can provide information about itself. * @author Brian Clozel * @since 1.2.0 */ -public interface TypedDataFetcher extends DataFetcher { +public interface SelfDescribingDataFetcher extends DataFetcher { /** - * The type declared by this {@link DataFetcher}. - *

The concrete type of the returned instance might differ from the declared one. - * @return the declared type for the data to be fetched. + * The type that the {@link DataFetcher} returns. + *

This could be a type from a {@code @Controller} method declaration + * or the type expected to be returned by the {@link DataFetcher} + * possibly backed by a Spring Data repository. + * The concrete type of the returned instance might be a subclass. + * @return the type of the data returned by the data fetcher. */ - ResolvableType getDeclaredType(); + ResolvableType getReturnType(); } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaInspectorTests.java b/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorTests.java similarity index 78% rename from spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaInspectorTests.java rename to spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorTests.java index 2f28e5d3..5372fa87 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaInspectorTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorTests.java @@ -20,9 +20,9 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import graphql.schema.GraphQLSchema; import graphql.schema.idl.RuntimeWiring; -import graphql.schema.idl.SchemaParser; -import graphql.schema.idl.TypeDefinitionRegistry; +import graphql.schema.idl.SchemaGenerator; import org.assertj.core.api.AbstractAssert; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -42,11 +42,11 @@ import org.springframework.stereotype.Controller; import static org.assertj.core.api.Assertions.assertThat; /** - * Tests for {@link SchemaInspector}. + * Tests for {@link SchemaMappingInspector}. * * @author Brian Clozel */ -class SchemaInspectorTests { +class SchemaMappingInspectorTests { @Nested @@ -59,7 +59,7 @@ class SchemaInspectorTests { greeting: String } """; - SchemaInspector.Report report = inspectSchema(schema, EmptyController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, EmptyController.class); assertThatReport(report).hasSize(1).missesOperations("Query", "greeting"); } @@ -70,7 +70,7 @@ class SchemaInspectorTests { greeting: String } """; - SchemaInspector.Report report = inspectSchema(schema, GreetingController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, GreetingController.class); assertThatReport(report).isEmpty(); } @@ -87,7 +87,7 @@ class SchemaInspectorTests { missing: Boolean } """; - SchemaInspector.Report report = inspectSchema(schema, BookController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, BookController.class); assertThatReport(report).hasSize(1).missesFields("Book", "missing"); } @@ -100,7 +100,7 @@ class SchemaInspectorTests { greeting: String } """; - SchemaInspector.Report report = inspectSchema(schema, EmptyController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, EmptyController.class); assertThatReport(report).hasSize(1).missesOperations("Query", "greeting"); } @@ -111,6 +111,9 @@ class SchemaInspectorTests { @Test void hasMissingOperationEntryWhenMissingQueryMapping() { String schema = """ + type Query{ + greeting: String + } type Mutation { createBook: Book } @@ -120,13 +123,16 @@ class SchemaInspectorTests { name: String } """; - SchemaInspector.Report report = inspectSchema(schema, EmptyController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, GreetingController.class); assertThatReport(report).hasSize(1).missesOperations("Mutation", "createBook"); } @Test void reportIsEmptyWhenMutationMapping() { String schema = """ + type Query{ + greeting: String + } type Mutation { createBook: Book } @@ -136,13 +142,16 @@ class SchemaInspectorTests { name: String } """; - SchemaInspector.Report report = inspectSchema(schema, BookController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, GreetingController.class, BookController.class); assertThatReport(report).isEmpty(); } @Test void inspectExtensionTypesForMutations() { String schema = """ + type Query { + greeting: String + } type Mutation { } extend type Mutation { @@ -153,7 +162,7 @@ class SchemaInspectorTests { name: String } """; - SchemaInspector.Report report = inspectSchema(schema, EmptyController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, GreetingController.class); assertThatReport(report).hasSize(1).missesOperations("Mutation", "createBook"); } @@ -164,6 +173,9 @@ class SchemaInspectorTests { @Test void hasMissingOperationEntryWhenMissingSubscriptionMapping() { String schema = """ + type Query{ + greeting: String + } type Subscription { bookSearch(author: String) : Book! } @@ -173,13 +185,16 @@ class SchemaInspectorTests { name: String } """; - SchemaInspector.Report report = inspectSchema(schema, EmptyController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, GreetingController.class); assertThatReport(report).hasSize(1).missesOperations("Subscription", "bookSearch"); } @Test void reportIsEmptyWhenSubscriptionMapping() { String schema = """ + type Query{ + greeting: String + } type Subscription { bookSearch(author: String) : Book! } @@ -189,13 +204,16 @@ class SchemaInspectorTests { name: String } """; - SchemaInspector.Report report = inspectSchema(schema, BookController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, GreetingController.class, BookController.class); assertThatReport(report).isEmpty(); } @Test void inspectExtensionTypesForSubscriptions() { String schema = """ + type Query{ + greeting: String + } type Subscription { } extend type Subscription { @@ -206,7 +224,7 @@ class SchemaInspectorTests { name: String } """; - SchemaInspector.Report report = inspectSchema(schema, EmptyController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, GreetingController.class); assertThatReport(report).hasSize(1).missesOperations("Subscription", "bookSearch"); } @@ -226,7 +244,7 @@ class SchemaInspectorTests { name: String } """; - SchemaInspector.Report report = inspectSchema(schema, BookController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, BookController.class); assertThatReport(report).isEmpty(); } @@ -243,7 +261,7 @@ class SchemaInspectorTests { fetcher: String } """; - SchemaInspector.Report report = inspectSchema(schema, BookController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, BookController.class); assertThatReport(report).isEmpty(); } @@ -260,7 +278,7 @@ class SchemaInspectorTests { missing: Boolean } """; - SchemaInspector.Report report = inspectSchema(schema, BookController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, BookController.class); assertThatReport(report).hasSize(1).missesFields("Book", "missing"); } @@ -283,7 +301,7 @@ class SchemaInspectorTests { missing: String } """; - SchemaInspector.Report report = inspectSchema(schema, BookController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, BookController.class); assertThatReport(report).hasSize(1).missesFields("Author", "missing"); } @@ -304,7 +322,7 @@ class SchemaInspectorTests { team: Team } """; - SchemaInspector.Report report = inspectSchema(schema, TeamController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, TeamController.class); assertThatReport(report).isEmpty(); } @@ -323,29 +341,10 @@ class SchemaInspectorTests { missing: Boolean } """; - SchemaInspector.Report report = inspectSchema(schema, BookController.class); + SchemaMappingInspector.Report report = inspectSchema(schema, BookController.class); assertThatReport(report).hasSize(1).missesFields("Book", "missing"); } - @Test - void hasMissingFieldEntryWhenMissingPropertyOnTypeProvidedByInterface() { - String schema = """ - type Query { - bookById(id: ID): Book - } - - interface LibraryItem { - missing: Boolean - } - type Book implements LibraryItem { - id: ID - name: String - } - """; - SchemaInspector.Report report = inspectSchema(schema, BookController.class); - assertThatReport(report).hasSize(1).missesFields("LibraryItem", "missing"); - } - } @Nested @@ -358,11 +357,8 @@ class SchemaInspectorTests { greeting: String } """; - SchemaInspector.Report report = inspectSchema(schema, EmptyController.class); - assertThat(report.getSummary()).isEqualTo("GraphQL schema inspection found missing mappings for [Query], no missing data fetchers for inspected types."); - assertThat(report.getDetailedReport()).isEqualTo(""" - - on Query: [greeting] - """); + SchemaMappingInspector.Report report = inspectSchema(schema, EmptyController.class); + assertThat(report.getSummary()).isEqualTo("GraphQL schema inspection found missing mappings for: Query[greeting]."); } @Test @@ -378,11 +374,8 @@ class SchemaInspectorTests { missing: Boolean } """; - SchemaInspector.Report report = inspectSchema(schema, BookController.class); - assertThat(report.getSummary()).isEqualTo("GraphQL schema inspection found no missing mappings for operations, missing data fetchers for types [Book]."); - assertThat(report.getDetailedReport()).isEqualTo(""" - - on Book: [missing] - """); + SchemaMappingInspector.Report report = inspectSchema(schema, BookController.class); + assertThat(report.getSummary()).isEqualTo("GraphQL schema inspection found missing mappings for: Book[missing]."); } } @@ -463,18 +456,12 @@ class SchemaInspectorTests { } - SchemaInspector.Report inspectSchema(String schema, Class... controllers) { - TypeDefinitionRegistry typeDefinitionRegistry = loadTypeDefinitionRegistryFromSchema(schema); + SchemaMappingInspector.Report inspectSchema(String schemaContent, Class... controllers) { + GraphQLSchema schema = SchemaGenerator.createdMockedSchema(schemaContent); RuntimeWiring.Builder builder = createRuntimeWiring(controllers); - return new SchemaInspector().inspectSchema(typeDefinitionRegistry, builder.build()); + return new SchemaMappingInspector().inspectSchemaMappings(schema, builder.build()); } - - TypeDefinitionRegistry loadTypeDefinitionRegistryFromSchema(String schema) { - return new SchemaParser().parse(schema); - } - - RuntimeWiring.Builder createRuntimeWiring(Class... handlerTypes) { AnnotationConfigApplicationContext appContext = new AnnotationConfigApplicationContext(); for (Class handlerType : handlerTypes) { @@ -491,13 +478,13 @@ class SchemaInspectorTests { return wiringBuilder; } - static SchemaInspectionReportAssert assertThatReport(SchemaInspector.Report actual) { + static SchemaInspectionReportAssert assertThatReport(SchemaMappingInspector.Report actual) { return new SchemaInspectionReportAssert(actual); } - static class SchemaInspectionReportAssert extends AbstractAssert { + static class SchemaInspectionReportAssert extends AbstractAssert { - public SchemaInspectionReportAssert(SchemaInspector.Report actual) { + public SchemaInspectionReportAssert(SchemaMappingInspector.Report actual) { super(actual, SchemaInspectionReportAssert.class); }