Stop creating NonNullableFieldValidator every for every object or list field #3929
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bbakerman
commented
Apr 24, 2025
| @@ -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); | |||
There was a problem hiding this comment.
No need for the executionStepInfo here. Its passed in during validate
So this instance gets used for the entirety of the operation
bbakerman
commented
Apr 24, 2025
| @@ -625,13 +625,10 @@ private FieldValueInfo completeField(GraphQLFieldDefinition fieldDef, ExecutionC | |||
| instrumentationParams, executionContext.getInstrumentationState() | |||
| )); | |||
|
|
|||
| NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, executionStepInfo); | |||
There was a problem hiding this comment.
no need to recreate it
bbakerman
commented
Apr 24, 2025
| @@ -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) | |||
There was a problem hiding this comment.
see - executionStepInfo is in the ExecutionStrategyParameters always
bbakerman
commented
Apr 24, 2025
| @@ -786,13 +783,10 @@ protected FieldValueInfo completeValueForList(ExecutionContext executionContext, | |||
|
|
|||
| ExecutionStepInfo stepInfoForListElement = executionStepInfoFactory.newExecutionStepInfoForListElement(executionStepInfo, indexedPath); | |||
|
|
|||
| NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, stepInfoForListElement); | |||
|
|
|||
bbakerman
commented
Apr 24, 2025
| @@ -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");; | |||
There was a problem hiding this comment.
Just to be more specific - this helped find tests that "half create" objects for testing reasons
bbakerman
commented
Apr 24, 2025
| 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)); |
There was a problem hiding this comment.
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
commented
Apr 24, 2025
| @@ -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)) | |||
There was a problem hiding this comment.
The rest of the changes are test fixups where they half create support objects with just enough brains to work
Test Results 312 files 312 suites 54s ⏱️ Results for commit d472058. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
validatemethod via the ExecutionStrategyParameters anywaySo 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