eschutho opened a new pull request, #42650:
URL: https://github.com/apache/superset/pull/42650
### SUMMARY
`superset/reports/schemas.py::validate_crontab()` only checks
`croniter.is_valid(str(value))`, which is purely syntactic — it returns `True`
for cron expressions that can never produce a real calendar date (e.g. `0 0 30
2 *` for February 30th, or `0 0 31 4 *` for April 31st).
When `ALERT_MINIMUM_INTERVAL`/`REPORT_MINIMUM_INTERVAL` is configured >= 120
seconds (a common operator setting to enforce a minimum report/alert interval),
`BaseReportScheduleCommand.validate_report_frequency()` calls
`croniter(cron_schedule)` and iterates it with `next(schedule)`. For a
never-matching crontab this raises `croniter.croniter.CroniterBadDateError` —
whose MRO is `(CroniterBadDateError, CroniterError, ValueError, Exception,
BaseException, object)`, i.e. **not** a marshmallow `ValidationError`. Both
`CreateReportScheduleCommand.run()` and `UpdateReportScheduleCommand.run()`
wrap the call in `except ValidationError`, so the raw croniter exception
propagates past that handler, uncaught, up to the API layer, producing an
opaque 500 instead of a 422.
This is the same underlying croniter behavior fixed once already in #42486
(`superset/tasks/cron_util.py::cron_schedule_window()`, the celery-beat
scheduler path — catch + log + skip), but this is a different site: synchronous
API-layer validation during report/alert create/update, where the correct fix
is to surface a proper validation error rather than silently skip.
Related prior fixes in this bug-class pipeline (raw system/library
exceptions propagating instead of proper Superset/marshmallow validation
errors): #42366, #42401, #42426, #42442, #42486.
### FIX
- Added `ReportScheduleCrontabNotValidError`
(`superset/commands/report/exceptions.py`), a marshmallow `ValidationError`
subclass on the `crontab` field, following the existing
`ReportScheduleFrequencyNotAllowed` pattern.
- Wrapped the `croniter` iteration in `validate_report_frequency()`
(`superset/commands/report/base.py`) in `try/except CroniterBadDateError`,
raising `ReportScheduleCrontabNotValidError` from the caught exception.
- Added a regression test in `tests/unit_tests/commands/report/base_test.py`
covering both `ReportScheduleType.ALERT` and `ReportScheduleType.REPORT` with
the `0 0 30 2 *` crontab.
### TESTING INSTRUCTIONS
- Confirmed empirically (before applying the fix) that `croniter.is_valid("0
0 30 2 *")` returns `True` in this environment's pinned croniter version, and
that calling `validate_report_frequency("0 0 30 2 *",
ReportScheduleType.ALERT)` with a minimum interval configured (e.g. 5 minutes)
raised an uncaught `CroniterBadDateError`, not a `ValidationError`.
- Applied the fix and confirmed the same call now raises
`ReportScheduleCrontabNotValidError` (a `ValidationError`) instead.
- Ran the full `tests/unit_tests/commands/report/base_test.py` file: 95
passed (93 pre-existing + 2 new, one per report type), no regressions.
- Ran `ruff check` and `ruff format --check` on all changed files: all
checks passed / already formatted.
- Ran `mypy` on the changed files: no new errors introduced (one
pre-existing, unrelated `call-arg` error on `not_found_exc()` at line 73 of
`base.py` was confirmed present on unmodified `HEAD` as well).
To manually verify: configure `ALERT_MINIMUM_INTERVAL = 300` (or any value
>= 120), then attempt to create an alert with crontab `0 0 30 2 *` via the API
— before this fix, the request returns a 500; after, it returns a 422 with a
clear "Invalid crontab schedule" message.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
**Tradeoffs:** This is a new, additive validation error on a
previously-uncaught crash path — no existing behavior changes for any crontab
that currently passes. This code path only triggers for genuinely never-firing
crontabs that admins would have needed to fix anyway; before this fix they got
an opaque 500, after this fix they get a clear 422 telling them the crontab is
invalid.
--
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]
