-
Notifications
You must be signed in to change notification settings - Fork 1.2k
21.x backport 3525 max result nodes #3528
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 |
|---|---|---|
|
|
@@ -62,6 +62,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; | ||
|
|
@@ -239,7 +240,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()); | ||
| GraphQLCodeRegistry codeRegistry = executionContext.getGraphQLSchema().getCodeRegistry(); | ||
| return fetchField(fieldDef, executionContext, parameters); | ||
| } | ||
|
|
||
| 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.
|
||
| } | ||
| } | ||
|
|
||
|
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. The next two lines (variables |
||
| MergedField field = parameters.getField(); | ||
| GraphQLObjectType parentType = (GraphQLObjectType) parameters.getExecutionStepInfo().getUnwrappedNonNullType(); | ||
|
|
||
| // if the DF (like PropertyDataFetcher) does not use the arguments or execution step info then dont build any | ||
|
|
||
|
|
@@ -274,6 +291,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. Here's where the codeRegistry line got moved to |
||
| DataFetcher<?> dataFetcher = codeRegistry.getDataFetcher(parentType, fieldDef); | ||
|
|
||
| Instrumentation instrumentation = executionContext.getInstrumentation(); | ||
|
|
@@ -568,6 +586,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(ExecutionResult.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. In master this second argument is This is because since 21.x was last released we have made a big change to remove unnecessary ExecutionResult objects. My test hung weirdly because in 21.x land, the engine never expects a null, it expects an ExecutionResult |
||
| } | ||
| } | ||
|
|
||
| ResultPath indexedPath = parameters.getPath().segment(index); | ||
|
|
||
| ExecutionStepInfo stepInfoForListElement = executionStepInfoFactory.newExecutionStepInfoForListElement(executionStepInfo, indexedPath); | ||
|
|
||
| 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. This has default visibility on current 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. This has default visibility on current 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; | ||
| } | ||
| } |
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 line is moved down in this PR
In latest master we added one extra method to break up the logic here. The code registry is used in the method being called here