fix(cron): validate comma-separated lists of ranges and steps#463
Open
patchwright wants to merge 1 commit into
Open
fix(cron): validate comma-separated lists of ranges and steps#463patchwright wants to merge 1 commit into
patchwright wants to merge 1 commit into
Conversation
The comma-list branch in _validate_cron_component was evaluated after the '-' and '/' branches, so any list element that was itself a range (e.g. '1-5') or step (e.g. '*/2') short-circuited and the whole expression was rejected. Move the comma-list split to run first, so each element is validated as a range/step/single value. Adds regression cases for list-of-ranges, list-with-single, and multi-list expressions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cron()rejects valid expressions whose components are comma-separated lists of ranges or steps, e.g.cron("1-5,10-20 * * * *")raisesValidationErroreven though the expression is valid.Reproduction
Validity currently depends on element order, which is the clearest sign this is a bug rather than an intended limitation:
Cause
In
src/validators/cron.py,_validate_cron_componentchecks the"-"and"/"branches before the","branch. A component like"1-5,10-20"therefore matches the range branch,split("-")yields three parts, and the whole expression is rejected.Fix
Evaluate the
","branch first, recursing on each element so each element can itself be a single value, a range ("1-5"), or a step ("*/2").if component == "*": return True + if "," in component: + for item in component.split(","): + if not _validate_cron_component(item, min_val, max_val): + return False + return True + if component.isdecimal():No previously-accepted input changes behaviour: any component containing a comma previously hit the range branch and was rejected, so this only accepts inputs that were wrongly rejected. The existing dead commented
all(...)block is removed.Tests
Adds four cases to
test_returns_true_on_valid_cron:"1-5,10-20 * * * *""1-5,30 * * * *""10,20-30 * * * *""1-5,10-20,30-40 * * * *"Each fails on the current code and passes with the fix. Full suite: 895 → 899 passing.
ruff check/ruff formatclean.I couldn't find an existing issue or PR covering this — happy to fold it into one if I missed it.