Expose replace event endpoint

[leoschwarz]

This is the endpoint I'd propose for ActivityWatch/activitywatch#71 (comment) , I can do it for the Rust version later if you agree on the API.

Missing parts: Test, explicit check if event_id does not exist (I assume it would cause a server error now). I'm not sure if I should bother with this if you plan to abandon this in spite of the Rust server?

[leoschwarz]

Actually I'm not sure if this is the right place to add it looking at aw-server-rust. There isn't a an endpoint to delete single events there either as far as I can see?

Maybe a multi-event update endpoint would be more efficient and I should add that instead?

PUT "/<bucket_id>/events"

and provide as data either:

• a dictionary with keys=ids, values=event dicts
• a list of event dicts (they should all include an id to the value they are to be updated)

[johan-bjareholt]

I can do it for the Rust version later if you agree on the API.

Would appreciate if you did, but if you don't I could do it as well.

Missing parts: Test, explicit check if event_id does not exist (I assume it would cause a server error now). I'm not sure if I should bother with this if you plan to abandon this in spite of the Rust server?

aw-client has function tests which won't be dropped, you could do your testing there if you want instead.
aw-server will be deprecated, but I have no idea if it will take a month or a year before that's the case.

[johan-bjareholt]

This is good enough for single events, but it would be nicer with a way to update and delete multiple events at a time.
We should IMO skip the event_id as a HTTP parameter and simply just make sure that the "id" field is set in all of the events.
Another thing I've considered before is to only allow sending an array of events and don't allow simply just an event straight in the body, that way the API would be more consistent without losing any functionality.

The DELETE endpoint for events is a very badly designed API, please don't use that as an inspiration.

[leoschwarz]

What do you think about merging insertions, updates and deletes in the same endpoint (or just insertions and updates)? I just read https://stackoverflow.com/questions/2421595/restful-way-for-deleting-a-bunch-of-items and it seems it wouldn't fit so neatly into REST API design to delete multiple event resources at once.

Here are the options I can think of, TL;DR point 3 is my suggestion. (Or 1 if you ever plan to add authentication to the api, so there can be different permission for inserting or changing data.)

1. Insertions and updates could be combined together with a POST request taking a list of event dicts: if the id field is present and not null then an event will be updated, if it doesn't have an id yet then it will be created. The response could contain the new ids depending on the feasability of the db infrastructure.
2. Updates and deletes could be combined with a POST (or PUT/PATCH tho these would break REST semantics) endpoint taking a dict, where each key is the id of the to be mutated and the value is null to be deleted or a dict if it has to be replaced.
3. A single endpoint for all 3 operations, maybe POST /<bucket_id>/events, where we take a list of dicts. Each item would be a dictionary with "action": "insert" | "replace/update" | "delete" and "event": ... keys. The problem may be backward compatibility (which could be resolved by renaming the new endpoint or accepting an incompatible change).

[leoschwarz]

4. POST /<bucket_id>/events to insert, POST /<bucket_id>/events/replace to replace (or "update"), POST /<bucket_id>/events/delete to delete. This would be straightforward, separate the concerns and not lead to any issues of semantics of the HTTP verbs.

[johan-bjareholt]

it seems it wouldn't fit so neatly into REST API design to delete multiple event resources at once.

The bucket API matches how DELETE is supposed to work according to the REST design philosophy as each bucket has its own resource. Events don't have their own resource as they are always fetched as a batch and never as a single entity. So the events API does not and cannot match according to how REST is supposed to work so we should not care about that in this case and just create the most clear, easy to use and intuitive API.

(Or 1 if you ever plan to add authentication to the api, so there can be different permission for inserting or changing data.)

We have plans for authentication, but we have never discussed different permissions for different users. Would be nice if we chose a solution where that is possible, but not a requirement IMO.

