fix: use datetime.fromisoformat() to handle timezone offsets in event… - #1634
fix: use datetime.fromisoformat() to handle timezone offsets in event…#1634mkanoor wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe websocket consumer now parses ChangesWebsocket timestamp handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
c26782d to
825aa65
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/wsapi/test_consumer.py (1)
306-306: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the persisted timestamp, not only the row count.
The test passes even if
createdis ignored or converted to the wrong instant. Retrieve the insertedJobInstanceEventand comparecreated_atwith the expected timestamp after applying the project’s timezone normalization.As per path instructions: “Focus on major issues impacting performance, readability, maintainability and security; avoid nitpicks and verbosity.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/wsapi/test_consumer.py` at line 306, Update the test around get_job_instance_event_count to retrieve the inserted JobInstanceEvent and assert its created_at matches the expected timestamp after applying the project’s timezone normalization. Retain the existing row-count assertion, but ensure the test validates persisted timestamp correctness rather than only insertion count.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aap_eda/wsapi/consumers.py`:
- Line 240: Update the JobInstanceEvent.created_at model/serialization path so
the parsed WebSocket timestamp from created is preserved when
JobInstanceEvent.objects.create(...) runs. Make created_at explicitly writable
(including any serializer or model configuration needed) and avoid auto_now_add
overriding the supplied value; retain the parsed timestamp as the stored event
time.
---
Nitpick comments:
In `@tests/integration/wsapi/test_consumer.py`:
- Line 306: Update the test around get_job_instance_event_count to retrieve the
inserted JobInstanceEvent and assert its created_at matches the expected
timestamp after applying the project’s timezone normalization. Retain the
existing row-count assertion, but ensure the test validates persisted timestamp
correctness rather than only insertion count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c1b6665f-b544-4b7f-9a66-53e38948b727
📒 Files selected for processing (2)
src/aap_eda/wsapi/consumers.pytests/integration/wsapi/test_consumer.py
| created = event_data.get("created") | ||
| if created: | ||
| created = datetime.strptime(created, "%Y-%m-%dT%H:%M:%S.%f") | ||
| created = datetime.fromisoformat(created) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
rg -n -C 3 'JobInstanceEvent|created_at\s*=' src/aap_eda/core/models/job.pyRepository: ansible/eda-server
Length of output: 757
Persist the WebSocket event timestamp in JobInstanceEvent.created_at.
JobInstanceEvent.created_at is defined with auto_now_add=True, so the parsed created value passed into JobInstanceEvent.objects.create(...) is overwritten by insertion time. Either make the field editable / add it to the serializer or omit it from the model if only the runtime event time should be kept.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aap_eda/wsapi/consumers.py` at line 240, Update the
JobInstanceEvent.created_at model/serialization path so the parsed WebSocket
timestamp from created is preserved when JobInstanceEvent.objects.create(...)
runs. Make created_at explicitly writable (including any serializer or model
configuration needed) and avoid auto_now_add overriding the supplied value;
retain the parsed timestamp as the stored event time.
Source: Path instructions
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1634 +/- ##
=======================================
Coverage 93.36% 93.37%
=======================================
Files 244 244
Lines 11412 11412
=======================================
+ Hits 10655 10656 +1
+ Misses 757 756 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| async def test_handle_events_with_timezone_in_created( | ||
| ws_communicator: WebsocketCommunicator, | ||
| created_timestamp: str, | ||
| ): | ||
| job_instance = await _prepare_job_instance() | ||
|
|
||
| initial_count = await get_job_instance_event_count() | ||
| payload = { | ||
| "type": "AnsibleEvent", | ||
| "event": { | ||
| "event": "verbose", | ||
| "job_id": job_instance.uuid, | ||
| "counter": 1, | ||
| "stdout": "the playbook is completed", | ||
| "created": created_timestamp, | ||
| }, | ||
| } | ||
| await ws_communicator.send_json_to(payload) | ||
| await ws_communicator.wait() | ||
|
|
||
| assert (await get_job_instance_event_count()) == initial_count + 1 |
There was a problem hiding this comment.
AFAICS, this test only checks that the event is processed, however nothing actually seems to check that the wanted created_timestamp string is parsed properly including the timezone. Am I missing anything?
There was a problem hiding this comment.
@ptoscano This was a parsing error that I was trying to fix, there is another issue which relates to created_at should be set differently that requires DB migration. I think that should be done in a different PR. This was limited to fixing the parsing error.
There was a problem hiding this comment.
there is another issue which relates to created_at should be set differently that requires DB migration. I think that should be done in a different PR. This was limited to fixing the parsing error.
That is a different problem, yes.
What I'm saying is different: this test does not actually test that the result of the created parsing is what is expected. If datetime.fromisoformat() would suddenly ignore bits (like the previous datetime.strptime() way) without an exception, then this test is not checking that.
825aa65 to
e737b9e
Compare
… timestamps
strptime with "%Y-%m-%dT%H:%M:%S.%f" fails on timestamps containing
timezone offsets (e.g. +00:00) with:
ValueError: unconverted data remains: +00:00
File "consumers.py", line 240, in insert_event_related_data
created = datetime.strptime(created, "%Y-%m-%dT%H:%M:%S.%f")
fromisoformat() handles all ISO 8601 variants including naive,
Z-suffixed, and offset timestamps.
This was happening on receipt of AnsibleEvents on the WebSocket channel
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
e737b9e to
d5422c7
Compare
|
4 participants
Failure on parsing date time in AnsibleEvents which contain time zone offset
strptime with "%Y-%m-%dT%H:%M:%S.%f" fails on timestamps containing timezone offsets (e.g. +00:00) with:
ValueError: unconverted data remains: +00:00
File "consumers.py", line 240, in insert_event_related_data
created = datetime.strptime(created, "%Y-%m-%dT%H:%M:%S.%f")
fromisoformat() handles all ISO 8601 variants including naive, Z-suffixed, and offset timestamps.
Summary by CodeRabbit