diff --git a/spring-graphql-docs/modules/ROOT/pages/request-execution.adoc b/spring-graphql-docs/modules/ROOT/pages/request-execution.adoc index 3b027486..fdc9b408 100644 --- a/spring-graphql-docs/modules/ROOT/pages/request-execution.adoc +++ b/spring-graphql-docs/modules/ROOT/pages/request-execution.adoc @@ -229,24 +229,25 @@ matching Java object property, will always be `null`. GraphQL Java does not perform checks to ensure every schema field is covered, and that can result in gaps that might not be discovered depending on test coverage. At runtime you may get a "silent" `null`, or an error if the field is not nullable. As a lower level -library, GraphQL Java simply does not know enough about `DataFetcher` implementations and -their return types, and therefore can't compare schema type structure against Java object -structure. +library, GraphQL Java simply does not have enough information about `DataFetcher` +implementations to know their return types or what arguments they depend on, and as a result +cannot perform such verifications. Spring for GraphQL defines the `SelfDescribingDataFetcher` interface to allow a -`DataFetcher` to expose return type information. All Spring `DataFetcher` implementations +`DataFetcher` to expose information about itself. All Spring `DataFetcher` implementations implement this interface. That includes those for xref:controllers.adoc[Annotated Controllers], and those for -xref:data.adoc#data.querydsl[Querydsl] and xref:data.adoc#data.querybyexample[Query by Example] Spring Data repositories. For annotated -controllers, the return type is derived from the declared return type on a -`@SchemaMapping` method. +xref:data.adoc#data.querydsl[Querydsl] and +xref:data.adoc#data.querybyexample[Query by Example] Spring Data repositories. +For annotated controllers, the return type is derived from the declared return type on +a `@SchemaMapping` method, while arguments are dervied from `@Argument` method parameters. -On startup, Spring for GraphQL can inspect schema fields, `DataFetcher` registrations, -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. +Spring for GraphQL can perform an inspection on startup to ensure the following: -To enable inspection of schema mappings: +- Schema fields have a `DataFetcher` registration or a corresponding Java property. +- `DataFetcher` registrations refer to a schema field that does exist. +- `DataFetcher` refers to schema arguments that exist. + +You can enable the inspection and take an appropriate action as follows: [source,java,indent=0,subs="verbatim,quotes"] ---- @@ -264,14 +265,16 @@ Below is an example report: GraphQL schema inspection: Unmapped fields: {Book=[title], Author[firstName, lastName]} // <1> Unmapped registrations: {Book.reviews=BookController#reviews[1 args]} <2> - Skipped types: [BookOrAuthor] // <3> + Unmapped arguments: {BookController#bookSearch[1 args]=[myAuthor]} // <3> + Skipped types: [BookOrAuthor] // <4> ---- -<1> List of schema fields and their source types that are not mapped -<2> List of `DataFetcher` registrations on fields that don't exist -<3> List of schema types that are skipped, as explained next +<1> Coordinates of schema fields that are not covered +<2> ``DataFetcher`` registered mapped to fields that don't exist +<3> `DataFetcher` arguments that don't exist +<4> Schema types that have been skipped (explained next) -There are limits to what schema field inspection can do, in particular when there is +There are limits to what schema 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 94e5012d..4b708aeb 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 @@ -29,6 +29,8 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.Executor; import java.util.function.BiConsumer; import java.util.function.Consumer; +import java.util.function.Predicate; +import java.util.stream.Collectors; import graphql.execution.DataFetcherResult; import graphql.schema.DataFetcher; @@ -45,15 +47,19 @@ import reactor.core.publisher.Mono; import org.springframework.beans.factory.NoSuchBeanDefinitionException; import org.springframework.context.ApplicationContext; import org.springframework.context.expression.BeanFactoryResolver; +import org.springframework.core.DefaultParameterNameDiscoverer; import org.springframework.core.KotlinDetector; import org.springframework.core.MethodParameter; +import org.springframework.core.ParameterNameDiscoverer; import org.springframework.core.ResolvableType; import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.data.domain.ScrollPosition; +import org.springframework.graphql.data.ArgumentValue; import org.springframework.graphql.data.GraphQlArgumentBinder; import org.springframework.graphql.data.method.HandlerMethod; import org.springframework.graphql.data.method.HandlerMethodArgumentResolver; import org.springframework.graphql.data.method.HandlerMethodArgumentResolverComposite; +import org.springframework.graphql.data.method.annotation.Argument; import org.springframework.graphql.data.method.annotation.BatchMapping; import org.springframework.graphql.data.method.annotation.SchemaMapping; import org.springframework.graphql.data.pagination.CursorStrategy; @@ -380,6 +386,8 @@ public class AnnotatedControllerConfigurer */ static class SchemaMappingDataFetcher implements SelfDescribingDataFetcher { + private static final ParameterNameDiscoverer parameterNameDiscoverer = new DefaultParameterNameDiscoverer(); + private final DataFetcherMappingInfo mappingInfo; private final HandlerMethodArgumentResolverComposite argumentResolvers; @@ -424,6 +432,20 @@ public class AnnotatedControllerConfigurer return ResolvableType.forMethodReturnType(this.mappingInfo.getHandlerMethod().getMethod()); } + @Override + public Map getArguments() { + + Predicate argumentPredicate = p -> + (p.getParameterAnnotation(Argument.class) != null || p.getParameterType() == ArgumentValue.class); + + return Arrays.stream(this.mappingInfo.getHandlerMethod().getMethodParameters()) + .filter(argumentPredicate) + .peek(p -> p.initParameterNameDiscovery(parameterNameDiscoverer)) + .collect(Collectors.toMap( + ArgumentMethodArgumentResolver::getArgumentName, + ResolvableType::forMethodParameter)); + } + /** * Return the {@link HandlerMethod} used to fetch data. */ 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 10f94b07..d9f0ec29 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 @@ -49,6 +49,7 @@ import org.springframework.core.ReactiveAdapterRegistry; import org.springframework.core.ResolvableType; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; @@ -135,13 +136,13 @@ public class SchemaMappingInspector { } /** - * Check the given {@code GraphQLFieldsContainer} against {@code DataFetcher} - * registrations, or Java properties of the given {@code ResolvableType}. - * @param fieldContainer the GraphQL interface or object type to check - * @param resolvableType the Java type to match against, or {@code null} if - * not applicable such as for Query, Mutation, or Subscription + * Check fields of the given {@code GraphQLFieldsContainer} to make sure there + * is either a {@code DataFetcher} registration, or a corresponding property + * in the given Java type, which may be {@code null} for the top-level types + * Query, Mutation, and Subscription. */ - private void checkFieldsContainer(GraphQLFieldsContainer fieldContainer, @Nullable ResolvableType resolvableType) { + private void checkFieldsContainer( + GraphQLFieldsContainer fieldContainer, @Nullable ResolvableType resolvableType) { String typeName = fieldContainer.getName(); Map dataFetcherMap = this.dataFetchers.getOrDefault(typeName, Collections.emptyMap()); @@ -159,16 +160,21 @@ public class SchemaMappingInspector { } /** - * Check the output {@link GraphQLType} of a field against the given DataFetcher return type. - * @param parent the parent of the field - * @param field the field to inspect - * @param dataFetcher the registered DataFetcher + * Perform the following: + *
    + *
  • Resolve the field type and the {@code DataFetcher} return type, and recurse + * with {@link #checkFieldsContainer} if there is sufficient type information. + *
  • Resolve the arguments the {@code DataFetcher} depends on and check they + * are defined in the schema. + *
*/ - private void checkField(GraphQLFieldsContainer parent, GraphQLFieldDefinition field, DataFetcher dataFetcher) { + private void checkField( + GraphQLFieldsContainer parent, GraphQLFieldDefinition field, DataFetcher dataFetcher) { ResolvableType resolvableType = ResolvableType.NONE; - if (dataFetcher instanceof SelfDescribingDataFetcher selfDescribingDataFetcher) { - resolvableType = selfDescribingDataFetcher.getReturnType(); + if (dataFetcher instanceof SelfDescribingDataFetcher selfDescribing) { + resolvableType = selfDescribing.getReturnType(); + checkFieldArguments(field, selfDescribing); } // Remove GraphQL type wrappers, and nest within Java generic types @@ -210,6 +216,17 @@ public class SchemaMappingInspector { checkFieldsContainer(fieldContainer, resolvableType); } + private void checkFieldArguments(GraphQLFieldDefinition field, SelfDescribingDataFetcher dataFetcher) { + + List arguments = dataFetcher.getArguments().keySet().stream() + .filter(name -> field.getArgument(name) == null) + .toList(); + + if (!arguments.isEmpty()) { + this.reportBuilder.unmappedArgument(dataFetcher, arguments); + } + } + private GraphQLType unwrapIfNonNull(GraphQLType type) { return (type instanceof GraphQLNonNull graphQLNonNull ? graphQLNonNull.getWrappedType() : type); } @@ -347,6 +364,8 @@ public class SchemaMappingInspector { private final Map> unmappedRegistrations = new LinkedHashMap<>(); + private final MultiValueMap, String> unmappedArguments = new LinkedMultiValueMap<>(); + private final List skippedTypes = new ArrayList<>(); public void unmappedField(FieldCoordinates coordinates) { @@ -357,12 +376,17 @@ public class SchemaMappingInspector { this.unmappedRegistrations.put(coordinates, dataFetcher); } + public void unmappedArgument(DataFetcher dataFetcher, List arguments) { + this.unmappedArguments.put(dataFetcher, arguments); + } + public void skippedType(GraphQLType type, FieldCoordinates coordinates) { this.skippedTypes.add(new DefaultSkippedType(type, coordinates)); } public SchemaReport build() { - return new DefaultSchemaReport(this.unmappedFields, this.unmappedRegistrations, this.skippedTypes); + return new DefaultSchemaReport( + this.unmappedFields, this.unmappedRegistrations, this.unmappedArguments, this.skippedTypes); } } @@ -377,14 +401,17 @@ public class SchemaMappingInspector { private final Map> unmappedRegistrations; + private final MultiValueMap, String> unmappedArguments; + private final List skippedTypes; public DefaultSchemaReport( List unmappedFields, Map> unmappedRegistrations, - List skippedTypes) { + MultiValueMap, String> unmappedArguments, List skippedTypes) { this.unmappedFields = Collections.unmodifiableList(unmappedFields); this.unmappedRegistrations = Collections.unmodifiableMap(unmappedRegistrations); + this.unmappedArguments = CollectionUtils.unmodifiableMultiValueMap(unmappedArguments); this.skippedTypes = Collections.unmodifiableList(skippedTypes); } @@ -398,6 +425,11 @@ public class SchemaMappingInspector { return this.unmappedRegistrations; } + @Override + public MultiValueMap, String> unmappedArguments() { + return this.unmappedArguments; + } + @Override public List skippedTypes() { return this.skippedTypes; @@ -421,6 +453,7 @@ public class SchemaMappingInspector { return "GraphQL schema inspection:\n" + "\tUnmapped fields: " + formatUnmappedFields() + "\n" + "\tUnmapped registrations: " + this.unmappedRegistrations + "\n" + + "\tUnmapped arguments: " + this.unmappedArguments + "\n" + "\tSkipped types: " + this.skippedTypes; } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaReport.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaReport.java index 5a8ee775..95eed633 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaReport.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaReport.java @@ -1,5 +1,5 @@ /* - * Copyright 2020-2023 the original author or authors. + * Copyright 2020-2024 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. @@ -27,6 +27,7 @@ import graphql.schema.GraphQLSchema; import graphql.schema.GraphQLType; import org.springframework.lang.Nullable; +import org.springframework.util.MultiValueMap; /** * Report produced as a result of inspecting schema mappings. @@ -61,6 +62,13 @@ public interface SchemaReport { */ Map> unmappedRegistrations(); + /** + * Return a map with {@link DataFetcher}s and the names of arguments they + * depend on that don't exist. + * @since 1.3 + */ + MultiValueMap, String> unmappedArguments(); + /** * Return types skipped during the inspection, either because the schema type * is not supported, e.g. union, or because there is insufficient Java type 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 643d431b..db790773 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 @@ -1,5 +1,5 @@ /* - * Copyright 2020-2023 the original author or authors. + * Copyright 2020-2024 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. @@ -16,6 +16,9 @@ package org.springframework.graphql.execution; +import java.util.Collections; +import java.util.Map; + import graphql.schema.DataFetcher; import org.springframework.core.ResolvableType; @@ -46,4 +49,13 @@ public interface SelfDescribingDataFetcher extends DataFetcher { */ ResolvableType getReturnType(); + /** + * Return a map with arguments that this {@link DataFetcher} looks up + * along with the Java types they are mapped to. + * @since 1.3 + */ + default Map getArguments() { + return Collections.emptyMap(); + } + } 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 fa175402..fd378b8b 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 @@ -235,7 +235,7 @@ class SchemaMappingInspectorTests { class SubscriptionTests { @Test - void reportContainsUnmappedSubscription() { + void unmappedSubscription() { String schema = """ type Query{ greeting: String @@ -254,26 +254,7 @@ class SchemaMappingInspectorTests { } @Test - void reportIsEmptyWhenSubscriptionIsMapped() { - String schema = """ - type Query{ - greeting: String - } - type Subscription { - bookSearch(author: String) : [Book!]! - } - - type Book { - id: ID - name: String - } - """; - SchemaReport report = inspectSchema(schema, GreetingController.class, BookController.class); - assertThatReport(report).hasUnmappedFieldCount(0).hasSkippedTypeCount(0); - } - - @Test - void reportWorksForSubscriptionWithExtensionType() { + void unmappedSubscriptionWithExtensionType() { String schema = """ type Query{ greeting: String @@ -292,6 +273,26 @@ class SchemaMappingInspectorTests { assertThatReport(report).hasUnmappedFieldCount(1).containsUnmappedFields("Subscription", "bookSearch"); } + @Test + void mappedSubscriptionWithUnmappedArgument() { + String schema = """ + type Query{ + greeting: String + } + type Subscription { + bookSearch(author: String) : [Book!]! + } + + type Book { + id: ID + name: String + } + """; + SchemaReport report = inspectSchema(schema, GreetingController.class, BookController.class); + assertThatReport(report).hasUnmappedFieldCount(0).hasSkippedTypeCount(0); + assertThatReport(report).hasUnmappedArgumentCount(1).containsUnmappedArguments("myAuthor"); + } + } @@ -552,11 +553,16 @@ class SchemaMappingInspectorTests { """; SchemaReport report = inspectSchema(schema, BookController.class); assertThatReport(report).hasUnmappedFieldCount(1).hasSkippedTypeCount(0); - assertThat(report.toString()) - .contains("GraphQL schema inspection:", "Unmapped fields: {Book=[missing]}", "Unmapped 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: []"); + assertThat(report.toString()).contains( + "GraphQL schema inspection:", + "Unmapped fields: {Book=[missing]}", + "Unmapped 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]", + "{BookController#bookSearch[1 args]=[myAuthor]}", + "Skipped types: []"); } } @@ -649,7 +655,7 @@ class SchemaMappingInspectorTests { } @SubscriptionMapping - public Flux> bookSearch(@Argument String author) { + public Flux> bookSearch(@Argument String myAuthor) { return Flux.empty(); } } @@ -746,6 +752,14 @@ class SchemaMappingInspectorTests { return this; } + public SchemaInspectionReportAssert hasUnmappedArgumentCount(int expected) { + isNotNull(); + if (this.actual.unmappedArguments().size() != expected) { + failWithMessage("Expected %s unmapped arguments, found %s.", expected, this.actual.unmappedArguments()); + } + return this; + } + public SchemaInspectionReportAssert hasSkippedTypeCount(int expected) { isNotNull(); if (this.actual.skippedTypes().size() != expected) { @@ -778,6 +792,18 @@ class SchemaMappingInspectorTests { return this; } + public SchemaInspectionReportAssert containsUnmappedArguments(String... arguments) { + isNotNull(); + List expected = Arrays.asList(arguments); + List actual = this.actual.unmappedArguments().entrySet().stream() + .flatMap(entry -> entry.getValue().stream()) + .toList(); + if (!actual.containsAll(expected)) { + failWithMessage("Expected unmapped arguments: %s, found %s", expected, actual); + } + return this; + } + public SchemaInspectionReportAssert containsSkippedTypes(String... fieldCoordinates) { isNotNull(); List expected = Arrays.asList(fieldCoordinates);