1. Is a decent idea, but I think the risk of someone mistakenly passing an "id" from a previously copied event and updating an event when expecting to create a new one is pretty high. It also breaks backwards compatibility as we currently just discard the "id" field, but I'm not aware anywhere where we actually pass the "id" field so it should be fine.

2. I don't like at all, seems very unintuitive.

3. Would work, but I'd preferably avoid replacing our old APIs if it's possible. We also miss the REST verbs which previously worked as a easy way to understand how the API worked without even reading any documentation of the API which is nice.

4. This is the best idea out of these 4 IMO. POST /<bucket_id>/events to insert could even in the future be deprecated and succeeded by POST /<bucket_id>/events/insert to be consistent with the others. Completely stopping to use the REST verbs is alright as what they are doing is properly described in their paths. POST on all of them could still though be replaced with POST/PUT/DELETE which would make it even more clear.

5. My fifth suggestion would be POST /<bucket_id>/events for inserts, DELETE /<bucket_id>/events for deleting events and PUT /<bucket_id>/events for replacing events.

So my opinion is that 4 or 5 are the only good options.

4 has the advantage of being the most logical one and easiest to extend. For example I think that our current DELETE API is shit and we could simply add DELETE /<bucket_id>/events/delete_range for deleting events within a timeperiod and DELETE /<bucket_id>/events/delete_list for deleting multiple events with a list of event IDs in the body. It also already fits with our POST /<bucket_id>/heartbeat which is nice.

However, 5 has the advantage of simply being backwards compatible which will save us a lot of development time. Having to deprecate old APIs which are just slightly bad is boring work. Adding the /delete_range and /delete_list would also still work, but it would not be as consistent with the old API.

[johan-bjareholt]

So I guess that my final proposal would be: Use PUT /<bucket_id>/events/replace and don't make any new endpoints as discussed in 4 as that would be excessive work for now. In the future it would be nice though.

I'd like to have @ErikBjare opinion, but he hasn't been on GitHub the past few days so who knows when he'll respond. I think he'll agree with me on this final proposal at least, but you can never be sure.

[johan-bjareholt]

I like option 5 both because of the use of HTTP verbs

HTTP verbs could be used in both 4 and 5 and both break the standard way PUT is supposed to work according to REST so it doesn't really matter.

the minimal changes to the API since we're not really in a place where we should be making breaking changes to the API.

Neither 4 nor 5 breaks the API as long as we only add this endpoint and don't change the rest for now.

However, IMO we should remove our current DELETE endpoint completely because it's pretty much broken by design since it can only support deleting a single event unless we break the API. No one is using it anyway currently as far as I know. That's a completely different topic though, we should probably make a seperate issue for that.

[ErikBjare]

I don't like 4. The distinction between POST and PUT is the difference between insert and update/replace. Having a URL end in /insert is just not what I'd expect to see in a REST API.

But yeah, the delete endpoint might be badly designed, although honestly, I don't see the harm of having single-event endpoints, the only inconvenience is that it adds another REST API route (which would just call the same function as the multi-event variant, but with only one event).

So very little extra code to maintain, and I don't think we should hurry with breaking it, we could just deprecate for now.

[leoschwarz]

@ErikBjare I agree it would be more RESTful (and again I made the mistake initially to suggest PUT instead of PATCH, PATCH would be correct semantically), but the initial problem in question was that there can often be issues when sending/receiving DELETE requests with a body (for a long time it was unspecified whether it is allowed or not), which we would need to delete multiple events at once. We can implement it anyhow and hope it works just fine, however it wouldn't be perfectly RESTful either to have delete take a list of ids.

