Conversation
…_iso_format_to_full_iso_format - Add _normalize_timezone_offset to expand +HH to +HH:00 before parsing - Python 3.10 datetime.fromisoformat() rejects +00 format from PostgreSQL - Also fix typo: 'covert' -> 'convert' in error log message Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
👋 @devin-ai-integration[bot] |
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughNormalize abbreviated timezone offsets (e.g., Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
elementary/utils/time.py
Outdated
| ) | ||
|
|
||
|
|
||
| _ABBREVIATED_TZ_OFFSET_PATTERN = re.compile(r"([+-])(\d{2})$") |
There was a problem hiding this comment.
🟡 Regex falsely matches date components (e.g., -15 in 2024-01-15) as abbreviated timezone offsets
The regex ([+-])(\d{2})$ is too broad and matches trailing -DD in date-only ISO strings, corrupting them before parsing.
Root Cause and Impact
The pattern _ABBREVIATED_TZ_OFFSET_PATTERN = re.compile(r"([+-])(\d{2})$") matches any string ending with +XX or -XX. This means a date-only string like "2024-01-15" matches on "-15" and gets rewritten to "2024-01-15:00".
Verified with the actual code:
"2024-01-15" → "2024-01-15:00" (corrupted!)
"2024-12" → "2024-12:00" (corrupted!)
In the current Python 3.10 environment, datetime.fromisoformat("2024-01-15:00") happens to parse identically to datetime.fromisoformat("2024-01-15") (both yield 2024-01-15 00:00:00) because fromisoformat accepts any single character as a date-time separator. So the output is currently identical by coincidence, but this is fragile — it relies on a quirk of fromisoformat's separator handling.
A safer pattern would anchor to a time component before the offset, e.g.:
re.compile(r"(\d{2}:\d{2}(?::\d{2})?(?:\.\d+)?)([+-])(\d{2})$")or at minimum require a T or digit before the +/-:
re.compile(r"(?<=\d)([+-])(\d{2})$")Impact: Currently minimal because the coincidental parse behavior produces the same result, but the function silently corrupts its input for date-only strings. This could become a real issue with future Python version changes to fromisoformat parsing behavior, or with other partial ISO format inputs (like "2024-12").
| _ABBREVIATED_TZ_OFFSET_PATTERN = re.compile(r"([+-])(\d{2})$") | |
| _ABBREVIATED_TZ_OFFSET_PATTERN = re.compile(r"(?<=\d{2})([+-])(\d{2})$") | |
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
elementary/utils/time.py (1)
100-107: Add regression tests for abbreviated offsets.This change is parsing-critical. Please add tests covering at least:
+00→+00:00, negative offsets (e.g.,-05), already-normalized offsets (+05:30), and no-offset inputs to prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/utils/time.py` around lines 100 - 107, Add unit tests for convert_partial_iso_format_to_full_iso_format to cover abbreviated and edge timezone offsets: test inputs like "2023-01-01T12:00:00+00" expecting "+00:00", "2023-01-01T12:00:00-05" expecting "-05:00", an already-normalized offset like "2023-01-01T12:00:00+05:30" expecting unchanged "+05:30", and a no-offset input like "2023-01-01T12:00:00" (expecting UTC or no offset behavior as produced by the function); for each invocation assert the returned string is a valid ISO timestamp with microseconds stripped (microsecond=0) and correct colon-normalized offset; add these tests alongside other time utilities and reference convert_partial_iso_format_to_full_iso_format (and _normalize_timezone_offset if needed) so future changes cannot regress these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@elementary/utils/time.py`:
- Around line 100-107: Add unit tests for
convert_partial_iso_format_to_full_iso_format to cover abbreviated and edge
timezone offsets: test inputs like "2023-01-01T12:00:00+00" expecting "+00:00",
"2023-01-01T12:00:00-05" expecting "-05:00", an already-normalized offset like
"2023-01-01T12:00:00+05:30" expecting unchanged "+05:30", and a no-offset input
like "2023-01-01T12:00:00" (expecting UTC or no offset behavior as produced by
the function); for each invocation assert the returned string is a valid ISO
timestamp with microseconds stripped (microsecond=0) and correct
colon-normalized offset; add these tests alongside other time utilities and
reference convert_partial_iso_format_to_full_iso_format (and
_normalize_timezone_offset if needed) so future changes cannot regress these
cases.
…fset
Anchors the pattern to :\d{2} (seconds) or .\d+ (fractional seconds)
before the +/- sign, preventing false matches on date-only strings
like '2024-01-15' where '-15' would incorrectly match.
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
elementary/utils/time.py
Outdated
| ) | ||
|
|
||
|
|
||
| _ABBREVIATED_TZ_OFFSET_PATTERN = re.compile(r"([+-])(\d{2})$") |
There was a problem hiding this comment.
Good catch — the original ([+-])(\d{2})$ would indeed match -15 in date-only strings like 2024-01-15.
I've tightened the pattern to (:\d{2}(?:\.\d+)?)([+-])(\d{2})$ which requires a time component (:SS or :SS.fraction) before the +/-, so it only fires on actual timezone offsets after timestamps. Pushed in b38a4b5.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
elementary/utils/time.py (2)
93-107: Add unit coverage for timezone normalization edge cases.Please add tests for
convert_partial_iso_format_to_full_iso_formatcovering+HH,-HH, fractional seconds, and date-only inputs to ensure the regex doesn’t regress parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/utils/time.py` around lines 93 - 107, Add unit tests for convert_partial_iso_format_to_full_iso_format that exercise the _ABBREVIATED_TZ_OFFSET_PATTERN normalization: include inputs with "+HH" (e.g., "2023-01-01T12:00:00+02"), "-HH" (e.g., "2023-01-01T12:00:00-05"), inputs with fractional seconds (e.g., "2023-01-01T12:00:00.123+03"), and date-only strings (e.g., "2023-01-01") to ensure _normalize_timezone_offset and convert_partial_iso_format_to_full_iso_format produce valid full ISO strings and do not break parsing; assert expected isoformat outputs and that no exceptions are raised.
109-110: Update monitors/alerts that match the old log string.The log message typo is fixed; ensure any Datadog monitors or alert rules keyed on the old string are updated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/utils/time.py` around lines 109 - 110, The log string passed to logger.exception that referenced partial_iso_format_time was corrected, so update any downstream Datadog monitors/alert rules that match the old message exactly to use the new message text; search for usages of logger.exception (and the variable partial_iso_format_time) in elementary/utils/time.py to find the new exact string and update monitors/alert query expressions or saved alerts to match it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@elementary/utils/time.py`:
- Around line 93-107: Add unit tests for
convert_partial_iso_format_to_full_iso_format that exercise the
_ABBREVIATED_TZ_OFFSET_PATTERN normalization: include inputs with "+HH" (e.g.,
"2023-01-01T12:00:00+02"), "-HH" (e.g., "2023-01-01T12:00:00-05"), inputs with
fractional seconds (e.g., "2023-01-01T12:00:00.123+03"), and date-only strings
(e.g., "2023-01-01") to ensure _normalize_timezone_offset and
convert_partial_iso_format_to_full_iso_format produce valid full ISO strings and
do not break parsing; assert expected isoformat outputs and that no exceptions
are raised.
- Around line 109-110: The log string passed to logger.exception that referenced
partial_iso_format_time was corrected, so update any downstream Datadog
monitors/alert rules that match the old message exactly to use the new message
text; search for usages of logger.exception (and the variable
partial_iso_format_time) in elementary/utils/time.py to find the new exact
string and update monitors/alert query expressions or saved alerts to match it.
…so_format Covers abbreviated offsets (+00, -05), fractional seconds, full offsets, no-offset input, and date-only strings to prevent regressions. Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
1 participant
fix(CORE-355): handle abbreviated timezone offsets in time string parsing
Summary
PostgreSQL can emit timestamps with abbreviated timezone offsets like
+00(2-digit, no minutes). Python 3.10'sdatetime.fromisoformat()rejects this format — it requires+00:00. This causes a high volume ofFailed to covert time stringerrors in Datadog (logger:elementary.utils.time).The fix adds a normalization step that expands abbreviated offsets (
+00→+00:00) before parsing. Also fixes a typo in the error log message ("covert" → "convert").Updates since last revision
([+-])(\d{2})$to(:\d{2}(?:\.\d+)?)([+-])(\d{2})$— now requires a time component (:SSor:SS.fraction) before the+/-sign, preventing false matches on date-only strings like2024-01-15where-15would have incorrectly matched.+00,-05), fractional seconds, already-correct full offsets (+00:00,+05:30), no-offset input, and date-only strings.Review & Testing Checklist for Human
"Failed to covert time string"to"Failed to convert time string". If any Datadog monitors or alerts filter on the old misspelled string, they'll need updating.:\d{2}before the offset, so a hypothetical input like15:26+00(minutes without seconds) would not be normalized. This format is unusual andfromisoformatrejects it regardless, so it's not a regression — just worth noting."Failed to convert time string"error volume drops to near-zero for theairflow_prodservice.Notes
Summary by CodeRabbit
Bug Fixes
Tests