-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix Issue #2418: parse_rfc3339 calling .groups() on None #2420
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
base: master
Are you sure you want to change the base?
fix Issue #2418: parse_rfc3339 calling .groups() on None #2420
Conversation
The committers listed above are authorized under a signed CLA. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omavashia2005 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @omavashia2005! |
/assign |
hour=dt[3], minute=dt[4], second=dt[5], | ||
microsecond=us, tzinfo=tz) | ||
except Exception: | ||
raise ValueError(f"Error in RFC339 Date Formatting {s}") |
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 think in the current implementation of parse_rfc3339
, the _re_rfc3339
regex accept several things that RFC 3339 forbids, also make some required pieces optional - as a result, in some cases datetime.datetime
will crash with a meaningful messages:
for example, _re_rfc3339
accept 25
as hour although RFC 3339 ABNF says:
time-hour = 2DIGIT ; 00-23
:
>>> from kubernetes.base.config import dateutil
>>> s = "2025-10-04t25:00:00"
>>> dateutil.parse_rfc3339(s)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/afshinpaydar/go/src/k8s.io/python/kubernetes/base/config/dateutil.py", line 74, in parse_rfc3339
return datetime.datetime(
^^^^^^^^^^^^^^^^^^
ValueError: hour must be in 0..23
>>>
but in this changes, the catch all section only shows Error in RFC339 Date Formatting ..
message, it seems misleading to me as the regex is not quite strict/correct:
>>> from kubernetes.base.config import dateutil
>>> s = "2025-10-04Z20:00:00"
>>> dateutil.parse_rfc3339(s)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/afshinpaydar/go/src/k8s.io/python/kubernetes/base/config/dateutil.py", line 60, in parse_rfc3339
raise ValueError(f"Error in RFC339 Date Formatting {s}")
ValueError: Error in RFC339 Date Formatting 2025-10-04Z20:00:00
maybe something like below output would be more helpful for users:
ValueError: Invalid RFC 3339 ... , Expected ...
IMHO, I really like the idea of doing less and enable more:
- groups = _re_rfc3339.search(s).groups()
+ m = _re_rfc3339.fullmatch(s.strip())
+ if m is None:
+ raise ValueError(
+ f"Invalid RFC3339 datetime: {s!r} "
+ "(expected YYYY-MM-DDTHH:MM:SS[.frac][Z|±HH:MM])"
+ )
+ groups = m.groups()
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes a runtime bug in
parse_rfc3339()
where the function would call.groups()
on aNone
result if the regex did not match. Previously caused AttributeError: 'NoneType' object has no attribute 'groups'Which issue(s) this PR fixes:
Fixes #2418
Special notes for your reviewer:
Includes unit test
test_2_parse_rfc3339
with malformed datetime strings to verify fix and prevent regressions.Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: