issue2550755: exceptions.NotFound(msg) msg is not reported to user in cgi.

rouilj · rouilj · commit f88f7e2ebec0 · 2016-08-15T20:53:37.000-04:00
When an invalid column is specified return error code 400 rather than
404. Make error code 400 also return an error message to the
user. Reported by: Bernhard Reiter.

After I implemented this it occurred to me that this may be better
implemented using a new exception BadRequest and handle that
instead. I really don't have a good feel for which is better from an
architectural view although I have a feeling the exception path would
be better.
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -317,6 +317,10 @@ Fixed:
   %20. Fixes to allow a url_query method to be applied to
   HTMLStringProperty to properly quote string values passed as part of
   a url.
+- issue2550755: exceptions.NotFound(msg) msg is not reported to user
+  in cgi. When an invalid column is specified return error code 400
+  rather than 404. Make error code 400 also return an error message to
+  the user. Reported by: Bernhard Reiter, analysis, fix by John Rouillard.
 
 2016-01-11: 1.5.1
 
diff --git a/roundup/cgi/actions.py b/roundup/cgi/actions.py
@@ -1256,11 +1256,11 @@ def handle(self):
         props = klass.getprops()
         for cname in columns:
             if cname not in props:
-                # TODO raise exceptions.NotFound(.....) does not give message
-                # so using SeriousError instead
-                self.client.response_code = 404
-                raise exceptions.SeriousError(
-                    self._('Column "%(column)s" not found on %(class)s')
+                # use error code 400: Bad Request. Do not use
+                # error code 404: Not Found.
+                self.client.response_code = 400
+                raise exceptions.NotFound(
+                    self._('Column "%(column)s" not found in %(class)s')
                     % {'column': cgi.escape(cname), 'class': request.classname})
 
         # full-text search
diff --git a/roundup/cgi/client.py b/roundup/cgi/client.py
@@ -550,16 +550,32 @@ def inner_main(self):
             self.response_code = 304
             self.header()
         except NotFound, e:
-            self.response_code = 404
-            self.template = '404'
-            try:
-                cl = self.db.getclass(self.classname)
-                self.write_html(self.renderContext())
-            except KeyError:
-                # we can't map the URL to a class we know about
-                # reraise the NotFound and let roundup_server
-                # handle it
-                raise NotFound(e)
+            if self.response_code == 400:
+                # We can't find a parameter (e.g. property name
+                # incorrect). Tell the user what was raised.
+                # Do not change to the 404 template since the
+                # base url is valid just query args are not.
+                # copy the page format from SeriousError _str_ exception.
+                error_page = """
+                <html><head><title>Roundup issue tracker: An error has occurred</title>
+                <link rel="stylesheet" type="text/css" href="@@file/style.css">
+                </head>
+                <body class="body" marginwidth="0" marginheight="0">
+                   <p class="error-message">%s</p>
+                </body></html>
+                """
+                self.write_html(error_page%str(e))
+            else:
+                self.response_code = 404
+                self.template = '404'
+                try:
+                    cl = self.db.getclass(self.classname)
+                    self.write_html(self.renderContext())
+                except KeyError:
+                    # we can't map the URL to a class we know about
+                    # reraise the NotFound and let roundup_server
+                    # handle it
+                    raise NotFound(e)
         except FormError, e:
             self.add_error_message(self._('Form Error: ') + str(e))
             self.write_html(self.renderContext())
