rc.* helpers are named incorrectly

Jacob Kaplan-Moss avatarJacob Kaplan-Moss created an issue

RFC 2616 defines names for the various HTTP response codes. Unfortunately, the names Piston uses for the rc.* helpers don't match. I found this really confusing, and it led to a number of places where I was using HTTP codes incorrectly.

These helpers should be renamed to match the RFC (http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html). These differences are important. Here's the differences between Piston and RFC; I'll explain why each is a problem below:

Code    Piston's name       RFC                 Note
200     OK                  OK                  
201     CREATED             Created             
204     DELETED             No Content          [1]
400     BAD_REQUEST         Bad Request         
401     FORBIDDEN           Unauthorized        [2]
404     NOT_FOUND           Not Found           
409     DUPLICATE_ENTRY     Conflict            [3]
410     NOT_HERE            Gone                [4]
501     NOT_IMPLEMENTED     Not Implemented
503     THROTTLED           Service Unavailable [5]

Okay, so why are these names misleading?

  1. `204` is indeed the correct response to a `DELETE`. However, it's also used in a number of other contexts -- it's a valid response to a `PUT` that updates a resource, for example. The RFC says that 204 is used whenever "the server has fulfilled the request but does not need to return an entity-body," which is a lot more often than after a `DELETE`.
  2. This is actually a really big problem. HTTP has two different types of "authentication failure" codes, and this naming confuses the two. 401 means "unauthorized"; it indicates that the given request requires authorization. The response can include a `WWW-Authenticate` header indicating how authentication ought to be performed. The RFC notes that "the client MAY repeat the request with a suitable Authorization header field". 403, on the other hand, is "forbidden"; it indicates that "authorization will not help and the request SHOULD NOT be repeated." You might return a 403 if the password given for the user doesn't match, for example. Naming 401 "Forbidden" is confusing, and wrong. It was that discovery that prompted me to write this bug.
  3. 409 conflict usually does indicate a duplicate entry, but it's also often used in transactional contexts to indicate that someone else's transaction conflicted with yours and prevented commit. The naming obscures that (and other) uses.
  4. "Not here" sounds a lot like "Not found" to me. "Gone", on the other hand, indicates that the resource once *was* there, but is no longer. There's an important difference which (again) the name obscures.
  5. 503 is useful for a lot more than throttling.

Again, please rename these codes to be consistent with the RFC.

Comments (8)

  1. Simon Law

    I agree with jacobian. Included is a patch that clarifies these problems and adds a few new status codes.

    As well, the default update() method returns the updated instance, to confirm that the resource you created was saved properly. And the default delete() method returns rc.NOT_FOUND as opposed to rc.GONE, since GONE means that the resource will never reappear.

  2. Simon Law

    Hmm, perhaps you could provide duplicates for the old names as compatibility. But rc.FORBIDDEN is really confusing and we discovered that we were using it incorrectly by accident.

    Is there some setting that could be provided to put the rc_factory into compatibility mode? And then some documentation on upgrade?

  3. Jesper Nøhr

    I've put a lot of effort into keeping backwards compatibility on all changes, with the exception of using warnings.

    It's difficult to distinguish between correct and incorrect usage of the ones that merely changed though, so what I suggest is, we keep the removed ones around for legacy purposes, and update the documentation. That shouldn't break too many things.

  4. Simon Law

    Well, rc.FORBIDDEN is the one that changed that's actually important. To me (and probably jacobian) this is a bug fix and probably requires authors to grep through their code to check if they really meant 401 or 403 for every use of rc.FORBIDDEN.

    Do you agree?

  5. Jesper Nøhr

    Absolutely.

    I think changing the meaning of a code is "OK", as long as we don't remove ones that people may use, which would consequently result in AttributeError's and code failure.

    We'll document the changed ones on the wiki and let people know via release notes and the mailing list.

  6. Simon Law

    Perfect. Would you like me to submit a new patch with the old values put back? Or will you do that?

    Also, what do you think about using warnings .warn() and DeprecationWarning for use of the old symbols?

  7. Log in to comment
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.