HTTP response setStatus has API problem with non-standard/proposed status codes

I literally *just* ran into this same exact problem. In our case, we want to use non-standard status codes that are outside of the http spec. Although the reason we specify gets used, the code is always set to 500.

The fix is simple, just change the following:

        else:
            if type(status) in StringTypes:
                status = status.lower()
            status = status_codes.get(status, 500)
        self._status = status

to (note indentation)

        else:
            if type(status) in StringTypes:
                status = status.lower()
                status = status_codes.get(status, 500)
        self._status = status

This is a major problem for me right now, so I'm going to fix it immediately.

+1.

Thanks.

+1

Paul Carduner, Stephan Richter and I talked about this. Paul and Stephan prefer the approach Paul describes in his comment. I (and the co-worker who encountered this issue) prefer the solution I initially proposed.

I think both sides have a reasonable argument, and we didn't come to a resolution. Community (esp. papal) input would be appreciated. While each side prefers different approaches, both sides agree that either approach is preferable to what we have now.

I'll try to summarize what I believe to be the most compelling points of the two arguments.

I feel that the common case is using standard status codes. Usually, specifying a non-standard status code *is* an error, and should be treated as such. We then want an extension mechanism for the exceptional cases.

Paul is concerned that my "addStatus" module function will be used either in module/package initialization, or require a new zcml (/grok) tag. That's perhaps a bit heavyweight.

Alternative solutions might be to have a new ``forceStatus`` method on the request that would work as Paul and Stephan prefer, and changing ``setStatus`` to act as I propose.

Thoughts?

Thanks

Gary

Just a few comments.

First, I really think this is a simple indentation bug. Fixing the bug would not be a new feature and could be part of a quick minor release.

The solution Gary is suggesting contains a new set of features that I would like to discuss on the zope-dev list at least a little bit, because section 6.1.1 of the HTTP 1.1 specification says that you can create new status codes freely. Having to register random status codes manually would be pretty lame in my opinion.

http://www.w3.org/Protocols/rfc2616/rfc2616-sec6.html#sec6.1.1

While, yes, you can create new status codes freely, if your client does not understand them, you are in a problematic situation. Our usual clients are browsers, and are not expected to understand non-HTTP status codes. As I said before, we are trying to support the common case, and make the exceptional case possible.

As you suggest, I am sending a note to zope-dev now.

Gary

I dug through the version history to find out if an indentation bug was introduced recently. I'm afraid not. Look for "def setStatus" in the version that's just about to have its 10th anniversary:

http://svn.zope.org/Zope/trunk/lib/python/ZPublisher/HTTPResponse.py?rev=15919&view=markup

Amazingly, Zope has never had support for arbitrary status codes in HTTP responses. So changing the indentation is in fact adding a feature we've never had.

OTOH, the existing behavior is highly unexpected, and adding a new API for something that should be very simple seems overkill to me. I suggest a combination of the suggestions:

        else:
            if isinstance(status, basestring):
                status = status_codes[status.lower()]
        self._status = status

This changes three things:

1. If you pass an integer, it goes through. It's rather surprising that it doesn't already.
2. If you pass a string that is not registered, it raises a KeyError.
3. "type(x) in y" is a deprecated idiom, we might as replace it with isinstance() while we're here.

BTW, it's not clear to me which suggestion Jim and Fred gave their +1 vote for.

BTW, I suggest (and Stephan agrees :-) ) that doctest authors should be able to write the following code:

   >>> response.setStatus(209)
   >>> response.getStatus()
   209

It would be surprising if something different happened.

I agree with Shane.

To Gary's comments.

- Section 6.1.1 of the HTTP spec is very clear that unknown status codes should be treated as x00.

- We already support more codes in the dictionary than are listed in Section 10 of the HTTP spec.

