bito-code-review[bot] commented on code in PR #44499:
URL: https://github.com/apache/superset/pull/44499#discussion_r4068759467
##########
superset/sqllab/api.py:
##########
@@ -410,6 +429,7 @@ def export_streaming_csv(self) -> Response:
"GRANULAR_EXPORT_CONTROLS"
) and not security_manager.can_access("can_export_data", "Superset"):
return self.response_403()
+ check_download_reason() # 400 via the SupersetErrorException handler
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: Missing tests for streaming gate</b></div>
<div id="fix">
Same gap as the GET route: the `check_download_reason()` call in
`export_streaming_csv` has no test exercising the 400 path when
`REQUIRE_DOWNLOAD_REASON` is enabled, nor the query-string-vs-form acceptance
documented in the new OpenAPI block. `tests/unit_tests/sqllab/api_test.py`
covers neither export route. Add flag-on/flag-off cases for this POST endpoint.
([CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<small><i>Code Review Run #ab324a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/sqllab/api.py:
##########
@@ -332,6 +341,7 @@ def export_csv(self, client_id: str) -> CsvResponse:
"GRANULAR_EXPORT_CONTROLS"
) and not security_manager.can_access("can_export_data", "Superset"):
return self.response_403()
+ check_download_reason() # 400 via the SupersetErrorException handler
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: Missing tests for download gate</b></div>
<div id="fix">
The new `check_download_reason()` gate in `export_csv` has no endpoint-level
test coverage: `tests/unit_tests/sqllab/api_test.py` has no test touching the
export route or the 400 path when `REQUIRE_DOWNLOAD_REASON` is enabled. The
helper has its own tests, but the wiring here (call placement after the
`GRANULAR_EXPORT_CONTROLS` gate, `DownloadReasonRequiredError` -> 400
propagation) is what can silently regress. Please add flag-on/flag-off cases.
([CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<small><i>Code Review Run #ab324a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/SqlLab/components/ResultSet/index.tsx:
##########
@@ -369,31 +374,59 @@ const ResultSet = ({
schema: query?.schema,
};
- const handleDownloadCsv = (event: React.MouseEvent<HTMLElement>) => {
- logAction(LOG_ACTIONS_SQLLAB_DOWNLOAD_CSV, {});
-
- if (limitingFactor === LimitingFactor.Dropdown && limit === rowsCount)
{
- event.preventDefault();
+ const handleDownloadCsv = async (
+ event: React.MouseEvent<HTMLElement>,
+ ) => {
+ const confirmLimit =
+ limitingFactor === LimitingFactor.Dropdown && limit === rowsCount;
+ if (!isDownloadReasonRequired() && !confirmLimit) {
+ // Flag off: keep the anchor's native navigation (cmd/ctrl-click,
+ // "open in new tab", ...) exactly as before.
+ logAction(LOG_ACTIONS_SQLLAB_DOWNLOAD_CSV, {});
+ return;
+ }
+ // Take over the navigation so the reason / confirm dialog runs first.
+ event.preventDefault();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing preventDefault duplicates download</b></div>
<div id="fix">
The flag-off fast path (line 382) returns before `event.preventDefault()`,
so with `RequireDownloadReason` disabled the anchor keeps its native navigation
— but the handler is attached even when `confirmLimit` is true, so the LIMIT
confirm dialog now shows while the browser simultaneously downloads `href`
(`getExportCsvUrl`, line 455). The old code called `preventDefault()` in this
branch; the download is now duplicated. Move `preventDefault()` above the early
return.
</div>
</div>
<small><i>Code Review Run #ab324a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
