Stop creating NonNullableFieldValidator every for every object or list field - #3929
Conversation
Sorry, something went wrong.
| 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
Sorry, something went wrong.
| instrumentationParams, executionContext.getInstrumentationState() | ||
| )); | ||
|
|
||
| NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, executionStepInfo); |
There was a problem hiding this comment.
no need to recreate it
Sorry, something went wrong.
| 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
Sorry, something went wrong.
| ExecutionStepInfo stepInfoForListElement = executionStepInfoFactory.newExecutionStepInfoForListElement(executionStepInfo, indexedPath); | ||
|
|
||
| NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, stepInfoForListElement); | ||
|
|
There was a problem hiding this comment.
No need - same reason
Sorry, something went wrong.
| 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
Sorry, something went wrong.
| 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
Sorry, something went wrong.
| .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
Sorry, something went wrong.
Test Results 314 files ±0 314 suites ±0 52s ⏱️ -2s 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. |
Sorry, something went wrong.
…ullableFieldValidator-all-the-time # Conflicts: # src/main/java/graphql/execution/ExecutionStrategy.java # src/main/java/graphql/execution/SubscriptionExecutionStrategy.java
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