-
Notifications
You must be signed in to change notification settings - Fork 98
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
disable pagination #393
Comments
just set |
yep this is my actual workaround :) i was searching just if there was some cleaner way maybe can be implemented like putting |
FYI, the workaround of setting an high default/maxLimit will cause a very inefficient query generated in case of relations on the table (something like #329) that is not happening using the suggested PR |
@FrancYescO Could elaborate in more detail about the differences? It still executes COUNT + FIND An issue with the current implementation would be that it introduces an attack surface. Allowing every client to remove the resource limit can cause perf issues on the db with large tables (DoS attack). |
The limit (and DoS protection) should be up to the dafault/maxLimit (so if the maxlimit is i.e. 20 don't allow the 0-infinite limit). Count + find will cause the result query to have a giant list of OR for each row on each key of joined table
That will cause like x100 of time to execute the query and get the results the PR suggested does not set any LIMIT in the query that will not cause this query composition. |
(1) Ok, the PR tho used an infinite limit 😅 (2) So, you are saying removing (3) doesn't (2) imply that we query an infinite result set? |
|
|
to (1) if we must do (2) then setting I think, what it comes down to is that there cannot be any DoS protection. But I understand that there are use cases where this is neglectable. So, I am okay adding this feature but it should be whitelisted. Something like |
enableLimitRemoval: true == maxLimit: 0 |
Okay with that as well. But then no pagination would be possible on that endpoint. So always infinite resource requests. |
Btw. not sure how typeorm handles this. But if I anyway query all items, I don't need a separate COUNT query. So you may optimize there as well |
pagination will be enabled if runtime i'll set the limit to something different than 0 (PR already act like this) |
once again, I am not okay with making it a default behavior due to the attack surface it opens I understood you suggested using |
sure i was not saying to set as defaultLimit/maxLimit to 0 (neither this is done in the PR), but just to use maxLimit: 0 as a know-what-you-are-doing flag. there are other usecase while pagination can be enabled also when maxLimit: 0:
|
ah okay, that's cool. I was reading it still literally but you meant sounds good to me. please add some lines to the README then :) |
just got another needs (get the total count of filtered items avoiding to get all datas) and probably a little rework on this PR can help...
|
cannot really follow. could you explain with example query + response? |
maybe easier in this way:
|
that actually makes sense semantically, lol.
no need to hijack those fields. so, yeah would accept PR 😅 |
addition:
|
Looking at the name of the project probably out of the scope :D
but is there a way to disable pagination (i just need to reuse the filters and maybe sorting and other applicable logic)?
The text was updated successfully, but these errors were encountered: