From ac3fabbce04f383e52e6309077d6c7bcae895c94 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Fri, 3 May 2024 08:44:21 +0100 Subject: [PATCH] Improve check for duplicate unmapped field Closes gh-961 --- .../execution/SchemaMappingInspector.java | 12 ++-- .../SchemaMappingInspectorInterfaceTests.java | 55 ++++++++++++++++--- .../SchemaMappingInspectorUnionTests.java | 14 ++--- 3 files changed, 59 insertions(+), 22 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 a30b602c..65bc6b1d 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 @@ -145,6 +145,10 @@ public final class SchemaMappingInspector { private void checkFieldsContainer( GraphQLFieldsContainer fieldContainer, @Nullable ResolvableType resolvableType) { + if (!this.inspectedTypes.add(fieldContainer.getName())) { + return; + } + String typeName = fieldContainer.getName(); Map dataFetcherMap = this.dataFetchers.getOrDefault(typeName, Collections.emptyMap()); @@ -196,10 +200,6 @@ public final class SchemaMappingInspector { TypePair typePair = TypePair.resolveTypePair(parent, field, resolvableType, this.schema); - if (addAndCheckIfAlreadyInspected(typePair.outputType())) { - return; - } - MultiValueMap typePairs = new LinkedMultiValueMap<>(); if (typePair.outputType() instanceof GraphQLUnionType unionType) { typePairs.putAll(this.interfaceUnionLookup.resolveUnion(unionType)); @@ -250,10 +250,6 @@ public final class SchemaMappingInspector { } } - private boolean addAndCheckIfAlreadyInspected(GraphQLType type) { - return (type instanceof GraphQLNamedOutputType outputType && !this.inspectedTypes.add(outputType.getName())); - } - private static boolean isNotScalarOrEnumType(GraphQLType type) { return !(type instanceof GraphQLScalarType || type instanceof GraphQLEnumType); } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorInterfaceTests.java b/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorInterfaceTests.java index d5deaeeb..0acafc2c 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorInterfaceTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/execution/SchemaMappingInspectorInterfaceTests.java @@ -17,6 +17,7 @@ package org.springframework.graphql.execution; import java.util.List; +import java.util.Map; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -53,7 +54,7 @@ public class SchemaMappingInspectorInterfaceTests extends SchemaMappingInspector @Nested - class InterfaceFieldsNotOnJavaInterface { + class UnmappedFields { @Test void reportUnmappedFields() { @@ -83,10 +84,10 @@ public class SchemaMappingInspectorInterfaceTests extends SchemaMappingInspector @Nested - class GraphQlAndJavaTypeNameMismatch { + class ClassNameAndClassResolver { @Test - void useClassNameFunction() { + void classNameFunction() { SchemaReport report = inspectSchema(schema, initializer -> initializer.classNameFunction(type -> type.getName() + "Impl"), @@ -100,13 +101,12 @@ public class SchemaMappingInspectorInterfaceTests extends SchemaMappingInspector } @Test - void useClassNameTypeResolver() { + void classNameTypeResolver() { - ClassNameTypeResolver typeResolver = new ClassNameTypeResolver(); - typeResolver.addMapping(CarImpl.class, "Car"); + Map, String> mappings = Map.of(CarImpl.class, "Car"); SchemaReport report = inspectSchema(schema, - initializer -> initializer.classResolver(ClassResolver.create(typeResolver.getMappings())), + initializer -> initializer.classResolver(ClassResolver.create(mappings)), VehicleController.class); assertThatReport(report) @@ -131,6 +131,47 @@ public class SchemaMappingInspectorInterfaceTests extends SchemaMappingInspector } + @Nested // gh-961 + class UnmappedFieldsReportedOnlyOnce { + + @Test + void reportUnmappedFields() { + + String schema = SchemaMappingInspectorInterfaceTests.schema + """ + extend type Query { + cars: [Car] + } + """; + + SchemaReport report = inspectSchema(schema, VehicleController.class); + assertThatReport(report) + .hasSkippedTypeCount(0) + .hasUnmappedFieldCount(2) + .containsUnmappedFields("Car", "price", "engineType"); + } + + interface Vehicle { + String name(); + } + record Car(String name) implements Vehicle { } + record Bike(String name, int price) implements Vehicle { } + + @Controller + static class VehicleController { + + @QueryMapping + List vehicles() { + throw new UnsupportedOperationException(); + } + + @QueryMapping + List cars() { + throw new UnsupportedOperationException(); + } + } + } + + @Nested class SkippedTypes { 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 49cb350a..87a52889 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 @@ -17,6 +17,7 @@ package org.springframework.graphql.execution; import java.util.List; +import java.util.Map; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -48,7 +49,7 @@ public class SchemaMappingInspectorUnionTests extends SchemaMappingInspectorTest @Nested - class InterfaceFieldsNotOnJavaInterface { + class UnmappedFields { @Test void reportUnmappedFieldsByCheckingReturnTypePackage() { @@ -97,10 +98,10 @@ public class SchemaMappingInspectorUnionTests extends SchemaMappingInspectorTest @Nested - class GraphQlAndJavaTypeNameMismatch { + class ClassNameAndClassResolver { @Test - void useClassNameFunction() { + void classNameFunction() { SchemaReport report = inspectSchema(schema, initializer -> initializer.classNameFunction(type -> type.getName() + "Impl"), @@ -114,13 +115,12 @@ public class SchemaMappingInspectorUnionTests extends SchemaMappingInspectorTest } @Test - void useClassNameTypeResolver() { + void classNameTypeResolver() { - ClassNameTypeResolver typeResolver = new ClassNameTypeResolver(); - typeResolver.addMapping(PhotoImpl.class, "Photo"); + Map, String> mappings = Map.of(PhotoImpl.class, "Photo"); SchemaReport report = inspectSchema(schema, - initializer -> initializer.classResolver(ClassResolver.create(typeResolver.getMappings())), + initializer -> initializer.classResolver(ClassResolver.create(mappings)), SearchController.class); assertThatReport(report)