codeant-ai-for-open-source[bot] commented on code in PR #41133: URL: https://github.com/apache/superset/pull/41133#discussion_r3571703457
########## superset/dashboards/excel_export/email.py: ########## @@ -0,0 +1,185 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Email rendering and delivery for dashboard Excel exports. + +Bodies use inline styles only (no external CSS, no logo) to match Superset's +existing report notification emails, and all user-controlled values (dashboard +title, chart names) are HTML-escaped to avoid injection. +""" + +from __future__ import annotations + +from datetime import datetime + +from flask import current_app +from flask_babel import gettext as __, ngettext +from markupsafe import escape + +from superset.utils.core import send_email_smtp + +_DATETIME_FORMAT = "%Y-%m-%d %H:%M:%S" +_FOOTER_STYLE = "color:#888;font-size:12px;" +_BUTTON_STYLE = ( + "display:inline-block;padding:10px 16px;background:#20a7c9;color:#ffffff;" + "text-decoration:none;border-radius:4px;" +) + +# Reason keys under which the export task groups charts it could not export. +# The task classifies each omitted chart under one of these; the email renders a +# separate, labelled section per non-empty group with its own remediation text. +ERROR_NO_QUERY_CONTEXT = "no-query-context" +ERROR_TIMEOUT = "timeout" +ERROR_GENERAL = "general-exception" + + +def _fmt(dt: datetime) -> str: + return dt.strftime(_DATETIME_FORMAT) + + +def _humanize_ttl(seconds: int) -> str: + """Render a TTL as a human-readable, pluralized, translatable duration. + + Whole hours read as "24 hours"; sub-hour and non-hour values keep their + minutes (e.g. "1 hour 30 minutes", "15 minutes") so the stated lifetime + always matches the real pre-signed URL expiration. + """ + hours, remainder = divmod(seconds, 3600) + parts: list[str] = [] + if hours: + parts.append(ngettext("%(num)d hour", "%(num)d hours", hours)) + if minutes := remainder // 60: + parts.append(ngettext("%(num)d minute", "%(num)d minutes", minutes)) + if not parts: + parts.append(ngettext("%(num)d second", "%(num)d seconds", seconds)) + return " ".join(parts) + + +def build_subject(dashboard_title: str, *, success: bool) -> str: + """Build the email subject, prefixed with EMAIL_REPORTS_SUBJECT_PREFIX.""" + prefix = current_app.config["EMAIL_REPORTS_SUBJECT_PREFIX"] + if success: + return prefix + __( + "Your dashboard export is ready: %(title)s", title=dashboard_title + ) + return prefix + __( + "Your dashboard export could not be completed: %(title)s", + title=dashboard_title, + ) + + +def _errored_section(errored: dict[str, list[str]]) -> str: + """Render one labelled, translated sub-list per non-empty error group. + + ``errored`` maps a reason key (see the ``ERROR_*`` constants) to the labels + of the charts that were omitted for that reason. Known reasons are rendered + first, in a stable order, each with its own remediation text; any unknown + reason key falls back to a generic message so nothing is silently dropped. + """ + if not errored: + return "" + notes = { Review Comment: **Suggestion:** Add an explicit type annotation to this dictionary variable to satisfy the type-hint requirement for relevant local variables. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> The rule requires type hints on relevant variables that can be annotated. This local dictionary is introduced without an explicit annotation, so the suggestion correctly identifies a real type-hint omission. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3017660124924ac4aef213fd16a6252b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3017660124924ac4aef213fd16a6252b&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/dashboards/excel_export/email.py **Line:** 95:95 **Comment:** *Custom Rule: Add an explicit type annotation to this dictionary variable to satisfy the type-hint requirement for relevant local variables. 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%2F41133&comment_hash=bcc15a63f4be68d1320b4f90f81ee341fb809c4f17e7f79e5ff523388e08371e&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=bcc15a63f4be68d1320b4f90f81ee341fb809c4f17e7f79e5ff523388e08371e&reaction=dislike'>๐</a> ########## superset/dashboards/excel_export/email.py: ########## @@ -0,0 +1,185 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Email rendering and delivery for dashboard Excel exports. + +Bodies use inline styles only (no external CSS, no logo) to match Superset's +existing report notification emails, and all user-controlled values (dashboard +title, chart names) are HTML-escaped to avoid injection. +""" + +from __future__ import annotations + +from datetime import datetime + +from flask import current_app +from flask_babel import gettext as __, ngettext +from markupsafe import escape + +from superset.utils.core import send_email_smtp + +_DATETIME_FORMAT = "%Y-%m-%d %H:%M:%S" +_FOOTER_STYLE = "color:#888;font-size:12px;" +_BUTTON_STYLE = ( + "display:inline-block;padding:10px 16px;background:#20a7c9;color:#ffffff;" + "text-decoration:none;border-radius:4px;" +) + +# Reason keys under which the export task groups charts it could not export. +# The task classifies each omitted chart under one of these; the email renders a +# separate, labelled section per non-empty group with its own remediation text. +ERROR_NO_QUERY_CONTEXT = "no-query-context" +ERROR_TIMEOUT = "timeout" +ERROR_GENERAL = "general-exception" + + +def _fmt(dt: datetime) -> str: + return dt.strftime(_DATETIME_FORMAT) + + +def _humanize_ttl(seconds: int) -> str: + """Render a TTL as a human-readable, pluralized, translatable duration. + + Whole hours read as "24 hours"; sub-hour and non-hour values keep their + minutes (e.g. "1 hour 30 minutes", "15 minutes") so the stated lifetime + always matches the real pre-signed URL expiration. + """ + hours, remainder = divmod(seconds, 3600) + parts: list[str] = [] + if hours: + parts.append(ngettext("%(num)d hour", "%(num)d hours", hours)) + if minutes := remainder // 60: + parts.append(ngettext("%(num)d minute", "%(num)d minutes", minutes)) + if not parts: + parts.append(ngettext("%(num)d second", "%(num)d seconds", seconds)) + return " ".join(parts) + + +def build_subject(dashboard_title: str, *, success: bool) -> str: + """Build the email subject, prefixed with EMAIL_REPORTS_SUBJECT_PREFIX.""" + prefix = current_app.config["EMAIL_REPORTS_SUBJECT_PREFIX"] + if success: + return prefix + __( + "Your dashboard export is ready: %(title)s", title=dashboard_title + ) + return prefix + __( + "Your dashboard export could not be completed: %(title)s", + title=dashboard_title, + ) + + +def _errored_section(errored: dict[str, list[str]]) -> str: + """Render one labelled, translated sub-list per non-empty error group. + + ``errored`` maps a reason key (see the ``ERROR_*`` constants) to the labels + of the charts that were omitted for that reason. Known reasons are rendered + first, in a stable order, each with its own remediation text; any unknown + reason key falls back to a generic message so nothing is silently dropped. + """ + if not errored: + return "" + notes = { + ERROR_NO_QUERY_CONTEXT: __( + "The following charts were omitted because they have no saved query " + "context. To include them, open each chart in Explore and re-save." + ), + ERROR_TIMEOUT: __( + "The following charts were omitted because they timed out before " + "their data could be exported:" + ), + ERROR_GENERAL: __( + "The following charts were omitted because an error occurred while " + "exporting them:" + ), + } + fallback = __("The following charts could not be exported:") + ordered = [ERROR_NO_QUERY_CONTEXT, ERROR_TIMEOUT, ERROR_GENERAL] Review Comment: **Suggestion:** Add an explicit list type annotation for this variable to comply with required type hints on relevant variables. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This is a newly added local list variable that can be explicitly typed but is not. That matches the type-hint requirement for relevant variables. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1b1ebe6df36e4444ba031d8bb97eb66f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1b1ebe6df36e4444ba031d8bb97eb66f&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/dashboards/excel_export/email.py **Line:** 110:110 **Comment:** *Custom Rule: Add an explicit list type annotation for this variable to comply with required type hints on relevant variables. 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%2F41133&comment_hash=6fbef3e69de8410edc73a410bfe37d04040bf936b9749409cf216df6e614c241&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=6fbef3e69de8410edc73a410bfe37d04040bf936b9749409cf216df6e614c241&reaction=dislike'>๐</a> ########## superset/dashboards/excel_export/email.py: ########## @@ -0,0 +1,185 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Email rendering and delivery for dashboard Excel exports. + +Bodies use inline styles only (no external CSS, no logo) to match Superset's +existing report notification emails, and all user-controlled values (dashboard +title, chart names) are HTML-escaped to avoid injection. +""" + +from __future__ import annotations + +from datetime import datetime + +from flask import current_app +from flask_babel import gettext as __, ngettext +from markupsafe import escape + +from superset.utils.core import send_email_smtp + +_DATETIME_FORMAT = "%Y-%m-%d %H:%M:%S" +_FOOTER_STYLE = "color:#888;font-size:12px;" +_BUTTON_STYLE = ( + "display:inline-block;padding:10px 16px;background:#20a7c9;color:#ffffff;" + "text-decoration:none;border-radius:4px;" +) + +# Reason keys under which the export task groups charts it could not export. +# The task classifies each omitted chart under one of these; the email renders a +# separate, labelled section per non-empty group with its own remediation text. +ERROR_NO_QUERY_CONTEXT = "no-query-context" +ERROR_TIMEOUT = "timeout" +ERROR_GENERAL = "general-exception" + + +def _fmt(dt: datetime) -> str: + return dt.strftime(_DATETIME_FORMAT) + + +def _humanize_ttl(seconds: int) -> str: + """Render a TTL as a human-readable, pluralized, translatable duration. + + Whole hours read as "24 hours"; sub-hour and non-hour values keep their + minutes (e.g. "1 hour 30 minutes", "15 minutes") so the stated lifetime + always matches the real pre-signed URL expiration. + """ + hours, remainder = divmod(seconds, 3600) + parts: list[str] = [] + if hours: + parts.append(ngettext("%(num)d hour", "%(num)d hours", hours)) + if minutes := remainder // 60: + parts.append(ngettext("%(num)d minute", "%(num)d minutes", minutes)) + if not parts: + parts.append(ngettext("%(num)d second", "%(num)d seconds", seconds)) + return " ".join(parts) + + +def build_subject(dashboard_title: str, *, success: bool) -> str: + """Build the email subject, prefixed with EMAIL_REPORTS_SUBJECT_PREFIX.""" + prefix = current_app.config["EMAIL_REPORTS_SUBJECT_PREFIX"] + if success: + return prefix + __( + "Your dashboard export is ready: %(title)s", title=dashboard_title + ) + return prefix + __( + "Your dashboard export could not be completed: %(title)s", + title=dashboard_title, + ) + + +def _errored_section(errored: dict[str, list[str]]) -> str: + """Render one labelled, translated sub-list per non-empty error group. + + ``errored`` maps a reason key (see the ``ERROR_*`` constants) to the labels + of the charts that were omitted for that reason. Known reasons are rendered + first, in a stable order, each with its own remediation text; any unknown + reason key falls back to a generic message so nothing is silently dropped. + """ + if not errored: + return "" + notes = { + ERROR_NO_QUERY_CONTEXT: __( + "The following charts were omitted because they have no saved query " + "context. To include them, open each chart in Explore and re-save." + ), + ERROR_TIMEOUT: __( + "The following charts were omitted because they timed out before " + "their data could be exported:" + ), + ERROR_GENERAL: __( + "The following charts were omitted because an error occurred while " + "exporting them:" + ), + } + fallback = __("The following charts could not be exported:") + ordered = [ERROR_NO_QUERY_CONTEXT, ERROR_TIMEOUT, ERROR_GENERAL] + reasons = ordered + [reason for reason in errored if reason not in ordered] + sections = [] Review Comment: **Suggestion:** Annotate this accumulator list with its element type so the variable is explicitly typed per the rule. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This accumulator list is created without a type annotation even though its element type is inferable from subsequent appends, so it is a valid omission under the type-hint rule. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8789e564e9394930ab2891d1e1e80020&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8789e564e9394930ab2891d1e1e80020&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/dashboards/excel_export/email.py **Line:** 112:112 **Comment:** *Custom Rule: Annotate this accumulator list with its element type so the variable is explicitly typed per the rule. 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%2F41133&comment_hash=96ec8b8551f9492f53a9d1ff323c3661fed0a61149faaf9264ec56e8f486b4df&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=96ec8b8551f9492f53a9d1ff323c3661fed0a61149faaf9264ec56e8f486b4df&reaction=dislike'>๐</a> ########## superset/config.py: ########## @@ -1435,6 +1435,27 @@ def sync_theme_logo_href( # note: index option should not be overridden EXCEL_EXPORT: dict[str, Any] = {} +# --------------------------------------------------- +# Dashboard "Export Data to Excel" (async, S3-backed) +# --------------------------------------------------- +# Destination S3 bucket for generated dashboard .xlsx exports. The feature is +# disabled until this is set: the export endpoint returns 501 when it is None. +EXCEL_EXPORT_S3_BUCKET: str | None = None +# Key prefix for export objects: {prefix}{dashboard_id}/{job_id}.xlsx +EXCEL_EXPORT_S3_KEY_PREFIX = "dashboard-exports/" Review Comment: **Suggestion:** Add an explicit type annotation to this newly introduced config constant to satisfy the type-hinting rule. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This is a newly added configuration constant without an explicit type annotation, and it is a plain string value that can and should be annotated under the Python type-hint rule. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=21a681dc72e34a4d992c5d4eed251de8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=21a681dc72e34a4d992c5d4eed251de8&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/config.py **Line:** 1445:1445 **Comment:** *Custom Rule: Add an explicit type annotation to this newly introduced config constant to satisfy the type-hinting rule. 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%2F41133&comment_hash=25e4e238aef6f7e5fa0a6e26642c48b4d511a1b3515e850fad2aa5d26f7cfee1&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=25e4e238aef6f7e5fa0a6e26642c48b4d511a1b3515e850fad2aa5d26f7cfee1&reaction=dislike'>๐</a> ########## superset/dashboards/api.py: ########## @@ -1574,6 +1585,109 @@ def export_as_example(self, pk: int) -> Response: response.set_cookie(token, "done", max_age=600) return response + @expose("/<pk>/export_xlsx/", methods=("POST",)) + @protect() + @safe + @permission_name("export") + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.export_xlsx", + log_to_statsd=False, + ) + def export_xlsx(self, pk: int) -> WerkzeugResponse: + """Export all of a dashboard's chart data to an Excel workbook (async). + --- + post: + summary: Export dashboard chart data to Excel + description: >- + Enqueues an async task that writes each chart's data to its own + worksheet, uploads the .xlsx to S3, and emails the requesting user a + pre-signed download link. Returns immediately with a job id. + parameters: + - in: path + schema: + type: integer + name: pk + description: The dashboard id + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/DashboardExportXlsxPostSchema' + responses: + 202: + description: Export task accepted + content: + application/json: + schema: + $ref: '#/components/schemas/DashboardExportXlsxResponseSchema' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + 501: + description: Excel export is not configured on this server + """ + if not current_app.config["EXCEL_EXPORT_S3_BUCKET"]: + return self.response( + 501, message="Excel export is not configured on this server." + ) + try: + # Tolerate an empty/non-JSON body (e.g. a POST with no Content-Type); + # request.json would otherwise raise 415. + payload = DashboardExportXlsxPostSchema().load( + request.get_json(silent=True) or {} + ) Review Comment: **Suggestion:** Add an explicit type annotation for the deserialized request payload to keep new code fully type-hinted. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This is newly added Python code that introduces a local variable without an explicit type annotation. Since the deserialized payload is a relevant variable that can be annotated, it matches the llmcfg-require-python-type-hints rule. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bf42e986ea4846f9a53312caeb10f451&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bf42e986ea4846f9a53312caeb10f451&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/dashboards/api.py **Line:** 1644:1646 **Comment:** *Custom Rule: Add an explicit type annotation for the deserialized request payload to keep new code fully type-hinted. 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%2F41133&comment_hash=96473fb03790aa85ba359686f3955f2eac07fafe9ba7b468c49580f148f8ac7f&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=96473fb03790aa85ba359686f3955f2eac07fafe9ba7b468c49580f148f8ac7f&reaction=dislike'>๐</a> ########## superset/config.py: ########## @@ -1435,6 +1435,27 @@ def sync_theme_logo_href( # note: index option should not be overridden EXCEL_EXPORT: dict[str, Any] = {} +# --------------------------------------------------- +# Dashboard "Export Data to Excel" (async, S3-backed) +# --------------------------------------------------- +# Destination S3 bucket for generated dashboard .xlsx exports. The feature is +# disabled until this is set: the export endpoint returns 501 when it is None. +EXCEL_EXPORT_S3_BUCKET: str | None = None +# Key prefix for export objects: {prefix}{dashboard_id}/{job_id}.xlsx +EXCEL_EXPORT_S3_KEY_PREFIX = "dashboard-exports/" +# Lifetime (seconds) of the pre-signed download URL emailed to the user (24h). +# Note: AWS S3 caps pre-signed URL lifetime at 7 days (604800 seconds); larger +# values are rejected by S3, so keep this at or below that when using AWS. +EXCEL_EXPORT_LINK_TTL_SECONDS = 86400 Review Comment: **Suggestion:** Add an explicit type annotation to this new numeric config constant so it is fully type-hinted. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This newly introduced numeric config constant lacks an explicit type annotation even though its type is clearly inferable and annotatable, so it violates the type-hinting rule. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0e4efc01b7b54e89b68d2ea7f32320ca&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0e4efc01b7b54e89b68d2ea7f32320ca&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/config.py **Line:** 1449:1449 **Comment:** *Custom Rule: Add an explicit type annotation to this new numeric config constant so it is fully type-hinted. 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%2F41133&comment_hash=e2f71653a82d8296c3011b97852219d56f9d7dfde042192cf0b63ac7d652c63c&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=e2f71653a82d8296c3011b97852219d56f9d7dfde042192cf0b63ac7d652c63c&reaction=dislike'>๐</a> ########## tests/integration_tests/dashboards/api_tests.py: ########## @@ -3262,6 +3264,100 @@ def test_get_all_related_viewers_with_extra_filters(self): response = json.loads(rv.data.decode("utf-8")) assert response["count"] > 0 + def test_export_xlsx_501_when_bucket_unset(self): Review Comment: **Suggestion:** Add explicit type hints to this test method signature, including a return annotation. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This new test method has no parameter or return type annotations, which violates the python type-hints rule for modified code. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c97cdbe5009f4edf9e60cbcdb7335b50&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c97cdbe5009f4edf9e60cbcdb7335b50&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:** tests/integration_tests/dashboards/api_tests.py **Line:** 3267:3267 **Comment:** *Custom Rule: Add explicit type hints to this test method signature, including a return annotation. 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%2F41133&comment_hash=e5aee7e767c3ecb3be4a91bd808d023f800724f003cb9aeba445bfe00a0d8ae1&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=e5aee7e767c3ecb3be4a91bd808d023f800724f003cb9aeba445bfe00a0d8ae1&reaction=dislike'>๐</a> ########## tests/unit_tests/utils/excel_streaming_tests.py: ########## @@ -0,0 +1,267 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from datetime import date, datetime +from decimal import Decimal +from pathlib import Path + +import pytest + +from superset.utils import excel_streaming +from superset.utils.excel_streaming import ( + _sanitize_cell, + sanitize_sheet_name, + StreamingXlsxWriter, +) + +# --- sanitize_sheet_name --- + + +def test_sheet_name_replaces_forbidden_chars() -> None: + assert sanitize_sheet_name("a/b:c*d?e[f]g\\h", set()) == "a_b_c_d_e_f_g_h" + + +def test_sheet_name_truncated_to_31() -> None: + assert sanitize_sheet_name("x" * 40, set()) == "x" * 31 + + +def test_sheet_name_dedupes_case_insensitively() -> None: + used: set[str] = set() + assert sanitize_sheet_name("Sales", used) == "Sales" + assert sanitize_sheet_name("sales", used) == "sales~2" + assert sanitize_sheet_name("SALES", used) == "SALES~3" + + +def test_sheet_name_dedupe_marker_respects_length_cap() -> None: + used: set[str] = set() + long_name = "y" * 31 + assert sanitize_sheet_name(long_name, used) == long_name + assert sanitize_sheet_name(long_name, used) == "y" * 29 + "~2" + + +def test_sheet_name_blank_falls_back() -> None: + assert sanitize_sheet_name(" ", set()) == "Sheet" + + +def test_sheet_name_reserved_history_is_escaped() -> None: + assert sanitize_sheet_name("History", set()) == "History_" + + +def test_sheet_name_strips_surrounding_apostrophes() -> None: + assert sanitize_sheet_name("'quoted'", set()) == "quoted" + + +# --- _sanitize_cell --- + + [email protected]( + "value,expected", + [ + (None, ""), + ("=SUM(A1)", "'=SUM(A1)"), + ("+1", "'+1"), + ("-1", "'-1"), + ("@handle", "'@handle"), + ("normal", "normal"), + (True, True), + (5, 5), + (1.5, 1.5), + (Decimal("2.5"), 2.5), + (datetime(2020, 1, 2, 3, 4, 5), "2020-01-02T03:04:05"), + (date(2020, 1, 2), "2020-01-02"), + ], +) +def test_sanitize_cell(value: object, expected: object) -> None: + assert _sanitize_cell(value) == expected + + +def test_sanitize_cell_large_int_becomes_string() -> None: + assert _sanitize_cell(10**16) == str(10**16) + + +def test_sanitize_cell_non_finite_floats_blanked() -> None: + assert _sanitize_cell(float("nan")) == "" + assert _sanitize_cell(float("inf")) == "" + + +def test_sanitize_cell_non_finite_decimals_blanked() -> None: + # float(Decimal("NaN")) is nan and float(Decimal("Infinity")) is inf, both + # of which xlsxwriter rejects; they must be blanked rather than crash. + assert _sanitize_cell(Decimal("NaN")) == "" + assert _sanitize_cell(Decimal("Infinity")) == "" + assert _sanitize_cell(Decimal("-Infinity")) == "" + + +def test_sanitize_cell_out_of_range_decimal_is_blanked() -> None: + # A Decimal too large for a float becomes inf (or, in edge cases, raises + # OverflowError); either way it must be neutralized rather than crash + # xlsxwriter or emit a bogus value. + assert _sanitize_cell(Decimal("1E10000")) == "" + + [email protected]( + "value,expected", + [ + (" =SUM(A1)", "' =SUM(A1)"), + ("\t=SUM(A1)", "'\t=SUM(A1)"), + (" +1", "' +1"), + ("\t@handle", "'\t@handle"), + ], +) +def test_sanitize_cell_quotes_formula_behind_whitespace( + value: str, expected: str +) -> None: + # Spreadsheet apps evaluate formulas even when preceded by spaces/tabs, so + # the formula guard must look past leading whitespace. + assert _sanitize_cell(value) == expected + + +# --- StreamingXlsxWriter (round-trip via openpyxl) --- + + +def _read_workbook(path: str) -> dict[str, list[list[object]]]: + openpyxl = pytest.importorskip("openpyxl") + workbook = openpyxl.load_workbook(path, read_only=True) + sheets = { + ws.title: [list(row) for row in ws.iter_rows(values_only=True)] + for ws in workbook.worksheets + } + workbook.close() + return sheets + + +def test_writer_writes_one_sheet_per_chart(tmp_path: Path) -> None: + path = str(tmp_path / "out.xlsx") + writer = StreamingXlsxWriter(path) + assert writer.add_sheet("10 - First", ["a", "b"], [[1, 2], [3, 4]]) == 2 + assert writer.add_sheet("20 - Second", ["c"], [["x"]]) == 1 + writer.close() + + sheets = _read_workbook(path) + assert list(sheets.keys()) == ["10 - First", "20 - Second"] + assert sheets["10 - First"] == [["a", "b"], [1, 2], [3, 4]] + assert sheets["20 - Second"] == [["c"], ["x"]] + + +def test_writer_quotes_formula_cells(tmp_path: Path) -> None: + path = str(tmp_path / "out.xlsx") + writer = StreamingXlsxWriter(path) + writer.add_sheet("data", ["col"], [["=cmd()"]]) + writer.close() + + sheets = _read_workbook(path) + assert sheets["data"][1][0] == "'=cmd()" + + +def test_writer_caps_rows_per_sheet( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(excel_streaming, "MAX_DATA_ROWS_PER_SHEET", 3) + path = str(tmp_path / "out.xlsx") + writer = StreamingXlsxWriter(path) + written = writer.add_sheet("data", ["col"], [[i] for i in range(5)]) + writer.close() + + # One row is reserved for the truncation notice, so only 2 data rows fit. + assert written == 2 + sheets = _read_workbook(path) + # header + 2 data rows + 1 truncation notice + assert len(sheets["data"]) == 4 + assert sheets["data"][-1][0] == "[Truncated: only first 2 rows exported]" + + +def test_writer_no_truncation_notice_when_data_fits( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(excel_streaming, "MAX_DATA_ROWS_PER_SHEET", 3) + path = str(tmp_path / "out.xlsx") + writer = StreamingXlsxWriter(path) + # Exactly fills the reserved capacity (MAX - 1) with no leftover rows. + written = writer.add_sheet("data", ["col"], [[i] for i in range(2)]) + writer.close() + + assert written == 2 + sheets = _read_workbook(path) + # header + 2 data rows, no notice + assert sheets["data"] == [["col"], [0], [1]] + + +def test_writer_empty_workbook_is_valid(tmp_path: Path) -> None: + path = str(tmp_path / "out.xlsx") + writer = StreamingXlsxWriter(path) + writer.close() + + sheets = _read_workbook(path) + assert list(sheets.keys()) == ["Export"] + + +def test_writer_summary_sheet(tmp_path: Path) -> None: + path = str(tmp_path / "out.xlsx") + writer = StreamingXlsxWriter(path) + writer.add_summary_sheet("Export Summary", ["Skipped charts:", "10 - Broken"]) + writer.close() + + sheets = _read_workbook(path) + assert sheets["Export Summary"] == [["Skipped charts:"], ["10 - Broken"]] + + +# --- add_image_sheet --- + +# A minimal valid 1x1 transparent PNG. +_PNG_1x1 = ( + b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01\x08\x06" + b"\x00\x00\x00\x1f\x15\xc4\x89\x00\x00\x00\rIDATx\x9cc\x00\x01\x00\x00\x05\x00" + b"\x01\r\n-\xb4\x00\x00\x00\x00IEND\xaeB`\x82" +) Review Comment: **Suggestion:** Add an explicit type annotation to the module-level PNG fixture constant so this new variable is fully typed. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> The new module-level constant `_PNG_1x1` is introduced without an explicit type annotation. This file adds new Python code, and the rule requires type hints for new or modified variables that can be annotated, so the suggestion matches a real violation. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d38b51d080ea4fd3804506c0e62196ff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d38b51d080ea4fd3804506c0e62196ff&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:** tests/unit_tests/utils/excel_streaming_tests.py **Line:** 226:230 **Comment:** *Custom Rule: Add an explicit type annotation to the module-level PNG fixture constant so this new variable is fully typed. 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%2F41133&comment_hash=d41b7bceba0edd05c7cdd8680afa7f9be22b6be243ec404071719b89e5c45b15&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=d41b7bceba0edd05c7cdd8680afa7f9be22b6be243ec404071719b89e5c45b15&reaction=dislike'>๐</a> ########## tests/integration_tests/dashboards/api_tests.py: ########## @@ -3262,6 +3264,100 @@ def test_get_all_related_viewers_with_extra_filters(self): response = json.loads(rv.data.decode("utf-8")) assert response["count"] > 0 + def test_export_xlsx_501_when_bucket_unset(self): + """Dashboard API: export_xlsx returns 501 when the S3 bucket is unset.""" + admin = self.get_user("admin") + dashboard = self.insert_dashboard("xlsx-501", None, [admin.id]) + self.login(ADMIN_USERNAME) + try: + rv = self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/") + assert rv.status_code == 501 + finally: + db.session.delete(dashboard) + db.session.commit() + + @with_config({"EXCEL_EXPORT_S3_BUCKET": "exports"}) + @patch("superset.dashboards.api.export_dashboard_excel") + def test_export_xlsx_404_for_missing_dashboard(self, mock_task): + """Dashboard API: export_xlsx returns 404 for an unknown dashboard.""" + self.login(ADMIN_USERNAME) + rv = self.client.post("api/v1/dashboard/99999999/export_xlsx/") + assert rv.status_code == 404 + mock_task.apply_async.assert_not_called() + + @with_config({"EXCEL_EXPORT_S3_BUCKET": "exports"}) + @patch("superset.dashboards.api.export_dashboard_excel") + def test_export_xlsx_400_for_empty_dashboard(self, mock_task): Review Comment: **Suggestion:** Add explicit type hints for the function parameters and return value on this method. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This newly added test function has an untyped mock argument and no return annotation, matching the type-hinting violation described. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=97c9bf85042d48c3bdadc4b41cb08e28&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=97c9bf85042d48c3bdadc4b41cb08e28&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:** tests/integration_tests/dashboards/api_tests.py **Line:** 3290:3290 **Comment:** *Custom Rule: Add explicit type hints for the function parameters and return value on this method. 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%2F41133&comment_hash=73feac3fda51211c7b30dae755665ec5ff31b8a64357f3effcf4213b9dae32ac&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=73feac3fda51211c7b30dae755665ec5ff31b8a64357f3effcf4213b9dae32ac&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: [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]
