Skip to content

Add range validation to is_valid_date for month and day (#12754)#12782

Merged
jimchamp merged 2 commits into
internetarchive:masterfrom
tranmin001:12754/fix/is-valid-date-range-validation
May 26, 2026
Merged

Add range validation to is_valid_date for month and day (#12754)#12782
jimchamp merged 2 commits into
internetarchive:masterfrom
tranmin001:12754/fix/is-valid-date-range-validation

Conversation

@tranmin001

Copy link
Copy Markdown
Contributor

Closes #12754

[fix]

Technical

Adds range validation to is_valid_date in openlibrary/plugins/upstream/checkins.py. The function previously only checked whether year was truthy and whether day required a month, but performed no range checks on the values themselves. This meant is_valid_date(2024, 13, None) returned True. The fix adds two guards after the year check: month must be between 1 and 12 when provided, and day must 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, and test_boundary_values to TestIsValidDate in openlibrary/plugins/upstream/tests/test_checkins.py.

To run:
sudo docker compose run --rm home pytest openlibrary/plugins/upstream/tests/test_checkins.py::TestIsValidDate -v

Ran make test locally — 3850 tests passed, 15 failed. New tests: 4 passed, 0 failed.

  • The 15 failures are pre-existing Pydantic/FastAPI compatibility errors in test_list_view_json.py, test_lists_json.py, and test_search.py, unrelated to this change.

Stakeholders

@jimchamp @mekarpeles

@mekarpeles

Copy link
Copy Markdown
Member

Thank you for this contribution, @tranmin001.

🤖 Copilot has been assigned for an initial review.

@jimchamp is assigned to this PR and currently has:

  • 7 open PR(s) of equal or higher priority to review first
PR triage checklist (maintainers / Pam)
  • PR description — not empty; explains what the change does and how to verify it
  • References an issue — PR body contains a #NNN reference
    • Linked issue is triaged — has a Priority: * label (not just Needs: Triage)
    • Linked issue is assigned — has at least one assignee
  • Commit history clean — no WIP/fixup/conflict noise; commit messages are meaningful
  • CI passing — no failing check-runs
  • Test cases present — if the change touches substantive logic, test coverage exists or is explained
  • Proof of testing — PR body includes a description of what was tested, a screenshot, or a video

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and day (1–31) in is_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.

Comment on lines +41 to +44
if month is not None and not 1 <= month <= 12:
return False
if day is not None and not 1 <= day <= 31:
return False
Comment on lines +44 to +48
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 jimchamp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread openlibrary/plugins/upstream/checkins.py Outdated
@jimchamp jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label May 22, 2026
Co-authored-by: jimchamp <28732543+jimchamp@users.noreply.github.com>
@github-actions github-actions Bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label May 23, 2026

@jimchamp jimchamp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@jimchamp jimchamp merged commit 48f2f5e into internetarchive:master May 26, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

is_valid_date accepts out-of-range month and day values (checkins.py)

4 participants