200 OK / HTML content type / empty response body when returning None from a branch route

[glyph]

In the following Klein application:

from klein import route, run

@route("/something", branch=True, strict_slashes=False)
def something(request):
    return None

run("localhost", 8080)

Then GET /something/else will result in a 200 OK with no content.

In fact, GET /something/ and GET /something have the same behavior, unless you set strict_slashes=True, in which case only GET /something/ will return a 404 (GET /something/else and GET /something still return 200s even with strict_slashes=True).

This seems off to me; a None ought to be 404s all around.

[brighid]

Point of information: I was able to reproduce this in version 15.1 with the exact same example code.

[brighid]

I made some time to dig into this, and I think there are actually two issues here.

The first one is "when a branch route returns None, it's a 200 OK response instead of a 404 NOT FOUND." I actually disagree that it should 404. The way I read it is, branch=True is like tacking *args, **kwargs onto the end of a function's arg list: it's saying "add as much you want here, I can handle it." So when you have a @route("/something", branch=True) function, yup, it matches GET /something/else and lets the function do whatever it does. Returning None is a thing functions ever do. If the function never calls request.setResponseCode, the response goes with the default value of t.w.http.Request.code, which is 200 OK. And just like the way a function can return None, an HTTP server can reply with an empty body:

The presence of a message body in a request is signaled by a Content-Length or Transfer-Encoding header field. [...] The presence of a message body in a response depends on both the request method to which it is responding and the response status code. [...] All 1xx (Informational), 204 (No Content), and 304 (Not Modified) responses do not include a message body. All other responses do include a message body, although the body might be of zero length.

So the empty response is definitely counter-intuitive, and the server really should be replying with 204 NO CONTENT, but it's legal. The idea that Klein might keep an eye out for endpoints that return None and twiddle the response codes of those to 404, 204, or anything else, gives me the heebie-jeebies pretty hard. That sounds like adding complexity whose payoff is small and which also creates a new behavior that is magic and is begging for trouble. To me, this falls firmly into the domain of "framework users are allowed to write bad code."

The other issue is that the behavior with branch routes and strict_slashes is inconsistent. I do think that this is a bug, and because strict_slashes controls Werkzeug behavior while branch controls Twisted behavior, I'd confidently bet that they have some impedance mismatch that's causing that bug. I'll sniff around a little at that one, but I recommend that we not change the "if your endpoint returns None and doesn't set a non-200 response code, the server's response will be a 200 OK" behavior.

[brighid]

Actually wait, because strict_slashes causes Werkzeug to do a redirect, I bet the inconsistency bug I was just talking about is the same as https://github.com/twisted/klein/issues/71

[glyph]

Thanks for the further investigation.

I think there's a slight mismatch between Werkzeug's model of the world here and Klein's.

The reason that I filed this bug, and the reason I expected this behavior, is that twisted.web's model of the world is that / is a Resource, and /foo is another Resource, and /foo/bar is another Resource, and so on. Returning None from getChild returns None as a Resource, which is pretty clearly a 404: we tried to find something, instead we found None, i.e. we found nothing, i.e. 404.

The inconsistency here is that Klein can dispatch to a Resource object by returning said Resource object from a route. However, the only way to consistently get the dispatch-to-an-arbitrary-Resource-from-an-arbitrary-route behavior is to make it a branch route. However, I think I buy your analogy to *args, so... maybe this works as expected?

On the other hand, maybe this is a place where we should refuse the temptation to guess, and just say that a None coming back from a route is a 500, and you should always return some valid value. The main reason that allowing a None return value is useful is that it's the default return value of a method, but it's not like there's a lot of utility in declaring routes just to return None from them frequently - if you want a 404 or a 204 or what have you, it's best to be explicit about that.

But since my mind has recently changed here I'm not too sure. Thoughts?

[brighid]

I definitely agree with the part about "there's a mismatch between Werkzeug's model of the world and Klein's." Also I didn't get the distinction about returning something written to the request vs. returning a t.w.r.Resource, thanks for explaining that.

What would happen if we said "because Klein & Werkzeug differ in weltanschauung, declaring a route to be both strict_slashes=True and branch=True is verboten"? This could take the shape of throwing an exception if both are truthy, of ignoring one if the other is set, or just emitting a warning that users may be setting themselves up for counterintuitive behavior. I'm not 100% sure that that would resolve the inconsistency, but I feel obliged to mention that sometimes removing features is the right answer, so I want to gesture in that direction.

