for-GET / http-decision-diagram

An activity diagram to describe the resolution of HTTP response status codes, given various headers.
https://github.com/for-GET/http-decision-diagram/blob/master/doc/2013-06-10-http-hell-no.md
Apache License 2.0
3.61k stars 196 forks source link

Path to 400 Bad Request on POST creation #44

Closed xpe closed 9 years ago

xpe commented 9 years ago

This is a question about understanding the path (i.e. the logical flow) to generate a 400 Bad Request when creating a resource via POST. (Note: I've referred to the Omnigraffle document in writing this; if it is not up-to-date, I would be missing some new information.)

First, I want to mention some relevant creation (i.e. the "create" section) behaviors from the decision diagram:

It seems pretty clear that resource creation must happen in create. But as I show above, if a POST resource creation fails, the decision diagram does not provide an opportunity for a 400 Bad Request response.

Next, I want to figure out where client validation should take place in order to generate a 400. If what I've written above is accurate, then resource validation must happen before the "create" block. So, looking around, there are only two prior checks that can result in 400 Bad Request: from_content and is_request_block_ok.

Is my logic accurate? Did I miss anything?

Now, an opinion: if I'm right, it seems unfortunate that is_request_block_ok is the correct place to do POST payload validation since it happens so much later in the decision diagram. I would expect POST-related logic to be 'closer' together.

For the point of argument, why not make a POST request's payload processing happen during the 'create' block? That would offer the advantage of letting the 'accept' block run beforehand -- why do the creation if the acceptance criteria are not met?

andreineculau commented 9 years ago

It seems pretty clear that resource creation must happen in create. But as I show above, if a POST resource creation fails, the decision diagram does not provide an opportunity for a 400 Bad Request response.

On the contrary, it allows for anything. All, and even more so the status codes returned by is_*_ok and 500 status codes are indicative. If you know what you're doing, the implementation should allow help you to do it, bypassing the diagram. If for some reason in from_content you want to set status code 404, because "you know what you're doing", then you should be able to do that (and ideally, also short circuit the flow; I don't know if the public implementation has it, but the steps should be queued and the queue should be available for modification on the flow).

So in create you should be able to set the status code to 422 (Unprocessable entity) rather than 400 (I hope you'll understand why), and return false (which would imply 500 by default, but will not overwrite the already set 400)

Now, an opinion: if I'm right, it seems unfortunate that is_request_block_ok is the correct place to do POST payload validation since it happens so much later in the decision diagram. I would expect POST-related logic to be 'closer' together.

The request block looks at the request from a pure syntactical/meta perspective - does it make sense when I check how it looks? It says the payload is application/json, can I parse the payload with a JSON parser? It says the payload is 1 gig, do I have resources to handle that? etc etc

The create/process blocks deal with the semantics only, just like you expect.

Makes more sense?

andreineculau.com http://www.andreineculau.com/

xpe commented 9 years ago

Thanks for the write-up!

Now, back to my central remaining concern (handling a POST with an incorrect payload).

If you know what you're doing, the implementation should allow help you to do [what you want], bypassing the diagram."

I agree that configurability is good. However, I think what I'm describing (a POST request being rejected for client errors due to say validation) is very common. I don't think it should require bypassing or changing the diagram. I do wonder if there should be an explicit decision point that would bail out to a 422 Unprocessable Entity.

Also, I wanted to check my assumption on something. In my understanding of the decision diagram (and how we're implementing it in Clojure*) the sideways squares are decision points and only return true or false. (I hope this is aligned with what you intended, because it is the normal way to interpret a flow chart.)

* My company is interested in open sourcing it after we kick the tires on a real project.

P.S. I didn't understand this part that you wrote (but I'm not sure that it is central for me right now):

If for some reason in from_content you want to set status code 404, because "you know what you're doing", then you should be able to do that (and ideally, also short circuit the flow; I don't know if the public implementation has it, but the steps should be queued and the queue should be available for modification on the flow).

andreineculau commented 9 years ago

On Thu, Mar 19, 2015 at 6:04 PM, David James notifications@github.com wrote:

I agree that configurability is good. However, I think what I'm describing (a POST request being rejected for client errors due to say validation) is very common. I don't think it should require bypassing or changing the diagram. I do wonder if there should be an explicit decision point that would bail out to a 422 Unprocessable Entity.

Hmm, it has been a while since 422 was dealt with in httpdd, so I don't remember the full chain of thought. One of the factors that contributed to not having 422 specifically added is that 422 is both applicable to all methods that can take a request payload, and is also very much internalized. what I mean by the latter is that many (most?) times you cannot do a dry-run create just to return false to some 422 callback, but instead you call some core function, with the goal of creating and fails in the middle saying that x doesn't make sense. With that in mind, a 422 callback would mostly be left alone (i.e. always let it pass), and the create callback would run, and maybe pass, maybe fail to process the request, maybe fail with internal error, maybe, maybe, maybe...

This is not hard science, and I'm not going to argue that the current httpdd version is "right" and it has to stay like it is now.. On the contrary. I'd open an issue on having 422 explicit and we can digest this better in time.

Also, I wanted to check my assumption on something. In my understanding of the decision diagram (and how I'm implementing it in Clojure*) the sideways squares are decision points and only return true or false. (I hope this is aligned with what you intended, because it is the normal way to interpret a flow chart.)

Is there any diamond that doesn't return true/false ? Each diamond should call 1 boolean callback (internal or not), and running 1+ callbacks.

  • My company is interested in open sourcing it after we kick the tires on a real project.

Great to hear. I'm willing to sign an NDA if necessary, if that gives me access to a WIP clojure repo, and if there's interest on your side of course.

P.S. I didn't understand this part that you wrote (but I'm not sure that it is central for me right now):

If for some reason in from_content you want to set status code 404, because "you know what you're doing", then you should be able to do that (and ideally, also short circuit the flow; I don't know if the public implementation has it, but the steps should be queued and the queue should be available for modification on the flow).

Merely saying that anywhere in the callbacks, you should be able to set status code, headers, etc, and in the future also fiddle with the flow at run-time.