bpo-30681: Support invalid date format or value in email Date header #10783
Conversation
There was a problem hiding this comment.
Just a minor comment for documenting the new defect.
Also, @bitdancer @warsaw I think this addresses all the comments on bpo-30681 and previous PR.
Thanks @timb07 for fixing this :)
| @@ -108,3 +108,6 @@ class NonASCIILocalPartDefect(HeaderDefect): | |||
| """local_part contains non-ASCII characters""" | |||
| # This defect only occurs during unicode parsing, not when | |||
| # parsing messages decoded from binary. | |||
|
|
|||
| class InvalidDateDefect(HeaderDefect): | |||
There was a problem hiding this comment.
Can you also please add this to documentation under email.errors.rst ?
| @@ -48,6 +48,22 @@ def test_parsedate_to_datetime_naive(self): | |||
| utils.parsedate_to_datetime(self.datestring + ' -0000'), | |||
| self.naive_dt) | |||
|
|
|||
| def test_parsedate_to_datetime_with_invalid_raises_typeerror(self): | |||
| with self.assertRaises(TypeError): | |||
There was a problem hiding this comment.
I think this is fine as is, but I think it would be improved if you used a loop and subtests:
invalid_dates = ['', '0', 'A Complete Waste of Time']
for dtstr in invalid_dates:
with self.subTest(dtstr=dtstr):
with self.assertRaises(TypeError):
utils.parsedate_to_datetime(dtstr)Obviously that's pretty nested (though you can probably cut out one layer of nesting by using self.assertRaises(TypeError, utils.parsedate_to_datetime, dtstr) rather than the context manager - I still prefer the context manager, myself), so I suppose it's a matter of taste whether DRY and cleaner separation of test cases matters more than keeping code flat.
Same comment also applies to the test for ValueError below.
|
Hi @timb07, just a friendly ping. Looks like if you can address the code reviews, then this will be merged. Thanks! |
|
This PR was very close to being merged, but seems to have been abandoned, so I am going to close it. It can be reopened if the original author is still interested in it or a new PR can be created to replace it. If someone else creates a new PR, please credit the original author in the comment message. |
…H-22090) I am re-submitting an older PR which was abandoned but is still relevant, #10783 by @timb07. The issue being solved () is still relevant. The original PR #10783 was closed as the final request changes were not applied and since abandoned. In this new PR I have re-used the original patch plus applied both comments from the review, by @maxking and @pganssle. For reference, here is the original PR description: In email.utils.parsedate_to_datetime(), a failure to parse the date, or invalid date components (such as hour outside 0..23) raises an exception. Document this behaviour, and add tests to test_email/test_utils.py to confirm this behaviour. In email.headerregistry.DateHeader.parse(), check when parsedate_to_datetime() raises an exception and add a new defect InvalidDateDefect; preserve the invalid value as the string value of the header, but set the datetime attribute to None. Add tests to test_email/test_headerregistry.py to confirm this behaviour; also added test to test_email/test_inversion.py to confirm emails with such defective date headers round trip successfully. This pull request incorporates feedback gratefully received from @bitdancer, @brettcannon, @Mariatta and @warsaw, and replaces the earlier PR #2254. Automerge-Triggered-By: GH:warsaw
…ythonGH-22090) I am re-submitting an older PR which was abandoned but is still relevant, python#10783 by @timb07. The issue being solved () is still relevant. The original PR python#10783 was closed as the final request changes were not applied and since abandoned. In this new PR I have re-used the original patch plus applied both comments from the review, by @maxking and @pganssle. For reference, here is the original PR description: In email.utils.parsedate_to_datetime(), a failure to parse the date, or invalid date components (such as hour outside 0..23) raises an exception. Document this behaviour, and add tests to test_email/test_utils.py to confirm this behaviour. In email.headerregistry.DateHeader.parse(), check when parsedate_to_datetime() raises an exception and add a new defect InvalidDateDefect; preserve the invalid value as the string value of the header, but set the datetime attribute to None. Add tests to test_email/test_headerregistry.py to confirm this behaviour; also added test to test_email/test_inversion.py to confirm emails with such defective date headers round trip successfully. This pull request incorporates feedback gratefully received from @bitdancer, @brettcannon, @Mariatta and @warsaw, and replaces the earlier PR python#2254. Automerge-Triggered-By: GH:warsaw
In
email.utils.parsedate_to_datetime(), a failure to parse the date, or invalid date components (such as hour outside 0..23) raises an exception. Document this behaviour, and add tests to test_email/test_utils.py to confirm this behaviour.In
email.headerregistry.DateHeader.parse(), check whenparsedate_to_datetime()raises an exception and add a new defectInvalidDateDefect; preserve the invalid value as the string value of the header, but set thedatetimeattribute toNone.Add tests to test_email/test_headerregistry.py to confirm this behaviour; also added test to test_email/test_inversion.py to confirm emails with such defective date headers round trip successfully.
This pull request incorporates feedback gratefully received from @bitdancer, @brettcannon, @Mariatta and @warsaw, and replaces the earlier PR #2254.
https://bugs.python.org/issue30681