746040 - Clarify spec re: deleted collections behavior

Status: VERIFIED FIXED

(Cloud Services Graveyard :: Server: Sync, defect)

Description

[Gregory Szorc [:gps]]

The spec is not clear about what happens when a collection is deleted by issuing a DELETE /storage/<collection> and subsequently queried with a GET.

According to the server functional tests, the server implementation currently responds with a 200 with the body containing an empty list of items:

  self.app.delete(self.root + '/storage/col2')
  res = self.app.get(self.root + '/storage/col2')
  self.assertEquals(len(res.json["items"]), 0)

This is troubling because I would expect a DELETE followed by a GET on *any URI* to return a 404. In the case of the storage service, I believe we break the idempotence requirement of DELETE because a DELETE on a collection that *never* existed is not the same as a DELETE on a collection that has existed. In the former, the resource is gone with no trace. In the latter, the resource is merely cleared. To be idempotent, they should have the same behavior.

I'm sure there is a reason we want to keep track of deleted collections on the server. Is there a reason we can't return 404 on GET /storage/<collection> after a collection has been deleted? Perhaps we can still expose the collection's deleted time via info/collections and the client can discover from a GET it no longer exists? Perhaps we could add a flag to info/collections to denote deleted collections?

Do we want to change the spec so deleting a collection is really "emptying" and we perform this via POST to preserve HTTP requirements? Or, we could debate the validity of the spec here (I recognize my idempotency argument is weaker than I'd like) and accept status quo?

Whatever we do, the spec needs clarified.

Comment 1

[Richard Newman [:rnewman]]

See Bug 687299. Empty collection 200, []. Deleted collection 404.

Comment 2

[Ryan Kelly [:rfkelly]]

(In reply to Gregory Szorc [:gps] from comment #0)
> This is troubling because I would expect a DELETE followed by a GET on *any
> URI* to return a 404. In the case of the storage service, I believe we break
> the idempotence requirement of DELETE because a DELETE on a collection that
> *never* existed is not the same as a DELETE on a collection that has
> existed. In the former, the resource is gone with no trace. In the latter,
> the resource is merely cleared. To be idempotent, they should have the same
> behavior.

All of this is exactly right, the tests and server are currently broken in this regard.  Bug 687299 has a patch waiting for me to land, I'll do that today.
 
> I'm sure there is a reason we want to keep track of deleted collections on
> the server. Is there a reason we can't return 404 on GET
> /storage/<collection> after a collection has been deleted?

The problem is actually that we *don't* keep track of deleted collections on the server.  Since we can't tell the difference between a deleted collection and an empty collection we default to assuming it's empty.

Changing this in the SQL backend requires some nontrivial work, per Bug 745072.  The Memcached+SQL backend will track them fine once the above-mentioned patch lands.

> Whatever we do, the spec needs clarified.

Given that what you thought should have been happening is, in fact, what is supposed to be happening, do you still feel it needs to be clarified?

Comment 3

[Gregory Szorc [:gps]]

(In reply to Ryan Kelly [:rfkelly] from comment #2)
> Given that what you thought should have been happening is, in fact, what is
> supposed to be happening, do you still feel it needs to be clarified?

I think an extra sentence in the DELETE collection section saying that you get a 404 after a successful request wouldn't hurt!

Comment 4

[Ryan Kelly [:rfkelly]]

Attachment: docs patch to clarify behaviour of delete on collections

Added some docs.  I've also split the DELETE /storage/<collection> call out separately from DELETE /storage/<collection>?ids=<ids>.  Despite using the same method and URL, they really do two separate things and I think it's cleaner to explain them separately.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 616310 [details] [diff] [review]
docs patch to clarify behaviour of delete on collections

Review of attachment 616310 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent!

Comment 6

[Ryan Kelly [:rfkelly]]

https://github.com/mozilla-services/docs/commit/3d79beea6b69e54eb9140dd702983850c733cd93

Comment 7

[James Bonacci [:jbonacci]]

QA can verify this in docs and close.

Comment 8

[James Bonacci [:jbonacci]]

Verified in the Docs: http://docs.services.mozilla.com/storage/apis-2.0.html