codeant-ai-for-open-source[bot] commented on code in PR #35083:
URL: https://github.com/apache/superset/pull/35083#discussion_r3454575172
##########
tests/integration_tests/reports/api_tests.py:
##########
@@ -2885,3 +2885,93 @@ def
test_dashboard_update_deleted_filter_multiple_reports_notifies_all_owners(
db.session.delete(report_b)
db.session.delete(dashboard)
db.session.commit()
+
+ @patch("superset.tasks.scheduler.execute.apply_async")
+ def test_execute_report_schedule(self, mock_execute):
+ """
+ ReportSchedule Api: Test execute report schedule
+ """
+ report_schedule = (
+ db.session.query(ReportSchedule)
+ .filter(ReportSchedule.name == "name1")
+ .one_or_none()
+ )
+
+ self.login(ADMIN_USERNAME)
+ uri = f"api/v1/report/{report_schedule.id}/execute"
Review Comment:
**Suggestion:** This test depends on a report named `name1` but does not
create fixtures for it, unlike adjacent tests. If no prior test left data
behind, `report_schedule` is `None` and dereferencing `report_schedule.id`
raises an exception. Add the same fixture setup used by the other execute tests
to make this test deterministic. [null pointer]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ `test_execute_report_schedule` may fail with AttributeError.
- ⚠️ Test suite becomes order-dependent and flaky.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect `test_execute_report_schedule` in
`tests/integration_tests/reports/api_tests.py:2890-2903`, which queries
`report_schedule =
(db.session.query(ReportSchedule).filter(ReportSchedule.name ==
"name1").one_or_none())`
at lines 2894-2898 without any
`@pytest.mark.usefixtures("create_report_schedules")`
annotation.
2. Compare with other tests in the same class that use the same `"name1"`
report (e.g.,
`test_get_report_schedule_disabled` and `test_get_report_schedule` at
`api_tests.py:7-37`,
and `test_update_report_schedule_database_not_allowed_on_report` at
`api_tests.py:7-20`),
all of which are explicitly marked with
`@pytest.mark.usefixtures("create_report_schedules")` to populate those rows
via the
`create_report_schedules` fixture defined at `api_tests.py:168-207`.
3. Run `test_execute_report_schedule` in isolation or in an environment where
`create_report_schedules` has not yet been used: since no report named
`"name1"` exists,
`.one_or_none()` returns `None` and `report_schedule.id` at line 2901 raises
an
`AttributeError`.
4. Even when the full suite is run, this test's success depends on other
tests having
created (and not yet cleaned up) `"name1"` via `create_report_schedules`,
making the test
order-dependent; adding the same fixture usage here removes the potential
null dereference
and makes the test deterministic.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d243f9a8bb404d44997e41624433f7c3&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=d243f9a8bb404d44997e41624433f7c3&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/reports/api_tests.py
**Line:** 2894:2901
**Comment:**
*Null Pointer: This test depends on a report named `name1` but does not
create fixtures for it, unlike adjacent tests. If no prior test left data
behind, `report_schedule` is `None` and dereferencing `report_schedule.id`
raises an exception. Add the same fixture setup used by the other execute tests
to make this test deterministic.
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%2F35083&comment_hash=7f103802ba08cf2197499da2390ade485c23425a5168d4ae5e2f3dd5e11b93f7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35083&comment_hash=7f103802ba08cf2197499da2390ade485c23425a5168d4ae5e2f3dd5e11b93f7&reaction=dislike'>👎</a>
##########
tests/integration_tests/reports/api_tests.py:
##########
@@ -2885,3 +2885,93 @@ def
test_dashboard_update_deleted_filter_multiple_reports_notifies_all_owners(
db.session.delete(report_b)
db.session.delete(dashboard)
db.session.commit()
+
+ @patch("superset.tasks.scheduler.execute.apply_async")
+ def test_execute_report_schedule(self, mock_execute):
+ """
+ ReportSchedule Api: Test execute report schedule
+ """
+ report_schedule = (
+ db.session.query(ReportSchedule)
+ .filter(ReportSchedule.name == "name1")
+ .one_or_none()
+ )
+
+ self.login(ADMIN_USERNAME)
+ uri = f"api/v1/report/{report_schedule.id}/execute"
+ rv = self.client.post(uri)
+ assert rv.status_code == 200
+ data = json.loads(rv.data.decode("utf-8"))
+ assert "execution_id" in data
+ assert "message" in data
+ assert data["message"] == "Report schedule execution started
successfully"
+
+ # Verify the task was called
+ mock_execute.assert_called_once()
+ # Verify that the task was called with the correct report_schedule_id
and eta
+ call_args = mock_execute.call_args
+ assert call_args[0][0] == (report_schedule.id,)
+ # Check that eta was set for manual execution
+ assert "eta" in call_args[1]
+ assert call_args[1]["eta"] is not None
+
+ @pytest.mark.usefixtures("create_report_schedules")
+ def test_execute_report_schedule_not_found(self):
+ """
+ ReportSchedule Api: Test execute report schedule not found
+ """
+ self.login(ADMIN_USERNAME)
+ uri = "api/v1/report/9999999/execute"
+ rv = self.client.post(uri)
+ assert rv.status_code == 404
+
+ @pytest.mark.usefixtures("create_report_schedules")
+ def test_execute_report_schedule_not_owned(self):
+ """
+ ReportSchedule Api: Test execute report schedule not owned
+ """
+ report_schedule = (
+ db.session.query(ReportSchedule)
+ .filter(ReportSchedule.name == "name1")
+ .one_or_none()
+ )
+
+ self.login(GAMMA_USERNAME)
+ uri = f"api/v1/report/{report_schedule.id}/execute"
+ rv = self.client.post(uri)
+ assert rv.status_code == 403
+
+ def test_execute_report_schedule_disabled(self):
+ """
+ ReportSchedule Api: Test execute report schedule 404s when feature is
disabled
+ """
+ self.login(ADMIN_USERNAME)
+ with patch("superset.is_feature_enabled", return_value=False):
Review Comment:
**Suggestion:** This patch target does not affect the function used by
`ReportScheduleRestApi`, because that module imported `is_feature_enabled`
directly. The test will not actually disable the feature flag and can fail with
the wrong status code. Patch the symbol in the API module namespace instead.
[incorrect variable usage]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Disabled ALERT_REPORTS branch for execute endpoint untested.
- ⚠️ Test may pass due to missing report, not feature flag.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `TestReportSchedulesApi.test_execute_report_schedule_disabled` in
`tests/integration_tests/reports/api_tests.py:2944-2952`, which wraps the
request in `with
patch("superset.is_feature_enabled", return_value=False):`.
2. Note that `ReportScheduleRestApi` imports the feature flag via `from
superset import
is_feature_enabled` at `superset/reports/api.py:29`, and its
`ensure_alert_reports_enabled` hook uses this local name at
`superset/reports/api.py:11-15` to return a 404 when `ALERT_REPORTS` is
disabled.
3. Because `is_feature_enabled` was imported by value into
`superset/reports/api.py`,
patching `superset.is_feature_enabled` in the test does not modify the
already-bound local
`is_feature_enabled` reference used by `ensure_alert_reports_enabled`, so the
before-request hook still calls the original implementation.
4. When the test posts to `api/v1/report/1/execute` (line 2950), any 404
response is
produced by `ExecuteReportScheduleNowCommand` raising
`ReportScheduleNotFoundError` in
`superset/commands/report/execute_now.py:139-141`, not by the feature flag
hook, so the
test never actually verifies the disabled-feature behavior; patching
`superset.reports.api.is_feature_enabled` instead would exercise the
intended branch.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1b92e90ca0dd43d49d79ac1c2267755e&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=1b92e90ca0dd43d49d79ac1c2267755e&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/reports/api_tests.py
**Line:** 2949:2949
**Comment:**
*Incorrect Variable Usage: This patch target does not affect the
function used by `ReportScheduleRestApi`, because that module imported
`is_feature_enabled` directly. The test will not actually disable the feature
flag and can fail with the wrong status code. Patch the symbol in the API
module namespace instead.
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%2F35083&comment_hash=5599f61e707ab50fdcbd9928c553305df0706651f1da3305b2d03a11e06ab8e2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35083&comment_hash=5599f61e707ab50fdcbd9928c553305df0706651f1da3305b2d03a11e06ab8e2&reaction=dislike'>👎</a>
##########
superset/commands/report/execute_now.py:
##########
@@ -0,0 +1,147 @@
+# 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.
+import logging
+from typing import Any, Optional
+from uuid import uuid4
+
+from flask import current_app
+
+from superset import security_manager
+from superset.commands.base import BaseCommand
+from superset.commands.exceptions import CommandException
+from superset.commands.report.exceptions import (
+ ReportScheduleCeleryNotConfiguredError,
+ ReportScheduleExecuteNowFailedError,
+ ReportScheduleForbiddenError,
+ ReportScheduleNotFoundError,
+)
+from superset.daos.report import ReportScheduleDAO
+from superset.exceptions import SupersetSecurityException
+from superset.reports.models import ReportSchedule
+from superset.utils.decorators import transaction
+
+logger = logging.getLogger(__name__)
+
+
+class ExecuteReportScheduleNowCommand(BaseCommand):
+ """
+ Execute a report schedule immediately (manual trigger).
+
+ This command validates permissions and triggers immediate execution
+ of a report or alert via Celery task, similar to scheduled execution
+ but without waiting for the cron schedule.
+ """
+
+ def __init__(self, model_id: int) -> None:
+ self._model_id = model_id
+ self._model: Optional[ReportSchedule] = None
+
+ @transaction()
+ def run(self) -> str:
+ """
+ Execute the command and return execution UUID for tracking.
+
+ Returns:
+ str: Execution UUID that can be used to track the execution status
+
+ Raises:
+ ReportScheduleNotFoundError: Report schedule not found
+ ReportScheduleForbiddenError: User doesn't have permission to
execute
+ ReportScheduleExecuteNowFailedError: Execution failed to start
+ """
+ try:
+ self.validate()
+ if not self._model:
+ raise ReportScheduleExecuteNowFailedError()
+
+ # Generate execution UUID for tracking
+ execution_id = str(uuid4())
+
+ # Trigger immediate execution via Celery
+ logger.info(
+ "Manually executing report schedule %s (id: %d), execution_id:
%s",
+ self._model.name,
+ self._model.id,
+ execution_id,
+ )
+
+ # Import the existing execute task to avoid circular imports
+ from superset.tasks.scheduler import execute
+
+ # Set async options similar to scheduler but for immediate
execution
+ async_options: dict[str, Any] = {"task_id": execution_id}
+ if self._model.working_timeout is not None and
current_app.config.get(
+ "ALERT_REPORTS_WORKING_TIME_OUT_KILL", True
+ ):
+ async_options["time_limit"] = (
+ self._model.working_timeout
+ +
current_app.config.get("ALERT_REPORTS_WORKING_TIME_OUT_LAG", 10)
+ )
+ async_options["soft_time_limit"] = (
+ self._model.working_timeout
+ + current_app.config.get(
+ "ALERT_REPORTS_WORKING_SOFT_TIME_OUT_LAG", 5
+ )
+ )
+
+ # Execute the task
+ try:
+ execute.apply_async((self._model.id,), **async_options)
Review Comment:
**Suggestion:** The manual trigger call never sets `eta` in `async_options`,
but the downstream `reports.execute` task reads `execute.request.eta` and
passes it into `AsyncExecuteReportScheduleCommand`/execution logging as
`scheduled_dttm` (which is required). For immediate tasks `eta` is `None`, so
executions can fail when writing logs with a null scheduled timestamp. Set
`eta` explicitly (for example to current UTC time) when enqueuing manual runs.
[api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Manual /api/v1/report/<id>/execute may crash.
- ❌ Report execution logs get invalid scheduled timestamps.
- ⚠️ Trigger Now UI for alerts/reports becomes unreliable.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger the manual execute endpoint `POST /api/v1/report/<id>/execute`
implemented in
`superset/reports/api.py:28-37`, which calls
`ExecuteReportScheduleNowCommand(pk).run()`
at `superset/reports/api.py:71-72`.
2. Inside `ExecuteReportScheduleNowCommand.run()`
(`superset/commands/report/execute_now.py:53-126`), after validation it
builds
`async_options` with only `{"task_id": execution_id}` and optional timeouts
at lines
86-99, then calls `execute.apply_async((self._model.id,), **async_options)`
at line 103
without setting `eta`.
3. The Celery task `execute` in `superset/tasks/scheduler.py:117-135` reads
`scheduled_dttm = execute.request.eta` at line 125; for an immediate
`apply_async` with no
`eta`, Celery leaves `request.eta` as `None`, so `scheduled_dttm` becomes
`None` for
manual runs.
4. `AsyncExecuteReportScheduleCommand(task_id, report_schedule_id,
scheduled_dttm)` in
`superset/commands/report/execute.py:34-45` passes this `scheduled_dttm` into
`ReportScheduleStateMachine` (`execute.py:1118-1129`), whose
`BaseReportState.create_log()` writes
`ReportExecutionLog(scheduled_dttm=self._scheduled_dttm, ...)` at
`execute.py:105-117` to
the `report_execution_log.scheduled_dttm` column declared `nullable=False` in
`superset/reports/models.py:10-13`, causing a runtime failure when
`scheduled_dttm` is
`None` during manual executions.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dbaea30d522c4ae5b0f845a7755c9389&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=dbaea30d522c4ae5b0f845a7755c9389&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/commands/report/execute_now.py
**Line:** 86:103
**Comment:**
*Api Mismatch: The manual trigger call never sets `eta` in
`async_options`, but the downstream `reports.execute` task reads
`execute.request.eta` and passes it into
`AsyncExecuteReportScheduleCommand`/execution logging as `scheduled_dttm`
(which is required). For immediate tasks `eta` is `None`, so executions can
fail when writing logs with a null scheduled timestamp. Set `eta` explicitly
(for example to current UTC time) when enqueuing manual runs.
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%2F35083&comment_hash=3ecf148e694563611911f3ff014bbba0dd42f97c6e469df5765e5b9162ae766a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35083&comment_hash=3ecf148e694563611911f3ff014bbba0dd42f97c6e469df5765e5b9162ae766a&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]
