| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-sdks - 2018-12-13 | |||
| 16:14:36 | elmiko | i think the only weird part to me is how long should that behavior persist after the delete? | |
| 16:14:54 | kmalloc | Shrews: now that my massive headache of doom has passed, i should be able to look at dogpile and the other issues outstanding | |
| 16:15:02 | elmiko | like, if i delete a resources ages ago, when should the server respond with a 404? | |
| 16:15:04 | kmalloc | Shrews, mordred: ^ thanks for bearing with me yesterday. | |
| 16:15:34 | elmiko | or should it ever? which implies keeping track of delete resources for the purposes of delete responses | |
| 16:15:54 | edleafe | RFC 7231 says that DELETE is idempotent: https://tools.ietf.org/html/rfc7231#section-8.1.3 | |
| 16:15:56 | dtantsur | tracking deleting resources is quite a change to many database models | |
| 16:16:41 | dtantsur | edleafe: that's one of the reasons HTTP is not a great match for API.. | |
| 16:17:03 | kmalloc | edleafe: oh good to know. | |
| 16:17:15 | kmalloc | edleafe: also... damn that means keystone has messed up a bunch. | |
| 16:18:07 | elmiko | edleafe: such a weird knot this is, it should be idempotent, but does that mean it should always return some 2XX for a previously deleted resource? | |
| 16:18:33 | elmiko | i mean, i know that's what we are proposing i'm just exploring the idea a little more | |
| 16:18:40 | dtantsur | yeah, this is a weird point | |
| 16:19:11 | elmiko | definitely | |
| 16:19:22 | edleafe | The impetus for adding this guideline is that it is assumed that an API has one implementation, but many, many consumers. Attempting to delete a resource that doesn't exist requires an exception handler. If you can return a 404 from the API, every single consumer of that API has to build code to handle that | |
| 16:19:36 | edleafe | elmiko: why not? | |
| 16:19:54 | edleafe | "I don't want resource foo to exist" "OK, it doesn't exist" | |
| 16:20:06 | dtantsur | "In effect, this method is similar to the rm command | |
| 16:20:09 | dtantsur | in UNIX: it expresses a deletion operation on the URI mapping of the | |
| 16:20:10 | dtantsur | origin server rather than an expectation that the previously | |
| 16:20:12 | dtantsur | associated information be deleted. | |
| 16:20:21 | dtantsur | I don't know how to interpret it.. | |
| 16:20:25 | elmiko | edleafe: it just makes me wonder what the impl starts to look like, for example do i need to keep a database table of delete resources to properly respond to all future delete requests? | |
| 16:20:40 | edleafe | elmiko: why? | |
| 16:20:40 | dtantsur | edleafe: the "very single customer" is simply incorrect | |
| 16:20:53 | elmiko | if i delete some resource /foo/bar | |
| 16:21:09 | dtantsur | I personally very rarely catch 404 from deletions because I *want* to know that something went south | |
| 16:21:15 | elmiko | then later a request comes in to DELETE /foo/bar, then i need to respond 2XX if it has been deleted. is that accurate? | |
| 16:22:25 | edleafe | elmiko: if you call DELETE /foo/bar, and `foo` is a valid resouce class, you return 204 whether or not you had to delete resource `bar` or not. You don't need to confirm that `bar` ever existed. | |
| 16:23:09 | edleafe | dtantsur: that's very different than "I want this resource gone" | |
| 16:23:31 | edleafe | dtantsur: if you care that it's there and getting deleted, do a GET/HEAD on it first | |
| 16:23:33 | dtantsur | well, "I want this resource gone" is not the semantic I use, so no wonder :) | |
| 16:23:38 | elmiko | edleafe: what if "bar" never existed as a foo? | |
| 16:23:46 | elmiko | shouldn't that be a 404? | |
| 16:23:50 | edleafe | elmiko: No | |
| 16:23:58 | dtantsur | edleafe: so instead of handling 404 by some consumers, some other consumers have to make 2 requests? | |
| 16:24:03 | elmiko | ah, i misunderstood you then | |
| 16:24:13 | edleafe | if you interpret delete as "I don't want resource bar to exist" | |
| 16:24:28 | dtantsur | then we should allow 'DELETE /some/nonsense' :) | |
| 16:24:46 | dtantsur | half-kidding, but the same logic can be applied to invalid URLs | |
| 16:24:47 | edleafe | If you care about the existence of a resource, you call GET or HEAD | |
| 16:24:59 | kmalloc | it's fine to say DELETE X and if the path is unrouted, get a 404 | |
| 16:25:08 | kmalloc | afaict. | |
| 16:25:14 | kmalloc | acutally | |
| 16:25:14 | kmalloc | no | |
| 16:25:15 | dtantsur | kmalloc: why? I want it to be gone, and it's gone | |
| 16:25:18 | edleafe | dtantsur: cdent clarified that. Just what kmalloc said | |
| 16:25:20 | kmalloc | that would be a method not allowed | |
| 16:25:31 | kmalloc | sorry, wrong error | |
| 16:25:31 | dtantsur | edleafe: not for me | |
| 16:25:36 | kmalloc | not 404, 40.. 405? | |
| 16:25:42 | elmiko | kmalloc: if it's possible that the resource _could_ have existed, then it should 404. if i understand edleafe's point | |
| 16:25:48 | elmiko | er should not! | |
| 16:25:49 | elmiko | sorry | |
| 16:25:53 | kmalloc | elmiko: right | |
| 16:26:13 | kmalloc | elmiko: it should be .. 200/204/2XX (not 201) | |
| 16:26:18 | elmiko | right | |
| 16:26:28 | edleafe | Generally 204 is returned from DELETE | |
| 16:26:41 | kmalloc | if delete is not allowed, e.g. the whole path is unrouted in an api | |
| 16:26:44 | edleafe | There is no response body | |
| 16:26:54 | kmalloc | XXXX/<resource> -> where XXXX is not a thing at all | |
| 16:26:56 | kmalloc | that should be 405 | |
| 16:27:03 | kmalloc | as DELETE is not supported | |
| 16:27:18 | kmalloc | but if XXXX is a thing, and there could be a resource under it | |
| 16:27:19 | edleafe | kmalloc: according to cdent, it's 404 | |
| 16:27:20 | dtantsur | what about GET XXXX? | |
| 16:27:44 | kmalloc | edleafe: in most of openstack 404 is a thing. | |
| 16:27:47 | cdent | sorry, I'm debugging a gate failure | |
| 16:27:48 | kmalloc | most of openstack is wrong | |
| 16:27:59 | kmalloc | from a standpoint of the RFC | |
| 16:28:12 | edleafe | dtantsur: GET /XXXX should also be 404 | |
| 16:28:16 | cdent | only if XXXXX/<resource> has any methods at all should it ever be a 405 on delete | |
| 16:28:26 | cdent | if it simply does not exist then it should be 404 | |
| 16:28:35 | kmalloc | hm. | |
| 16:28:43 | kmalloc | i could see that interpretation | |
| 16:28:47 | dtantsur | kmalloc: well, we're talking of taking an RFC designed for hypertext and applying it to API | |
| 16:28:51 | kmalloc | order of precidence on routing | |
| 16:28:52 | cdent | kmalloc: that interpretation is right :) | |
| 16:28:53 | edleafe | From the front page of our guidelines: "Note Where this guidance is incomplete or ambiguous, refer to the HTTP RFCs—RFC 7230, RFC 7231, RFC 7232, RFC 7233, RFC 7234, and RFC 7235—as the ultimate authority." | |
| 16:28:54 | dtantsur | this has gone wrong many times, this is no exception | |
| 16:28:58 | cdent | the URI is the primary thing | |
| 16:29:01 | kmalloc | XXX is checked before XXXX/YYYY | |
| 16:29:05 | kmalloc | so, XXX hits 404 | |
| 16:29:09 | kmalloc | sure. | |
| 16:29:10 | cdent | if there is _nothing_ that has that URI, then 404 | |
| 16:29:19 | cdent | if you can GET it, but can't delete it, then 405 | |
| 16:29:54 | kmalloc | but a 404 on a delete where a resource could exist is explictly wrong | |
| 16:30:00 | kmalloc | it's always a success if delete could work | |
| 16:30:10 | kmalloc | cdent: ++ don't have to convince me too hard on that front :P | |
| 16:30:14 | cdent | that's effectively what ed's thing say | |
| 16:30:25 | edleafe | kmalloc: that's what the point of https://review.openstack.org/#/c/616610/ is | |
| 16:30:31 | edleafe | jinx | |
| 16:30:34 | kmalloc | hehe | |
| 16:30:40 | cdent | if this is a valid uri, and there might have been a thing here, or ever will be, respond 204 on delete when it is not there, or has just been removed | |
| 16:30:48 | kmalloc | yep. | |
| 16:30:51 | cdent | but if the url is invalid because you can't even GET there, then 404 | |
| 16:30:56 | cdent | s/even/ever/ | |
| 16:30:58 | kmalloc | yes. | |
| 16:31:07 | kmalloc | i could see that, i'll have to go poke and see how flask handles it | |
| 16:31:13 | kmalloc | i think it does it the correct way | |