fix: order legacy filter date range by parsed datetime instead of string comparison - #9569
fix: order legacy filter date range by parsed datetime instead of string comparison#9569eeshsaxena wants to merge 1 commit into
Conversation
_convert_date_value ordered the range with min/max on the raw date strings, which only matches chronological order for zero-padded ISO YYYY-MM-DD. Since _validate_date accepts any dateutil-parseable format, a non-ISO input like 9/1/2023 vs 10/1/2023 sorted lexicographically and produced a reversed range. Compare parsed datetimes instead. Fixes makeplane#9567.
|
|
📝 WalkthroughWalkthroughThe date range converter now compares parsed datetime values instead of raw date strings. It retains the original date values while assigning chronological start and end bounds. ChangesDate range conversion
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/utils/filters/converters.py`:
- Around line 309-315: Update the date-bound ordering logic around
_process_date_field to handle mixed naive and timezone-aware results from
dateutil_parse without leaking TypeError: normalize both parsed bounds according
to the converter’s timezone policy before comparison, or explicitly convert
mixed awareness to ValueError so existing strict and non-strict handling
applies. Preserve non-ISO chronological ordering and add coverage for both
non-ISO bounds and mixed-awareness inputs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e844834-f5fe-4093-bcd1-e85813516c29
📒 Files selected for processing (1)
apps/api/plane/utils/filters/converters.py
| # Order the bounds chronologically by comparing parsed datetimes, not the | ||
| # raw strings: _validate_date accepts non-ISO formats that do not sort | ||
| # lexicographically (e.g. "9/1/2023" vs "10/1/2023"). | ||
| if dateutil_parse(after_dates[0]) <= dateutil_parse(before_dates[0]): | ||
| start_date, end_date = after_dates[0], before_dates[0] | ||
| else: | ||
| start_date, end_date = before_dates[0], after_dates[0] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from dateutil.parser import parse
naive = parse("2023-09-01")
aware = parse("2023-10-01T00:00:00Z")
assert naive.tzinfo is None
assert aware.tzinfo is not None
try:
naive <= aware
except TypeError:
print("Reproduced: mixed naive/aware datetime ordering raises TypeError")
else:
raise SystemExit("Expected TypeError")
PYRepository: makeplane/plane
Length of output: 218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/api/plane/utils/filters/converters.py"
printf '--- file size ---\n'
wc -l "$FILE"
printf '\n--- outline around date converter functions ---\n'
ast-grep outline "$FILE" --view expanded | sed -n '1,220p' || true
printf '\n--- relevant sections ---\n'
sed -n '220,330p' "$FILE" | nl -ba -v220
printf '\n--- dateutil import ---\n'
rg -n "dateutil_parse|_validate_date|_process_date_field|dateutil|parse" "$FILE"Repository: makeplane/plane
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/api/plane/utils/filters/converters.py"
printf '%s\n' '--- file size ---'
wc -l "$FILE"
printf '%s\n' ''
printf '%s\n' '--- relevant sections ---'
sed -n '220,330p' "$FILE" | nl -ba -v220
printf '%s\n' ''
printf '%s\n' '--- dateutil import and converter references ---'
rg -n "dateutil_parse|_validate_date|_process_date_field|dateutil|parse" "$FILE"Repository: makeplane/plane
Length of output: 286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/api/plane/utils/filters/converters.py"
printf '%s\n' '--- file size ---'
wc -l "$FILE"
printf '%s\n' ''
printf '%s\n' '--- relevant sections lines 1-340 ---'
awk '{printf "%5d %s\n", NR, $0}' "$FILE" | sed -n '1,340p'
printf '%s\n' ''
printf '%s\n' '--- dateutil converter references ---'
grep -En "dateutil_parse|_validate_date|_process_date_field|dateutil|parse" "$FILE" || trueRepository: makeplane/plane
Length of output: 16608
Normalize timezone awareness before comparing date bounds.
Mixed naive and timezone-aware date inputs can pass _validate_date, but ordering them at line 312 raises TypeError. For example, 2023-09-01;after parses to a naive datetime, while 2023-10-01T00:00:00Z;before parses to an aware datetime; naive <= aware is rejected by Python. Since _process_date_field only catches ValueError, this error escapes in both strict and non-strict conversion. Normalize both parsed bounds to the converter’s date/timezone policy before comparing, or raise ValueError explicitly for mixed awareness so the existing handler reports it. Add coverage for non-ISO ordering and mixed-awareness inputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/plane/utils/filters/converters.py` around lines 309 - 315, Update
the date-bound ordering logic around _process_date_field to handle mixed naive
and timezone-aware results from dateutil_parse without leaking TypeError:
normalize both parsed bounds according to the converter’s timezone policy before
comparison, or explicitly convert mixed awareness to ValueError so existing
strict and non-strict handling applies. Preserve non-ISO chronological ordering
and add coverage for both non-ISO bounds and mixed-awareness inputs.
2 participants
Fixes #9567.
Problem
LegacyToRichFiltersConverter._convert_date_valuebuilds a date range withmin()/max()on the raw date strings:String comparison only matches chronological order for zero-padded ISO
YYYY-MM-DD. But_validate_dateusesdateutil_parse, so it accepts many other formats (M/D/YYYY, non-zero-padded months, etc.). A non-ISO input like9/1/2023vs10/1/2023sorts lexicographically ("10..." < "9...") and produces a reversed[start, end]range that matches nothing.Fix
Order the two bounds by their parsed
datetimevalues instead of by string, keeping the emitted values in their original form. This makes the range correct for every format_validate_dateaccepts, and is a no-op for already-ISO inputs.Summary by CodeRabbit