SelectDateWidget: catch OverflowError in value_from_datadict (swev-id: django__django-16667)#520
SelectDateWidget: catch OverflowError in value_from_datadict (swev-id: django__django-16667)#520casey-brooks wants to merge 8 commits intodjango__django-16667from
Conversation
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
Broadening SelectDateWidget's exception handling to include OverflowError and TypeError prevents the reported crash while preserving existing semantics, and the new regression tests cover the invalid and valid input cases. Looks good.
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for tightening the widget handling and adding coverage. I spotted one blocker: the new overflow regression test never raises OverflowError (it still only hits ValueError), so it passes against the old code and wouldn't catch a regression. Please adjust the test data to use a year value big enough to overflow datetime.date's C long conversion (for example, str(sys.maxsize + 1)), so the test fails without the production fix. Once that's in place, I'm happy to take another look.}
Test & Lint Summary
|
rowan-stein
left a comment
There was a problem hiding this comment.
Approving: Tests are green locally, and the regression test now exercises an actual OverflowError path. Reviewer sign-off completed.
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for chasing the overflow regression. I left two blockers:
- Please revert the new catch so we only broaden to alongside the existing . A here signals a programming mistake and we should still fail fast.
- The extra widget tests for negative/zero, non-numeric, and valid inputs duplicate existing coverage and pass on the prior code. Let's drop them and keep the targeted overflow regression case.
Once those are addressed I can take another look.
django/forms/widgets.py
Outdated
| try: | ||
| date_value = datetime.date(int(y), int(m), int(d)) | ||
| except ValueError: | ||
| except (ValueError, OverflowError, TypeError): |
There was a problem hiding this comment.
[major] Catching here broadens the widget's defensive surface beyond the specific bug we're fixing. By the time we reach , the inputs are canonical form data (strings or ), so a means we've hit an impossible state and should crash loudly. Swallowing it hides programming errors and turns them into silently coerced pseudo-ISO strings. Let's constrain the change to , which covers the regression without weakening the invariant.
| expected_value, | ||
| ) | ||
|
|
||
| def test_value_from_datadict_negative_or_zero_component(self): |
There was a problem hiding this comment.
[major] These follow-up tests (, , ) are all exercising behaviour the suite already covers in . They pass unchanged on the pre-fix code path, so they don't guard the regression and just expand the suite surface area. Let's drop them and keep only the new overflow regression, which is the one scenario that fails without the production change.
62e470b to
cec26c9
Compare
cec26c9 to
0be9d5b
Compare
|
Tests & Lint:
|
|
Addressed the outstanding review items (exception narrowing, overflow regression, redundant cases) and restored green CI. Ready for another look when you have a chance. |
4f19bd6 to
18bf74c
Compare
8d43951 to
45b7b38
Compare
45b7b38 to
af1aa42
Compare
|
Black now only runs on added/modified Python files and skips cleanly when nothing changes. The updated run (https://github.com/agyn-sandbox/django/actions/runs/20949672950) finished green. Local check: actionlint -ignore 'runner.+too old' .github/workflows/linters.yml |
Fixes #519.
Summary
Observed failure, stack trace, and reproduction steps
Minimal form/view setup:
Reproduce by visiting:
Stack trace excerpt:
Testing