Type extension field name uniqueness is checked only once by bbakerman · Pull Request #4076 · graphql-java/graphql-java

bbakerman · 2025-08-04T12:39:55Z

This is related to #4034 but supersedes it. This is more performant than @xuorig original PR.

This iterates the list of type extensions (for a type kind) only once to check if fields are unique

Before this iterated ALL type extensions (for a type kind) multiple times (once per type extension)

…raphql-java into xuorig-field-extension-uniqueness-perf

…tensions

bbakerman · 2025-08-04T12:41:18Z

src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java

-                                forEachBut(extension, extensions,
-                                        otherTypeExt -> checkForFieldRedefinition(errors, otherTypeExt, otherTypeExt.getFieldDefinitions(), fieldDefinitions));
-
-                                //


This code here is inside a loop already - so

for every type extentions

do this check where we loop every type extension

This created a N*M loop

bbakerman · 2025-08-04T12:42:12Z

src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java

+                                    extensions,
+                                    ObjectTypeDefinition::getFieldDefinitions
+                            );
+


The new code is OUTSIDE the extentions loop and the new checkForTypeExtensionFieldUniqueness loops extensions only once for field unique names so its order N

bbakerman · 2025-08-04T12:42:26Z

src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java

+                            errors,
+                            extensions,
+                            InterfaceTypeDefinition::getFieldDefinitions
+                    );


same but for interfaces

bbakerman · 2025-08-04T12:42:40Z

src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java

+                            errors,
+                            extensions,
+                            InputObjectTypeDefinition::getInputValueDefinitions
+                    );


some but for input types

github-actions · 2025-08-04T12:53:41Z

Test Results

325 files - 650 325 suites - 650 3m 31s ⏱️ - 7m 5s
5 020 tests - 345 5 014 ✅ - 346 6 💤 + 1 0 ❌ ±0
5 109 runs - 10 215 5 103 ✅ - 10 204 6 💤 - 11 0 ❌ ±0

Results for commit 1001ed4. ± Comparison against base commit 0b5c762.

This pull request removes 517 and adds 148 tests. Note that renamed tests count towards both.

	?

	, expected: combo-\"\\\b\f\n\r\t, #4]
                __schema { types { fields { args { type { name fields { name }}}}}}
                __schema { types { fields { type { name fields { name }}}}}
                __schema { types { inputFields { type { inputFields { name }}}}}
                __schema { types { interfaces { fields { type { interfaces { name } } } } } }
                __schema { types { name} }
                __type(name : "t") { name }
                a1: __schema { types { name} }
                a1: __type(name : "t") { name }
…

graphql.ScalarsBooleanTest ‑ parseValue throws exception for invalid input <java.lang.Object@1121ea0a>
graphql.ScalarsBooleanTest ‑ serialize throws exception for invalid input <java.lang.Object@8364f0e>
graphql.ScalarsIDTest ‑ parseValue allows any object via String.valueOf <java.lang.Object@75a5ff46>
graphql.ScalarsIDTest ‑ serialize allows any object via String.valueOf <java.lang.Object@2278792c>
graphql.ScalarsIntTest ‑ parseValue throws exception for invalid input <java.lang.Object@d65690b>
graphql.ScalarsIntTest ‑ serialize throws exception for invalid input <java.lang.Object@354c3e18>
graphql.analysis.QueryTraverserTest ‑ builder doesn't allow null arguments [document: Document{definitions=[OperationDefinition{name='null', operation=QUERY, variableDefinitions=[], directives=graphql.language.NodeUtil$DirectivesHolder@5721cd1b, selectionSet=SelectionSet{selections=[Field{name='foo', alias='null', arguments=[], directives=graphql.language.NodeUtil$DirectivesHolder@e647858, selectionSet=null}]}}]}, operationName: foo, root: Field{name='null', alias='null', arguments=[], directives=graphql.language.NodeUtil$DirectivesHolder@511da368, selectionSet=null}, rootParentType: null, fragmentsByName: [:], #3]
graphql.analysis.QueryTraverserTest ‑ builder doesn't allow null arguments [document: Document{definitions=[OperationDefinition{name='null', operation=QUERY, variableDefinitions=[], directives=graphql.language.NodeUtil$DirectivesHolder@682fbf03, selectionSet=SelectionSet{selections=[Field{name='foo', alias='null', arguments=[], directives=graphql.language.NodeUtil$DirectivesHolder@6c926fd9, selectionSet=null}]}}]}, operationName: foo, root: Field{name='null', alias='null', arguments=[], directives=graphql.language.NodeUtil$DirectivesHolder@17baf1d6, selectionSet=null}, rootParentType: null, fragmentsByName: null, #1]
graphql.analysis.QueryTraverserTest ‑ builder doesn't allow null arguments [document: Document{definitions=[OperationDefinition{name='null', operation=QUERY, variableDefinitions=[], directives=graphql.language.NodeUtil$DirectivesHolder@6ceca8ff, selectionSet=SelectionSet{selections=[Field{name='foo', alias='null', arguments=[], directives=graphql.language.NodeUtil$DirectivesHolder@42aa3ded, selectionSet=null}]}}]}, operationName: null, root: Field{name='null', alias='null', arguments=[], directives=graphql.language.NodeUtil$DirectivesHolder@4e8faebb, selectionSet=null}, rootParentType: null, fragmentsByName: null, #0]
graphql.analysis.QueryTraverserTest ‑ builder doesn't allow null arguments [document: Document{definitions=[OperationDefinition{name='null', operation=QUERY, variableDefinitions=[], directives=graphql.language.NodeUtil$DirectivesHolder@7d048304, selectionSet=SelectionSet{selections=[Field{name='foo', alias='null', arguments=[], directives=graphql.language.NodeUtil$DirectivesHolder@68c6fc00, selectionSet=null}]}}]}, operationName: foo, root: Field{name='null', alias='null', arguments=[], directives=graphql.language.NodeUtil$DirectivesHolder@1755ba66, selectionSet=null}, rootParentType: Mock for type 'GraphQLObjectType', fragmentsByName: null, #2]
…

