Ticket #733: 400_404_arg_diff.txt

Index: test/test_objectmapping.py
===================================================================
--- test/test_objectmapping.py  (revision 2029)
+++ test/test_objectmapping.py  (working copy)
@@ -268,11 +268,10 @@
         self.getPage("/dir1/dir2/5/3/sir")
         self.assertBody("default for dir1, param is:('dir2', '5', '3', 'sir')")
         
-        # test that extra positional args raises an error.
-        # 500 for now, maybe 404 in the future.
+        # test that extra positional args raises an 404 Not Found
         # See http://www.cherrypy.org/ticket/733.
         self.getPage("/dir1/dir2/script_name/extra/stuff")
-        self.assertStatus(500)
+        self.assertStatus(404)
     
     def testExpose(self):
         # Test the cherrypy.expose function/decorator
Index: test/test_core.py
===================================================================
--- test/test_core.py   (revision 2029)
+++ test/test_core.py   (working copy)
@@ -98,7 +98,41 @@
         def default(self, *args, **kwargs):
             return "args: %s kwargs: %s" % (args, kwargs)
 
+    class ParamErrors(Test):
 
+        def one_positional(self, param1):
+            return "data"
+        one_positional.exposed = True
+
+        def one_positional_args(self, param1, *args):
+            return "data"
+        one_positional_args.exposed = True
+
+        def one_positional_args_kwargs(self, param1, *args, **kwargs):
+            return "data"
+        one_positional_args_kwargs.exposed = True
+
+        def one_positional_kwargs(self, param1, **kwargs):
+            return "data"
+        one_positional_kwargs.exposed = True
+
+        def no_positional(self):
+            return "data"
+        no_positional.exposed = True
+
+        def no_positional_args(self, *args):
+            return "data"
+        no_positional_args.exposed = True
+
+        def no_positional_args_kwargs(self, *args, **kwargs):
+            return "data"
+        no_positional_args_kwargs.exposed = True
+
+        def no_positional_kwargs(self, **kwargs):
+            return "data"
+        no_positional_kwargs.exposed = True
+
+
     class Status(Test):
         
         def index(self):
@@ -444,15 +478,6 @@
         self.getPage("/params/?thing=a&thing=b&thing=c")
         self.assertBody("['a', 'b', 'c']")
         
