[fix] Eliminate silent query limits - #4980
Conversation
barnabasdomozi
left a comment
There was a problem hiding this comment.
In general, it's considered a good practice to have some hard limits on the amount of data and API endpoint can return from the database.
As an example, it's not uncommon to have production databases with more than 1 million rows in the RunHistory table. Without any enforced limit, the client could request all information at once which is an excessive amount of data.
Silently limiting the results may cause practical problems, too. For example, when we filter on runs with a joker character (e.g. myrun*) then the results from the first 500 runs return only.
For which API endpoints is this a problem?
I think, that's why these functions have a
In this specific case the client queried the run IDs matching a run name pattern |
Yes, but if we remove the guard, essentially the client controls this limit parameter. If the limit set by the client is
Maybe you meant a different function, but as of today, I think this server-side limit enforcement should be investigated on a case by case basis, and not removed from everywhere. |
a37b170 to
6564394
Compare
Many report_server API functions have a limit parameter for limiting the number of results. When the parameter is 0 or None, then the MAX_QUERY_SIZE is used which is currently 500. Silently limiting the results may cause practical problems, too. For example, when we filter on runs with a joker character (e.g. myrun*) then the results from the first 500 runs return only.
| resolve(res); | ||
| })); | ||
| }); | ||
| while (true) { |
There was a problem hiding this comment.
Why do we need to return all runs in this case?
There was a problem hiding this comment.
This is just a helper function for those who need to get all runs. The users of this function would make several requests with increasing offset values either way.
The semantics of the code hasn't been changed. Even in the previous version, all runs were queried. So the question is: why do the callers of this function would need all runs?
The answer for this specific question is that getRuns() is used at 2 places:
- In
getSameReports()the run names are queried that contain a specific bug hash. In the worst case scenario a bug hash is found in all runs. Since we don't have a paging functionality for displaying them, we show them all. It could be paginated in the future. - In
ReportInfo.vuethis function is used for querying a run for a specific run ID. Yes, it's unnecessary to use thisgetRuns()in that case. I can fix that.
There was a problem hiding this comment.
Sorry, I was looking at a wrong branch. This function is used at several places. At those places all runs were queried with pagination even before.
Many report_server API functions have a limit parameter for limiting the number of results. When the parameter is 0 or None, then the MAX_QUERY_SIZE is used which is currently 500.
Silently limiting the results may cause practical problems, too. For example, when we filter on runs with a joker character (e.g. myrun*) then the results from the first 500 runs return only.