[fix] Eliminate silent query limits - #4980
Conversation
barnabasdomozi
left a comment
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. |
6564394 to
e9614fb
Compare
e9614fb to
3ef554a
Compare
| max_query_limit) | ||
| limit = max_query_limit | ||
| return limit | ||
| if not limit or limit not in range(constants.MAX_QUERY_SIZE + 1): |
There was a problem hiding this comment.
Consider the following if expression which is more Pythonic and easier to read:
| if not limit or limit not in range(constants.MAX_QUERY_SIZE + 1): | |
| if limit not in range(1, constants.MAX_QUERY_SIZE + 1): |
| ccService.getClient().getRunData(filter, null, null, null, | ||
| handleThriftError(runDataList => { | ||
| if (runDataList.length !== 1) return; | ||
| ccService.getRunData(filter).then(runDataList => { |
There was a problem hiding this comment.
Why was .getClient() dropped from here? ccService has no exported function named getRunData.
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.
3ef554a to
3601a39
Compare
| const runFilter = new RunFilter({ ids: [ props.value.runId ] }); | ||
| ccService.getClient().getRunData(runFilter, 1).then(runs => { | ||
| runName.value = runs[0].name; | ||
| console.warn(runName.value); |
There was a problem hiding this comment.
Debug prints should be removed.
|
|
||
| .splitpanes__pane { | ||
| overflow-y: auto; | ||
| overflow-y: hidden; |
There was a problem hiding this comment.
Why was this CSS property changed? Looks unrelated to this patch.
| ccService.getClient().getRunData(filter, null, null, null, | ||
| handleThriftError(runDataList => { | ||
| if (runDataList.length !== 1) return; | ||
| ccService.getClient().getRunData(filter).then(runDataList => { |
There was a problem hiding this comment.
The limit parameter is not supplied in this case and due to the new server-side design this will fail.
| run_ids = [r.runId for r in runs] | ||
|
|
||
| run_history = client.getRunHistory(run_ids, None, None, None) | ||
| run_history = client.getRunHistory( |
There was a problem hiding this comment.
This change breaks backward compatibility. Many users still rely on older CodeChecker clients which will still pass None as the limit parameter to the server. Due to this new server design, the API requests e.g. getRunHistory will fail for these older clients.
|
The issue with this new enforcement is that older CodeChecker clients will still rely on passing |
2 participants
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.