Copilot commented on code in PR #42193:
URL: https://github.com/apache/superset/pull/42193#discussion_r3623687040
##########
tests/unit_tests/charts/test_chart_data_api.py:
##########
@@ -236,3 +236,130 @@ def test_extract_export_filename_preserves_normal_name()
-> None:
def test_extract_export_filename_all_special_falls_back_to_none() -> None:
"""A name with no usable characters becomes None (generated downstream)."""
assert _extract_filename("***") is None
+
+
+def test_send_chart_response_uses_chart_name_for_csv_filename() -> None:
+ """
+ Regression test: the non-streaming CSV export branch of
+ _send_chart_response must include the chart's name in the
+ Content-Disposition header, not just a bare timestamp, mirroring the
+ streaming CSV export path.
+ """
+ from superset.charts.data.api import ChartDataRestApi
+ from superset.common.chart_data import ChartDataResultFormat,
ChartDataResultType
+
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.CSV
+
+ 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={"slice_name": "My Chart", "row_limit": 10}
+ )
+
+ content_disposition = response.headers["Content-Disposition"]
+ assert "My_Chart" in content_disposition
+
+
+def test_send_chart_response_uses_chart_name_for_xlsx_filename() -> None:
+ """Same regression as above, for the XLSX export branch."""
+ from superset.charts.data.api import ChartDataRestApi
+ from superset.common.chart_data import ChartDataResultFormat,
ChartDataResultType
+
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.XLSX
+
+ result = {
+ "query_context": query_context,
+ "queries": [{"data": b"fake-xlsx-bytes"}],
+ }
+
+ 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),
+ ):
Review Comment:
This test invokes _send_chart_response without an active application
context. While XLSX itself may not hit CsvResponse, the API code still uses
flask.current_app in nearby paths; using `app.app_context()` here keeps the
tests consistent and avoids context-related failures if the code path changes.
##########
tests/unit_tests/charts/test_chart_data_api.py:
##########
@@ -236,3 +236,130 @@ def test_extract_export_filename_preserves_normal_name()
-> None:
def test_extract_export_filename_all_special_falls_back_to_none() -> None:
"""A name with no usable characters becomes None (generated downstream)."""
assert _extract_filename("***") is None
+
+
+def test_send_chart_response_uses_chart_name_for_csv_filename() -> None:
+ """
+ Regression test: the non-streaming CSV export branch of
+ _send_chart_response must include the chart's name in the
+ Content-Disposition header, not just a bare timestamp, mirroring the
+ streaming CSV export path.
+ """
+ from superset.charts.data.api import ChartDataRestApi
+ from superset.common.chart_data import ChartDataResultFormat,
ChartDataResultType
+
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.CSV
+
+ 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={"slice_name": "My Chart", "row_limit": 10}
+ )
+
+ content_disposition = response.headers["Content-Disposition"]
+ assert "My_Chart" in content_disposition
+
+
+def test_send_chart_response_uses_chart_name_for_xlsx_filename() -> None:
+ """Same regression as above, for the XLSX export branch."""
+ from superset.charts.data.api import ChartDataRestApi
+ from superset.common.chart_data import ChartDataResultFormat,
ChartDataResultType
+
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.XLSX
+
+ result = {
+ "query_context": query_context,
+ "queries": [{"data": b"fake-xlsx-bytes"}],
+ }
+
+ 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={"slice_name": "My Chart", "row_limit": 10}
+ )
+
+ content_disposition = response.headers["Content-Disposition"]
+ assert "My_Chart" in content_disposition
+
+
+def test_send_chart_response_uses_chart_name_for_zip_filename() -> None:
+ """Same regression as above, for the multi-query zip export branch."""
+ from superset.charts.data.api import ChartDataRestApi
+ from superset.common.chart_data import ChartDataResultFormat,
ChartDataResultType
+
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.CSV
+
+ result = {
+ "query_context": query_context,
+ "queries": [
+ {"data": "col_a\n1\n"},
+ {"data": "col_a\n2\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={"slice_name": "My Chart", "row_limit": 10}
+ )
+
+ content_disposition = response.headers["Content-Disposition"]
+ assert "My_Chart" in content_disposition
+
+
+def test_send_chart_response_does_not_double_extension_for_csv_filename() ->
None:
+ """
+ Regression test: a client-supplied filename that already includes the
+ ``.csv`` extension must not be doubled (e.g. ``export.csv.csv``) by the
+ non-streaming CSV export branch of _send_chart_response.
+ """
+ from superset.charts.data.api import ChartDataRestApi
+ from superset.common.chart_data import ChartDataResultFormat,
ChartDataResultType
+
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.CSV
+
+ 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),
+ ):
Review Comment:
This regression test exercises the CSV (non-streaming) branch, which creates
a CsvResponse that reads current_app.config["CSV_EXPORT"]. Without an app
context the test will error; add the `app` fixture and include
`app.app_context()` in the `with` statement.
##########
tests/unit_tests/charts/test_chart_data_api.py:
##########
@@ -236,3 +236,130 @@ def test_extract_export_filename_preserves_normal_name()
-> None:
def test_extract_export_filename_all_special_falls_back_to_none() -> None:
"""A name with no usable characters becomes None (generated downstream)."""
assert _extract_filename("***") is None
+
+
+def test_send_chart_response_uses_chart_name_for_csv_filename() -> None:
+ """
+ Regression test: the non-streaming CSV export branch of
+ _send_chart_response must include the chart's name in the
+ Content-Disposition header, not just a bare timestamp, mirroring the
+ streaming CSV export path.
+ """
+ from superset.charts.data.api import ChartDataRestApi
+ from superset.common.chart_data import ChartDataResultFormat,
ChartDataResultType
+
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.CSV
+
+ 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),
+ ):
Review Comment:
These new tests call ChartDataRestApi._send_chart_response without a Flask
application context. That code path uses flask.current_app (e.g., CSV streaming
threshold, CsvResponse encoding, zip encoding), so the test will raise “Working
outside of application context”. Add the unit-test `app` fixture and include
`app.app_context()` in the patched `with` block.
##########
tests/unit_tests/charts/test_chart_data_api.py:
##########
@@ -236,3 +236,130 @@ def test_extract_export_filename_preserves_normal_name()
-> None:
def test_extract_export_filename_all_special_falls_back_to_none() -> None:
"""A name with no usable characters becomes None (generated downstream)."""
assert _extract_filename("***") is None
+
+
+def test_send_chart_response_uses_chart_name_for_csv_filename() -> None:
+ """
+ Regression test: the non-streaming CSV export branch of
+ _send_chart_response must include the chart's name in the
+ Content-Disposition header, not just a bare timestamp, mirroring the
+ streaming CSV export path.
+ """
+ from superset.charts.data.api import ChartDataRestApi
+ from superset.common.chart_data import ChartDataResultFormat,
ChartDataResultType
+
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.CSV
+
+ 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={"slice_name": "My Chart", "row_limit": 10}
+ )
+
+ content_disposition = response.headers["Content-Disposition"]
+ assert "My_Chart" in content_disposition
+
+
+def test_send_chart_response_uses_chart_name_for_xlsx_filename() -> None:
+ """Same regression as above, for the XLSX export branch."""
+ from superset.charts.data.api import ChartDataRestApi
+ from superset.common.chart_data import ChartDataResultFormat,
ChartDataResultType
+
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.XLSX
+
+ result = {
+ "query_context": query_context,
+ "queries": [{"data": b"fake-xlsx-bytes"}],
+ }
+
+ 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={"slice_name": "My Chart", "row_limit": 10}
+ )
+
+ content_disposition = response.headers["Content-Disposition"]
+ assert "My_Chart" in content_disposition
+
+
+def test_send_chart_response_uses_chart_name_for_zip_filename() -> None:
+ """Same regression as above, for the multi-query zip export branch."""
+ from superset.charts.data.api import ChartDataRestApi
+ from superset.common.chart_data import ChartDataResultFormat,
ChartDataResultType
+
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_context.result_format = ChartDataResultFormat.CSV
+
+ result = {
+ "query_context": query_context,
+ "queries": [
+ {"data": "col_a\n1\n"},
+ {"data": "col_a\n2\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),
+ ):
Review Comment:
The zip export branch encodes CSV strings using
current_app.config["CSV_EXPORT"], so this test will fail without a Flask app
context. Add the unit-test `app` fixture and include `app.app_context()` in the
context manager list.
--
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]