I do think, however, that saying "a route returning None is synonymous with setting the response code to 500" is not best; it feels like that is guessing. The default response in t.w.h.Request is 200 and the default return value of a function is None, so if you the framework user don't act to change those values, you get the defaults. The "that's the default, change it if you want different behavior," though, assumes that a user knows both of those facts off the top of their head—and the Twisted one is not expressed with magnificent clarity by the docs, while the Python one is broadly-but-not-universally known (my personal context: just last week I unthinkingly dropped into Lisp mode and assumed that the return value of the innermost function call would just bubble up; I got None instead and had a maddening bug for an hour or so).

Please help me out with a point I'm not quite getting: does branch=True on a route mean "I might return a Resource, but I might just return a string or IRenderable like a non-branching route" or does it mean "I guarantee that I return a Resource"?

Also, do you think this is the same Klein/Werkzeug impedance mismatch bug #71?

[glyph]

I definitely agree with the part about "there's a mismatch between Werkzeug's model of the world and Klein's." Also I didn't get the distinction about returning something written to the request vs. returning a t.w.r.Resource, thanks for explaining that.

Glad to have imparted some useful information, more people who understand the underpinnings here are really needed :-).

What would happen if we said "because Klein & Werkzeug differ in weltanschauung,

OK so "werkzeug" is already stretching it; weltanschauung is a bridge too far :).

declaring a route to be both strict_slashes=True and branch=True is verboten"?

Actually, I think that this makes sense, at least, in the sense that if you have a "collection" Resource, one that has a useful getChild but not a useful render_XXX, then strict_slashes makes perfect sense.

This could take the shape of throwing an exception if both are truthy, of ignoring one if the other is set, or just emitting a warning that users may be setting themselves up for counterintuitive behavior. I'm not 100% sure that that would resolve the inconsistency, but I feel obliged to mention that sometimes removing features is the right answer, so I want to gesture in that direction.

While I am not opposed in principle to removing features, I think that it is quite possible to just make #71 work correctly, since it is not ambiguous what to do when werkzeug raises its redirect exception.

I do think, however, that saying "a route returning None is synonymous with setting the response code to 500" is not best; it feels like that is guessing. The default response in t.w.h.Request is 200 and the default return value of a function is None, so if you the framework user don't act to change those values, you get the defaults. The "that's the default, change it if you want different behavior," though, assumes that a user knows both of those facts off the top of their head—and the Twisted one is not expressed with magnificent clarity by the docs, while the Python one is broadly-but-not-universally known (my personal context: just last week I unthinkingly dropped into Lisp mode and assumed that the return value of the innermost function call would just bubble up; I got None instead and had a maddening bug for an hour or so).

Consider this "frustration matrix" on the potential consequences here, though:

	None returns 404	None returns empty string	None returns 500
I expect None to be an error page/404	OK	WTF where is my response	oh I guess that's an error in [function from error page], edit code to set response code 404 / return custom error page
I expect None to be an empty string/200	edit route to return b""	OK	oh I guess that's an error in [function from error page], edit code to return b""
I have no expectations of None; I returned it by mistake.	WTF where is my route	WTF where is my response	oh oops I guess that's an error in [function from error page], edit code to return something

In the case of the 500, you have to do a bit of extra work, but you always immediately know what that extra work is, rather than the other cases where if you didn't happen to already expect the behavior you got, you are baffled. So I think this is by far the least frustrating, even if we assume a uniform distribution of behavior expectations, violating their expectation by explicitly displaying a message / logging an error that says "do something else" will result in only momentary frustration, but failing to return any data at all when the user expected some will result in prolonged investigation and confusion.

Please help me out with a point I'm not quite getting: does branch=True on a route mean "I might return a Resource, but I might just return a string or IRenderable like a non-branching route" or does it mean "I guarantee that I return a Resource"?

The former. You can always return a Resource in Klein, but if you don't use a branch=True route for doing so, you will get (maybe?) surprising behavior where Klein will fail to match any routes rather than call your getChild method with the rest of the path.

Also, do you think this is the same Klein/Werkzeug impedance mismatch bug #71?

They're definitely related, but no; my understanding (I haven't re-verified it just now) is that None from any route is presently an empty response / 200 error code, it's just most surprising to me on branch routes due to my explanation above; #71 is much more specifically about redirects.