-
Notifications
You must be signed in to change notification settings - Fork 0
fix: limit the maximum number of data rows fetched at once, other parameter checks #116
Merged
+264
−56
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If we would leave limit as possibly null, we could have an SQL query with LIMIT NULL, which would return all rows
It does not make sense to allow these to be null. They should either be set explicitly, or not be set by the user so they use the default.
This is so someone does not attempt to fetch our whole database at the same time, using aliases as explained in requestLimit.ts
hestro
previously approved these changes
Nov 12, 2026
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.
Nice! This worked as expected for me
The merge-base changed after approval.
johahor
previously approved these changes
Nov 12, 2026
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.
Same, works well. Merging.
johahor
approved these changes
Nov 12, 2026
johahor
previously approved these changes
Nov 12, 2026
The merge-base changed after approval.
hestro
previously approved these changes
Nov 12, 2026
The merge-base changed after approval.
|
Merged main into here now, so it should be good, but let's wait for CI first. |
hestro
approved these changes
Nov 12, 2026
Sign in
to join this conversation on GitHub.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The purpose of this PR is to check the user-provided parameters to the GraphQL queries, so we do not return unexpected results, or too many rows of data at once.
Specifically:
limitparameter is capped at 100 rows of data at oncenullvalues for the parametersfuzzy,offset,sortBy,sortDirectionand other that do not make sense if they're set tonullexplicitlyBecause of these backend changes, the following has changed on the frontend:
Feel free to test these features when checking the PR.
This branch should be merged after #115, since it builds upon that PR.
Closes #113