fix(alerts-reports): catch CroniterBadDateError in report frequency validation - #42650
Conversation
…alidation Fixes an uncaught CroniterBadDateError in report/alert frequency validation. marshmallow's validate_crontab only checks croniter.is_valid(), which is purely syntactic and accepts crontabs that never produce a real calendar date (e.g. "0 0 30 2 *" for February 30th). When ALERT_MINIMUM_INTERVAL or REPORT_MINIMUM_INTERVAL is configured >= 120s, validate_report_frequency iterates the schedule with croniter, which raises CroniterBadDateError instead of a ValidationError, propagating past the existing except ValidationError handling in CreateReportScheduleCommand and UpdateReportScheduleCommand and surfacing as an opaque 500.
dosubot
Bot
added
the
alert-reports
Code Review Agent Run #285e51Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| try: | ||
| schedule = croniter(cron_schedule) | ||
| current_exec = next(schedule) | ||
|
|
||
| for _i in range(iterations): | ||
| next_exec = next(schedule) | ||
| diff, current_exec = next_exec - current_exec, next_exec | ||
| if int(diff) < minimum_interval: | ||
| raise ReportScheduleFrequencyNotAllowed( | ||
| report_type=report_type, minimum_interval=minimum_interval | ||
| ) | ||
| except CroniterBadDateError as ex: | ||
| raise ReportScheduleCrontabNotValidError( | ||
| cron_schedule=cron_schedule | ||
| ) from ex |
There was a problem hiding this comment.
Suggestion: The never-matching-date check is only reached after the minimum_interval < 120 early return. With the default minimum interval of 0 (or any value below 120 seconds), schedules such as 0 0 30 2 * still pass schema validation and are accepted even though the scheduler can never produce an execution for them. Validate calendar validity independently of the frequency-limit threshold, or move this check before the early return. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Default zero-minute configuration bypasses calendar validation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/report/base.py
**Line:** 292:306
**Comment:**
*Incomplete Implementation: The never-matching-date check is only reached after the `minimum_interval < 120` early return. With the default minimum interval of `0` (or any value below 120 seconds), schedules such as `0 0 30 2 *` still pass schema validation and are accepted even though the scheduler can never produce an execution for them. Validate calendar validity independently of the frequency-limit threshold, or move this check before the early return.
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
|
The flagged issue is valid. The current implementation performs the frequency check before the calendar validation, meaning if the frequency check returns early (e.g., due to a low Here is a concise fix to ensure calendar validation occurs regardless of the frequency check: # superset/commands/report/base.py
iterations = 60 if minimum_interval <= 3660 else 24
try:
schedule = croniter(cron_schedule)
current_exec = next(schedule)
except CroniterBadDateError as ex:
raise ReportScheduleCrontabNotValidError(
cron_schedule=cron_schedule
) from ex
for _i in range(iterations):
next_exec = next(schedule)
diff, current_exec = next_exec - current_exec, next_exec
if int(diff) < minimum_interval:
raise ReportScheduleFrequencyNotAllowed(
report_type=report_type, minimum_interval=minimum_interval
)Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset/commands/report/base.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42650 +/- ##
==========================================
+ Coverage 55.88% 65.44% +9.56%
==========================================
Files 2810 2810
Lines 159362 159368 +6
Branches 36372 36372
==========================================
+ Hits 89055 104299 +15244
+ Misses 69475 53027 -16448
- Partials 832 2042 +1210
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
SUMMARY
superset/reports/schemas.py::validate_crontab()only checkscroniter.is_valid(str(value)), which is purely syntactic — it returnsTruefor cron expressions that can never produce a real calendar date (e.g.0 0 30 2 *for February 30th, or0 0 31 4 *for April 31st).When
ALERT_MINIMUM_INTERVAL/REPORT_MINIMUM_INTERVALis configured >= 120 seconds (a common operator setting to enforce a minimum report/alert interval),BaseReportScheduleCommand.validate_report_frequency()callscroniter(cron_schedule)and iterates it withnext(schedule). For a never-matching crontab this raisescroniter.croniter.CroniterBadDateError— whose MRO is(CroniterBadDateError, CroniterError, ValueError, Exception, BaseException, object), i.e. not a marshmallowValidationError. BothCreateReportScheduleCommand.run()andUpdateReportScheduleCommand.run()wrap the call inexcept 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
ReportScheduleCrontabNotValidError(superset/commands/report/exceptions.py), a marshmallowValidationErrorsubclass on thecrontabfield, following the existingReportScheduleFrequencyNotAllowedpattern.croniteriteration invalidate_report_frequency()(superset/commands/report/base.py) intry/except CroniterBadDateError, raisingReportScheduleCrontabNotValidErrorfrom the caught exception.tests/unit_tests/commands/report/base_test.pycovering bothReportScheduleType.ALERTandReportScheduleType.REPORTwith the0 0 30 2 *crontab.TESTING INSTRUCTIONS
croniter.is_valid("0 0 30 2 *")returnsTruein this environment's pinned croniter version, and that callingvalidate_report_frequency("0 0 30 2 *", ReportScheduleType.ALERT)with a minimum interval configured (e.g. 5 minutes) raised an uncaughtCroniterBadDateError, not aValidationError.ReportScheduleCrontabNotValidError(aValidationError) instead.tests/unit_tests/commands/report/base_test.pyfile: 95 passed (93 pre-existing + 2 new, one per report type), no regressions.ruff checkandruff format --checkon all changed files: all checks passed / already formatted.mypyon the changed files: no new errors introduced (one pre-existing, unrelatedcall-argerror onnot_found_exc()at line 73 ofbase.pywas confirmed present on unmodifiedHEADas well).To manually verify: configure
ALERT_MINIMUM_INTERVAL = 300(or any value >= 120), then attempt to create an alert with crontab0 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
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.