diff --git a/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java b/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java index 0a99a65b36..c80bdcce01 100644 --- a/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java +++ b/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java @@ -24,9 +24,11 @@ import graphql.schema.idl.errors.TypeExtensionMissingBaseTypeError; import graphql.util.FpKit; +import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.function.Consumer; +import java.util.Set; +import java.util.function.Function; import java.util.stream.Collectors; import static graphql.schema.idl.SchemaTypeChecker.checkNamedUniqueness; @@ -80,18 +82,20 @@ private void checkObjectTypeExtensions(List errors, TypeDefinition checkNamedUniqueness(errors, directive.getArguments(), Argument::getName, (argumentName, argument) -> new NonUniqueArgumentError(extension, fld, argumentName)))); - // - // fields must be unique within a type extension - forEachBut(extension, extensions, - otherTypeExt -> checkForFieldRedefinition(errors, otherTypeExt, otherTypeExt.getFieldDefinitions(), fieldDefinitions)); - - // // then check for field re-defs from the base type ObjectTypeDefinition baseTypeDef = typeRegistry.getTypeOrNull(extension.getName(), ObjectTypeDefinition.class); if (baseTypeDef != null) { checkForFieldRedefinition(errors, extension, fieldDefinitions, baseTypeDef.getFieldDefinitions()); } }); + + // fields must be unique within a type extension + checkForTypeExtensionFieldUniqueness( + errors, + extensions, + ObjectTypeDefinition::getFieldDefinitions + ); + } ); } @@ -125,11 +129,6 @@ private void checkInterfaceTypeExtensions(List errors, TypeDefinit checkNamedUniqueness(errors, directive.getArguments(), Argument::getName, (argumentName, argument) -> new NonUniqueArgumentError(extension, fld, argumentName)))); - // - // fields must be unique within a type extension - forEachBut(extension, extensions, - otherTypeExt -> checkForFieldRedefinition(errors, otherTypeExt, otherTypeExt.getFieldDefinitions(), fieldDefinitions)); - // // then check for field re-defs from the base type InterfaceTypeDefinition baseTypeDef = typeRegistry.getTypeOrNull(extension.getName(), InterfaceTypeDefinition.class); @@ -137,6 +136,12 @@ private void checkInterfaceTypeExtensions(List errors, TypeDefinit checkForFieldRedefinition(errors, extension, fieldDefinitions, baseTypeDef.getFieldDefinitions()); } }); + // fields must be unique within a type extension + checkForTypeExtensionFieldUniqueness( + errors, + extensions, + InterfaceTypeDefinition::getFieldDefinitions + ); }); } @@ -192,11 +197,6 @@ private void checkEnumTypeExtensions(List errors, TypeDefinitionRe checkNamedUniqueness(errors, enumValueDefinitions, EnumValueDefinition::getName, (namedField, enumValue) -> new NonUniqueNameError(extension, enumValue)); - // - // enum values must be unique within a type extension - forEachBut(extension, extensions, - otherTypeExt -> checkForEnumValueRedefinition(errors, otherTypeExt, otherTypeExt.getEnumValueDefinitions(), enumValueDefinitions)); - // // then check for field re-defs from the base type EnumTypeDefinition baseTypeDef = typeRegistry.getTypeOrNull(extension.getName(), EnumTypeDefinition.class); @@ -206,7 +206,7 @@ private void checkEnumTypeExtensions(List errors, TypeDefinitionRe }); - + checkForTypeExtensionEnumFieldUniqueness(errors, extensions, EnumTypeDefinition::getEnumValueDefinitions); }); } @@ -249,23 +249,25 @@ private void checkInputObjectTypeExtensions(List errors, TypeDefin checkNamedUniqueness(errors, directive.getArguments(), Argument::getName, (argumentName, argument) -> new NonUniqueArgumentError(extension, fld, argumentName)))); // - // fields must be unique within a type extension - forEachBut(extension, extensions, - otherTypeExt -> checkForInputValueRedefinition(errors, otherTypeExt, otherTypeExt.getInputValueDefinitions(), inputValueDefinitions)); - - // // then check for field re-defs from the base type InputObjectTypeDefinition baseTypeDef = typeRegistry.getTypeOrNull(extension.getName(), InputObjectTypeDefinition.class); if (baseTypeDef != null) { checkForInputValueRedefinition(errors, extension, inputValueDefinitions, baseTypeDef.getInputValueDefinitions()); } }); + // + // fields must be unique within a type extension + checkForTypeExtensionInputFieldUniqueness( + errors, + extensions, + InputObjectTypeDefinition::getInputValueDefinitions + ); }); } - private void checkTypeExtensionHasCorrespondingType(List errors, TypeDefinitionRegistry typeRegistry, String name, List extTypeList, Class targetClass) { + private void checkTypeExtensionHasCorrespondingType(List errors, TypeDefinitionRegistry typeRegistry, String name, List> extTypeList, Class> targetClass) { TypeDefinition extensionDefinition = extTypeList.get(0); TypeDefinition typeDefinition = typeRegistry.getTypeOrNull(TypeName.newTypeName().name(name).build(), targetClass); if (typeDefinition == null) { @@ -273,8 +275,6 @@ private void checkTypeExtensionHasCorrespondingType(List errors, T } } - @SuppressWarnings("unchecked") - private void checkForFieldRedefinition(List errors, TypeDefinition typeDefinition, List fieldDefinitions, List referenceFieldDefinitions) { Map referenceMap = FpKit.getByName(referenceFieldDefinitions, FieldDefinition::getName, mergeFirst()); @@ -307,12 +307,57 @@ private void checkForEnumValueRedefinition(List errors, TypeDefini }); } - private void forEachBut(T butThisOne, List list, Consumer consumer) { - for (T t : list) { - if (t == butThisOne) { - continue; + private > void checkForTypeExtensionFieldUniqueness( + List errors, + List extensions, + Function> getFieldDefinitionsFunc + ) { + Set seenFields = new HashSet<>(); + + for (T extension : extensions) { + for (FieldDefinition field : getFieldDefinitionsFunc.apply(extension)) { + if (seenFields.contains(field.getName())) { + errors.add(new TypeExtensionFieldRedefinitionError(extension, field)); + } else { + seenFields.add(field.getName()); + } + } + } + } + + private > void checkForTypeExtensionInputFieldUniqueness( + List errors, + List extensions, + Function> getFieldDefinitionsFunc + ) { + Set seenFields = new HashSet<>(); + + for (T extension : extensions) { + for (InputValueDefinition field : getFieldDefinitionsFunc.apply(extension)) { + if (seenFields.contains(field.getName())) { + errors.add(new TypeExtensionFieldRedefinitionError(extension, field)); + } else { + seenFields.add(field.getName()); + } + } + } + } + + private > void checkForTypeExtensionEnumFieldUniqueness( + List errors, + List extensions, + Function> getFieldDefinitionsFunc + ) { + Set seenFields = new HashSet<>(); + + for (T extension : extensions) { + for (EnumValueDefinition field : getFieldDefinitionsFunc.apply(extension)) { + if (seenFields.contains(field.getName())) { + errors.add(new TypeExtensionEnumValueRedefinitionError(extension, field)); + } else { + seenFields.add(field.getName()); + } } - consumer.accept(t); } } } diff --git a/src/test/groovy/graphql/schema/idl/SchemaTypeCheckerTest.groovy b/src/test/groovy/graphql/schema/idl/SchemaTypeCheckerTest.groovy index 50cbade723..a109e0583c 100644 --- a/src/test/groovy/graphql/schema/idl/SchemaTypeCheckerTest.groovy +++ b/src/test/groovy/graphql/schema/idl/SchemaTypeCheckerTest.groovy @@ -943,7 +943,7 @@ class SchemaTypeCheckerTest extends Specification { expect: !result.isEmpty() - result.size() == 4 + result.size() == 5 } def "test that field args are unique"() { @@ -1824,4 +1824,120 @@ class SchemaTypeCheckerTest extends Specification { then: errorContaining(result, "member type 'Bar' in Union 'DuplicateBar' is not unique. The member types of a Union type must be unique.") } + + def "how many errors do we get on type extension field redefinition"() { + def sdl = """ + + type Query { + foo : Foo + } + + type Foo { + foo : String + } + + extend type Foo { + redefinedField : String + } + + extend type Foo { + otherField1 : String + } + + extend type Foo { + otherField2 : String + } + + extend type Foo { + redefinedField : String + } + + extend type Foo { + redefinedField : String + } + + interface InterfaceType { + foo : String + } + + extend interface InterfaceType { + redefinedInterfaceField : String + } + + extend interface InterfaceType { + otherField1 : String + } + + extend interface InterfaceType { + otherField2 : String + } + + extend interface InterfaceType { + redefinedInterfaceField : String + } + + extend interface InterfaceType { + redefinedInterfaceField : String + } + + input Bar { + bar : String + } + + extend input Bar { + redefinedInputField : String + } + + extend input Bar { + otherField1 : String + } + + extend input Bar { + otherField2 : String + } + + extend input Bar { + redefinedInputField : String + } + + extend input Bar { + redefinedInputField : String + } + + enum Baz { + baz + } + + extend enum Baz { + redefinedEnumValue + } + + extend enum Baz { + otherField1 + } + + extend enum Baz { + otherField2 + } + + extend enum Baz { + redefinedEnumValue + } + + extend enum Baz { + redefinedEnumValue + } + + """ + + when: + def result = check(sdl) + + then: + result.size() == 8 + errorContaining(result, "'Foo' extension type [@n:n] tried to redefine field 'redefinedField' [@n:n]") + errorContaining(result, "'InterfaceType' extension type [@n:n] tried to redefine field 'redefinedInterfaceField' [@n:n]") + errorContaining(result, "'Bar' extension type [@n:n] tried to redefine field 'redefinedInputField' [@n:n]") + errorContaining(result, "'Baz' extension type [@n:n] tried to redefine enum value 'redefinedEnumValue' [@n:n]") + } }