fix(reports): humanize day-of-month + day-of-week crontabs as OR - #43307
fix(reports): humanize day-of-month + day-of-week crontabs as OR#43307aminghadersohi wants to merge 2 commits into
Conversation
cron_descriptor joins a restricted day-of-month and a restricted day-of-week with a comma, which reads as an intersection: "0 9 7-11 * 2" renders as "At 09:00 AM, on day 7 through 11 of the month, only on Tuesday". POSIX cron - and croniter, which selects the fire times - unions the two fields instead, so the report also runs on every Tuesday outside that range. Join the clauses with "or" so the Schedule column matches when the report actually fires.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43307 +/- ##
==========================================
- Coverage 66.67% 66.66% -0.01%
==========================================
Files 2876 2876
Lines 164007 164020 +13
Branches 37834 37836 +2
==========================================
Hits 109347 109347
- Misses 52514 52524 +10
- Partials 2146 2149 +3
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:
|
dosubot
Bot
added
the
alert-reports
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| fields = cron.split() | ||
| if len(fields) != 5: | ||
| return description |
There was a problem hiding this comment.
Suggestion: Expressions are only corrected when they contain exactly five fields. However, validate_crontab and cron_schedule_window both delegate to croniter, which accepts six- and seven-field expressions, including schedules with restricted day-of-month and day-of-week fields. Those schedules bypass the OR replacement and continue to display the original intersection-like description. Either support all cron formats accepted by the scheduler or explicitly reject non-five-field expressions during validation. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ API-accepted extended cron schedules show misleading descriptions.
- ⚠️ Report list and detail views disagree with scheduler fire times.
- ⚠️ Users may misinterpret alert or report execution dates.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/tasks/cron_util.py
**Line:** 50:52
**Comment:**
*Api Mismatch: Expressions are only corrected when they contain exactly five fields. However, `validate_crontab` and `cron_schedule_window` both delegate to `croniter`, which accepts six- and seven-field expressions, including schedules with restricted day-of-month and day-of-week fields. Those schedules bypass the OR replacement and continue to display the original intersection-like description. Either support all cron formats accepted by the scheduler or explicitly reject non-five-field expressions during validation.
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 correct. The current implementation of To resolve this, you can update the validation logic to support extended cron formats. Since Would you like me to fetch all other comments on this PR to validate them and implement a minimal fix for the rest as well? superset/tasks/cron_util.py |
Code Review Agent Run #9a5f44Actionable 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 |
1 participant
SUMMARY
The Schedule column on Alerts & Reports shows a human-readable rendering of the report's crontab, produced by
ReportSchedule.crontab_humanized→cron_descriptor.get_description(). When a schedule restricts both the day-of-month and the day-of-week fields, that description is wrong.0 9 7-11,19-23 * 2renders as:The comma plus "only on" reads as an intersection — "the 7th–11th and 19th–23rd, but only when that day is a Tuesday". POSIX cron does the opposite: when both fields are restricted they are OR'd.
croniter, which selects the actual fire times insuperset/tasks/cron_util.py, implements that correctly, so the report also fires on every Tuesday outside those two ranges. Verified againstcroniter: for the expression above the first fire time after 2026-09-01 is 2026-09-01 09:00 — a Tuesday that is in neither day-of-month range.So the picker is right and the description is wrong. The frontend cron picker (
react-js-cron, viaCronPicker) already renders this correctly: its locale setsprefixWeekDaysForMonthAndYearPeriod: 'or', so the builder shows "… on day 7-11,19-23 or Tuesday". Only the backend-generated text disagreed, which is exactly the mismatch users hit — they configure a schedule in the picker and then read a contradictory description in the list.Rather than swapping out
cron_descriptor(which is otherwise fine and handlesL,W,#n, i18n, etc.), this adds a smallget_cron_description()wrapper insuperset/tasks/cron_util.py— the module that already owns cron interpretation viacroniter. It only changes the output for the ambiguous case: when neither the day-of-month nor the day-of-week field is*/?, the day-of-week clause is rejoined with "or". Every other expression is passed throughcron_descriptoruntouched.0 9 7-11,19-23 * 20 9 1,15 * 1-50 9 L * 50 9 * * 20 9 7-11 * *Known limitations: the rewrite is applied to standard 5-field expressions only (what the picker and the API validator produce); 6-field expressions fall through to the unmodified
cron_descriptoroutput because the field offsets are ambiguous. The "only " prefix strip is English-specific —cron_descriptoris invoked with its defaulten_USlocale here, and if the prefix isn't present the clause is emitted verbatim, so a different locale degrades to ", or " rather than breaking.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI code changed — this is the text returned by the
crontab_humanizedfield ofGET /api/v1/report/, rendered verbatim in the Schedule column. The before/after strings are in the table above.TESTING INSTRUCTIONS
Regression tests in
tests/unit_tests/tasks/test_cron_util.py:test_get_cron_description— parametrized over the ambiguous and unambiguous cases; the ambiguous rows fail onmaster(assert '…of the month, only on Tuesday' == '…of the month, or on Tuesday').test_get_cron_description_matches_fire_times— pins the description against whatcroniteractually schedules, so the two can't drift apart again.Manually: create a report with schedule type "CRON Schedule" and crontab
0 9 7-11,19-23 * 2, then look at the Schedule column on the Alerts & Reports list. It should read "At 09:00 AM, on day 7 through 11 and 19 through 23 of the month, or on Tuesday", matching what the recurring picker shows for the same expression.ADDITIONAL INFORMATION