- Notifications
You must be signed in to change notification settings - Fork1.2k
19.x Backport PR 3525 max result nodes#3537
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| packagegraphql.execution | ||
| importgraphql.GraphQLContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Test changes: all tests instantiating a ExecutionContext ought to have had an empty or mocked GraphQLContext object added
In practice, during execution there is always an GraphQLContext inside the ExecutionContext, which is why we don't have a billion null checks for this in the engine. These tests have been brought up to date
| importgraphql.ErrorType | ||
| importgraphql.ExecutionResult | ||
| importgraphql.GraphQLContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Test changes: all tests instantiating a ExecutionContext ought to have had an empty or mocked GraphQLContext object added
In practice, during execution there is always an GraphQLContext inside the ExecutionContext, which is why we don't have a billion null checks for this in the engine. These tests have been brought up to date
In this file I use a Mock and in the next I use the default GraphQLContext. They mean the same, but I wanted to keep these files as close to master as possible. A Mock is used in this file in latest master to mock a field for the new defer feature.
| */ | ||
| publicstaticGraphQLContextgetDefault() { | ||
| returnGraphQLContext.newContext().build(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This convenience method was added in a later version - backported to help with this PR
| privatefinalObjectlocalContext; | ||
| privatefinalImmutableList<GraphQLError>errors; | ||
| privateFetchedValue(ObjectfetchedValue,ObjectrawFetchedValue,ImmutableList<GraphQLError>errors,ObjectlocalContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changed visibility to be same as master
| privatefinalCompletableFuture<ExecutionResult>fieldValue; | ||
| privatefinalList<FieldValueInfo>fieldValueInfos; | ||
| privateFieldValueInfo(CompleteValueTypecompleteValueType,CompletableFuture<ExecutionResult>fieldValue,List<FieldValueInfo>fieldValueInfos) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changed visibility to be same as master
| if ((maxNodes =executionContext.getGraphQLContext().get(MAX_RESULT_NODES)) !=null) { | ||
| if (resultNodesCount >maxNodes) { | ||
| executionContext.getResultNodesInfo().maxResultNodesExceeded(); | ||
| returnCompletableFuture.completedFuture(newFetchedValue(null,null,ImmutableKit.emptyList(),null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
FetchedValue constructor (and object) slightly different here vs master - there is one more field. But the meaning is the same, return early with a null
| .queryDirectives(queryDirectives) | ||
| .build(); | ||
| GraphQLCodeRegistrycodeRegistry =executionContext.getGraphQLSchema().getCodeRegistry(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Line was moved down
| if ((maxNodes =executionContext.getGraphQLContext().get(MAX_RESULT_NODES)) !=null) { | ||
| if (resultNodesCount >maxNodes) { | ||
| executionContext.getResultNodesInfo().maxResultNodesExceeded(); | ||
| returnnewFieldValueInfo(NULL,completedFuture(ExecutionResultImpl.newExecutionResult().build()),fieldValueInfos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Difference to master - must have a completed value of type ExecutionResult. Can't return a plain null here or the engine is going to hang because prior to forthcoming v22 changes, the engine previously always expected an ExecutionResult
v19 is different to the other backports. This builder moved files after v19
Uh oh!
There was an error while loading.Please reload this page.
Backports PR#3525, which enables setting a maximum number of result nodes