feat: limit number of items of aql queries for queries and mutations … #318
Conversation
Etienne-Buschong
force-pushed
the
limit-root-entities-query-and-mutation
branch
from
80443ed
to
f568a50
Compare
Etienne-Buschong
force-pushed
the
limit-root-entities-query-and-mutation
branch
from
f568a50
to
54e45be
Compare
|
@Yogu Is there anything left from your side that I should adapt? |
currently reviewing it |
| const MANUAL_MAX_COUNT_UPPER_BOUND = 50000; | ||
| const DEFAULT_MAX_COUNT_UPPER_BOUND = 10000; |
There was a problem hiding this comment.
can you make these configurable via ExecutionOptions?
for MANUAL_MAX_COUNT_UPPER_BOUND, we could call the option something like maxLimitForRootEntityQueries.
for DEFAULT_MAX_COUNT_UPPER_BOUND, we could call it something like implicitLimitForRootEntityQueries
not 100% sure about the names. "first" would be closer than "limit", but I don't think you would think of the argument if you read "maxFirst" or "maxFirstValue"...
the options should document which queries this limit applies to and that it will throw an error if the limit is exceeded.
It should be possible to turn both features off, I would even turn them off by default (if kept as undefined)
| const MANUAL_MAX_COUNT_UPPER_BOUND = 50000; | ||
| const DEFAULT_MAX_COUNT_UPPER_BOUND = 10000; | ||
|
|
||
| export type LimitTypeCheckType = 'ResultValidator' | 'Resolver'; |
There was a problem hiding this comment.
cruddl usually uses enums instead of string unions
| if (isRuntimeErrorValue(value)) { | ||
| throw extractRuntimeError(value); | ||
| } | ||
|
|
||
| if (transformResult) { | ||
| return transformResult(value, args); | ||
| } |
There was a problem hiding this comment.
we could swap the two (and run them one after the other), then you would be able to swallow or create runtime errors in the transformer
| * Will be called in the actual graphql field resolver for this node. | ||
| * Can be used to transform and validate a fields data after query-execution. | ||
| * |
There was a problem hiding this comment.
| * Will be called in the actual graphql field resolver for this node. | |
| * Can be used to transform and validate a fields data after query-execution. | |
| * | |
| * Will be called in the actual graphql field resolver for this node. | |
| * Can be used to transform and validate a fields data after query-execution. | |
| * | |
| * Needs be synchronous, it is not possible to return a promise here. | |
| * |
because in a regular resolver, you would be able to return a promise. For performance reasons it's probably better to not add the await while we don't need it
| maxCount > MANUAL_MAX_COUNT_UPPER_BOUND | ||
| ) { | ||
| return new RuntimeErrorQueryNode( | ||
| `The requested number of elements via the \"first\" parameter (${maxCount}) exceeds the maximum limit of ${MANUAL_MAX_COUNT_UPPER_BOUND}. Reduce the number of items you fetch and try again.`, |
There was a problem hiding this comment.
| `The requested number of elements via the \"first\" parameter (${maxCount}) exceeds the maximum limit of ${MANUAL_MAX_COUNT_UPPER_BOUND}. Reduce the number of items you fetch and try again.`, | |
| `The requested number of elements via the \"first\" parameter (${maxCount}) exceeds the maximum limit of ${MANUAL_MAX_COUNT_UPPER_BOUND}.`, |
I think the action is obvious from the problem description
| ) { | ||
| return new RuntimeErrorQueryNode( | ||
| `The requested number of elements via the \"first\" parameter (${maxCount}) exceeds the maximum limit of ${MANUAL_MAX_COUNT_UPPER_BOUND}. Reduce the number of items you fetch and try again.`, | ||
| { code: NOT_SUPPORTED_ERROR }, |
There was a problem hiding this comment.
NOT_SUPPORTED means a feature or is not supported in a specific context. I don't think any of the existing error codes are fitting. You probably don't need to catch this error and handle it specifically - if it occurs, you need to change your page sizes in the code. So maybe a plain ARGUMENT_OUT_OF_RANGE?
| throw new RuntimeValidationError( | ||
| `Collection is truncated by default to ${validationData.maximumExpectedNumberOfItems} elements but contains more elements than this limit. Specify a limit manually to retrieve all elements of the collection.`, | ||
| { | ||
| code: validationData.errorCode, |
There was a problem hiding this comment.
you don't pass this errorCode in getValidatorData(). But I don't think we need to make it configurable if the message is hard-coded.
regarding the code - I could see TOO_MANY_OBJECTS - we already have FLEX_SEARCH_TOO_MANY_OBJECTS and it's kind of similar.
|
|
||
| if (result.length > validationData.maximumExpectedNumberOfItems) { | ||
| throw new RuntimeValidationError( | ||
| `Collection is truncated by default to ${validationData.maximumExpectedNumberOfItems} elements but contains more elements than this limit. Specify a limit manually to retrieve all elements of the collection.`, |
There was a problem hiding this comment.
I would not use the term "truncated" here because for the user, nothing is truncated - an error is thrown. Implicit truncation is exactly what we want to avoid doing.
There's also a second reason you might want to specify a limit: if you want a truncated result. So I would include that in the error message.
I think phrasing the error is easier when we know the kind of operation we do
'Query would return more than {limit} objects. Specify "first" to increase the limit or truncate the result.'
'Mutation would return more than {limit} objects. Specify "first" to increase the limit or truncate the result.'
| const fieldWithListArgs = this.listAugmentation.augment(fieldBase, rootEntityType, { | ||
| orderByAugmentationOptions: { firstLimitCheckType: 'ResultValidator' }, | ||
| }); |
There was a problem hiding this comment.
We also need to apply the logic to generateDeleteAllField (forgot about that when we talked about it). It returns all deleted objects, so we also need to limit it.
…(wip)