Add range validation to is_valid_date for month and day (#12754)#12782
Conversation
|
Thank you for this contribution, @tranmin001. 🤖 Copilot has been assigned for an initial review. @jimchamp is assigned to this PR and currently has:
PR triage checklist (maintainers / Pam)
Note This comment was automatically generated by Pam, Open Library's Project AI Manager, on behalf of @mekarpeles. Pam is designed to provide status visibility, perform basic project management functions and relevant codebase research, and provide actionable feedback so contributors aren't left waiting. |
There was a problem hiding this comment.
Pull request overview
Fixes is_valid_date validation for reading-log check-ins by rejecting out-of-range month and day values, preventing invalid partial dates from being accepted and stored.
Changes:
- Added numeric range guards for
month(1–12) andday(1–31) inis_valid_date. - Added unit tests covering out-of-range and boundary values for month/day.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
openlibrary/plugins/upstream/checkins.py |
Adds range validation guards to is_valid_date to reject invalid month/day values. |
openlibrary/plugins/upstream/tests/test_checkins.py |
Adds new tests to ensure out-of-range and boundary month/day values behave as expected. |
| if month is not None and not 1 <= month <= 12: | ||
| return False | ||
| if day is not None and not 1 <= day <= 31: | ||
| return False |
| def test_month_out_of_range(self): | ||
| assert is_valid_date(1999, 0, None) is False | ||
| assert is_valid_date(1999, 13, None) is False | ||
| assert is_valid_date(1999, -1, None) is False | ||
|
|
jimchamp
left a comment
There was a problem hiding this comment.
Thanks @tranmin001!
Please coerce month and day into integers during the range check. These are being passed as type int to the validation function today (verified locally), but some future regression could potentially change this.
Co-authored-by: jimchamp <28732543+jimchamp@users.noreply.github.com>
Closes #12754
[fix]
Technical
Adds range validation to
is_valid_dateinopenlibrary/plugins/upstream/checkins.py. The function previously only checked whetheryearwas truthy and whetherdayrequired amonth, but performed no range checks on the values themselves. This meantis_valid_date(2024, 13, None)returnedTrue. The fix adds two guards after the year check:monthmust be between 1 and 12 when provided, anddaymust be between 1 and 31 when provided.Open to any feedback or change!
Testing
Added
test_month_out_of_range,test_day_out_of_range, andtest_boundary_valuestoTestIsValidDateinopenlibrary/plugins/upstream/tests/test_checkins.py.To run:
sudo docker compose run --rm home pytest openlibrary/plugins/upstream/tests/test_checkins.py::TestIsValidDate -vRan
make testlocally — 3850 tests passed, 15 failed. New tests: 4 passed, 0 failed.test_list_view_json.py,test_lists_json.py, andtest_search.py, unrelated to this change.Stakeholders
@jimchamp @mekarpeles