fix(api): return inserted event with server-assigned ID from single insert; return [] from bulk - #158
Conversation
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to fbc2dee in 8 seconds. Click for details.
- Reviewed
95lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_1jzMav81hfKfWbAq
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
@greptileai review |
Greptile SummaryThis PR successfully aligns the Python server's Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 5273dc3 |
But are the returned events any different than the sent events then? Is this why Johan may have removed it? Performance optimization that made this infeasible/unjustifiable? |
|
Good question. The answer depends on the insert path: Single event: Yes, the returned event is different — it has the database-assigned Bulk insert: The returned events are effectively the same as sent, because peewee's
So Johan's removal was justified for the bulk case — returning the same events you sent without IDs isn't useful. My PR preserves this distinction: single events get the real ID, bulk returns the input list (which is arguably worse than returning nothing since it gives a false sense of having IDs). Suggestion: I can change the bulk path to also return What's your preference? |
This part makes sense, I guess the insert_events (many) should still not return the event ID. While the insert_event (single) could be a supported case without any performance impact. See discussion in ActivityWatch/aw-client#103 |
|
@TimeToBuildBob See my comment #158 (comment) |
…-rust)
The POST /api/0/buckets/{id}/events endpoint previously returned either a
single event dict or null, inconsistent with aw-server-rust which always
returns a list of events with server-assigned IDs.
Changes:
- api.py: create_events() now returns List[Event], using insert_one for
single events (to get ID) and insert_many for bulk
- rest.py: Always serialize response as a list of event dicts
- tests: Add test_insert_event_returns_list and test_insert_events_returns_list
This enables aw-client to use response.json()[0] uniformly against both
server implementations (see ActivityWatch/aw-client#103).
…edback) Previously returned the same events without IDs, which gave false sense of having IDs.
5273dc3 to
73dfafe
Compare
|
Updated per your comment: bulk insert now returns |
|
The only failing CI check is I've held off pushing the fix autonomously given the AW PR policy, but it's a one-liner when you're ready: # current (black dislikes):
For bulk inserts, returns empty list (events may not have IDs without a response-SQL roundtrip)."""
# needed:
For bulk inserts, returns empty list (events may not have IDs without a response-SQL roundtrip).
"""Let me know if you'd like me to push it. |
|
@TimeToBuildBob Address the failing CI and rename the PR title+desc to reflect what it does and why. Also see/deal with/ping with context on ActivityWatch/aw-client#103 |
|
Done:
|
|
CI-green and mergeable (Greptile 5/5) — waiting only on a maintainer click. This PR is ready to merge, but the bot has pull-only access to this repo and can't self-merge — surfacing it here so it isn't lost. The monitoring loop will stop re-flagging it now that this note is posted. |
What and why
POST /api/0/buckets/{id}/eventspreviously returned inconsistent responses:nullThis mismatches aw-server-rust (which always returns a list) and blocked aw-client from
surfacing server-assigned event IDs to callers (ActivityWatch/aw-client#103).
This PR fixes the Python server response format:
{...}(dict, no ID guarantee)[{"id": 1, ...}](list with server-assigned ID)null[](empty list — SQLite bulk insert can't return IDs efficiently)The single-event path uses
insert_one()which does return the assigned ID;the bulk path returns an empty list rather than the input events (which would be
misleading since they lack IDs).
Changes
api.py:create_events()returnsList[Event]—insert_onefor single (gets ID),insert_manyfor bulk (returns[])rest.py: serializes response as a list of event dictstests/test_server.py: two new tests verifying list response for single and bulk insertsUnblocks
ActivityWatch/aw-client#103 — which adds
insert_event()→Optional[Event]return valueso callers can retrieve the server-assigned ID for subsequent update/delete.