Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-126845: Some edge cases in email.utils.parsedate_to_datetime seem to differ from RFC2822 spec#134438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This parses0001 correctly, but breaks parsing of real two-digit years.
Lib/email/_parseaddr.py Outdated
| @@ -149,7 +149,7 @@ def _parsedate_tz(data): | |||
| # calls for a two-digit yy, but RFC 2822 (which obsoletes RFC 822) | |||
| # mandates a 4-digit yy. For more information, see the documentation for | |||
| # the time module. | |||
| if yy < 100: | |||
| iflen(str(yy)) >= 2 andyy < 100: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yy is now a number, so ifyy < 100 (and it's positive),len(str(yy)) >= 2 is never true.
You need to keep the original year string for this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was able to remove this check following what@bitdancer said below
Lib/test/test_email/test_email.py Outdated
| @@ -3227,7 +3227,7 @@ def test_parsedate_y2k(self): | |||
| """ | |||
| self.assertEqual(utils.parsedate_tz('25 Feb 03 13:47:26 -0800'), | |||
| utils.parsedate_tz('25 Feb2003 13:47:26 -0800')) | |||
| utils.parsedate_tz('25 Feb3 13:47:26 -0800')) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This makes the test ineffective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reverted this
| import unittest | ||
| from email.utils import parsedate_to_datetime | ||
| class ParsedateToDatetimeTest(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
parsedate_to_datetime tests belong in test_utils in DateTimeTests. (Those tests could maybe use some refactoring, but let's ignore that ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just moved it :)
| def test(self): | ||
| expectations = { | ||
| "Sat, 15 Aug 0001 23:12:09 +0500": "0001", | ||
| "Thu, 1 Sep 1 23:12:09 +0800": "0001", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Logically when doing a Postel "be generous in what you accept" recovery here, a single digit year should be treated as if it were a two digit year, which means the result here should 2001, which is what the current code produces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| expectations = { | ||
| "Sat, 15 Aug 0001 23:12:09 +0500": "0001", | ||
| "Thu, 1 Sep 1 23:12:09 +0800": "0001", | ||
| "Thu, 7 Oct 123 23:12:09 +0500": "0123", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And this result should be 2023, per the rfc text you quoted (three digit years get 1900 added). I have no idea why that settled on that logic, it makes no sense to me, but that's what the RFC says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed!
| "Thu, 1 Sep 1 23:12:09 +0800": "0001", | ||
| "Thu, 7 Oct 123 23:12:09 +0500": "0123", | ||
| } | ||
| for input_string, output_string in expectations.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
subTest would be good here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed!
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I have made the requested changes; please review again |
Thanks for making the requested changes! @bitdancer: please review the changes made to this pull request. |
Uh oh!
There was an error while loading.Please reload this page.
Continuation of PR#134350 which was mistakenly closed due to deleted local fork.
Regarding no.1 from#126845
Also added tests for different digit years and updated a test in test_email.py to correctly assert 1 digit years (03 is evaluated to 3)
email.utils.parsedate_to_datetimeseem to differ from RFC2822 spec #126845