Skip to content

Stop creating NonNullableFieldValidator every for every object or list field #3929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
May 15, 2025
Merged

Stop creating NonNullableFieldValidator every for every object or list field #3929

merged 3 commits into from
May 15, 2025

Conversation

bbakerman
Copy link
Member

Today we create a NonNullableFieldValidator for every list item and every object we encounter. We do this because the constructor took a ExecutionStepInfo which IF there was a problem was used say what field was in error

BUT we dont need this. It is passed into the validate method via the ExecutionStrategyParameters anyway

So we can create 1 at execution time and use it through out

This save nemory allocation for cases where you have lots of object fields and / or lost of list fields.

This is NOT a breaking change - NonNullableFieldValidator is a internal class and this only changes the constructor shape

bbakerman
bbakerman commented Apr 24, 2025
View reviewed changes
src/main/java/graphql/execution/Execution.java
@@ -187,7 +187,7 @@ private CompletableFuture<ExecutionResult> executeOperation(ExecutionContext exe

ResultPath path = ResultPath.rootPath();
ExecutionStepInfo executionStepInfo = newExecutionStepInfo().type(operationRootType).path(path).build();
NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, executionStepInfo);
NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for the executionStepInfo here. Its passed in during validate

So this instance gets used for the entirety of the operation

bbakerman
bbakerman commented Apr 24, 2025
View reviewed changes
src/main/java/graphql/execution/ExecutionStrategy.java
@@ -625,13 +625,10 @@ private FieldValueInfo completeField(GraphQLFieldDefinition fieldDef, ExecutionC
instrumentationParams, executionContext.getInstrumentationState()
));

NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, executionStepInfo);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to recreate it

bbakerman
bbakerman commented Apr 24, 2025
View reviewed changes
src/main/java/graphql/execution/ExecutionStrategy.java Outdated
@@ -625,13 +625,10 @@ private FieldValueInfo completeField(GraphQLFieldDefinition fieldDef, ExecutionC
instrumentationParams, executionContext.getInstrumentationState()
));

NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, executionStepInfo);

