-
Notifications
You must be signed in to change notification settings - Fork 1.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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,7 @@ | |
| import static graphql.execution.FieldValueInfo.CompleteValueType.NULL; | ||
| import static graphql.execution.FieldValueInfo.CompleteValueType.OBJECT; | ||
| import static graphql.execution.FieldValueInfo.CompleteValueType.SCALAR; | ||
| import static graphql.execution.ResultNodesInfo.MAX_RESULT_NODES; | ||
| import static graphql.execution.instrumentation.SimpleInstrumentationContext.nonNullCtx; | ||
| import static graphql.schema.DataFetchingEnvironmentImpl.newDataFetchingEnvironment; | ||
| import static graphql.schema.GraphQLTypeUtil.isEnum; | ||
|
|
@@ -237,8 +238,23 @@ protected CompletableFuture<FetchedValue> fetchField(ExecutionContext executionC | |
| MergedField field = parameters.getField(); | ||
| GraphQLObjectType parentType = (GraphQLObjectType) parameters.getExecutionStepInfo().getUnwrappedNonNullType(); | ||
| GraphQLFieldDefinition fieldDef = getFieldDef(executionContext.getGraphQLSchema(), parentType, field.getSingleField()); | ||
| return fetchField(fieldDef, executionContext, parameters); | ||
| } | ||
|
|
||
| GraphQLCodeRegistry codeRegistry = executionContext.getGraphQLSchema().getCodeRegistry(); | ||
| private CompletableFuture<FetchedValue> fetchField(GraphQLFieldDefinition fieldDef, ExecutionContext executionContext, ExecutionStrategyParameters parameters) { | ||
|
|
||
| int resultNodesCount = executionContext.getResultNodesInfo().incrementAndGetResultNodesCount(); | ||
|
|
||
| Integer maxNodes; | ||
| if ((maxNodes = executionContext.getGraphQLContext().get(MAX_RESULT_NODES)) != null) { | ||
| if (resultNodesCount > maxNodes) { | ||
| executionContext.getResultNodesInfo().maxResultNodesExceeded(); | ||
| return CompletableFuture.completedFuture(new FetchedValue(null, null, ImmutableKit.emptyList(), null)); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| } | ||
| } | ||
|
|
||
| MergedField field = parameters.getField(); | ||
| GraphQLObjectType parentType = (GraphQLObjectType) parameters.getExecutionStepInfo().getUnwrappedNonNullType(); | ||
| GraphQLOutputType fieldType = fieldDef.getType(); | ||
|
|
||
| // if the DF (like PropertyDataFetcher) does not use the arguments of execution step info then dont build any | ||
|
|
@@ -252,7 +268,6 @@ protected CompletableFuture<FetchedValue> fetchField(ExecutionContext executionC | |
| DataFetchingFieldSelectionSet fieldCollector = DataFetchingFieldSelectionSetImpl.newCollector(executionContext.getGraphQLSchema(), fieldType, normalizedFieldSupplier); | ||
| QueryDirectives queryDirectives = new QueryDirectivesImpl(field, executionContext.getGraphQLSchema(), executionContext.getVariables()); | ||
|
|
||
|
|
||
| DataFetchingEnvironment environment = newDataFetchingEnvironment(executionContext) | ||
| .source(parameters.getSource()) | ||
| .localContext(parameters.getLocalContext()) | ||
|
|
@@ -266,6 +281,7 @@ protected CompletableFuture<FetchedValue> fetchField(ExecutionContext executionC | |
| .queryDirectives(queryDirectives) | ||
| .build(); | ||
|
|
||
| GraphQLCodeRegistry codeRegistry = executionContext.getGraphQLSchema().getCodeRegistry(); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line was moved down |
||
| DataFetcher<?> dataFetcher = codeRegistry.getDataFetcher(parentType, fieldDef); | ||
|
|
||
| Instrumentation instrumentation = executionContext.getInstrumentation(); | ||
|
|
@@ -528,6 +544,15 @@ protected FieldValueInfo completeValueForList(ExecutionContext executionContext, | |
| List<FieldValueInfo> fieldValueInfos = new ArrayList<>(size.orElse(1)); | ||
| int index = 0; | ||
| for (Object item : iterableValues) { | ||
| int resultNodesCount = executionContext.getResultNodesInfo().incrementAndGetResultNodesCount(); | ||
| Integer maxNodes; | ||
| if ((maxNodes = executionContext.getGraphQLContext().get(MAX_RESULT_NODES)) != null) { | ||
| if (resultNodesCount > maxNodes) { | ||
| executionContext.getResultNodesInfo().maxResultNodesExceeded(); | ||
| return new FieldValueInfo(NULL, completedFuture(ExecutionResultImpl.newExecutionResult().build()), fieldValueInfos); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| } | ||
| } | ||
|
|
||
| ResultPath indexedPath = parameters.getPath().segment(index); | ||
|
|
||
| ExecutionStepInfo stepInfoForListElement = executionStepInfoFactory.newExecutionStepInfoForListElement(executionStepInfo, index); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ public class FetchedValue { | |
| private final Object localContext; | ||
| private final ImmutableList<GraphQLError> errors; | ||
|
|
||
| private FetchedValue(Object fetchedValue, Object rawFetchedValue, ImmutableList<GraphQLError> errors, Object localContext) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed visibility to be same as master |
||
| FetchedValue(Object fetchedValue, Object rawFetchedValue, ImmutableList<GraphQLError> errors, Object localContext) { | ||
| this.fetchedValue = fetchedValue; | ||
| this.rawFetchedValue = rawFetchedValue; | ||
| this.errors = errors; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,7 @@ public enum CompleteValueType { | |
| private final CompletableFuture<ExecutionResult> fieldValue; | ||
| private final List<FieldValueInfo> fieldValueInfos; | ||
|
|
||
| private FieldValueInfo(CompleteValueType completeValueType, CompletableFuture<ExecutionResult> fieldValue, List<FieldValueInfo> fieldValueInfos) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed visibility to be same as master |
||
| FieldValueInfo(CompleteValueType completeValueType, CompletableFuture<ExecutionResult> fieldValue, List<FieldValueInfo> fieldValueInfos) { | ||
| assertNotNull(fieldValueInfos, () -> "fieldValueInfos can't be null"); | ||
| this.completeValueType = completeValueType; | ||
| this.fieldValue = fieldValue; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| package graphql.execution; | ||
|
|
||
| import graphql.Internal; | ||
| import graphql.PublicApi; | ||
|
|
||
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| /** | ||
| * This class is used to track the number of result nodes that have been created during execution. | ||
| * After each execution the GraphQLContext contains a ResultNodeInfo object under the key {@link ResultNodesInfo#RESULT_NODES_INFO} | ||
| * <p> | ||
| * The number of result can be limited (and should be for security reasons) by setting the maximum number of result nodes | ||
| * in the GraphQLContext under the key {@link ResultNodesInfo#MAX_RESULT_NODES} to an Integer | ||
| * </p> | ||
| */ | ||
| @PublicApi | ||
| public class ResultNodesInfo { | ||
|
|
||
| public static final String MAX_RESULT_NODES = "__MAX_RESULT_NODES"; | ||
| public static final String RESULT_NODES_INFO = "__RESULT_NODES_INFO"; | ||
|
|
||
| private volatile boolean maxResultNodesExceeded = false; | ||
| private final AtomicInteger resultNodesCount = new AtomicInteger(0); | ||
|
|
||
| @Internal | ||
| public int incrementAndGetResultNodesCount() { | ||
| return resultNodesCount.incrementAndGet(); | ||
| } | ||
|
|
||
| @Internal | ||
| public void maxResultNodesExceeded() { | ||
| this.maxResultNodesExceeded = true; | ||
| } | ||
|
|
||
| /** | ||
| * The number of result nodes created. | ||
| * Note: this can be higher than max result nodes because | ||
| * a each node that exceeds the number of max nodes is set to null, | ||
| * but still is a result node (with value null) | ||
| * | ||
| * @return number of result nodes created | ||
| */ | ||
| public int getResultNodesCount() { | ||
| return resultNodesCount.get(); | ||
| } | ||
|
|
||
| /** | ||
| * If the number of result nodes has exceeded the maximum allowed numbers. | ||
| * | ||
| * @return true if the number of result nodes has exceeded the maximum allowed numbers | ||
| */ | ||
| public boolean isMaxResultNodesExceeded() { | ||
| return maxResultNodesExceeded; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package graphql.execution | |
|
|
||
| import graphql.ErrorType | ||
| import graphql.ExecutionResult | ||
| import graphql.GraphQLContext | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| import graphql.execution.instrumentation.ExecutionStrategyInstrumentationContext | ||
| import graphql.execution.instrumentation.SimpleInstrumentation | ||
| import graphql.execution.instrumentation.parameters.InstrumentationExecutionStrategyParameters | ||
|
|
@@ -28,6 +29,8 @@ import static org.awaitility.Awaitility.await | |
|
|
||
| class AsyncExecutionStrategyTest extends Specification { | ||
|
|
||
| def graphqlContextMock = Mock(GraphQLContext) | ||
|
|
||
| GraphQLSchema schema(DataFetcher dataFetcher1, DataFetcher dataFetcher2) { | ||
| GraphQLFieldDefinition.Builder fieldDefinition = newFieldDefinition() | ||
| .name("hello") | ||
|
|
@@ -82,6 +85,7 @@ class AsyncExecutionStrategyTest extends Specification { | |
| .operationDefinition(operation) | ||
| .instrumentation(SimpleInstrumentation.INSTANCE) | ||
| .valueUnboxer(ValueUnboxer.DEFAULT) | ||
| .graphQLContext(graphqlContextMock) | ||
| .build() | ||
| ExecutionStrategyParameters executionStrategyParameters = ExecutionStrategyParameters | ||
| .newParameters() | ||
|
|
@@ -121,6 +125,7 @@ class AsyncExecutionStrategyTest extends Specification { | |
| .operationDefinition(operation) | ||
| .valueUnboxer(ValueUnboxer.DEFAULT) | ||
| .instrumentation(SimpleInstrumentation.INSTANCE) | ||
| .graphQLContext(graphqlContextMock) | ||
| .build() | ||
| ExecutionStrategyParameters executionStrategyParameters = ExecutionStrategyParameters | ||
| .newParameters() | ||
|
|
@@ -162,6 +167,7 @@ class AsyncExecutionStrategyTest extends Specification { | |
| .operationDefinition(operation) | ||
| .valueUnboxer(ValueUnboxer.DEFAULT) | ||
| .instrumentation(SimpleInstrumentation.INSTANCE) | ||
| .graphQLContext(graphqlContextMock) | ||
| .build() | ||
| ExecutionStrategyParameters executionStrategyParameters = ExecutionStrategyParameters | ||
| .newParameters() | ||
|
|
@@ -202,6 +208,7 @@ class AsyncExecutionStrategyTest extends Specification { | |
| .operationDefinition(operation) | ||
| .instrumentation(SimpleInstrumentation.INSTANCE) | ||
| .valueUnboxer(ValueUnboxer.DEFAULT) | ||
| .graphQLContext(graphqlContextMock) | ||
| .build() | ||
| ExecutionStrategyParameters executionStrategyParameters = ExecutionStrategyParameters | ||
| .newParameters() | ||
|
|
@@ -262,6 +269,7 @@ class AsyncExecutionStrategyTest extends Specification { | |
| } | ||
| } | ||
| }) | ||
| .graphQLContext(graphqlContextMock) | ||
| .build() | ||
| ExecutionStrategyParameters executionStrategyParameters = ExecutionStrategyParameters | ||
| .newParameters() | ||
|
|
||
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