Fix 204 responses, hangs and crashes with REST.

Remove Content-Type and make sure no content is returned by OPTIONS
request in REST interface.

In write_html set the Content-Length when response is not
encoded/compressed (fixes hang due to missing content-length with
unencoded data).

In REST interface do not raise UsageError for invalid api version.
Return json error with proper message. Fixes crash.

Author: rouilj

roundup/cgi/client.py

@@ -645,8 +645,11 @@ def handle_rest(self):
 
         # type header set by rest handler
         # self.setHeader("Content-Type", "text/xml")
-        self.setHeader("Content-Length", str(len(output)))
-        self.write(output)
+        if self.response_code == 204: # no body with 204
+            self.write("")
+        else:
+            self.setHeader("Content-Length", str(len(output)))
+            self.write(output)
 
     def add_ok_message(self, msg, escape=True):
         add_message(self._ok_message, msg, escape)
@@ -848,7 +851,7 @@ def inner_main(self):
             else:
                 # in debug mode, only write error to screen.
                 self.write_html(e.html)
-        except:
+        except Exception as e:
             # Something has gone badly wrong.  Therefore, we should
             # make sure that the response code indicates failure.
             if self.response_code == http_.client.OK:
@@ -2196,6 +2199,8 @@ def write_html(self, content):
             if 'Content-Type' not in self.additional_headers:
                 self.additional_headers['Content-Type'] = \
                     'text/html; charset=%s' % self.charset
+            if 'Content-Length' not in self.additional_headers:
+                self.additional_headers['Content-Length'] = len(content)
             self.header()
 
         if self.env['REQUEST_METHOD'] == 'HEAD':
@@ -2497,6 +2502,9 @@ def header(self, headers=None, response=None):
         if headers.get('Content-Type', 'text/html') == 'text/html':
             headers['Content-Type'] = 'text/html; charset=utf-8'
 
+        if response == 204: # has no body so no content-type
+            del(headers['Content-Type'])
+
         headers = list(headers.items())
 
         for ((path, name), (value, expire)) in self._cookies.items():

roundup/rest.py

@@ -2052,7 +2052,7 @@ def dispatch(self, method, uri, input):
             #    Use default if not specified for now.
             self.api_version = self.__default_api_version
         elif self.api_version not in self.__supported_api_versions:
-            raise UsageError(msg % self.api_version)
+            output = self.error_obj(400, msg % self.api_version)
 
         # sadly del doesn't work on FieldStorage which can be the type of
         # input. So we have to ignore keys starting with @ at other

test/rest_common.py

@@ -2272,13 +2272,15 @@ def testAcceptHeaderParsing(self):
         headers={"accept": "application/json; version=99"
         }
         self.headers=headers
-        with self.assertRaises(UsageError) as ctx:
-            results = self.server.dispatch('GET',
-                                           "/rest/data/status/1",
-                                           self.empty_form)
+        results = self.server.dispatch('GET',
+                                       "/rest/data/status/1",
+                                       self.empty_form)
         print(results)
-        self.assertEqual(self.server.client.response_code, 200)
-        self.assertEqual(ctx.exception.args[0],
+        json_dict = json.loads(b2s(results))
+        self.assertEqual(self.server.client.response_code, 400)
+        self.assertEqual(self.server.client.additional_headers['Content-Type'],
+                         "application/json")
+        self.assertEqual(json_dict['error']['msg'],
                          "Unrecognized version: 99. See /rest without "
                          "specifying version for supported versions.")
 

test/test_liveserver.py

@@ -113,8 +113,7 @@ def test_rest_endpoint_root_options(self):
         print(f.headers)
 
         self.assertEqual(f.status_code, 204)
-        expected = { 'Content-Type': 'application/json',
-                     'Access-Control-Allow-Origin': '*',
+        expected = { 'Access-Control-Allow-Origin': '*',
                      'Access-Control-Allow-Headers': 'Content-Type, Authorization, X-HTTP-Method-Override',
                      'Allow': 'OPTIONS, GET',
                      'Access-Control-Allow-Methods': 'HEAD, OPTIONS, GET, PUT, DELETE, PATCH',
@@ -134,8 +133,7 @@ def test_rest_endpoint_data_options(self):
         print(f.headers)
 
         self.assertEqual(f.status_code, 204)
-        expected = { 'Content-Type': 'application/json',
-                     'Access-Control-Allow-Origin': '*',
+        expected = { 'Access-Control-Allow-Origin': '*',
                      'Access-Control-Allow-Headers': 'Content-Type, Authorization, X-HTTP-Method-Override',
                      'Allow': 'OPTIONS, GET',
                      'Access-Control-Allow-Methods': 'HEAD, OPTIONS, GET, PUT, DELETE, PATCH',
@@ -154,8 +152,7 @@ def test_rest_endpoint_collection_options(self):
         print(f.headers)
 
         self.assertEqual(f.status_code, 204)
-        expected = { 'Content-Type': 'application/json',
-                     'Access-Control-Allow-Origin': '*',
+        expected = { 'Access-Control-Allow-Origin': '*',
                      'Access-Control-Allow-Headers': 'Content-Type, Authorization, X-HTTP-Method-Override',
                      'Allow': 'OPTIONS, GET, POST',
                      'Access-Control-Allow-Methods': 'HEAD, OPTIONS, GET, PUT, DELETE, PATCH',
@@ -175,8 +172,7 @@ def test_rest_endpoint_item_options(self):
         print(f.headers)
 
         self.assertEqual(f.status_code, 204)
-        expected = { 'Content-Type': 'application/json',
-                     'Access-Control-Allow-Origin': '*',
+        expected = { 'Access-Control-Allow-Origin': '*',
                      'Access-Control-Allow-Headers': 'Content-Type, Authorization, X-HTTP-Method-Override',
                      'Allow': 'OPTIONS, GET, PUT, DELETE, PATCH',
                      'Access-Control-Allow-Methods': 'HEAD, OPTIONS, GET, PUT, DELETE, PATCH',
@@ -195,8 +191,7 @@ def test_rest_endpoint_attribute_options(self):
         print(f.headers)
 
         self.assertEqual(f.status_code, 204)
-        expected = { 'Content-Type': 'application/json',
-                     'Access-Control-Allow-Origin': '*',
+        expected = { 'Access-Control-Allow-Origin': '*',
                      'Access-Control-Allow-Headers': 'Content-Type, Authorization, X-HTTP-Method-Override',
                      'Allow': 'OPTIONS, GET, PUT, DELETE, PATCH',
                      'Access-Control-Allow-Methods': 'HEAD, OPTIONS, GET, PUT, DELETE, PATCH',