From dceb3aff94698c7accc4b32b5979433e527c463c Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Fri, 3 May 2024 15:12:21 +0100 Subject: [PATCH] Correctly report skipped concrete union/interface types Closes gh-962 --- .../execution/SchemaMappingInspector.java | 53 +++++++++++--- .../SchemaMappingInspectorUnionTests.java | 73 ++++++++++++++++--- 2 files changed, 102 insertions(+), 24 deletions(-) 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 5af4cd26..505e6109 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 @@ -220,14 +220,15 @@ public final class SchemaMappingInspector { // Can we inspect GraphQL type? if (!(graphQlType instanceof GraphQLFieldsContainer fieldContainer)) { if (isNotScalarOrEnumType(graphQlType)) { - this.reportBuilder.skippedType(graphQlType, parent, field, "Unsupported schema type"); + this.reportBuilder.skippedType(graphQlType, parent, field, "Unsupported schema type", false); } continue; } // Can we inspect the Class? if (currentResolvableType.resolve(Object.class) == Object.class) { - this.reportBuilder.skippedType(graphQlType, parent, field, "No class information"); + boolean isDerived = !graphQlType.equals(typePair.outputType()); + this.reportBuilder.skippedType(graphQlType, parent, field, "No class information", isDerived); continue; } @@ -723,7 +724,9 @@ public final class SchemaMappingInspector { private final MultiValueMap, String> unmappedArguments = new LinkedMultiValueMap<>(); - private final List skippedTypes = new ArrayList<>(); + private final List skippedTypes = new ArrayList<>(); + + private final List candidateSkippedTypes = new ArrayList<>(); void unmappedField(FieldCoordinates coordinates) { this.unmappedFields.add(coordinates); @@ -737,19 +740,44 @@ public final class SchemaMappingInspector { this.unmappedArguments.put(dataFetcher, arguments); } - void skippedType(GraphQLType type, GraphQLFieldsContainer parent, GraphQLFieldDefinition field, String reason) { - DefaultSkippedType skippedType = DefaultSkippedType.create(type, parent, field); + void skippedType( + GraphQLType type, GraphQLFieldsContainer parent, GraphQLFieldDefinition field, + String reason, boolean isDerivedType) { + + DefaultSkippedType skippedType = DefaultSkippedType.create(type, parent, field, reason); + + if (!isDerivedType) { + skippedType(skippedType); + return; + } + + // Keep skipped union member or interface implementing types aside to the end. + // Use of concrete types elsewhere may provide more information. + + this.candidateSkippedTypes.add(skippedType); + } + + private void skippedType(DefaultSkippedType skippedType) { if (logger.isDebugEnabled()) { - logger.debug("Skipping '" + skippedType + "': " + reason); + logger.debug("Skipping '" + skippedType + "': " + skippedType.reason()); } this.skippedTypes.add(skippedType); } SchemaReport build() { + + this.candidateSkippedTypes.forEach((skippedType) -> { + if (skippedType.type() instanceof GraphQLFieldsContainer fieldsContainer) { + if (SchemaMappingInspector.this.inspectedTypes.contains(fieldsContainer.getName())) { + return; + } + } + skippedType(skippedType); + }); + return new DefaultSchemaReport( this.unmappedFields, this.unmappedRegistrations, this.unmappedArguments, this.skippedTypes); } - } @@ -768,7 +796,7 @@ public final class SchemaMappingInspector { DefaultSchemaReport( List unmappedFields, Map> unmappedRegistrations, - MultiValueMap, String> unmappedArguments, List skippedTypes) { + MultiValueMap, String> unmappedArguments, List skippedTypes) { this.unmappedFields = Collections.unmodifiableList(unmappedFields); this.unmappedRegistrations = Collections.unmodifiableMap(unmappedRegistrations); @@ -834,17 +862,18 @@ public final class SchemaMappingInspector { * Default implementation of a {@link SchemaReport.SkippedType}. */ private record DefaultSkippedType( - GraphQLType type, FieldCoordinates fieldCoordinates) implements SchemaReport.SkippedType { + GraphQLType type, FieldCoordinates fieldCoordinates, String reason) + implements SchemaReport.SkippedType { @Override public String toString() { - return (type instanceof GraphQLNamedType namedType) ? namedType.getName() : type.toString(); + return (this.type instanceof GraphQLNamedType named) ? named.getName() : this.type.toString(); } public static DefaultSkippedType create( - GraphQLType type, GraphQLFieldsContainer parent, GraphQLFieldDefinition field) { + GraphQLType type, GraphQLFieldsContainer parent, GraphQLFieldDefinition field, String reason) { - return new DefaultSkippedType(type, FieldCoordinates.coordinates(parent, field)); + return new DefaultSkippedType(type, FieldCoordinates.coordinates(parent, field), reason); } } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorUnionTests.java b/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorUnionTests.java index 87a52889..44066fd0 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorUnionTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorUnionTests.java @@ -22,6 +22,7 @@ import java.util.Map; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.springframework.graphql.data.method.annotation.Argument; import org.springframework.graphql.data.method.annotation.QueryMapping; import org.springframework.graphql.execution.SchemaMappingInspector.ClassResolver; import org.springframework.stereotype.Controller; @@ -35,9 +36,10 @@ public class SchemaMappingInspectorUnionTests extends SchemaMappingInspectorTest private static final String schema = """ type Query { - search: [SearchResult!]! + search: [SearchResult] + article(id: ID): Article } - union SearchResult = Photo | Video + union SearchResult = Article | Photo | Video type Photo { height: Int width: Int @@ -45,6 +47,9 @@ public class SchemaMappingInspectorUnionTests extends SchemaMappingInspectorTest type Video { title: String } + type Article { + content: String + } """; @@ -55,8 +60,10 @@ public class SchemaMappingInspectorUnionTests extends SchemaMappingInspectorTest void reportUnmappedFieldsByCheckingReturnTypePackage() { SchemaReport report = inspectSchema(schema, SearchController.class); assertThatReport(report) - .hasSkippedTypeCount(0) - .hasUnmappedFieldCount(3) + .hasSkippedTypeCount(1) + .containsSkippedTypes("Article") + .hasUnmappedFieldCount(4) + .containsUnmappedFields("Query", "article") .containsUnmappedFields("Photo", "height", "width") .containsUnmappedFields("Video", "title"); } @@ -65,17 +72,20 @@ public class SchemaMappingInspectorUnionTests extends SchemaMappingInspectorTest void reportUnmappedFieldsByCheckingControllerTypePackage() { SchemaReport report = inspectSchema(schema, ObjectSearchController.class); assertThatReport(report) - .hasSkippedTypeCount(0) - .hasUnmappedFieldCount(3) + .hasSkippedTypeCount(1) + .containsSkippedTypes("Article") + .hasUnmappedFieldCount(4) + .containsUnmappedFields("Query", "article") .containsUnmappedFields("Photo", "height", "width") .containsUnmappedFields("Video", "title"); } - sealed interface ResultItem permits Photo, Video { } record Photo() implements ResultItem { } record Video() implements ResultItem { } + sealed interface ResultItem permits Photo, Video { } + @Controller static class SearchController { @@ -108,8 +118,10 @@ public class SchemaMappingInspectorUnionTests extends SchemaMappingInspectorTest SearchController.class); assertThatReport(report) - .hasSkippedTypeCount(0) - .hasUnmappedFieldCount(3) + .hasSkippedTypeCount(1) + .containsSkippedTypes("Article") + .hasUnmappedFieldCount(4) + .containsUnmappedFields("Query", "article") .containsUnmappedFields("Photo", "height", "width") .containsUnmappedFields("Video", "title"); } @@ -124,8 +136,11 @@ public class SchemaMappingInspectorUnionTests extends SchemaMappingInspectorTest SearchController.class); assertThatReport(report) - .hasUnmappedFieldCount(2).containsUnmappedFields("Photo", "height", "width") - .hasSkippedTypeCount(1).containsSkippedTypes("Video"); + .hasSkippedTypeCount(2) + .containsSkippedTypes("Article", "Video") + .hasUnmappedFieldCount(3) + .containsUnmappedFields("Query", "article") + .containsUnmappedFields("Photo", "height", "width"); } sealed interface ResultItem permits PhotoImpl, VideoImpl { } @@ -149,7 +164,7 @@ public class SchemaMappingInspectorUnionTests extends SchemaMappingInspectorTest @Test void reportSkippedImplementations() { SchemaReport report = inspectSchema(schema, SearchController.class); - assertThatReport(report).hasSkippedTypeCount(2).containsSkippedTypes("Photo", "Video"); + assertThatReport(report).hasSkippedTypeCount(3).containsSkippedTypes("Article", "Photo", "Video"); } interface ResultItem { } @@ -164,4 +179,38 @@ public class SchemaMappingInspectorUnionTests extends SchemaMappingInspectorTest } } + + @Nested + class CandidateSkippedTypes { + + // A union member type is only a candidate to be skipped until the inspection is done. + // Use of the concrete type elsewhere may provide more information. + + @Test + void candidateNotSkippedIfConcreteUseElsewhere() { + SchemaReport report = inspectSchema(schema, SearchController.class); + assertThatReport(report).hasSkippedTypeCount(2).containsSkippedTypes("Photo", "Video"); + } + + @Controller + static class SearchController { + + @QueryMapping + List search() { + throw new UnsupportedOperationException(); + } + + @QueryMapping + Article article(@Argument Long id) { + throw new UnsupportedOperationException(); + } + } + } + + + /** + * Declared outside {@link CandidateSkippedTypes}, so the union lookup won't find it. + */ + private record Article(String content) { } + }