Correctly report skipped concrete union/interface types

Closes gh-962

Author: rstoyanchev

spring-graphql/src/main/java/org/springframework/graphql/execution/SchemaMappingInspector.java

@@ -220,14 +220,15 @@ else if (typePair.outputType() instanceof GraphQLInterfaceType interfaceType) {
 // 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 @@ private final class ReportBuilder {
 
 private final MultiValueMap<DataFetcher<?>, String> unmappedArguments = new LinkedMultiValueMap<>();
 
-private final List<SchemaReport.SkippedType> skippedTypes = new ArrayList<>();
+private final List<DefaultSkippedType> skippedTypes = new ArrayList<>();
+
+private final List<DefaultSkippedType> candidateSkippedTypes = new ArrayList<>();
 
 void unmappedField(FieldCoordinates coordinates) {
 this.unmappedFields.add(coordinates);
@@ -737,19 +740,44 @@ void unmappedArgument(DataFetcher<?> dataFetcher, List<String> arguments) {
 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 @@ private final class DefaultSchemaReport implements SchemaReport {
 
 DefaultSchemaReport(
 List<FieldCoordinates> unmappedFields, Map<FieldCoordinates, DataFetcher<?>> unmappedRegistrations,
-MultiValueMap<DataFetcher<?>, String> unmappedArguments, List<SkippedType> skippedTypes) {
+MultiValueMap<DataFetcher<?>, String> unmappedArguments, List<DefaultSkippedType> skippedTypes) {
 
 this.unmappedFields = Collections.unmodifiableList(unmappedFields);
 this.unmappedRegistrations = Collections.unmodifiableMap(unmappedRegistrations);
@@ -834,17 +862,18 @@ private String formatUnmappedFields() {
  * 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);
 }
 }
 

spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorUnionTests.java

@@ -22,6 +22,7 @@
 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,16 +36,20 @@ 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
 }
 type Video {
 title: String
 }
+type Article {
+content: String
+}
 """;
 
 
@@ -55,8 +60,10 @@ class UnmappedFields {
 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 @@ void reportUnmappedFieldsByCheckingReturnTypePackage() {
 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 @@ void classNameFunction() {
 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 @@ void classNameTypeResolver() {
 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 @@ class SkippedTypes {
 @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 @@ List<ResultItem> search() {
 }
 }
 
+
+@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<Object> 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) { }
+
 }