You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/07/18 07:50:42 UTC

[PR] fix(charts): use chart name in CSV/XLSX/zip export filenames [superset]

rusackas opened a new pull request, #42193:
URL: https://github.com/apache/superset/pull/42193

   ### SUMMARY
   Chart data exports (CSV/XLSX/zip) from `/api/v1/chart/data` were always
   named with a bare timestamp (e.g. `20260127_160233.csv`), even though the
   endpoint already extracts a sanitized `filename` from the request. Only
   the large-dataset *streaming* CSV path honored that filename (and its
   chart-name-based fallback); the regular non-streaming CSV, XLSX, and
   multi-query zip branches in `_send_chart_response` called
   `generate_download_headers()` with no filename at all, dropping it on the
   floor.
   
   This PR:
   - Passes the extracted `filename` through to `generate_download_headers()`
     in the CSV, XLSX, and zip branches.
   - When no filename is supplied, falls back to the same chart-name +
     timestamp default already used by the streaming CSV path (extracted into
     a shared `_get_default_export_filename` helper to avoid duplicating that
     logic).
   - Report/email attachments already use the report name, so this change is
     scoped to the chart data export endpoints only.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   N/A (backend header change only)
   
   ### TESTING INSTRUCTIONS
   1. Open any saved chart in Explore.
   2. Use Download → Export to CSV (or Excel).
   3. Confirm the downloaded filename starts with the chart's name instead of
      just a timestamp.
   
   Also see the added unit tests in
   `tests/unit_tests/charts/test_chart_data_api.py`, which pin the bug by
   asserting the `Content-Disposition` header contains the chart name for
   the CSV, XLSX, and zip export branches.
   
   ### ADDITIONAL INFORMATION
   - [x] Has associated issue: Fixes #37487
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(charts): use chart name in CSV/XLSX/zip export filenames [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42193:
URL: https://github.com/apache/superset/pull/42193#issuecomment-5010494324

   <!-- Bito Reply -->
   The flagged issue is correct. When a user provides a filename with an extension (e.g., `my_export.csv`), `generate_download_headers` appends the format extension again, resulting in double extensions like `my_export.csv.csv`.
   
   To resolve this, you should strip the extension from the provided `export_filename` before passing it to `generate_download_headers`. You can use `os.path.splitext` for this purpose.
   
   ### Proposed Fix
   
   In `superset/charts/data/api.py`, update the `_send_chart_response` method to normalize the filename:
   
   ```python
   import os
   
   # ... inside _send_chart_response ...
   export_filename = filename or self._get_default_export_filename(form_data)
   base_filename = os.path.splitext(export_filename)[0]
   
   if is_csv_format:
       return CsvResponse(
           data, headers=generate_download_headers("csv", base_filename)
       )
   ```
   
   Since there are no other comments in the provided PR context, there are no additional issues to address.
   
   **superset/charts/data/api.py**
   ```
   import os
   
   # ... inside _send_chart_response ...
   export_filename = filename or self._get_default_export_filename(form_data)
   base_filename = os.path.splitext(export_filename)[0]
   
   if is_csv_format:
       return CsvResponse(
           data, headers=generate_download_headers("csv", base_filename)
       )
   ```


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(charts): use chart name in CSV/XLSX/zip export filenames [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42193:
URL: https://github.com/apache/superset/pull/42193#discussion_r3624113761


##########
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 pass as-is... the unit test conftest has an autouse `app_context` fixture that wraps every test in `app.app_context()`, so there's always an app context here. Just ran the file locally and all 13 pass. Same goes for the other three comments below, so resolving those too.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(charts): use chart name in CSV/XLSX/zip export filenames [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #42193:
URL: https://github.com/apache/superset/pull/42193


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(charts): use chart name in CSV/XLSX/zip export filenames [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42193:
URL: https://github.com/apache/superset/pull/42193#issuecomment-5010491565

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42193?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `15.38462%` with `11 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 56.17%. Comparing base ([`f6acf68`](https://app.codecov.io/gh/apache/superset/commit/f6acf68dfaa8e29d2568e0f002f45e9edbcaf2ca?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b4788d2`](https://app.codecov.io/gh/apache/superset/commit/b4788d2e2138190077bab9249b016f623668b977?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/42193?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [superset/charts/data/api.py](https://app.codecov.io/gh/apache/superset/pull/42193?src=pr&el=tree&filepath=superset%2Fcharts%2Fdata%2Fapi.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY2hhcnRzL2RhdGEvYXBpLnB5) | 15.38% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42193?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   > :exclamation:  There is a different number of reports uploaded between BASE (f6acf68) and HEAD (b4788d2). Click for more details.
   > 
   > <details><summary>HEAD has 23 uploads less than BASE</summary>
   >
   >| Flag | BASE (f6acf68) | HEAD (b4788d2) |
   >|------|------|------|
   >|python|21|1|
   >|presto|4|1|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #42193      +/-   ##
   ==========================================
   - Coverage   65.08%   56.17%   -8.91%     
   ==========================================
     Files        2758     2758              
     Lines      155229   155233       +4     
     Branches    35572    35572              
   ==========================================
   - Hits       101026    87199   -13827     
   - Misses      52288    67205   +14917     
   + Partials     1915      829    -1086     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/42193/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/42193/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [mysql](https://app.codecov.io/gh/apache/superset/pull/42193/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [postgres](https://app.codecov.io/gh/apache/superset/pull/42193/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/42193/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.90% <15.38%> (+<0.01%)` | :arrow_up: |
   | [python](https://app.codecov.io/gh/apache/superset/pull/42193/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.90% <15.38%> (-18.21%)` | :arrow_down: |
   | [sqlite](https://app.codecov.io/gh/apache/superset/pull/42193/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/42193/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42193?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(charts): use chart name in CSV/XLSX/zip export filenames [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42193:
URL: https://github.com/apache/superset/pull/42193#issuecomment-5010481668

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(charts): use chart name in CSV/XLSX/zip export filenames [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42193:
URL: https://github.com/apache/superset/pull/42193#issuecomment-5012203737

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
   
   <details>
   <summary>
   Bito Usage Guide
   </summary>
   
   **Commands**
         
   Type the following command in the pull request comment and save the comment.
         
   - `/review` - Manually triggers a full AI review.
   
   - `/pause` - Pauses automatic reviews on this pull request.
   - `/resume` - Resumes automatic reviews.
   - `/resolve` - Marks all Bito-posted review comments as resolved.
   - `/abort` - Cancels all in-progress reviews.
   
         
   Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
         
   **Configuration**
   
   This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
         
   **Documentation & Help**
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
   
   </details>
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(charts): use chart name in CSV/XLSX/zip export filenames [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42193:
URL: https://github.com/apache/superset/pull/42193#discussion_r3607947519


##########
superset/charts/data/api.py:
##########
@@ -495,13 +495,19 @@ def _send_chart_response(  # noqa: C901
                     result, form_data, filename=filename, expected_rows=expected_rows
                 )
 
+            export_filename = filename or self._get_default_export_filename(form_data)

Review Comment:
   **Suggestion:** `filename` can already include an extension (for example, `my_export.csv`), but it is passed directly as `export_filename` into `generate_download_headers`, which also appends the format extension; this produces duplicated names like `.csv.csv`/`.xlsx.xlsx` in downloads. Normalize the provided filename to a base name (or strip a matching trailing extension) before calling `generate_download_headers`. [logic error]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ⚠️ CSV exports with client-specified names show double extensions.
   - ⚠️ Affects /api/v1/chart/data non-streaming CSV responses.
   - ⚠️ Third-party clients using filename param get confusing filenames.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Call the chart data export endpoint `GET /api/v1/chart/data` (implemented in
   `superset/charts/data/api.py` in the `ChartDataRestApi` class) with
   `result_format=ChartDataResultFormat.CSV` and a single query, and include a `filename`
   parameter such as `filename=my_export.csv` so that `_send_chart_response` receives a
   non-empty `filename` argument.
   
   2. In `_send_chart_response` at `superset/charts/data/api.py:490-506`, the CSV path skips
   streaming because `_should_use_streaming(result, form_data)` at lines 493-496 returns
   `False` for normal-sized datasets, and line 498 sets `export_filename = filename or
   self._get_default_export_filename(form_data)`, so `export_filename` is the literal string
   `my_export.csv` including the `.csv` extension.
   
   3. Still in `_send_chart_response`, for a single-query CSV result, the branch at lines
   500-506 executes `return CsvResponse(data, headers=generate_download_headers("csv",
   export_filename))`, passing `"csv"` as the format and `"my_export.csv"` as the
   `export_filename` argument into `generate_download_headers`.
   
   4. In `generate_download_headers` (defined in `superset/utils/core.py`, which constructs
   the `Content-Disposition` header by appending the format extension to the provided
   filename), the header becomes `attachment; filename="my_export.csv.csv"`, so the browser
   downloads a file named `my_export.csv.csv` instead of `my_export.csv`, demonstrating the
   duplicated extension whenever the client supplies a filename that already includes the
   extension.
   ```
   </details>
   
   [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=55c4b648db0b47519e1dad3e6354b6cf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=55c4b648db0b47519e1dad3e6354b6cf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/charts/data/api.py
   **Line:** 498:498
   **Comment:**
   	*Logic Error: `filename` can already include an extension (for example, `my_export.csv`), but it is passed directly as `export_filename` into `generate_download_headers`, which also appends the format extension; this produces duplicated names like `.csv.csv`/`.xlsx.xlsx` in downloads. Normalize the provided filename to a base name (or strip a matching trailing extension) before calling `generate_download_headers`.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
   Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42193&comment_hash=7742c6a61740580ac0b44f79b81bf6ee39bace11e39afbae8b908aa358685077&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42193&comment_hash=7742c6a61740580ac0b44f79b81bf6ee39bace11e39afbae8b908aa358685077&reaction=dislike'>👎</a>



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(charts): use chart name in CSV/XLSX/zip export filenames [superset]

Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #42193:
URL: https://github.com/apache/superset/pull/42193#issuecomment-5011586997

   ### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
   
   
   |  Name | Link |
   |:-:|------------------------|
   |<span aria-hidden="true">🔨</span> Latest commit | 2f576020946c96be2abd30c6ad1a5ae76200cab9 |
   |<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a5b8aa39ce8b70008373b27 |
   |<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-42193--superset-docs-preview.netlify.app](https://deploy-preview-42193--superset-docs-preview.netlify.app) |
   |<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTQyMTkzLS1zdXBlcnNldC1kb2NzLXByZXZpZXcubmV0bGlmeS5hcHAifQ.mB2jF5F6SCIRN7a1eZibUzKwZ4j4fhReVND8-nI1Tbw)<br /><br />_Use your smartphone camera to open QR code link._</details> |
   |<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctBCoAgEEDRu8w6SbIa6wYdY7QJBUuxAiO6e67-37wXUo57umAGaMBkOqyrv_nS-vO8WSjsNQrrKF-CS4o1mw980M4VrJxCfJa1kpEGo4nUZFkblFJqhcp0CN8P) |
   ---
   <!-- [superset-docs-preview Preview](https://deploy-preview-42193--superset-docs-preview.netlify.app) -->
   _To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(charts): use chart name in CSV/XLSX/zip export filenames [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42193:
URL: https://github.com/apache/superset/pull/42193#discussion_r3608515061


##########
superset/charts/data/api.py:
##########
@@ -495,13 +495,19 @@ def _send_chart_response(  # noqa: C901
                     result, form_data, filename=filename, expected_rows=expected_rows
                 )
 
+            export_filename = filename or self._get_default_export_filename(form_data)

Review Comment:
   Confirmed — the frontend only appends `.csv` to `filename` on the streaming-export path, but the backend independently recomputes `_should_use_streaming`, so a mismatch can route that already-extensioned name into the non-streaming branch and double it. Fixed by stripping a matching trailing extension before it hits `generate_download_headers`.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(charts): use chart name in CSV/XLSX/zip export filenames [superset]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org