Skip to content

fix: use datetime.fromisoformat() to handle timezone offsets in event… #1634

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.

Sign up for GitHub

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

Open
mkanoor wants to merge 1 commit into
base: main
Choose a base branch
from
+35 −1
Open

fix: use datetime.fromisoformat() to handle timezone offsets in event… #1634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/aap_eda/wsapi/consumers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ def insert_event_related_data(self, message: AnsibleEventMessage) -> None:

created = event_data.get("created")
if created:
created = datetime.strptime(created, "%Y-%m-%dT%H:%M:%S.%f")
created = datetime.fromisoformat(created)

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

rg -n -C 3 'JobInstanceEvent|created_at\s*=' src/aap_eda/core/models/job.py

Repository: 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


job_instance_event = models.JobInstanceEvent.objects.create(
job_uuid=event_data.get("job_id"),
Expand Down
34 changes: 34 additions & 0 deletions tests/integration/wsapi/test_consumer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,40 @@ async def test_handle_events(ws_communicator: WebsocketCommunicator):
assert (await get_job_instance_event_count()) == 1


@pytest.mark.parametrize(
"created_timestamp",
[
pytest.param("2023-03-29T15:00:17.260803", id="naive"),
pytest.param("2023-03-29T15:00:17.260803Z", id="utc_z_suffix"),
pytest.param("2023-03-29T15:00:17.260803+00:00", id="utc_offset"),
pytest.param("2023-03-29T15:00:17.260803+05:30", id="positive_offset"),
pytest.param("2023-03-29T15:00:17.260803-04:00", id="negative_offset"),
],
Comment thread
ptoscano marked this conversation as resolved.
)
@pytest.mark.django_db(transaction=True)
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
Comment on lines +279 to +299

@ptoscano ptoscano Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@mkanoor mkanoor Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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.

@ptoscano ptoscano Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.



@pytest.mark.django_db(transaction=True)
async def test_handle_actions_multiple_firing(
ws_communicator: WebsocketCommunicator,
Expand Down