Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for “MARS-style” date rule strings in datasource match.date configuration (single/list/range/stepped-range), alongside existing old-style relative comparisons.
Changes:
- Implement MARS date token parsing plus list/range(/by step) expansion and matching logic in
date_check. - Add unit tests covering token parsing, expansion, rule matching, and
date_checkOR/AND semantics. - Update the documented JSON schema descriptions/examples for date matching rules.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
polytope_server/common/datasource/date_check.py |
Adds MARS-style parsing/expansion/matching and updates date_check logic to support OR-matching across multiple MARS rules. |
tests/unit/test_date_check.py |
New unit test suite for MARS-style date parsing/matching and old-style compatibility. |
docs/source/schemas/schema.json |
Updates match/date documentation and expands schema typing for match values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if len(parts) == 5: | ||
| if parts[3].strip().lower() != "by": | ||
| raise DateError(f"Invalid Mars date string: {date_str!r}") | ||
| step = abs(int(parts[4].strip())) | ||
| elif len(parts) != 3: | ||
| raise DateError(f"Invalid Mars date string: {date_str!r}") | ||
|
|
||
| dates = [] | ||
| if start_d <= end_d: | ||
| current = start_d | ||
| while current <= end_d: | ||
| dates.append(current) | ||
| current += timedelta(days=step) | ||
| else: | ||
| current = start_d | ||
| while current >= end_d: | ||
| dates.append(current) | ||
| current -= timedelta(days=step) | ||
| return dates |
There was a problem hiding this comment.
expand_mars_dates() allows /by/0 (or a non-integer step) to reach step = abs(int(...)) without validation. With step 0, the while current <= end_d: / while current >= end_d: loop never advances and will hang. Please validate step is an integer >= 1 and raise DateError on invalid steps before entering the loop.
| step = 1 | ||
| if len(parts) == 5: | ||
| if parts[3].strip().lower() != "by": | ||
| raise DateError(f"Invalid Mars date rule: {rule_str!r}") | ||
| step = abs(int(parts[4].strip())) | ||
| elif len(parts) != 3: | ||
| raise DateError(f"Invalid Mars date rule: {rule_str!r}") | ||
|
|
||
| min_d = min(start_d, end_d) | ||
| max_d = max(start_d, end_d) | ||
| if not (min_d <= date_d <= max_d): | ||
| return False | ||
| # Date must fall on a step boundary from start | ||
| return abs((date_d - start_d).days) % step == 0 |
There was a problem hiding this comment.
date_in_mars_rule() has the same /by/0 issue: step = abs(int(...)) can produce 0 (or raise ValueError), and then ... % step will crash with ZeroDivisionError. Add explicit validation for step >= 1 (and a try/except around int(...)) and raise DateError with a helpful message when the rule is malformed.
| # New-style Mars date rules: each user date must match at least one rule | ||
| user_dates = expand_mars_dates(date) | ||
| for user_date in user_dates: | ||
| if not any(date_in_mars_rule(user_date, rule) for rule in allowed_values): | ||
| raise DateError(f"Date {user_date} does not match any allowed date rule: {allowed_values}") |
There was a problem hiding this comment.
New-style matching expands the user-provided date string via expand_mars_dates(date) (including full daily expansion of ranges). A request like 19000101/to/21000101 can allocate tens of thousands of date objects per datasource match, and much larger ranges can become a DoS vector. Consider avoiding full expansion (e.g., validate user range against rules using boundary/step arithmetic), or enforce a hard maximum number of expanded dates and fail fast with DateError if exceeded.
Description
Add the ability to set date match rules with mars date strings, e.g.
Where the date must match
one ofthe rule stringsContributor Declaration
By opening this pull request, I affirm the following: