-
Notifications
You must be signed in to change notification settings - Fork 1.2k
20.x Backport max result nodes PR 3525 #3536
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 |
|---|---|---|
|
|
@@ -61,6 +61,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; | ||
|
|
@@ -238,7 +239,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. The next two lines (variables field and parentType) are not part of max result backport. I have done this to make this method look as close as possible to master |
||
| 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 | ||
|
|
||
|
|
@@ -273,6 +290,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. codeRegistry line moved down to here |
||
| DataFetcher<?> dataFetcher = codeRegistry.getDataFetcher(parentType, fieldDef); | ||
|
|
||
| Instrumentation instrumentation = executionContext.getInstrumentation(); | ||
|
|
@@ -555,6 +573,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. Difference to master - must have a completed value of type |
||
| } | ||
| } | ||
|
|
||
| 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. I changed visibility to be the 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. I changed visibility to line up to master. Yes there is a builder too |
||
| 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; | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
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.
Slight difference to master - FetchedValue constructor used to have an extra argument (in second position). Doesn't matter as meaning is the same (return early with a null)