bito-code-review[bot] commented on code in PR #43280:
URL: https://github.com/apache/superset/pull/43280#discussion_r3803513573
##########
tests/unit_tests/charts/test_chart_data_api.py:
##########
@@ -858,3 +859,252 @@ def
test_send_chart_response_does_not_double_extension_for_csv_filename() -> Non
content_disposition = response.headers["Content-Disposition"]
assert "my_export.csv.csv" not in content_disposition
assert "my_export.csv" in content_disposition
+
+
+def test_default_export_filename_prefers_explicit_slice_name() -> None:
+ """An explicit slice_name in the form data wins over the chart object."""
+ filename = ChartDataRestApi._get_default_export_filename(
+ {"slice_name": "Explicit Name", "viz_type": "table"},
+ MagicMock(slice_name="Saved Chart"),
+ )
+
+ assert filename.startswith("superset_Explicit_Name_")
+
+
+def test_default_export_filename_uses_chart_name() -> None:
+ """
+ Regression test: the chart's own name must be used for the export
+ filename. Real clients never place slice_name inside the form data
+ (Slice.form_data only injects slice_id, viz_type and datasource), so
+ the name has to come from the resolved Slice object.
+ """
+ filename = ChartDataRestApi._get_default_export_filename(
+ {"viz_type": "table"},
+ MagicMock(slice_name="Revenue by Region"),
+ )
+
+ assert filename.startswith("superset_Revenue_by_Region_")
+
+
+def test_default_export_filename_ignores_non_string_chart_name() -> None:
+ """
+ A slice whose name is not a plain string (e.g. a mock in tests, or an
+ unloaded attribute) must not leak its repr into the filename.
+ """
+ filename = ChartDataRestApi._get_default_export_filename(
+ {"viz_type": "table"},
+ MagicMock(), # slice_name is itself a MagicMock, not a str
+ )
+
+ assert filename.startswith("superset_table_")
+
+
+def test_default_export_filename_falls_back_when_name_sanitizes_to_nothing()
-> None:
+ """
+ secure_filename reduces a chart name written entirely in a non-latin
+ alphabet to an empty string; the viz_type must be used instead so the
+ filename keeps a meaningful segment.
+ """
+ filename = ChartDataRestApi._get_default_export_filename(
+ {"viz_type": "table"},
+ MagicMock(slice_name="销售报表"),
+ )
+
+ assert filename.startswith("superset_table_")
+
+
+def test_default_export_filename_without_any_candidate() -> None:
+ """With no form data and no slice the generic fallback is preserved."""
+ filename = ChartDataRestApi._get_default_export_filename(None, None)
+
+ assert filename.startswith("superset_export_")
+
+
+def test_default_export_filename_caps_very_long_chart_names() -> None:
+ """
+ Chart names can be up to 250 characters; the generated filename must
+ stay within the 255-character single-component limit of common
+ filesystems once the extension is appended, or consumers that honor
+ Content-Disposition verbatim (curl -OJ, wget) fail to save the file.
+ """
+ filename = ChartDataRestApi._get_default_export_filename(
+ {"viz_type": "table"},
+ MagicMock(slice_name="x" * 250),
+ )
+
+ assert filename.startswith("superset_" + "x" * 50)
+ assert len(filename) + len(".xlsx") <= 255
+
+
+def test_send_chart_response_uses_query_context_slice_name() -> None:
+ """
+ POST /api/v1/chart/data: when the submitted form data carries a
+ slice_id, the query context factory resolves the Slice, and its name
+ must reach the CSV filename even though slice_name itself is absent
+ from the form data.
+ """
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.CSV
+ query_context.slice_ = MagicMock(slice_name="Quarterly Revenue")
+
+ result = {
+ "query_context": query_context,
+ "queries": [{"data": "col_a,col_b\n1,2\n"}],
+ }
+
+ api = ChartDataRestApi()
+ with (
+ patch("superset.charts.data.api.security_manager") as
mock_security_manager,
+ patch("superset.charts.data.api.is_feature_enabled",
return_value=False),
+ ):
+ mock_security_manager.can_access.return_value = True
+ response = api._send_chart_response(
+ result, form_data={"viz_type": "table", "row_limit": 10}
+ )
+
+ assert "Quarterly_Revenue" in response.headers["Content-Disposition"]
+
+
+def test_send_chart_response_uses_route_slice_when_context_has_none() -> None:
+ """
+ GET /api/v1/chart/<pk>/data/: a chart's saved query context rarely
+ carries a slice_id, so the route passes the chart it loaded and that
+ name must be used for the filename.
+ """
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.CSV
+ query_context.slice_ = None
+
+ result = {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Extract duplicate test setup code</b></div>
<div id="fix">
Code duplication detected in test file. The code snippet patterns (mock
patches for security_manager and is_feature_enabled, plus ChartDataRestApi
instantiation) appear at lines 980-994 and 1007-1021 in
tests/unit_tests/charts/test_chart_data_api.py. Consider extracting this common
setup code into a reusable fixture to reduce duplication and improve
maintainability.
</div>
</div>
<small><i>Code Review Run #e6cdee</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]