-        # Test friendly error message when given params are not accepted.
-        ignore = helper.webtest.ignored_exceptions
-        ignore.append(TypeError)
-        try:
-            self.getPage("/params/?notathing=meeting")
-            self.assertInBody("index() got an unexpected keyword argument 'notathing'")
-        finally:
-            ignore.pop()
-        
         # Test "% HEX HEX"-encoded URL, param keys, and values
         self.getPage("/params/%d4%20%e3/cheese?Gruy%E8re=Bulgn%e9ville")
         self.assertBody(r"args: ('\xd4 \xe3', 'cheese') "
@@ -466,7 +491,82 @@
         # Test coordinates sent by <img ismap>
         self.getPage("/params/ismap?223,114")
         self.assertBody("Coordinates: 223, 114")
-    
+
+    def testParamErrors(self):
+
+        # test that all of the handlers work when given 
+        # the correct parameters in order to ensure that the
+        # errors below aren't coming from some other source.
+        for uri in (
+                '/paramerrors/one_positional?param1=foo',
+                '/paramerrors/one_positional_args?param1=foo',
+                '/paramerrors/one_positional_args/foo',
+                '/paramerrors/one_positional_args/foo/bar/baz',
+                '/paramerrors/one_positional_args_kwargs?param1=foo&param2=bar',
+                '/paramerrors/one_positional_args_kwargs/foo?param2=bar&param3=baz',
+                '/paramerrors/one_positional_args_kwargs/foo/bar/baz?param2=bar&param3=baz',
+                '/paramerrors/one_positional_kwargs?param1=foo&param2=bar&param3=baz',
+                '/paramerrors/one_positional_kwargs/foo?param4=foo&param2=bar&param3=baz',
+                '/paramerrors/no_positional',
+                '/paramerrors/no_positional_args/foo',
+                '/paramerrors/no_positional_args/foo/bar/baz',
+                '/paramerrors/no_positional_args_kwargs?param1=foo&param2=bar',
+                '/paramerrors/no_positional_args_kwargs/foo?param2=bar',
+                '/paramerrors/no_positional_args_kwargs/foo/bar/baz?param2=bar&param3=baz',
+                '/paramerrors/no_positional_kwargs?param1=foo&param2=bar',
+            ):
+            self.getPage(uri)
+            self.assertStatus(200)
+
+        # query string parameters are part of the URI, so if they are wrong
+        # for a particular handler, the status MUST be a 404.
+        for uri in (
+                '/paramerrors/one_positional',
+                '/paramerrors/one_positional?foo=foo',
+                '/paramerrors/one_positional/foo/bar/baz',
+                '/paramerrors/one_positional/foo?param1=foo',
+                '/paramerrors/one_positional/foo?param1=foo&param2=foo',
+                '/paramerrors/one_positional_args/foo?param1=foo&param2=foo',
+                '/paramerrors/one_positional_args/foo/bar/baz?param2=foo',
+                '/paramerrors/one_positional_args_kwargs/foo/bar/baz?param1=bar&param3=baz',
+                '/paramerrors/one_positional_kwargs/foo?param1=foo&param2=bar&param3=baz',
+                '/paramerrors/no_positional/boo',
+                '/paramerrors/no_positional?param1=foo',
+                '/paramerrors/no_positional_args/boo?param1=foo',
+                '/paramerrors/no_positional_kwargs/boo?param1=foo',
+            ):
+            self.getPage(uri)
+            self.assertStatus(404)
+
+        # if body parameters are wrong, a 400 must be returned.
+        for uri, body in (
+                ('/paramerrors/one_positional/foo', 'param1=foo',),
+                ('/paramerrors/one_positional/foo', 'param1=foo&param2=foo',),
+                ('/paramerrors/one_positional_args/foo', 'param1=foo&param2=foo',),
+                ('/paramerrors/one_positional_args/foo/bar/baz', 'param2=foo',),
+                ('/paramerrors/one_positional_args_kwargs/foo/bar/baz', 'param1=bar&param3=baz',),
+                ('/paramerrors/one_positional_kwargs/foo', 'param1=foo&param2=bar&param3=baz',),
+                ('/paramerrors/no_positional', 'param1=foo',),
+                ('/paramerrors/no_positional_args/boo', 'param1=foo',),
+            ):
+            self.getPage(uri, method='POST', body=body)
+            self.assertStatus(400)
+
+
+        # even if body parameters are wrong, if we get the uri wrong, then 
+        # it's a 404
+        for uri, body in (
+                ('/paramerrors/one_positional?param2=foo', 'param1=foo',),
+                ('/paramerrors/one_positional/foo/bar', 'param2=foo',),
+                ('/paramerrors/one_positional_args/foo/bar?param2=foo', 'param3=foo',),
+                ('/paramerrors/one_positional_kwargs/foo/bar', 'param2=bar&param3=baz',),
+                ('/paramerrors/no_positional?param1=foo', 'param2=foo',),
+                ('/paramerrors/no_positional_args/boo?param2=foo', 'param1=foo',),
+            ):
+            self.getPage(uri, method='POST', body=body)
+            self.assertStatus(404)
+
+
     def testStatus(self):
         self.getPage("/status/")
         self.assertBody('normal')
Index: _cpdispatch.py
===================================================================
--- _cpdispatch.py      (revision 2029)
+++ _cpdispatch.py      (working copy)
@@ -21,9 +21,129 @@
         self.kwargs = kwargs
     
     def __call__(self):
-        return self.callable(*self.args, **self.kwargs)
+        try:
+            return self.callable(*self.args, **self.kwargs)
+        except TypeError, x:
+            test_callable_spec(self.callable, self.args, self.kwargs)
+            raise
 
+def test_callable_spec(callable, callable_args, callable_kwargs):
+    """
+    Inspect callable and test to see if the given args are suitable for it.
 
+    When an error occurs during the handler's invoking stage there are 2
+    erroneous cases:
+    1.  Too many parameters passed to a function which doesn't define
+        one of *args or **kwargs.
+    2.  Too little parameters are passed to the function.
+
+    There are 3 sources of parameters to a cherrypy handler.
+    1.  query string parameters are passed as keyword parameters to the handler.
+    2.  body parameters are also passed as keyword parameters.
+    3.  when partial matching occurs, the final path atoms are passed as
+        positional args.
+    Both the query string and path atoms are part of the URI.  If they are
+    incorrect, then a 404 Not Found should be raised. Conversely the body
+    parameters are part of the request; if they are invalid a 400 Bad Request.
+    """
+    (args, varargs, varkw, defaults) = inspect.getargspec(callable)
+
+    if args and args[0] == 'self':
+        args = args[1:]
+
+    arg_usage = dict([(arg, 0,) for arg in args])
+    vararg_usage = 0
+    varkw_usage = 0
+    extra_kwargs = set()
+
+    for i, value in enumerate(callable_args):
+        try:
+            arg_usage[args[i]] += 1
+        except IndexError:
+            vararg_usage += 1
+
+    for key in callable_kwargs.keys():
+        try:
+            arg_usage[key] += 1
+        except KeyError:
+            varkw_usage += 1
+            extra_kwargs.add(key)
+
+    for i, val in enumerate(defaults or []):
+        # Defaults take effect only when the arg hasn't been used yet.
+        if arg_usage[args[i]] == 0:
+            arg_usage[args[i]] += 1
+
+    missing_args = []
+    multiple_args = []
+    for key, usage in arg_usage.iteritems():
+        if usage == 0:
+            missing_args.append(key)
+        elif usage > 1:
+            multiple_args.append(key)
+
+    if missing_args:
+        # In the case where the method allows body arguments
+        # there are 3 potential errors:
+        # 1. not enough query string parameters -> 404
+        # 2. not enough body parameters -> 400
+        # 3. not enough path parts (partial matches) -> 404
+        #
+        # We can't actually tell which case it is, 
+        # so I'm raising a 404 because that covers 2/3 of the
+        # possibilities
+        # 
+        # In the case where the method does not allow body
+        # arguments it's definitely a 404.
+        raise cherrypy.HTTPError(404,
+                message="Missing parameters: %s" % ",".join(missing_args))
+
+    # the extra positional arguments come from the path - 404 Not Found
+    if not varargs and vararg_usage > 0:
+        raise cherrypy.HTTPError(404)
+
+    body_params = cherrypy.request.body_params or {}
+    body_params = set(body_params.keys())
+    qs_params = set(callable_kwargs.keys()) - body_params
+
+    if multiple_args:
+
+        if qs_params.intersection(set(multiple_args)):
+            # If any of the multiple parameters came from the query string then
+            # it's a 404 Not Found
+            error = 404
+        else:
+            # Otherwise it's a 400 Bad Request
+            error = 400
+
+        raise cherrypy.HTTPError(error,
+                message="Multiple values for parameters: "\
+                        "%s" % ",".join(multiple_args))
+
+    if not varkw and varkw_usage > 0:
+
+        # If there were extra query string parameters, it's a 404 Not Found
+        extra_qs_params = set(qs_params).intersection(extra_kwargs)
+        if extra_qs_params:
+            raise cherrypy.HTTPError(404,
+                message="Unexpected query string "\
+                        "parameters: %s" % ", ".join(extra_qs_params))
+
+        # If there were any extra body parameters, it's a 400 Not Found
+        extra_body_params = set(body_params).intersection(extra_kwargs)
+        if extra_body_params:
+            raise cherrypy.HTTPError(400,
+                message="Unexpected body parameters: "\
+                        "%s" % ", ".join(extra_body_params))
+
+
+try:
+    import inspect
+except ImportError:
+    test_callable_spec = lambda callable, args, kwargs: None
+
+
+
 class LateParamPageHandler(PageHandler):
     """When passing cherrypy.request.params to the page handler, we do not
     want to capture that dict too early; we want to give tools like the