Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle parsing ISO datetimes where the microseconds round up to 1s or more #1084

Merged
merged 3 commits into from
Nov 29, 2023

Conversation

tonyandrewmeyer
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer commented Nov 28, 2023

We handle microseconds with more than 6 digits if the number of microseconds can be rounded to a number < 1M; for example:

>>> timeconv.parse_rfc3339("2006-08-28T13:20:00.9999991+12:00")
datetime.datetime(2006, 8, 28, 13, 20, 0, 999999, tzinfo=datetime.timezone(datetime.timedelta(seconds=43200)))

However, if rounding takes us to the next second, we fail; for example:

>>> timeconv.parse_rfc3339("2006-08-28T13:20:00.9999999+12:00")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/tameyer/code/operator/ops/_private/timeconv.py", line 54, in parse_rfc3339
    return datetime.datetime(int(y), int(m), int(d), int(hh), int(mm), int(ss),
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: microsecond must be in 0..999999

Handle this case by putting an upper bound on the microseconds of 999,999 (truncating to that if the value would round to be higher).

Note that it looks like the Python 3.11+ standard library (which includes this functionality) truncates instead of rounds (at least in the Python _pydatetime - I haven't looked at the C implementation). Rounding seems more correct to me, but we should duplicate the stdlib behaviour since we'll presumably move to using that once we have Python 3.11 as a minimum Python version.

Fixes #1083

@tonyandrewmeyer
Copy link
Contributor Author

Hmm, actually, this could overflow into minutes, then into hours, etc. Will fix.

@tonyandrewmeyer tonyandrewmeyer marked this pull request as draft November 28, 2023 21:37
@tonyandrewmeyer tonyandrewmeyer marked this pull request as ready for review November 28, 2023 21:43
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Huh, nice find! This bug has been lurking here since I wrote it. It will happen 500/1,000,000,000 times, or 1 in 2 million, so it's definitely not crazy that it happened.

Yeah, I do think we should truncate to converge on the new stdlib's behaviour. Does it truncate only when the nanoseconds are >= 999999500, or for all values?

ops/_private/timeconv.py Outdated Show resolved Hide resolved
@tonyandrewmeyer
Copy link
Contributor Author

Huh, nice find! This bug has been lurking here since I wrote it. It will happen 500/1,000,000,000 times, or 1 in 2 million, so it's definitely not crazy that it happened.

Although with those odds I think you can legitimately feel unlucky if it happens to you 😂

Yeah, I do think we should truncate to converge on the new stdlib's behaviour. Does it truncate only when the nanoseconds are >= 999999500, or for all values?

Only then, basically. It's just slicing the original string and ignoring anything after the 6th digit. I checked the CPython code for _datetime and it's basically doing the same as the Python code.

@benhoyt
Copy link
Collaborator

benhoyt commented Nov 29, 2023

Okay, in that case let's keep using round() but add a min(microsecond, 999999) after.

@tonyandrewmeyer tonyandrewmeyer changed the title fix: handle parsing ISO datetimes where the microseconds round up to more than 1s fix: handle parsing ISO datetimes where the microseconds round up to 1s or more Nov 29, 2023
@benhoyt benhoyt merged commit 681bce2 into canonical:main Nov 29, 2023
19 checks passed
@benhoyt
Copy link
Collaborator

benhoyt commented Nov 29, 2023

Thanks for the report and the fix!

@tonyandrewmeyer tonyandrewmeyer deleted the timeconv-parse-rounding-1083 branch November 29, 2023 02:55
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.

Encountered an error running timeconv.parse_rfc3339 in ops.pebble
2 participants