Closed Bug 1106562 Opened 10 years ago Closed 9 years ago

Delete multiple rooms in a single invocation to Loop server

Categories

(Hello (Loop) :: Server, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: oteo, Assigned: rhubscher)

References

Details

(Whiteboard: [loop-server 0.15.0])

Attachments

(2 files)

We have a User Story requested by Product team to implement multiple selection and deletion of Rooms (Bug 1106555) and we would need deleting several created rooms in a single invocation to Loop server.
Otherwise, this action will take longer (as we have to delete each room individually) and user could be waiting for it for a long time in case many rooms are selected
Blocks: 1106555
Whiteboard: [loop-server 0.15.0]
Assignee: nobody → rhubscher
In order to handle this I am making this API proposal: https://wiki.mozilla.org/Loop/Architecture/Rooms#DELETE_.2Frooms

Does this looks good to you? Instead of the "rooms" mapping key, would you rather prefer "roomTokens" or "tokens"?
Flags: needinfo?(alexis+bugs)
Flags: needinfo?(adam)
(In reply to Rémy Hubscher (:natim) from comment #1)
> In order to handle this I am making this API proposal:
> https://wiki.mozilla.org/Loop/Architecture/Rooms#DELETE_.2Frooms
> 
> Does this looks good to you? Instead of the "rooms" mapping key, would you
> rather prefer "roomTokens" or "tokens"?

"roomTokens" would be more consistent with the rest of the interface, so I'd prefer to go with that.

I've had feedback on our use of DELETE being out of the spirit of that verb from an HTTP perspective in some places -- notably, those that refer to a resource that then continues to exist (in this case, we're calling DELETE on "/rooms" but subsequent operations on "/rooms" don't return 404's or 410's). On the other hand, we've already established this pattern for "/registration", "/account", and "/session", so I suppose that ship has sailed.
Flags: needinfo?(adam)
(In reply to Adam Roach [:abr] from comment #2)
> "roomTokens" would be more consistent with the rest of the interface, so I'd
> prefer to go with that.

Ok

> I've had feedback on our use of DELETE being out of the spirit of that verb
> from an HTTP perspective in some places -- notably, those that refer to a
> resource that then continues to exist (in this case, we're calling DELETE on
> "/rooms" but subsequent operations on "/rooms" don't return 404's or 410's).

Yes you are right, I though about the meaning of DELETE on a collection and it is not as REST as I wanted it to be.

Would you prefer to use PATCH /rooms for this purpose? It makes more sense.

> On the other hand, we've already established this pattern for
> "/registration", "/account", and "/session", so I suppose that ship has
> sailed.

True.
Actually it appears that sending body request on the DELETE verb is not a good idea. We already had problems with that for loop.

I propose to use PATCH /rooms with a body of the form {"deleteRoomTokens": [<list of room tokens to delete>]}
Attachment #8542562 - Flags: review?(alexis+bugs)
Attachment #8542562 - Flags: feedback?(mathieu)
Flags: needinfo?(alexis+bugs)
How should we handle errors?

What happen if one of the token doesn't exists anymore? If the user doesn't owns it?

What I did in this patch is to ignore invalid tokens if at least one of them is valid (exists and is owned by the user) and I return a 404 if none of them are found for the user.

Mathieu proposed to return a 400 in that case. What do you guys think?
Flags: needinfo?(oteo)
Flags: needinfo?(alexis+bugs)
Flags: needinfo?(adam)
Attached file Update documentation
Attachment #8542574 - Flags: review?(alexis+bugs)
(In reply to Rémy Hubscher (:natim) from comment #4)
> Actually it appears that sending body request on the DELETE verb is not a
> good idea. We already had problems with that for loop.
> 
> I propose to use PATCH /rooms with a body of the form {"deleteRoomTokens":
> [<list of room tokens to delete>]}

Thinking of /rooms as a collection and PATCHing it seems like a good way to model this.

(In reply to Rémy Hubscher (:natim) from comment #6)
> How should we handle errors?
> 
> What happen if one of the token doesn't exists anymore? If the user doesn't
> owns it?
> 
> What I did in this patch is to ignore invalid tokens if at least one of them
> is valid (exists and is owned by the user) and I return a 404 if none of
> them are found for the user.
> 
> Mathieu proposed to return a 400 in that case. What do you guys think?

In general, when you have multiple operations in a single transaction, there are two ways to handle reporting status: (a) have all operations succeed or fail monolithically, and report a single status that reflects whether all operations succeeded or failed, or (b) return a response with a body containing the status for each operation.

I was assuming we'd be doing (a) here (so, e.g., if you sent four tokens but owned only three, then no rooms would be removed and you'd get an error), but it sounds like you want a system with partial success, which pushes us towards (b). Since this operation is basically an optimization, that seems reasonable.

There's already a model for this in HTTP, developed for operations on WebDAV collections. It would make sense that we should re-use some of these mechanics, such as the "multi-status" response code. Given that we're using JSON everywhere, it probably doesn't make sense to go all the way to the WebDAV XML multi-status bodies. This means that a completely successful response to the example you have in the docs would look something like this:

> HTTP/1.1 207 Multi-Status
> Connection: keep-alive
> Date: Wed, 16 Jul 2014 13:12:46 GMT
> Server-Authorization: <stripped>
> 
> {
>   "responses": {
>     "_nxD4V4FflQ": { code: 200 },
>     "_xaB2Z5GdTV": { code: 200 }
>   }
> }

While a partial success would look something like this:

> HTTP/1.1 207 Multi-Status 
> Connection: keep-alive
> Date: Wed, 16 Jul 2014 13:12:46 GMT
> Server-Authorization: <stripped>
> 
> {
>   "responses": {
>     "_nxD4V4FflQ": { code: 200 },
>     "_xaB2Z5GdTV": { code: 403, errno: 120, message: "Not room owner" },
>   }
> }

Note that this is re-using the code/errno/message structure that we return in normal error messages.
Flags: needinfo?(adam)
Ok that makes a lot of sense. Thank you.
For clarity, an operation on several rooms where each room failed individually would still use a 207:

> HTTP/1.1 207 Multi-Status 
> Connection: keep-alive
> Date: Wed, 16 Jul 2014 13:12:46 GMT
> Server-Authorization: <stripped>
> 
> {
>   "responses": {
>     "_nxD4V4FflQ": { code: 404, errno: 105, message: "Room not found" },
>     "_xaB2Z5GdTV": { code: 403, errno: 120, message: "Not room owner" },
>   }
> }

(That is, the patch operation is okay, but each sub-operation failed)

But if the server doesn't get as far as trying each room (e.g., because the user authentication is bad), then you'd send an error to the PATCH operation:

> HTTP/1.1 401 Unauthorized
> Connection: keep-alive
> Date: Wed, 16 Jul 2014 13:12:46 GMT
> 
> { code: 401, errno: 110 }
Option B makes sense to me, although in the Loop Mobile Client we don't expect these situations to happen as we expect to be always in sync. Anyhow is always good to be in the safe side.
Flags: needinfo?(oteo)
I have updated the documentation accordingly: https://wiki.mozilla.org/Loop/Architecture/Rooms#PATCH_.2Frooms
Flags: needinfo?(alexis+bugs)
I have also updated the PR.
Comment on attachment 8542562 [details]
Link to Github PR — #272.

I've added a few comments on the github PR. Once adressed, this can land :)
Attachment #8542562 - Flags: review?(alexis+bugs) → review+
Actually, just having "responses" as a response to a PATCH seems not enough to me.

For instance, in case we ask in the PATCH to DELETE *and* UPDATE some information, the answer from the server isn't really helpful.

I propose we should rename the `responses` attribute by several ones, depending what actually hapenned (in this case, `deleted`).
Comment on attachment 8542562 [details]
Link to Github PR — #272.

Putting back the review flag here since we need to change `responses` to something else.
Attachment #8542562 - Flags: review+ → review?(alexis+bugs)
Comment on attachment 8542574 [details] [review]
Update documentation

The documentation looks good to me.
Attachment #8542574 - Flags: review?(alexis+bugs) → review+
The specification we are borrowing the Multi-Status response field specs we should use "reponses", but I feel we are losing some information with it.

https://tools.ietf.org/html/rfc4918#section-13

Adam, do you have thoughts about that?
Flags: needinfo?(adam)
We didn't plan to support multiple edit here. But if you'd like to be future proof, maybe we could describe something like this: https://github.com/mozilla-services/readinglist/wiki/API-Design-proposal#post-batch

The request being:

[
  {
    "method" : "DELETE",
    "path" : "/rooms/_nxD4V4FflQ"
  },
  {
    "method" : "DELETE",
    "path" : "/rooms/_xaB2Z5GdTV"
  }
]

And the response being:

[
  {
    "path" : "/rooms/_nxD4V4FflQ",
    "status": 200
  },
  {
    "path" : "/rooms/_xaB2Z5GdTV",
    "status": 404,
    "body" : {
      "code": 404,
      "errno": 105,
      "message": "Room not found"
    }
  }
]
(In reply to Alexis Metaireau (:alexis) from comment #15)
> Actually, just having "responses" as a response to a PATCH seems not enough
> to me.
> 
> For instance, in case we ask in the PATCH to DELETE *and* UPDATE some
> information, the answer from the server isn't really helpful.

Presumably, the client knows what it asked for -- success means "the thing you requested has been successfully executed." There's really not any more ambiguity here than there is with normal HTTP requests.

For now, I'd propose that we stay with the design described in comment 8 and comment 10. If we later discover that we need something more general, then we can move to the approach that Rémy describes in comment 19.
Flags: needinfo?(adam)
(In reply to Rémy Hubscher (:natim) from comment #12)
> I have updated the documentation accordingly:
> https://wiki.mozilla.org/Loop/Architecture/Rooms#PATCH_.2Frooms

I'd like to more closely model the RFC4918 approach here.

Look, for example, at the HTTP transaction shown in http://tools.ietf.org/html/rfc4918#section-9.1.4

This represents two operations, and so the multistatus body contains two <response> elements. Both elements are success ("200 OK").

However, the *overall* response code is "HTTP/1.1 207 Multi-Status", *even* *though* all of the operations were successful.

Make sense?
Flags: needinfo?(rhubscher)
(In reply to Adam Roach [:abr] from comment #21)

> However, the *overall* response code is "HTTP/1.1 207 Multi-Status", *even*
> *though* all of the operations were successful.
> 
> Make sense?

Yes ok.
Flags: needinfo?(rhubscher)
Thanks Adam, let's go down this path then. The case I was considering "not clear enough" is when we ask to do different actions (edit and delete for instance) on the same resource. But this probably should return a 400 anyway, since doing two different (non compatible) actions on the same resource doesn't make a lot of sense.

Also, in case we find we need to update the protocol, we could do it in a backward-compatible way anyway so that makes sense to stay with the proposal made in comment 8 and comment 10.
Comment on attachment 8542562 [details]
Link to Github PR — #272.

Code changes are at https://github.com/mozilla-services/loop-server/commit/cfe2bcf6b8008c9eabdd5b53688594980c74212c

This has reflects the decisions and design of last comments of this thread.

Clearing the feedback info for Mat (please tell me if I shouldn't).
Attachment #8542562 - Flags: review?(alexis+bugs)
Attachment #8542562 - Flags: review+
Attachment #8542562 - Flags: feedback?(mathieu)
Documentation updated at https://github.com/mozilla-services/docs/commit/d1ccd721f746f8f5cfeb3920a0adc9e65204960d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 1104764
Blocks: 1125058
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: