From 02bfb4a8e3630d734f07b804b55cf3865a7169a5 Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Wed, 2 Jul 2025 11:38:30 -0400 Subject: [PATCH 1/3] implement a new checkForTypeExtensionFieldUniqueness --- .../idl/SchemaTypeExtensionsChecker.java | 38 +++++++++++++++---- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java b/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java index 36b4a2c3ff..1a11912811 100644 --- a/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java +++ b/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java @@ -24,10 +24,13 @@ 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.Optional; +import java.util.Set; import java.util.function.Consumer; +import java.util.function.Function; import java.util.stream.Collectors; import static graphql.schema.idl.SchemaTypeChecker.checkNamedUniqueness; @@ -81,12 +84,13 @@ 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)); + checkForTypeExtensionFieldUniqueness( + errors, + extensions, + ObjectTypeDefinition::getFieldDefinitions + ); - // // then check for field re-defs from the base type Optional baseTypeOpt = typeRegistry.getType(extension.getName(), ObjectTypeDefinition.class); baseTypeOpt.ifPresent(baseTypeDef -> checkForFieldRedefinition(errors, extension, fieldDefinitions, baseTypeDef.getFieldDefinitions())); @@ -125,10 +129,12 @@ 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)); + checkForTypeExtensionFieldUniqueness( + errors, + extensions, + InterfaceTypeDefinition::getFieldDefinitions + ); // // then check for field re-defs from the base type @@ -138,6 +144,24 @@ private void checkInterfaceTypeExtensions(List errors, TypeDefinit }); } + private > void checkForTypeExtensionFieldUniqueness( + List errors, + List extensions, + Function> getFieldDefinitions + ) { + Set seenFields = new HashSet<>(); + + for (T extension : extensions) { + for (FieldDefinition field : getFieldDefinitions.apply(extension)) { + if (seenFields.contains(field.getName())) { + errors.add(new TypeExtensionFieldRedefinitionError(extension, field)); + } else { + seenFields.add(field.getName()); + } + } + } + } + /* * Union type extensions have the potential to be invalid if incorrectly defined. * From ef0f329e1169a547fc5572e61b29f6a8d3609940 Mon Sep 17 00:00:00 2001 From: bbaker Date: Mon, 4 Aug 2025 22:36:48 +1000 Subject: [PATCH 2/3] Extension type field unique ness is only checked once for all type extensions --- .../idl/SchemaTypeExtensionsChecker.java | 63 ++++++++----- .../schema/idl/SchemaTypeCheckerTest.groovy | 93 ++++++++++++++++++- 2 files changed, 133 insertions(+), 23 deletions(-) diff --git a/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java b/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java index e64042c06b..a0d7b9c4e9 100644 --- a/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java +++ b/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java @@ -27,7 +27,6 @@ import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.function.Consumer; import java.util.function.Function; @@ -84,19 +83,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 - checkForTypeExtensionFieldUniqueness( - errors, - extensions, - ObjectTypeDefinition::getFieldDefinitions - ); - // 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 + ); + } ); } @@ -130,13 +130,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 - checkForTypeExtensionFieldUniqueness( - errors, - extensions, - InterfaceTypeDefinition::getFieldDefinitions - ); - // // then check for field re-defs from the base type InterfaceTypeDefinition baseTypeDef = typeRegistry.getTypeOrNull(extension.getName(), InterfaceTypeDefinition.class); @@ -144,18 +137,42 @@ 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 + ); }); } private > void checkForTypeExtensionFieldUniqueness( List errors, List extensions, - Function> getFieldDefinitions + Function> getFieldDefinitionsFunc ) { Set seenFields = new HashSet<>(); for (T extension : extensions) { - for (FieldDefinition field : getFieldDefinitions.apply(extension)) { + 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 { @@ -274,17 +291,19 @@ 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 + ); }); } diff --git a/src/test/groovy/graphql/schema/idl/SchemaTypeCheckerTest.groovy b/src/test/groovy/graphql/schema/idl/SchemaTypeCheckerTest.groovy index 50cbade723..0ba036f6f0 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,95 @@ 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 + } + + """ + + when: + def result = check(sdl) + + then: + result.size() == 6 + 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]") + } } From 71625e9e3680736763bcc77256a7b49d6135f8ce Mon Sep 17 00:00:00 2001 From: bbaker Date: Tue, 5 Aug 2025 19:50:11 +1000 Subject: [PATCH 3/3] Extension type field unique ness is only checked once for all type extensions - for completeness did enums as well --- .../idl/SchemaTypeExtensionsChecker.java | 103 +++++++++--------- .../schema/idl/SchemaTypeCheckerTest.groovy | 27 ++++- 2 files changed, 78 insertions(+), 52 deletions(-) diff --git a/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java b/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java index a0d7b9c4e9..c80bdcce01 100644 --- a/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java +++ b/src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java @@ -28,7 +28,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.function.Consumer; import java.util.function.Function; import java.util.stream.Collectors; @@ -146,42 +145,6 @@ private void checkInterfaceTypeExtensions(List errors, TypeDefinit }); } - 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()); - } - } - } - } - /* * Union type extensions have the potential to be invalid if incorrectly defined. * @@ -234,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); @@ -248,7 +206,7 @@ private void checkEnumTypeExtensions(List errors, TypeDefinitionRe }); - + checkForTypeExtensionEnumFieldUniqueness(errors, extensions, EnumTypeDefinition::getEnumValueDefinitions); }); } @@ -309,7 +267,7 @@ private void checkInputObjectTypeExtensions(List errors, TypeDefin } - 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) { @@ -317,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()); @@ -351,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 0ba036f6f0..a109e0583c 100644 --- a/src/test/groovy/graphql/schema/idl/SchemaTypeCheckerTest.groovy +++ b/src/test/groovy/graphql/schema/idl/SchemaTypeCheckerTest.groovy @@ -1903,6 +1903,30 @@ class SchemaTypeCheckerTest extends Specification { 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 + } """ @@ -1910,9 +1934,10 @@ class SchemaTypeCheckerTest extends Specification { def result = check(sdl) then: - result.size() == 6 + 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]") } }