This pull request skips 1 test.

graphql.schema.fetching.LambdaFetchingSupportTest ‑ different class loaders induce certain behaviours

♻️ This comment has been updated with latest results.

bbakerman · 2025-08-05T08:35:28Z

I just looked into graphql-js to see how they do it and they dont do "extends type" validation at "node -> fields creation time"

function buildFieldMap(
nodes: ReadonlyArray<
| InterfaceTypeDefinitionNode
| InterfaceTypeExtensionNode
| ObjectTypeDefinitionNode
| ObjectTypeExtensionNode
>,
): GraphQLFieldConfigMap<unknown, unknown> {
const fieldConfigMap = Object.create(null);
for (const node of nodes) {
const nodeFields = node.fields ?? [];

  for (const field of nodeFields) {
    fieldConfigMap[field.name.value] = {
      // Note: While this could make assertions to get the correctly typed
      // value, that would throw immediately while type system validation
      // with validateSchema() will produce more actionable results.
      type: getWrappedType(field.type),
      description: field.description?.value,
      args: buildArgumentMap(field.arguments),
      deprecationReason: getDeprecationReason(field),
      astNode: field,
    };
  }
}
return fieldConfigMap;

}
So they build and invalid type (via extends) and then do validation on the runtime schema - eg field redefinition later

This might be faster but its likely less precise - eg which extend type led to the error say. Maybe this is not true.

The spec is not precise here

https://spec.graphql.org/October2021/#sec-Object-Extensions

Object type extensions have the potential to be invalid if incorrectly defined.

The named type must already be defined and must be an Object type.
The fields of an Object type extension must have unique names; no two fields may share the same name.
Any fields of an Object type extension must not be already defined on the original Object type.
Any non-repeatable directives provided must not already apply to the original Object type.
Any interfaces provided must not be already implemented by the original Object type.
The resulting extended object type must be a super-set of all interfaces it implements.
The fields of an Object type extension must have unique names; no two fields may share the same name.

Does this mean unique with a single extend type node or with all extend type nodes? I guess its all nodes since this is how we are treating it today (via our repeated looping)

…tensions - for completeness did enums as well

dondonz · 2025-08-10T01:10:38Z

src/test/groovy/graphql/schema/idl/SchemaTypeCheckerTest.groovy

        expect:

        !result.isEmpty()
-        result.size() == 4


I was curious what the extra error was: it is a field redefinition error, not a NonUniqueNameError like the others in this list.

'Query' extension type [@8:13] tried to redefine field 'fieldB' [@10:17]

This is fine to have another error, it is correct anyway

dondonz

Nice work

…ion-uniqueness-perf # Conflicts: # src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java

…ion-uniqueness-perf

Marc-Andre Giroux and others added 3 commits July 2, 2025 11:38

implement a new checkForTypeExtensionFieldUniqueness

02bfb4a

Merge branch 'field-extension-uniqueness-perf' of github.com:xuorig/g…

0ebb109

…raphql-java into xuorig-field-extension-uniqueness-perf

Extension type field unique ness is only checked once for all type ex…

ef0f329

…tensions

bbakerman added this to the 25.x breaking changes milestone Aug 4, 2025

bbakerman commented Aug 4, 2025

View reviewed changes

bbakerman mentioned this pull request Aug 4, 2025

implement a new checkForTypeExtensionFieldUniqueness #4034

Closed

dondonz added the performance work that is primarily targeted as performance improvements label Aug 4, 2025

Extension type field unique ness is only checked once for all type ex…

71625e9

…tensions - for completeness did enums as well

dondonz reviewed Aug 10, 2025

View reviewed changes

dondonz approved these changes Aug 10, 2025

View reviewed changes

bbakerman added 2 commits August 10, 2025 17:02

Merge remote-tracking branch 'origin/master' into xuorig-field-extens…

925208d

…ion-uniqueness-perf # Conflicts: # src/main/java/graphql/schema/idl/SchemaTypeExtensionsChecker.java

Merge remote-tracking branch 'origin/master' into xuorig-field-extens…

1001ed4

…ion-uniqueness-perf

bbakerman merged commit 6267d65 into master Aug 12, 2025
2 checks passed

dondonz deleted the xuorig-field-extension-uniqueness-perf branch August 16, 2025 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type extension field name uniqueness is checked only once#4076

Type extension field name uniqueness is checked only once#4076
bbakerman merged 6 commits intomasterfrom
xuorig-field-extension-uniqueness-perf

bbakerman commented Aug 4, 2025

Uh oh!

bbakerman Aug 4, 2025

Uh oh!

bbakerman Aug 4, 2025

Uh oh!

bbakerman Aug 4, 2025

Uh oh!

bbakerman Aug 4, 2025

Uh oh!

github-actions bot commented Aug 4, 2025 •

edited

Loading

Uh oh!

bbakerman commented Aug 5, 2025

Uh oh!

dondonz Aug 10, 2025

Uh oh!

dondonz Aug 10, 2025

Uh oh!

dondonz left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

bbakerman commented Aug 4, 2025

Uh oh!

bbakerman Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

bbakerman Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

bbakerman Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

bbakerman Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

bbakerman commented Aug 5, 2025

Uh oh!

dondonz Aug 10, 2025

Choose a reason for hiding this comment

Uh oh!

dondonz Aug 10, 2025

Choose a reason for hiding this comment

Uh oh!

dondonz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Aug 4, 2025 •

edited

Loading