diff --git a/spring-graphql-docs/src/docs/asciidoc/index.adoc b/spring-graphql-docs/src/docs/asciidoc/index.adoc index 930241b9..58841c0d 100644 --- a/spring-graphql-docs/src/docs/asciidoc/index.adoc +++ b/spring-graphql-docs/src/docs/asciidoc/index.adoc @@ -418,21 +418,25 @@ controllers, the return type is derived from the declared return type on a `@SchemaMapping` method. On startup, Spring for GraphQL inspects all schema fields, `DataFetcher` registrations, -and the properties of Java objects returned from `DataFetcher` implementations in order -to ensure that every schema field has either an explicitly registered `DataFetcher`, or -a matching Java object property. This inspection is performed automatically, and results -in a report that is always logged on startup at INFO level. For example: +and the properties of Java objects returned from `DataFetcher` implementations to check +if all schema fields are covered either by an explicitly registered `DataFetcher`, or +a matching Java object property. The inspection also performs a reverse check looking for +`DataFetcher` registrations against schema fields that don't exist. This inspection is +performed automatically, and results in a report that is logged at INFO level on startup. +For example: ---- GraphQL schema inspection: Unmapped fields: {Book=[title], Author[firstName, lastName]} // <1> - Skipped types: [BookOrAuthor] // <2> + Unmapped DataFetcher registrations: {Book.reviews=BookController#reviews[1 args]} <2> + Skipped types: [BookOrAuthor] // <3> ---- <1> List of schema fields and their source types that are not mapped -<2> List of schema types that are skipped, as explained next +<2> List of `DataFetcher` registrations on fields that don't exist +<3> List of schema types that are skipped, as explained next -There are limits to what schema mappings inspection can do, in particular when there is +There are limits to what schema field inspection can do, in particular when there is insufficient Java type information. This is the case if an annotated controller method is declared to return `java.lang.Object`, or if the return type has an unspecified generic parameter such as `List`, or if the `DataFetcher` does not implement 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 a87cd83b..3bd764cb 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 @@ -640,6 +640,11 @@ public class AnnotatedControllerConfigurer implements ApplicationContextAware, I this.subscription = this.info.getCoordinates().getTypeName().equalsIgnoreCase("Subscription"); } + @Override + public String getDescription() { + return this.info.getHandlerMethod().getShortLogMessage(); + } + @Override public ResolvableType getReturnType() { return ResolvableType.forMethodReturnType(this.info.getHandlerMethod().getMethod()); @@ -702,6 +707,11 @@ public class AnnotatedControllerConfigurer implements ApplicationContextAware, I .switchIfEmpty(Mono.error(ex)); } + @Override + public String toString() { + return getDescription(); + } + } 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 fb62e905..ffffb115 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 @@ -120,6 +120,15 @@ public abstract class QueryByExampleDataFetcher { } + /** + * Provides shared implementation of + * {@link SelfDescribingDataFetcher#getDescription()} for all subclasses. + * @since 1.2.0 + */ + public String getDescription() { + return "QueryByExampleDataFetcher<" + this.domainType.getType().getName() + ">"; + } + /** * Prepare an {@link Example} from GraphQL request arguments. * @param environment contextual info for the GraphQL request @@ -170,6 +179,11 @@ public abstract class QueryByExampleDataFetcher { return RepositoryUtils.buildScrollSubrange(environment, this.cursorStrategy); } + @Override + public String toString() { + return getDescription(); + } + /** * Create a new {@link Builder} accepting {@link QueryByExampleExecutor} 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 7ef76c95..dc8bc804 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 @@ -134,6 +134,15 @@ public abstract class QuerydslDataFetcher { } + /** + * Provides shared implementation of + * {@link SelfDescribingDataFetcher#getDescription()} for all subclasses. + * @since 1.2.0 + */ + public String getDescription() { + return "QuerydslDataFetcher<" + this.domainType.getType().getName() + ">"; + } + /** * Prepare a {@link Predicate} from GraphQL request arguments, also applying * any {@link QuerydslBinderCustomizer} that may have been configured. @@ -194,6 +203,11 @@ public abstract class QuerydslDataFetcher { return RepositoryUtils.buildScrollSubrange(environment, this.cursorStrategy); } + @Override + public String toString() { + return getDescription(); + } + /** * Create a new {@link Builder} accepting {@link QuerydslPredicateExecutor} 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 index 744fac55..b00362bc 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaMappingInspector.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaMappingInspector.java @@ -17,11 +17,13 @@ package org.springframework.graphql.execution; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.Map; import java.util.Set; import graphql.schema.DataFetcher; +import graphql.schema.FieldCoordinates; import graphql.schema.GraphQLEnumType; import graphql.schema.GraphQLFieldDefinition; import graphql.schema.GraphQLFieldsContainer; @@ -51,9 +53,11 @@ import org.springframework.util.MultiValueMap; * Declares an {@link #inspect(GraphQLSchema, RuntimeWiring)} method that checks * if schema fields are covered either by a {@link DataFetcher} registration, * or match a Java object property. Fields that have neither are reported as - * "unmapped" in the resulting {@link Report Resport}. + * "unmapped" in the resulting {@link Report Resport}. The inspection also + * performs a reverse check for {@code DataFetcher} registrations against schema + * fields that don't exist. * - *

The inspection depends on {@code DataFetcher}s to be + *

The schema field inspection depends on {@code DataFetcher}s to be * {@link SelfDescribingDataFetcher} to be able to compare schema type and Java * object type structure. If a {@code DataFetcher} does not implement this * interface, then the Java type remains unknown, and the field type is reported @@ -108,21 +112,23 @@ class SchemaMappingInspector { */ public Report inspect() { - inspectType(this.schema.getQueryType(), null); + inspectSchemaType(this.schema.getQueryType(), null); if (this.schema.isSupportingMutations()) { - inspectType(this.schema.getMutationType(), null); + inspectSchemaType(this.schema.getMutationType(), null); } if (this.schema.isSupportingSubscriptions()) { - inspectType(this.schema.getSubscriptionType(), null); + inspectSchemaType(this.schema.getSubscriptionType(), null); } + inspectDataFetcherRegistrations(); + return this.reportBuilder.build(); } @SuppressWarnings("rawtypes") - private void inspectType(GraphQLType type, @Nullable ResolvableType resolvableType) { + private void inspectSchemaType(GraphQLType type, @Nullable ResolvableType resolvableType) { Assert.notNull(type, "No GraphQLType"); type = unwrapNonNull(type); @@ -166,9 +172,9 @@ class SchemaMappingInspector { for (GraphQLFieldDefinition field : fieldContainer.getFieldDefinitions()) { String fieldName = field.getName(); if (dataFetcherMap.containsKey(fieldName)) { - DataFetcher fetcher = dataFetcherMap.get(fieldName); + DataFetcher fetcher = dataFetcherMap.get(fieldName); if (fetcher instanceof SelfDescribingDataFetcher selfDescribingDataFetcher) { - inspectType(field.getType(), selfDescribingDataFetcher.getReturnType()); + inspectSchemaType(field.getType(), selfDescribingDataFetcher.getReturnType()); } else if (isNotScalarOrEnumType(field.getType())) { if (logger.isDebugEnabled()) { @@ -233,6 +239,17 @@ class SchemaMappingInspector { return (adapter != null ? resolvableType.getNested(2).resolve(Object.class) : clazz); } + @SuppressWarnings("rawtypes") + private void inspectDataFetcherRegistrations() { + this.runtimeWiring.getDataFetchers().forEach((typeName, registrations) -> + registrations.forEach((fieldName, fetcher) -> { + FieldCoordinates coordinates = FieldCoordinates.coordinates(typeName, fieldName); + if (this.schema.getFieldDefinition(coordinates) == null) { + this.reportBuilder.addUnmappedDataFetcher(coordinates, fetcher); + } + })); + } + /** * Check the schema against {@code DataFetcher} registrations, and produce a report. @@ -249,15 +266,20 @@ class SchemaMappingInspector { /** * The report produced as a result of schema mappings inspection. - * @param unmappedFields a map with type names as keys, and unmapped field names as values + * @param unmappedFields map with type names as keys, and unmapped field names as values + * @param unmappedDataFetchers map with unmapped {@code DataFetcher}s and their field coordinates * @param skippedTypes the names of types skipped by the inspection */ - public record Report(MultiValueMap unmappedFields, Set skippedTypes) { + public record Report( + MultiValueMap unmappedFields, + Map> unmappedDataFetchers, + Set skippedTypes) { @Override public String toString() { return "GraphQL schema inspection:\n" + "\tUnmapped fields: " + this.unmappedFields + "\n" + + "\tUnmapped DataFetcher registrations: " + this.unmappedDataFetchers + "\n" + "\tSkipped types: " + this.skippedTypes; } } @@ -270,6 +292,8 @@ class SchemaMappingInspector { private final MultiValueMap unmappedFields = new LinkedMultiValueMap<>(); + private final Map> unmappedDataFetchers = new LinkedHashMap<>(); + private final Set skippedTypes = new LinkedHashSet<>(); /** @@ -279,6 +303,13 @@ class SchemaMappingInspector { this.unmappedFields.add(typeName, fieldName); } + /** + * Add an unmapped {@code DataFetcher} registration. + */ + public void addUnmappedDataFetcher(FieldCoordinates coordinates, DataFetcher dataFetcher) { + this.unmappedDataFetchers.put(coordinates, dataFetcher); + } + /** * Add a skipped type name. */ @@ -289,6 +320,7 @@ class SchemaMappingInspector { public Report build() { return new Report( new LinkedMultiValueMap<>(this.unmappedFields), + new LinkedHashMap<>(this.unmappedDataFetchers), new LinkedHashSet<>(this.skippedTypes)); } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/execution/SelfDescribingDataFetcher.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/SelfDescribingDataFetcher.java index ea5eb325..643d431b 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/execution/SelfDescribingDataFetcher.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/SelfDescribingDataFetcher.java @@ -30,6 +30,14 @@ import org.springframework.core.ResolvableType; */ public interface SelfDescribingDataFetcher extends DataFetcher { + /** + * Provide a description of the {@code DataFetcher} for display or logging + * purposes. Depending on the underlying implementation, this could be a + * controller method, a Spring Data repository backed {@code DataFetcher}, + * or other. + */ + String getDescription(); + /** * The return type of this {@link DataFetcher}. *

This could be derived from the method signature of an annotated diff --git a/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorTests.java b/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorTests.java index 446a14f6..9bde1ea6 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorTests.java @@ -21,6 +21,7 @@ import java.util.Collections; import java.util.List; import java.util.Set; +import graphql.schema.FieldCoordinates; import graphql.schema.GraphQLSchema; import graphql.schema.idl.RuntimeWiring; import graphql.schema.idl.SchemaGenerator; @@ -181,7 +182,7 @@ class SchemaMappingInspectorTests { } """; SchemaMappingInspector.Report report = inspectSchema(schema, GreetingController.class, BookController.class); - assertThatReport(report).isEmpty(); + assertThatReport(report).hasUnmappedFieldCount(0).hasSkippedTypeCount(0); } @Test @@ -245,7 +246,7 @@ class SchemaMappingInspectorTests { } """; SchemaMappingInspector.Report report = inspectSchema(schema, GreetingController.class, BookController.class); - assertThatReport(report).isEmpty(); + assertThatReport(report).hasUnmappedFieldCount(0).hasSkippedTypeCount(0); } @Test @@ -287,7 +288,7 @@ class SchemaMappingInspectorTests { } """; SchemaMappingInspector.Report report = inspectSchema(schema, BookController.class); - assertThatReport(report).isEmpty(); + assertThatReport(report).hasUnmappedFieldCount(0).hasSkippedTypeCount(0); } @Test @@ -304,7 +305,7 @@ class SchemaMappingInspectorTests { } """; SchemaMappingInspector.Report report = inspectSchema(schema, BookController.class); - assertThatReport(report).isEmpty(); + assertThatReport(report).hasUnmappedFieldCount(0).hasSkippedTypeCount(0); } @Test @@ -324,6 +325,17 @@ class SchemaMappingInspectorTests { assertThatReport(report).hasUnmappedFieldCount(1).containsUnmappedFields("Book", "missing"); } + @Test + void reportHasUnmappedDataFetcher() { + String schema = """ + type Query { + anything: String + } + """; + SchemaMappingInspector.Report report = inspectSchema(schema, GreetingController.class); + assertThatReport(report).hasUnmappedDataFetcherCount(1).containsUnmappedDataFetchersFor("Query", "greeting"); + } + @Test void reportHasUnmappedFieldOnNestedType() { String schema = """ @@ -431,6 +443,22 @@ class SchemaMappingInspectorTests { assertThatReport(report).hasUnmappedFieldCount(0).hasSkippedTypeCount(1).containsSkippedTypes("Book"); } + @Test + void reportHasSkippedTypeForObjectReturnType() { + String schemaContent = """ + type Query { + bookObject(id: ID): Book + } + + type Book { + id: ID + name: String + } + """; + SchemaMappingInspector.Report report = inspectSchema(schemaContent, BookController.class); + assertThatReport(report).hasUnmappedFieldCount(0).hasSkippedTypeCount(1).containsSkippedTypes("Book"); + } + @Test void reportIsEmptyIfUnknownDataFetcherReturnsSimpleType() { String schemaContent = """ @@ -448,22 +476,6 @@ class SchemaMappingInspectorTests { assertThatReport(report).hasUnmappedFieldCount(0).hasSkippedTypeCount(0); } - @Test - void reportHasSkippedTypeForObjectReturnType() { - String schemaContent = """ - type Query { - bookObject(id: ID): Book - } - - type Book { - id: ID - name: String - } - """; - SchemaMappingInspector.Report report = inspectSchema(schemaContent, BookController.class); - assertThatReport(report).hasUnmappedFieldCount(0).hasSkippedTypeCount(1).containsSkippedTypes("Book"); - } - } @@ -476,18 +488,28 @@ class SchemaMappingInspectorTests { type Query { allBooks: [Book] } - + type Mutation { + createBook: Book + } + type Subscription { + bookSearch(author: String) : Book! + } type Book { id: ID name: String missing: Boolean + author: Author } + type Author { + id: ID + } """; SchemaMappingInspector.Report report = inspectSchema(schema, BookController.class); assertThatReport(report).hasUnmappedFieldCount(1).hasSkippedTypeCount(0); assertThat(report.toString()).isEqualTo(""" GraphQL schema inspection: Unmapped fields: {Book=[missing]} + Unmapped DataFetcher registrations: {Book.fetcher=BookController#fetcher[1 args], Query.paginatedBooks=BookController#paginatedBooks[0 args], Query.bookObject=BookController#bookObject[1 args], Query.bookById=BookController#bookById[1 args]} Skipped types: []"""); } @@ -631,7 +653,13 @@ class SchemaMappingInspectorTests { public void isEmpty() { isNotNull(); if (!this.actual.unmappedFields().isEmpty()) { - failWithMessage("Report contains missing fields for %s", this.actual.unmappedFields().keySet()); + failWithMessage("Report contains missing fields: %s", this.actual.unmappedFields()); + } + if (!this.actual.unmappedDataFetchers().isEmpty()) { + failWithMessage("Report contains missing DataFetcher registrations for %s", this.actual.unmappedDataFetchers()); + } + if (!this.actual.skippedTypes().isEmpty()) { + failWithMessage("Report contains skipped types: %s", this.actual.skippedTypes()); } } @@ -644,6 +672,14 @@ class SchemaMappingInspectorTests { return this; } + public SchemaInspectionReportAssert hasUnmappedDataFetcherCount(int expected) { + isNotNull(); + if (this.actual.unmappedDataFetchers().size() != expected) { + failWithMessage("Expected %s unmapped fields, found %s.", expected, this.actual.unmappedFields()); + } + return this; + } + public SchemaInspectionReportAssert hasSkippedTypeCount(int expected) { isNotNull(); if (this.actual.skippedTypes().size() != expected) { @@ -662,6 +698,17 @@ class SchemaMappingInspectorTests { return this; } + public SchemaInspectionReportAssert containsUnmappedDataFetchersFor(String typeName, String... fieldNames) { + isNotNull(); + List expected = Arrays.stream(fieldNames) + .map(field -> FieldCoordinates.coordinates(typeName, field)).toList(); + if (!this.actual.unmappedDataFetchers().keySet().containsAll(expected)) { + failWithMessage("Expected unmapped DataFetchers for %s, found %s", + expected, this.actual.unmappedDataFetchers()); + } + return this; + } + public SchemaInspectionReportAssert containsSkippedTypes(String... typeNames) { isNotNull(); List expected = Arrays.asList(typeNames);