codeant-ai-for-open-source[bot] commented on code in PR #37538:
URL: https://github.com/apache/superset/pull/37538#discussion_r2739258151
##########
superset/commands/report/execute.py:
##########
@@ -796,7 +796,7 @@ def is_on_working_timeout(self) -> bool:
return (
self._report_schedule.working_timeout is not None
and self._report_schedule.last_eval_dttm is not None
- and datetime.utcnow()
+ and datetime.now(timezone.utc)
- timedelta(seconds=self._report_schedule.working_timeout)
> last_working.end_dttm
)
Review Comment:
**Suggestion:** The working-timeout check subtracts a timedelta from
`datetime.now(timezone.utc)` and compares to `last_working.end_dttm`; if
`last_working.end_dttm` is None or naive, the subtraction/comparison will raise
or be incorrect—validate and normalize `end_dttm` first. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Working-timeout check may raise TypeError.
- ⚠️ ReportWorkingState.next may fail unexpectedly.
- ⚠️ Most logs created here are timezone-aware; issue rare.
```
</details>
```suggestion
if (
self._report_schedule.working_timeout is None
or self._report_schedule.last_eval_dttm is None
or last_working is None
):
return False
now = datetime.now(timezone.utc)
end_dttm = last_working.end_dttm
if end_dttm is None:
return False
if end_dttm.tzinfo is None:
end_dttm = end_dttm.replace(tzinfo=timezone.utc)
return now -
timedelta(seconds=self._report_schedule.working_timeout) > end_dttm
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Have a ReportSchedule with working_timeout set and last_state==WORKING so
ReportWorkingState.next runs (ReportWorkingState.next calls
is_on_working_timeout at
superset/commands/report/execute.py:796).
2. Ensure
ReportScheduleDAO.find_last_entered_working_log(self._report_schedule) (called
in is_on_working_timeout) returns a last_working whose end_dttm is NULL or a
naive
datetime (no tzinfo). This can be simulated by inserting/updating a legacy
ReportExecutionLog row outside the code path.
3. Invoke AsyncExecuteReportScheduleCommand.run
(superset/commands/report/execute.py:1055)
so the state machine evaluates is_on_working_timeout().
4. Observe the expression datetime.now(timezone.utc) - timedelta(...) >
last_working.end_dttm raising TypeError when comparing aware and naive
datetimes, causing
ReportWorkingState.next to error. Note: create_log in this file
(superset/commands/report/execute.py:176) sets end_dttm with timezone, so
reproducing
needs legacy/altered rows.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/execute.py
**Line:** 796:802
**Comment:**
*Type Error: The working-timeout check subtracts a timedelta from
`datetime.now(timezone.utc)` and compares to `last_working.end_dttm`; if
`last_working.end_dttm` is None or naive, the subtraction/comparison will raise
or be incorrect—validate and normalize `end_dttm` first.
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.
```
</details>
##########
superset/utils/cache.py:
##########
@@ -73,7 +73,7 @@ def set_and_log_cache(
if timeout == CACHE_DISABLED_TIMEOUT:
return
try:
- dttm = datetime.utcnow().isoformat().split(".")[0]
+ dttm = datetime.now(timezone.utc).isoformat().split(".")[0]
Review Comment:
**Suggestion:** Type/format change bug: the code stores `dttm` as a string
produced by splitting the ISO string, which changes the data type compared to
previously storing a datetime and can break consumers that expect a datetime
(or a full ISO with timezone). Use a timezone-aware datetime object when adding
`dttm` to the cache value to preserve type and timezone information. [type
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Cached entries via set_and_log_cache hold string dttm.
- ⚠️ Consumers expecting datetime may misbehave.
- ⚠️ ETag/Last-Modified assignments can be inconsistent.
```
</details>
```suggestion
dttm = datetime.now(timezone.utc)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the repository and locate function `set_and_log_cache` in
`superset/utils/cache.py`. The ISO-string assignment is at
`superset/utils/cache.py:76`
and the dict merge at `superset/utils/cache.py:77`.
2. From a Python REPL or unit test, import the function and a cache instance:
- from `superset.utils.cache` import set_and_log_cache (function defined
around the
hunk starting at line 73)
- from `superset.utils.cache_manager` import cache_manager; use `cache =
cache_manager.cache`.
3. Call set_and_log_cache(cache, "test_key", {"foo": "bar"},
cache_timeout=60). Execution
hits the lines at `superset/utils/cache.py:76-77`, storing `dttm` as a
truncated ISO
string (e.g., "2024-01-02T15:04:05+00:00").
4. Read back the cached value via `cache.get("test_key")` (or via any code
path that
inspects cached dicts). Observe `dttm` is a string, not a datetime object.
If a consumer
expects a datetime (for .timestamp(), tz-aware math, or direct assignment to
response.last_modified), this type mismatch is visible and may break
downstream logic.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/cache.py
**Line:** 76:76
**Comment:**
*Type Error: Type/format change bug: the code stores `dttm` as a string
produced by splitting the ISO string, which changes the data type compared to
previously storing a datetime and can break consumers that expect a datetime
(or a full ISO with timezone). Use a timezone-aware datetime object when adding
`dttm` to the cache value to preserve type and timezone information.
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.
```
</details>
##########
superset/commands/report/log_prune.py:
##########
@@ -41,7 +41,7 @@ def run(self) -> None:
for report_schedule in db.session.query(ReportSchedule).all():
if report_schedule.log_retention is not None:
- from_date = datetime.utcnow() - timedelta(
+ from_date = datetime.now(timezone.utc) - timedelta(
Review Comment:
**Suggestion:** Mixing a timezone-aware `datetime` (created via
`datetime.now(timezone.utc)`) with database timestamps that are naive (no
tzinfo) can raise a TypeError when SQLAlchemy/Python compares them; use a naive
UTC datetime (`datetime.utcnow()`) to avoid aware-vs-naive comparison issues
unless the DB/columns are known to be timezone-aware. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Report schedule log pruning job may raise TypeError.
- ⚠️ ReportScheduleDAO bulk deletion may silently fail.
- ⚠️ Background maintenance task behavior becomes inconsistent.
```
</details>
```suggestion
from_date = datetime.utcnow() - timedelta(
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger the prune routine by invoking
AsyncPruneReportScheduleLogCommand.run() located
at superset/commands/report/log_prune.py:41 (method run). The method
iterates schedules at
superset/commands/report/log_prune.py:42 and enters the branch at line 43
when a schedule
has log_retention set.
2. At superset/commands/report/log_prune.py:44-46 the code computes
from_date =
datetime.now(timezone.utc) - timedelta(...). This produces a timezone-aware
datetime
object (tzinfo=UTC).
3. The code then calls ReportScheduleDAO.bulk_delete_logs(...) at
approximately
superset/commands/report/log_prune.py:48 (call site immediately after the
from_date
computation). If ReportScheduleDAO.bulk_delete_logs performs an in-Python
comparison (for
example, iterating log records and comparing model.timestamp < from_date) or
constructs
SQLAlchemy filters that require Python-level datetime comparison, Python
will raise
TypeError: "can't compare offset-naive and offset-aware datetimes".
4. Observe TypeError raised from the comparison inside
ReportScheduleDAO.bulk_delete_logs,
causing the prune run to either append the exception to prune_errors
(superset/commands/report/log_prune.py:57) or fail depending on where the
exception
occurs. If your DB/timestamps are timezone-aware and the DAO uses DB-side
filtering, no
error is raised — the failure is conditional on how timestamps are
represented and how
bulk_delete_logs implements the comparison.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/log_prune.py
**Line:** 44:44
**Comment:**
*Type Error: Mixing a timezone-aware `datetime` (created via
`datetime.now(timezone.utc)`) with database timestamps that are naive (no
tzinfo) can raise a TypeError when SQLAlchemy/Python compares them; use a naive
UTC datetime (`datetime.utcnow()`) to avoid aware-vs-naive comparison issues
unless the DB/columns are known to be timezone-aware.
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.
```
</details>
##########
superset/commands/report/execute.py:
##########
@@ -762,7 +762,7 @@ def is_in_grace_period(self) -> bool:
return (
last_success is not None
and self._report_schedule.grace_period
- and datetime.utcnow()
+ and datetime.now(timezone.utc)
- timedelta(seconds=self._report_schedule.grace_period)
< last_success.end_dttm
)
Review Comment:
**Suggestion:** Comparing an aware datetime produced by
`datetime.now(timezone.utc)` with `last_success.end_dttm` can raise a TypeError
if `last_success.end_dttm` is a naive datetime or be incorrect if it's None;
ensure `last_success.end_dttm` is present and timezone-aware before the
subtraction and comparison. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Report grace-period check may raise TypeError.
- ⚠️ Alert grace handling (ReportSuccessState.next) affected.
- ⚠️ Most logs are timezone-aware; issue is rare.
```
</details>
```suggestion
if last_success is None or not self._report_schedule.grace_period:
return False
now = datetime.now(timezone.utc)
end_dttm = last_success.end_dttm
if end_dttm is None:
return False
if end_dttm.tzinfo is None:
end_dttm = end_dttm.replace(tzinfo=timezone.utc)
return now - timedelta(seconds=self._report_schedule.grace_period) <
end_dttm
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Ensure a ReportSchedule exists with a non-zero grace_period (db row).
2. Have a legacy or manually-inserted ReportExecutionLog row returned by
ReportScheduleDAO.find_last_success_log(self._report_schedule) (called at
superset/commands/report/execute.py:762) whose end_dttm is NULL or a naive
datetime (no
tzinfo).
3. Trigger report execution via AsyncExecuteReportScheduleCommand.run
(superset/commands/report/execute.py:1055) so the state machine reaches
ReportSuccessState.next and calls is_in_grace_period()
(superset/commands/report/execute.py:762).
4. At is_in_grace_period(), the expression datetime.now(timezone.utc) -
timedelta(...) <
last_success.end_dttm executes and Python raises TypeError when comparing an
aware
datetime to a naive one (or the logic yields incorrect result if end_dttm is
None). Note:
create_log (superset/commands/report/execute.py:176) sets end_dttm to
datetime.now(timezone.utc) for logs created by this code, so reproducing
requires
pre-existing legacy/hand-inserted rows or external writers.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/execute.py
**Line:** 762:768
**Comment:**
*Type Error: Comparing an aware datetime produced by
`datetime.now(timezone.utc)` with `last_success.end_dttm` can raise a TypeError
if `last_success.end_dttm` is a naive datetime or be incorrect if it's None;
ensure `last_success.end_dttm` is present and timezone-aware before the
subtraction and comparison.
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.
```
</details>
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -1776,7 +1780,7 @@ def
test_report_schedule_success_grace(create_alert_slack_chart_success):
with freeze_time(current_time):
AsyncExecuteReportScheduleCommand(
- TEST_ID, create_alert_slack_chart_success.id, datetime.utcnow()
+ TEST_ID, create_alert_slack_chart_success.id,
datetime.now(timezone.utc)
Review Comment:
**Suggestion:** This call supplies a timezone-aware `scheduled_dttm`
(`datetime.now(timezone.utc)`) while the related fixture
`create_alert_slack_chart_success.last_eval_dttm` is a naive datetime;
comparisons or arithmetic between these will raise TypeError—use
`datetime.utcnow()` to keep datetimes naive and consistent. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ This integration test can raise TypeError.
- ⚠️ Alert scheduling test paths affected.
- ⚠️ CI test suite stability impacted.
- ⚠️ ReportSchedule comparison logic implicated.
```
</details>
```suggestion
TEST_ID, create_alert_slack_chart_success.id, datetime.utcnow()
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open tests/integration_tests/reports/commands_tests.py and find
test_report_schedule_success_grace where the block starting at line 1781
calls
AsyncExecuteReportScheduleCommand(..., datetime.now(timezone.utc)).run()
(tests/integration_tests/reports/commands_tests.py:1781-1783).
2. The fixture create_alert_slack_chart_success used in this test sets
last_eval_dttm to a
naive datetime (report_schedule.last_eval_dttm = datetime(2020, 1, 1, 0, 0))
in the same
test module (fixture create_alert_slack_chart_success definition earlier in
tests/integration_tests/reports/commands_tests.py).
3. Running this test executes AsyncExecuteReportScheduleCommand.run
(superset/commands/report/execute.py:1032-1087), which forwards the
scheduled_dttm to the
state machine that performs datetime arithmetic/comparisons with the
fixture's
last_eval_dttm.
4. With a timezone-aware scheduled_dttm (datetime.now(timezone.utc))
compared against the
fixture's naive last_eval_dttm, Python raises a TypeError. Replacing the
scheduled
argument with datetime.utcnow() avoids mixing aware/naive datetimes and
reproduces a
passing run.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/reports/commands_tests.py
**Line:** 1783:1783
**Comment:**
*Type Error: This call supplies a timezone-aware `scheduled_dttm`
(`datetime.now(timezone.utc)`) while the related fixture
`create_alert_slack_chart_success.last_eval_dttm` is a naive datetime;
comparisons or arithmetic between these will raise TypeError—use
`datetime.utcnow()` to keep datetimes naive and consistent.
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.
```
</details>
##########
superset/daos/log.py:
##########
@@ -142,7 +142,7 @@ def get_recent_activity(
"item_title": item_title,
"time": datetime_to_epoch(log.dttm),
"time_delta_humanized": humanize.naturaltime(
- datetime.utcnow() - log.dttm
+ datetime.now(timezone.utc) - log.dttm
Review Comment:
**Suggestion:** Mixing timezone-aware `datetime.now(timezone.utc)` with a
possibly naive `log.dttm` will raise a TypeError ("can't subtract offset-naive
and offset-aware datetimes"). Ensure both sides use the same timezone-awareness
before subtracting: detect if `log.dttm` is naive and make it timezone-aware
(or convert both to UTC-aware) before performing the subtraction. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Recent activity listing can raise TypeError and fail.
- ⚠️ API endpoints calling get_recent_activity may return 500s.
- ⚠️ Admin/UI "Recent Activity" timeline displays broken responses.
```
</details>
```suggestion
(datetime.now(timezone.utc) - (log.dttm if
log.dttm.tzinfo is not None else log.dttm.replace(tzinfo=timezone.utc)))
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the application (or a Python shell) with the PR code so
`superset/daos/log.py` is
loaded.
2. Create or ensure a Log row whose `dttm` is stored as an offset-naive
datetime (e.g.,
construct `superset.models.core.Log(dttm=datetime(2020,1,1))` and persist it
via
`db.session.add(...); db.session.commit()`).
3. Call LogDAO.get_recent_activity(...) in `superset/daos/log.py` (function
context where
humanize is invoked; see lines around 142-146) — the query returns that Log
instance and
the code reaches the loop that evaluates
`humanize.naturaltime(datetime.now(timezone.utc)
- log.dttm)` at line 145.
4. At runtime Python raises TypeError: "can't subtract offset-naive and
offset-aware
datetimes" when `datetime.now(timezone.utc)` (aware) is subtracted by a
naive `log.dttm`,
causing the request/operation that called get_recent_activity to fail.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/daos/log.py
**Line:** 145:145
**Comment:**
*Type Error: Mixing timezone-aware `datetime.now(timezone.utc)` with a
possibly naive `log.dttm` will raise a TypeError ("can't subtract offset-naive
and offset-aware datetimes"). Ensure both sides use the same timezone-awareness
before subtracting: detect if `log.dttm` is naive and make it timezone-aware
(or convert both to UTC-aware) before performing the subtraction.
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.
```
</details>
##########
tests/integration_tests/superset_test_custom_template_processors.py:
##########
@@ -40,7 +40,7 @@ class CustomPrestoTemplateProcessor(PrestoTemplateProcessor):
def process_template(self, sql: str, **kwargs) -> str:
"""Processes a sql template with $ style macro using regex."""
# Add custom macros functions.
- macros = {"DATE": partial(DATE, datetime.utcnow())} # type: Dict[str,
Any]
+ macros = {"DATE": partial(DATE, datetime.now(timezone.utc))} # type:
Dict[str, Any]
Review Comment:
**Suggestion:** Passing raw string args from the regex directly into `DATE`
can raise a ValueError when an argument is empty (e.g. `$DATE(,3)`) or not an
integer-like string; also the current binding fixes the timestamp at
`process_template` call time which may be surprising. Wrap the macro in a small
adapter that (1) converts empty strings to 0 and casts numeric strings to int,
and (2) computes the current UTC timestamp at call time to avoid stale binding.
[type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Template macro expansion raises uncaught ValueError.
- ⚠️ Integration tests using this processor fail.
- ⚠️ Any test utilities using DATE macro fail.
```
</details>
```suggestion
def _safe_DATE(*args):
# Convert string args to ints, treat empty or missing args as 0,
# and compute the current UTC timestamp at call time.
converted = []
for a in args:
if isinstance(a, str):
a = a.strip()
if a == "" or a is None:
converted.append(0)
else:
converted.append(int(a))
return DATE(datetime.now(timezone.utc), *converted)
macros = {"DATE": _safe_DATE} # type: Dict[str, Any]
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open tests/integration_tests/superset_test_custom_template_processors.py
and locate
CustomPrestoTemplateProcessor.process_template (defined starting at the hunk
around line
40). Observe the macros binding at line 43 where DATE is pre-bound: `macros
= {"DATE":
partial(DATE, datetime.now(timezone.utc))}`.
2. Call CustomPrestoTemplateProcessor().process_template(sql, ...) with a
template
containing an empty first DATE argument, e.g. sql = "SELECT $DATE(,3)". The
replacer
function (defined immediately after macros.update(kwargs) in the same file)
executes for
that match.
3. The replacer (in the same file, directly below the macros block) splits
the args_str
into ['','3'] and does not normalize the empty string (note the code path:
args =
[a.strip() for a in args_str.split(",")]; the special-case only handles
[""]).
4. replacer then calls f = macros["DATE"] and executes f(*args) which invokes
DATE(datetime_obj, '', '3'). Inside DATE (the top-level DATE function in the
same file),
day_offset = int(day_offset) is executed, which raises ValueError for the
empty string and
causes template processing to crash.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/superset_test_custom_template_processors.py
**Line:** 43:43
**Comment:**
*Type Error: Passing raw string args from the regex directly into
`DATE` can raise a ValueError when an argument is empty (e.g. `$DATE(,3)`) or
not an integer-like string; also the current binding fixes the timestamp at
`process_template` call time which may be surprising. Wrap the macro in a small
adapter that (1) converts empty strings to 0 and casts numeric strings to int,
and (2) computes the current UTC timestamp at call time to avoid stale binding.
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.
```
</details>
##########
superset/utils/cache.py:
##########
@@ -226,7 +226,7 @@ def wrapper(*args: Any, **kwargs: Any) -> Response: #
noqa: C901
# Check if the cache is stale. Default the content_changed_time to
now
# if we don't know when it was last modified.
- content_changed_time = datetime.utcnow()
+ content_changed_time = datetime.now(timezone.utc)
if get_last_modified:
content_changed_time = get_last_modified(*args, **kwargs)
if (
Review Comment:
**Suggestion:** Timezone normalization bug: `get_last_modified` may return a
naive datetime (no tzinfo) or an aware datetime in another timezone; the code
does not normalize it to UTC before comparisons, which can lead to incorrect
stale checks. Normalize any `get_last_modified` result to an aware UTC datetime
(convert or attach UTC tzinfo) before using it. [time/date bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Conditional GETs may incorrectly bypass caches.
- ⚠️ GET endpoints using etag_cache validate wrongly.
- ⚠️ Caching behavior inconsistent across timezones.
```
</details>
```suggestion
if content_changed_time is None:
content_changed_time = datetime.now(timezone.utc)
elif content_changed_time.tzinfo is None:
content_changed_time =
content_changed_time.replace(tzinfo=timezone.utc)
else:
content_changed_time =
content_changed_time.astimezone(timezone.utc)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Locate the etag decorator wrapper in `superset/utils/cache.py`. The
default
`content_changed_time` is set at `superset/utils/cache.py:229` and
`get_last_modified` is
invoked at `superset/utils/cache.py:231`.
2. Create a view decorated with `@etag_cache(get_last_modified=...)` where
the provided
`get_last_modified` returns a naive datetime (e.g., `return
datetime.utcnow()` or `return
datetime.now()` without tzinfo). This is a realistic caller pattern for
legacy code that
uses naive datetimes.
3. Issue a GET request to that view (an endpoint using the decorator). The
wrapper
executes and assigns `content_changed_time` from the naive
`get_last_modified` result in
`superset/utils/cache.py:231`.
4. The code compares `response.last_modified.timestamp()` with
`content_changed_time.timestamp()` (later in the same wrapper). Because the
timezone of
`content_changed_time` is not normalized, comparisons are based on epoch
timestamps which
may hide timezone/naive mismatches and lead to incorrect stale detection for
consumers
relying on consistent UTC normalization.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/cache.py
**Line:** 232:232
**Comment:**
*Time Date Bug: Timezone normalization bug: `get_last_modified` may
return a naive datetime (no tzinfo) or an aware datetime in another timezone;
the code does not normalize it to UTC before comparisons, which can lead to
incorrect stale checks. Normalize any `get_last_modified` result to an aware
UTC datetime (convert or attach UTC tzinfo) before using it.
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.
```
</details>
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -1729,7 +1733,7 @@ def
test_report_schedule_working(create_report_slack_chart_working):
AsyncExecuteReportScheduleCommand(
TEST_ID,
create_report_slack_chart_working.id,
- datetime.utcnow(),
+ datetime.now(timezone.utc),
Review Comment:
**Suggestion:** Mixing timezone-aware datetimes
(`datetime.now(timezone.utc)`) with existing naive datetimes (e.g. fixtures set
`last_eval_dttm = datetime(2020, 1, 1, 0, 0)`) will raise TypeError when the
code compares or subtracts them; use a naive UTC datetime (datetime.utcnow())
here so scheduled timestamps are consistent with the fixtures and avoid
comparing aware vs naive datetimes. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Multiple integration tests in this file fail.
- ⚠️ Report execution test paths break comparisons.
- ⚠️ AsyncExecuteReportScheduleCommand workflows affected.
- ⚠️ ReportScheduleStateMachine datetime logic impacted.
```
</details>
```suggestion
datetime.utcnow(),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open tests/integration_tests/reports/commands_tests.py and locate the
test function
test_report_schedule_working (the AsyncExecuteReportScheduleCommand call is
at the hunk
starting at line 1733 in the PR). The test invokes
AsyncExecuteReportScheduleCommand(...,
datetime.now(timezone.utc)).run() (file:
tests/integration_tests/reports/commands_tests.py:1733-1736).
2. The test uses the fixture create_report_slack_chart_working which sets
report_schedule.last_eval_dttm to a naive datetime literal
(report_schedule.last_eval_dttm
= datetime(2020, 1, 1, 0, 0)) inside the same test module (fixture
definition present
earlier in tests/integration_tests/reports/commands_tests.py).
3. Running the test leads to AsyncExecuteReportScheduleCommand.run() being
executed (see
superset/commands/report/execute.py:1032-1087). That method passes the
scheduled_dttm
value to ReportScheduleStateMachine(...).run(), which performs datetime
arithmetic/comparisons involving model.last_eval_dttm.
4. When scheduled_dttm is timezone-aware (datetime.now(timezone.utc)) and
model.last_eval_dttm is naive (datetime(2020, 1, 1, 0, 0)), Python raises a
TypeError on
comparison/arithmetic. If this behavior is intentional across the codebase,
update
fixtures instead; otherwise, changing the scheduled timestamp to naive
(datetime.utcnow())
avoids the TypeError.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/reports/commands_tests.py
**Line:** 1736:1736
**Comment:**
*Type Error: Mixing timezone-aware datetimes
(`datetime.now(timezone.utc)`) with existing naive datetimes (e.g. fixtures set
`last_eval_dttm = datetime(2020, 1, 1, 0, 0)`) will raise TypeError when the
code compares or subtracts them; use a naive UTC datetime (datetime.utcnow())
here so scheduled timestamps are consistent with the fixtures and avoid
comparing aware vs naive datetimes.
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.
```
</details>
##########
tests/unit_tests/dao/queries_test.py:
##########
@@ -69,7 +69,7 @@ def test_query_dao_get_queries_changed_after(session:
Session) -> None:
database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
- now = datetime.utcnow()
+ now = datetime.now(timezone.utc)
Review Comment:
**Suggestion:** Mixing a timezone-aware datetime
(`datetime.now(timezone.utc)`) with application/model code that uses naive UTC
datetimes (the models use `datetime.utcnow` by default) can cause TypeError or
incorrect comparisons when the test stores `changed_on` and when DAO code
compares timestamps; use a naive UTC datetime to match the model's
expectations. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unit test may raise TypeError on datetime comparisons.
- ⚠️ Query DAO timestamp-compare logic may behave inconsistently.
- ⚠️ Tests covering time-based query filters may flake.
```
</details>
```suggestion
now = datetime.utcnow()
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the test function `test_query_dao_get_queries_changed_after` in
tests/unit_tests/dao/queries_test.py (hunk header shows the function context
at line ~69).
The PR added the line at tests/unit_tests/dao/queries_test.py:72: `now =
datetime.now(timezone.utc)`.
2. The test creates Query objects with `changed_on=now - timedelta(days=3)`
and
`changed_on=now - timedelta(days=1)` (the first Query construction starts at
tests/unit_tests/dao/queries_test.py:74 where `old_query_obj = Query(` is
created). These
assignments pass a timezone-aware datetime into the model field.
3. Inspect the Query model declaration in superset/models/sql_lab.py: the
column is
declared as
`changed_on = Column(DateTime, default=datetime.utcnow,
onupdate=datetime.utcnow,
nullable=True)` (see Query class in superset/models/sql_lab.py in the
provided
downstream context). The model defaults use naive UTC datetimes
(`datetime.utcnow`).
4. Run the unit test (pytest -k test_query_dao_get_queries_changed_after).
Because the
test is explicitly passing timezone-aware datetimes while the model defaults
and many
codepaths expect naive UTC datetimes, comparisons or arithmetic between
naive and aware
datetimes (or DB-layer conversions) can raise TypeError or behave
inconsistently. Changing
the test to use `datetime.utcnow()` (naive UTC) makes the test align with
the model
definition and avoids mixing tz-aware/naive datetimes.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/dao/queries_test.py
**Line:** 72:72
**Comment:**
*Type Error: Mixing a timezone-aware datetime
(`datetime.now(timezone.utc)`) with application/model code that uses naive UTC
datetimes (the models use `datetime.utcnow` by default) can cause TypeError or
incorrect comparisons when the test stores `changed_on` and when DAO code
compares timestamps; use a naive UTC datetime to match the model's expectations.
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.
```
</details>
##########
docs/docs/configuration/sql-templating.mdx:
##########
@@ -189,7 +189,7 @@ class
CustomPrestoTemplateProcessor(PrestoTemplateProcessor):
"""Processes a sql template with $ style macro using regex."""
# Add custom macros functions.
macros = {
- "DATE": partial(DATE, datetime.utcnow())
+ "DATE": partial(DATE, datetime.now(timezone.utc))
Review Comment:
**Suggestion:** NameError risk: the snippet references `timezone` but
there's no import shown in the surrounding example; if the example uses `from
datetime import datetime` (common in examples) then `timezone` will be
undefined at runtime and raise a NameError. Replace with a construct that does
not require `timezone` to be imported. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Import error if snippet copied into superset_config.py.
- ⚠️ Confusing documentation causing developer friction.
- ⚠️ Example missing required datetime import statement.
```
</details>
```suggestion
"DATE": partial(DATE, datetime.utcnow())
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the documentation example at
docs/docs/configuration/sql-templating.mdx, locate
the code block defining CustomPrestoTemplateProcessor (the macros assignment
is at line
192 in the final file state).
2. Copy the shown macros snippet exactly (including the line at 192) into a
real Python
file used by Superset (for example, paste into superset_config.py at project
root).
3. Restart Superset (or import the module) so Python executes the top-level
code in
superset_config.py; Python will evaluate datetime.now(timezone.utc) during
import.
4. Observe a NameError: "name 'timezone' is not defined" raised at the
inserted line (the
exact location corresponds to the copied line, originating from the docs
snippet). Note:
the docs file itself is not imported by Superset; this failure only occurs
if a developer
copies the snippet into an actual runtime module without adding the
necessary import for
timezone.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/docs/configuration/sql-templating.mdx
**Line:** 192:192
**Comment:**
*Possible Bug: NameError risk: the snippet references `timezone` but
there's no import shown in the surrounding example; if the example uses `from
datetime import datetime` (common in examples) then `timezone` will be
undefined at runtime and raise a NameError. Replace with a construct that does
not require `timezone` to be imported.
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.
```
</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: [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]
