Skip to content

Expose replace event endpoint #57

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

Closed
leoschwarz wants to merge 1 commit into from
Closed

Expose replace event endpoint #57

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
7 changes: 6 additions & 1 deletion aw_server/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,15 @@ def get_eventcount(self, bucket_id: str,
return self.db[bucket_id].get_eventcount(start, end)

@check_bucket_exists
def delete_event(self, bucket_id: str, event_id) -> bool:
def delete_event(self, bucket_id: str, event_id: int) -> bool:
"""Delete a single event from a bucket"""
return self.db[bucket_id].delete(event_id)

@check_bucket_exists
def replace_event(self, bucket_id: str, event_id: int, event: Event) -> bool:
"""Replace the specified event by a new one."""
return self.db[bucket_id].replace(event_id, event)

@check_bucket_exists
def heartbeat(self, bucket_id: str, heartbeat: Event, pulsetime: float) -> Event:
"""
Expand Down
13 changes: 13 additions & 0 deletions aw_server/rest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,19 @@ class EventResource(Resource):
# events = current_app.api.get_events(bucket_id, limit=limit, start=start, end=end)
# return events, 200

@api.expect(event)
def put(self, bucket_id, event_id):

@johan-bjareholt johan-bjareholt Jun 29, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 leoschwarz Jun 29, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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 leoschwarz Jun 29, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

  1. 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 johan-bjareholt Jun 30, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 johan-bjareholt Jun 30, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 johan-bjareholt Jul 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 ErikBjare Jul 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 leoschwarz Jul 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@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 johan-bjareholt Jul 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 johan-bjareholt Jul 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

data = request.get_json()
logger.debug("Received put request for event with id '{}' in bucket '{}' and data: {}".format(event_id, bucket_id, data))

if isinstance(data, dict):
event = Event(**data)
else:
raise BadRequest("Invalid PUT data", "")

event = current_app.api.replace_event(bucket_id, event_id, event)
return event.to_json_dict() if event else None, 200

@copy_doc(ServerAPI.delete_event)
def delete(self, bucket_id, event_id):
logger.debug("Received delete request for event with id '{}' in bucket '{}'".format(event_id, bucket_id))
Expand Down