feat(client): return created Event(s) from insert_event/insert_events#104
feat(client): return created Event(s) from insert_event/insert_events#104TimeToBuildBob wants to merge 1 commit intoActivityWatch:masterfrom
Conversation
The server already returns the created event(s) with server-assigned IDs in the response body, but the client was discarding this data. This makes it possible for callers to get the event ID immediately after insertion, enabling direct use in delete_event/get_event without a separate lookup. Closes ActivityWatch#77
|
@greptileai review |
Greptile SummaryThis PR changes Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant AWClient
participant AWServer
Note over Caller,AWServer: insert_event (before PR) — returns None
Caller->>AWClient: insert_event(bucket_id, event)
AWClient->>AWServer: POST /buckets/{id}/events [event]
AWServer-->>AWClient: 200 OK [created_event_with_id]
AWClient-->>Caller: None (response discarded)
Note over Caller,AWServer: insert_event (after PR) — returns Event with server ID
Caller->>AWClient: insert_event(bucket_id, event)
AWClient->>AWServer: POST /buckets/{id}/events [event]
AWServer-->>AWClient: 200 OK [created_event_with_id]
AWClient-->>Caller: Event(id=server_assigned_id, ...)
Note over Caller,AWServer: insert_events (after PR) — returns List[Event] with server IDs
Caller->>AWClient: insert_events(bucket_id, [e1, e2, e3])
AWClient->>AWServer: POST /buckets/{id}/events [e1, e2, e3]
AWServer-->>AWClient: 200 OK [e1_created, e2_created, e3_created]
AWClient-->>Caller: [Event(id=1,...), Event(id=2,...), Event(id=3,...)]
Last reviewed commit: 666d088 |
| self._post(endpoint, data) | ||
| response = self._post(endpoint, data) | ||
| created = response.json() | ||
| return Event(**created[0]) if created else event |
There was a problem hiding this comment.
Silent fallback returns event without server-assigned ID
When the server returns an empty list, insert_event silently falls back to returning the original event object (which has id=None if not pre-assigned). This breaks the stated contract of the function — callers expect an Event with a server-assigned ID, but they silently receive the input event instead.
Any caller that does client.delete_event(bucket_id, client.insert_event(bucket_id, e).id) would then call delete_event(bucket_id, None), causing a confusing error downstream rather than a clear failure at the insertion step.
The insert_events sibling doesn't have this fallback at all — it just returns [] on an empty response — creating an inconsistency in error handling between the two methods.
A more defensive approach would be to raise a clear error if the server unexpectedly returns an empty list, rather than silently swallowing the failure:
def insert_event(self, bucket_id: str, event: Event) -> Event:
endpoint = f"buckets/{bucket_id}/events"
data = [event.to_json_dict()]
response = self._post(endpoint, data)
created = response.json()
if not created:
raise Exception("Server returned empty response when inserting event")
return Event(**created[0])| def insert_events(self, bucket_id: str, events: List[Event]) -> List[Event]: | ||
| endpoint = f"buckets/{bucket_id}/events" | ||
| data = [event.to_json_dict() for event in events] | ||
| self._post(endpoint, data) | ||
| response = self._post(endpoint, data) | ||
| created = response.json() | ||
| return [Event(**e) for e in created] |
There was a problem hiding this comment.
insert_events silently returns fewer events than inserted
If the server returns fewer events than were passed in (e.g., partial insertion, empty response), the function silently returns a shorter list — including [] for a completely empty response — with no warning or error. The caller has no way to detect that some events were not created.
Consider validating that the returned count matches the input count:
def insert_events(self, bucket_id: str, events: List[Event]) -> List[Event]:
endpoint = f"buckets/{bucket_id}/events"
data = [event.to_json_dict() for event in events]
response = self._post(endpoint, data)
created = response.json()
if len(created) != len(events):
raise Exception(
f"Server returned {len(created)} events, expected {len(events)}"
)
return [Event(**e) for e in created]|
Duplicate of #103 |
|
Closing as duplicate of #103 (already existed, same change). Thanks @ErikBjare for catching this. |
Summary
The server already returns the created event(s) with server-assigned IDs in the response body for both the
POST /buckets/{id}/eventsendpoint, but the Python client was discarding this data by returningNone.This PR changes
insert_eventandinsert_eventsto return the created event(s) with their server-assigned IDs:insert_event(bucket_id, event) -> Event— returns the created event with IDinsert_events(bucket_id, events) -> List[Event]— returns the list of created events with IDsBackward compatible: callers that ignore the return value continue to work unchanged.
Motivation
The main use case (raised in #77) is getting the server-assigned event ID immediately after insertion, so you can directly use it in
delete_event()orget_event()without a separate lookup.Before:
After:
Changes
aw_client/client.py: Updatedinsert_eventandinsert_eventsto capture and return the responsetests/test_client.py: Updated tests to verify returned events have server-assigned IDs; simplified event deletion to use the ID directly frominsert_eventCloses #77