(See this discussion I mentioned before: https://stackoverflow.com/questions/2421595/restful-way-for-deleting-a-bunch-of-items I find some of the suggestions are overkill for a simple API and I might even prefer simply calling single event delete many times, my suggestion would be make the DELETE endpoint take in the body a list of ids to be deleted.)

[johan-bjareholt]

Having a URL end in /insert is just not what I'd expect to see in a REST API.

No it's not, but like I said before the events endpoint is not really adhering to the usual REST semantics anyway. REST is defined for resources, in this case the bucket is the resource and the events are a part of that resource rather than a resource itself so REST does not make any sense here. I've seen other projects who have had this issue as well and they simply just used POST for all the cases for insert/delete/modify which would be an option.

Also, heartbeats is also a type of insert. It is already there to patch up the fact that a simple insert is not good enough. Maybe we come up with an even better alternative in the future, who knows! For delete I'd expect to have at least two delete endpoints, one for deleting a list of events and one for deleting events between two timeperiods similar to how we get events but they are deleted instead. And it's not a good idea to have two possible inputs for one endpoint which I'll describe why further down.

But yeah, the delete endpoint might be badly designed, although honestly, I don't see the harm of having single-event endpoints, the only inconvenience is that it adds another REST API route (which would just call the same function as the multi-event variant, but with only one event).

The issue is that it creates inconsistency in the API.
For example, currently on our POST /<bucket_id>/events endpoint we allow taking in both a single event as well as a list of events. This is stupid because the difference between sending one event and sending one event in a list is two characters. That's all we save, two characters. What flaws does it bring? The fact that we now have a path that accepts request with different formats. This might sound trivial and not much of a problem, but there's actually already a problem with it: swagger does not support marshalling two possible formats so currently the example in swagger is only the single event insert example which might confuse users of this API.

So very little extra code to maintain, and I don't think we should hurry with breaking it, we could just deprecate for now.

Hurry with breaking it means that we will break less in the future if more users of the API comes. Now is the perfect time as we are not aware of any at all, in the future this will be very hard (or impossible).

however it wouldn't be perfectly RESTful either to have delete take a list of ids.

Yeah, I regret saying the idea of having a DELETE with a body, it's probably better IMO to have a POST /<bucket_id>/events/delete as that adheres to how REST is supposed to work.

(See this discussion I mentioned before: https://stackoverflow.com/questions/2421595/restful-way-for-deleting-a-bunch-of-items I find some of the suggestions are overkill for a simple API and I might even prefer simply calling single event delete many times, my suggestion would be make the DELETE endpoint take in the body a list of ids to be deleted.)

From the two discussions I've been in before about DELETE endpoints with bodies both ended with the agreement that DELETE endpoints with bodies are 99% of the time used in APIs designed by people who either don't understand or simply don't care that the DELETE endpoint is for deleting resources. Screwing over how REST is supposed to work though is fine if it would make more sense to the end user.

[johan-bjareholt]

So with what I wrote before in mind, this would probably be my utopian events API.

• POST /<bucket_id>/events/insert Only takes a list of events, fail if any event hast the "id" property set
• POST /<bucket_id>/events/heartbeat Currently good as it is
• POST /<bucket_id>/events/delete_ids Takes a list of event IDs to delete
• GET /<bucket_id>/events/delete_period Takes parameters for starttime and endtime of the range to delete events
• POST /<bucket_id>/events/update Takes a list of events to update, fail if any event does not have the "id" property set

[johan-bjareholt]

Hello again,
Sorry for starting a long API discussion and not stating any clear way forward.

The switch to aw-server-rust is getting close now, to document the discussions we have here I updated the ActivityWatch/aw-server-rust#60 issue in aw-server-rust and added the ActivityWatch/aw-server-rust#115 issue. Please comment on them with feedback, would like to have some opinions on them.

I do however not feel like it's worth merging this PR if aw-server-python will be dropped so soon unfortunately. if you have any clear use-case for this new endpoint that you have right now I will prioritize the aw-server-rust issues so they get done somewhat soon.

[leoschwarz]

Thanks for your continued efforts and sorry from my side for never following up on our discussion here, I'm working on other projects at this time so feel free to priorize as you like I don't need the API (and if I really needed it I know Rust myself, definitely better to have just one server implementation while the API is still evolving).