fix Issue #2418: parse_rfc3339 calling .groups() on None#2420
fix Issue #2418: parse_rfc3339 calling .groups() on None#2420omavashia2005 wants to merge 1 commit intokubernetes-client:masterfrom
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. DetailsNeeds 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.
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()
…#2418) - Replace search() with fullmatch() for strict RFC3339 format validation - Provide clear, actionable error messages with expected format - Add input whitespace stripping before validation - Improve exception handling with specific ValueError messages - Add comprehensive test cases for invalid formats - Addresses reviewer feedback from PR kubernetes-client#2420 Test: Existing tests pass + new test cases added
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
…#2418) - Replace search() with fullmatch() for strict RFC3339 format validation - Provide clear, actionable error messages with expected format - Add input whitespace stripping before validation - Improve exception handling with specific ValueError messages - Add comprehensive test cases for invalid formats - Addresses reviewer feedback from PR kubernetes-client#2420 Test: Existing tests pass + new test cases added
…#2418) - Replace search() with fullmatch() for strict RFC3339 format validation - Provide clear, actionable error messages with expected format - Add input whitespace stripping before validation - Improve exception handling with specific ValueError messages - Add comprehensive test cases for invalid formats - Addresses reviewer feedback from PR kubernetes-client#2420 Test: Existing tests pass + new test cases added
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 aNoneresult 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_rfc3339with 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.: