JSON api to return INVALID_CHANGE_NUMBER if review request does not exist

Review Request #966 — Created Aug. 14, 2009 and discarded


Review Board SVN (deprecated)


I tried to extract review requests using parts of the post-review code. Whenever I requested a request ID which is _not_ available on Review Board I got a HTTP404 exception.
In my opinion this is unhandy because there seems to be no other way available to obtain the review requests from the server. I'm forced to poll for each id and it is quite usual that some do not exist.

I would like to change the review_request API to return INVALID_CHANGE_NUMBER or DOES_NOT_EXIST if the review object cannot be found.

Side info: In post-review I'm using get_review_request which calls to api/json/reviewrequests/<RID>/ to retrieve the review requests.

  2. //trunk/reviewboard/webapi/json.py (Diff revision 1)
    A change number is very different from a review request ID. It's actually more correct to return a 404, as it says "This review request does not exist." It's more RESTful.
    1. Would it be consistent to return DOES_NOT_EXIST instead?
    2. I don't think either of us understand. Why is this better than using the existing HTTP error code? The HTTP error code means "the object wasn't found". That's what's happening in this case.
    3. The main reason why I'm asking for this change is because I'm using code from post-review.py. 
      It's mainly about ReviewBoardServer's get_review_request function. When a HTTP404 is received my script is simply terminated. Of course, in this case I cannot poll for review requests on the server.
      Instead of throwing 404 I changed RB to return DOES_NOT_EXIST.
      Various API functions return DOES_NOT_EXIST and so far I did not figure our why sometimes 404 and sometimes DOES_NOT_EXIST is returned.
      Please find below ReviewBoardServer's http_post function:
          def http_post(self, path, fields, files=None):
              Performs an HTTP POST on the specified path, storing any cookies that
              were set.
              if fields:
                  debug_fields = fields.copy()
                  debug_fields = {}
              if 'password' in debug_fields:
                  debug_fields["password"] = "**************"
              url = self._make_url(path)
              debug('HTTP POSTing to %s: %s' % (url, debug_fields))
              content_type, body = self._encode_multipart_formdata(fields, files)
              headers = {
                  'Content-Type': content_type,
                  'Content-Length': str(len(body))
                  r = urllib2.Request(url, body, headers)
                  data = urllib2.urlopen(r).read()
                  return data
              except urllib2.URLError, e:
                  except AttributeError:
                  die("Unable to access %s. The host path may be invalid\n%s" % \
                      (url, e))
              except urllib2.HTTPError, e:
                  die("Unable to access %s (%s). The host path may be invalid\n%s" % \
                      (url, e.code, e.read()))
    4. If you're modifying post-review code to do this, just change this to not die() when it gets an HTTPError with a 404.
    5. Yeah, I'd really like to see us move our API in a more RESTful direction than it is. That means using proper error codes instead of always returning a 200 when there's proper data. Remember that a 200, 500, or most any other error code can still return data with more informative information. So, given that, I'd MUCH prefer that post-review starts moving in the direction of handling error codes appropriately, seeing if there's a payload and parsing it, rather than just die()'ing.
    6. I'm going to discard this request, since it's not what we want. If you have changes to the post-review code to better handle 404, please post them.