If you read Roy Fielding's dissertation (chapter 5 or 6), it is very clear that it was always intended to be able to define codes as needed. Also, with Web 2.0, Zope 3 starts to become a middleware server whose clients are non-browsers, such as XmlHttpRequest and Flex.

To Stephan: While I doubt he wants his IRC opinion to be canon, the co-worker with whom I consulted is the author of the O'Reilly REST book. We are in fact working in exactly the manner that you describe. The question is not whether one should be able to define codes as needed, but whether unusual or exceptional codes should need to be declared explicitly before use. I liken it to using string constants in your code, rather than typing out the strings each time. Sometimes those kinds of seatbelts are appropriate, and sometimes not. I feel that this is one of those times, and you don't, so we're asking others for opinions.

To Shane's alternative suggestion: to make passing an integer be the "trust me I know what I'm doing" flag seems not very discoverable or intuitive to me. If passing '200' were not already acceptable (i.e., we could make passing string status codes an error) I might feel differently, because then using an int would not be a flag but the API. With our current legacy, I'd prefer Stephan and Paul's approach over this one.

Hmm? Stephan and Paul's approach is essentially the same as mine. Our suggestion conforms with the interface documentation already in interfaces/http.py:

    def setStatus(status, reason=None):
        """Sets the HTTP status code of the response

        The argument may either be an integer or a string from { OK,
        Created, Accepted, NoContent, MovedPermanently,
        MovedTemporarily, NotModified, BadRequest, Unauthorized,
        Forbidden, NotFound, InternalError, NotImplemented,
        BadGateway, ServiceUnavailable } that will be converted to the
        correct integer value.
        """

The documentation doesn't say that the integer value is restricted. It also forbids passing the string "200"; the fact that it accepts "200" is in fact undocumented behavior.

OTOH, you could reasonably argue that the documentation is in error.

My $0.02: the framework should get the hell out of the application's
way, here: if the application passes an int, or even a string which can
be converted to an int, then the lookup should be omitted altogether.

In any non-int case, then passing something which can't be looked up
could reasonably be construed as an error in the application, and should
*raise an exception* rather than DWIMing a 500; the traceback would
then point the finger at the offending bit of code, as it should, rather
than hiding it.

This behavior matches the contract spelled out in the interface, except
that it allows int()-able strings. We should squish the bit of the docs
which disallow passing '200' or 200: that is silly.

So, my version would look like::

    def setStatus(self, status, reason=None):
        """See IHTTPResponse"""
        if status is None:
            status = 200
        try:
            status = int(status)
        except ValueError:
            if type(status) in StringTypes:
                status = status.lower()
            status = status_codes[status] # raise KeyError
        self._status = status

        if reason is None:
            reason = status_reasons.get(status, "Unknown")

        self._reason = reason
        self._status_set = True

+1 for Tres' version, including an update to the interface doc to explicitly allow int-able strings. Let's not put policy enforcement here.

I certainly prefer Tres' version to the other alternatives in that general vein. If no one else weighs in, it looks to me like there's enough of a consensus then (and it already feels a bit like a tempest in a teapot).

Paul, are you up for implementing Tres' approach? Shall I (no promises when, but hopefully soonish)? Or anyone else want to volunteer?

I'm working on this code today to help Stephan and Paul, so I'll do it.

I hit a snag: lots of tests expect to be able to setStatus(ValueError()), expressing the intent that unrecognized exception types should lead to a status of 500. This is how Zope has always behaved. I'm inclined to use Tres' version except make it continue to default the status to 500 for unrecognized values, and update the docs to reflect this behavior. Any disagreement?

svn.zope.org revision #95601

Thank you!

I find the silent 500 to be horrible, myself. However, you've documented it, and I definitely understand the reasoning for it (that is, legacy support). If I were going down this road, I suppose I would add to the interface that, because of the silent 500 behavior, passing an integer is recommended. But that's the smallest of niggles; obviously it didn't seem necessary to you; and, as I said, at least it's documented.

Thanks again.