ExecutionStrategyParameters newParameters = parameters.transform(builder ->
builder.executionStepInfo(executionStepInfo)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see - executionStepInfo is in the ExecutionStrategyParameters always

bbakerman
bbakerman commented Apr 24, 2025
View reviewed changes
src/main/java/graphql/execution/ExecutionStrategy.java
@@ -786,13 +783,10 @@ protected FieldValueInfo completeValueForList(ExecutionContext executionContext,

ExecutionStepInfo stepInfoForListElement = executionStepInfoFactory.newExecutionStepInfoForListElement(executionStepInfo, indexedPath);

NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, stepInfoForListElement);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need - same reason

bbakerman
bbakerman commented Apr 24, 2025
View reviewed changes
src/main/java/graphql/execution/ExecutionStrategyParameters.java
@@ -37,7 +37,7 @@ private ExecutionStrategyParameters(ExecutionStepInfo executionStepInfo,
this.localContext = localContext;
this.fields = assertNotNull(fields, () -> "fields is null");
this.source = source;
this.nonNullableFieldValidator = nonNullableFieldValidator;
this.nonNullableFieldValidator = assertNotNull(nonNullableFieldValidator, () -> "requires a NonNullValidator");;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be more specific - this helped find tests that "half create" objects for testing reasons

bbakerman
bbakerman commented Apr 24, 2025
View reviewed changes
src/main/java/graphql/execution/SubscriptionExecutionStrategy.java Outdated
return parameters.transform(builder -> builder.field(firstField).path(fieldPath));
NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext);
return parameters.transform(builder ->
builder.field(firstField).path(fieldPath).nonNullFieldValidator(nonNullableFieldValidator));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subscriptions is a special case - because it starts a new ExecutionContext instance - So we do re-create it here - but only once - not per published message

bbakerman
bbakerman commented Apr 24, 2025
View reviewed changes
src/test/groovy/graphql/execution/AsyncExecutionStrategyTest.groovy
@@ -117,6 +117,7 @@ abstract class AsyncExecutionStrategyTest extends Specification {
.newParameters()
.executionStepInfo(typeInfo)
.fields(mergedSelectionSet(['hello': mergedField([Field.newField('hello').build()]), 'hello2': mergedField([Field.newField('hello2').build()])]))
.nonNullFieldValidator(new NonNullableFieldValidator(executionContext))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the changes are test fixups where they half create support objects with just enough brains to work

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 24, 2025 •
edited
Loading

Test Results

  314 files  ±0    314 suites  ±0   52s ⏱️ -2s
3 610 tests ±0  3 605 ✅ ±0  5 💤 ±0  0 ❌ ±0 
3 699 runs  ±0  3 694 ✅ ±0  5 💤 ±0  0 ❌ ±0 

Results for commit 80f4a3b. ± Comparison against base commit 397c050.

This pull request removes 174 and adds 152 tests. Note that renamed tests count towards both. 
	?
                __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 }
                a2 :  __type(name : "t1") { name }
…

graphql.DataFetcherTest ‑ get Boolean property value [fetcher: <graphql.schema.PropertyDataFetcher@7f1ef916 propertyName=booleanField function=null>, #0]
graphql.DataFetcherTest ‑ get Boolean property value [fetcher: <graphql.schema.SingletonPropertyDataFetcher@5c441290>, #1]
graphql.DataFetcherTest ‑ get Boolean property value with get [fetcher: <graphql.schema.PropertyDataFetcher@17dd671f propertyName=booleanFieldWithGet function=null>, #0]
graphql.DataFetcherTest ‑ get Boolean property value with get [fetcher: <graphql.schema.SingletonPropertyDataFetcher@5c441290>, #1]
graphql.DataFetcherTest ‑ get property value [fetcher: <graphql.schema.PropertyDataFetcher@47c40b56 propertyName=property function=null>, #0]
graphql.DataFetcherTest ‑ get property value [fetcher: <graphql.schema.SingletonPropertyDataFetcher@5c441290>, #1]
graphql.DataFetcherTest ‑ get public field value as property [fetcher: <graphql.schema.PropertyDataFetcher@9b9a327 propertyName=publicField function=null>, #0]
graphql.DataFetcherTest ‑ get public field value as property [fetcher: <graphql.schema.SingletonPropertyDataFetcher@5c441290>, #1]
graphql.ScalarsBooleanTest ‑ parseValue throws exception for invalid input <java.lang.Object@3f17bc1e>
graphql.ScalarsBooleanTest ‑ serialize throws exception for invalid input <java.lang.Object@1f9b614d>
…

♻️ This comment has been updated with latest results.

@bbakerman bbakerman added the performance work that is primarily targeted as performance improvements label Apr 24, 2025
@bbakerman bbakerman requested review from andimarek and dondonz April 27, 2025 06:31
@bbakerman bbakerman mentioned this pull request Apr 29, 2025
Open
@dondonz dondonz modified the milestones: 25.x breaking changes, 24.0 May 13, 2025
bbakerman added 2 commits May 15, 2025 15:04
@bbakerman
Merge remote-tracking branch 'origin/master' into dont-alllocate-NonN…
3685647 
…ullableFieldValidator-all-the-time

# Conflicts:
#	src/main/java/graphql/execution/ExecutionStrategy.java
#	src/main/java/graphql/execution/SubscriptionExecutionStrategy.java
@bbakerman
Merged in master
80f4a3b
@dondonz dondonz merged commit abb2cdd into master May 15, 2025
2 checks passed
@dondonz dondonz mentioned this pull request May 15, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dondonz dondonz dondonz approved these changes

@andimarek andimarek Awaiting requested review from andimarek

Assignees
No one assigned
Labels
performance work that is primarily targeted as performance improvements
Projects
None yet
Milestone
24.0
Development

Successfully merging this pull request may close these issues.
2 participants
@bbakerman @dondonz