refactor webish.py slightly, improve the test coverage a bit. One test is marked SKIP pending improvements in webish.py

src/allmydata/test/test_web.py

@@ -548,6 +548,14 @@ class Web(WebMixin, unittest.TestCase):
                                                       self.NEWFILE_CONTENTS))
         return d
 
+    def test_PUT_NEWFILEURL_localfile_missingarg(self):
+        url = self.public_url + "/foo/new.txt?t=upload"
+        d = self.shouldHTTPError2("test_PUT_NEWFILEURL_localfile_missing",
+                                  400, "Bad Request",
+                                  "t=upload requires localfile= or localdir=",
+                                  self.PUT, url, "")
+        return d
+
     def test_PUT_NEWFILEURL_localfile_disabled(self):
         localfile = os.path.abspath("web/PUT_NEWFILEURL_local file_disabled")
         url = (self.public_url + "/foo/new.txt?t=upload&localfile=%s" %
@@ -596,6 +604,13 @@ class Web(WebMixin, unittest.TestCase):
         d.addCallback(_check2)
         return d
 
+    def test_GET_FILEURL_badtype(self):
+        d = self.shouldHTTPError2("GET t=bogus", 400, "Bad Request",
+                                  "bad t=bogus",
+                                  self.GET,
+                                  self.public_url + "/foo/bar.txt?t=bogus")
+        return d
+
     def test_GET_FILEURL_uri_missing(self):
         d = self.GET(self.public_url + "/foo/missing?t=uri")
         d.addBoth(self.should404, "test_GET_FILEURL_uri_missing")
@@ -634,6 +649,14 @@ class Web(WebMixin, unittest.TestCase):
 
         return d
 
+    def test_GET_DIRURL_badtype(self):
+        d = self.shouldHTTPError2("test_GET_DIRURL_badtype",
+                                  400, "Bad Request",
+                                  "bad t=bogus",
+                                  self.GET,
+                                  self.public_url + "/foo?t=bogus")
+        return d
+
     def test_GET_DIRURL_large(self):
         # Nevow has a problem showing more than about 192 children of a
         # directory: it uses defer.success() and d.addCallback in a way that
@@ -817,6 +840,14 @@ class Web(WebMixin, unittest.TestCase):
         d.addCallback(_check)
         return d
 
+    def test_GET_DIRURL_localdir_nolocaldir(self):
+        d = self.shouldHTTPError2("GET_DIRURL_localdir_nolocaldir",
+                                  400, "Bad Request",
+                                  "t=download requires localdir=",
+                                  self.GET,
+                                  self.public_url + "/foo?t=download")
+        return d
+
     def touch(self, localdir, filename):
         path = os.path.join(localdir, filename)
         f = open(path, "wb")
@@ -954,6 +985,18 @@ class Web(WebMixin, unittest.TestCase):
                                            "contents of three/bar.txt\n"))
         return d
 
+    def test_PUT_NEWDIRURL_localdir_missing(self):
+        raise unittest.SkipTest("fix PUTHandler._upload_localdir to return "
+                                "an error instead of silently passing")
+        localdir = os.path.abspath("web/PUT_NEWDIRURL_localdir_missing")
+        # we do *not* create it, to trigger an error
+        url = (self.public_url + "/foo/subdir/newdir?t=upload&localdir=%s"
+               % urllib.quote(localdir))
+        d = self.shouldHTTPError2("test_PUT_NEWDIRURL_localdir_missing",
+                                  400, "Bad Request", "random",
+                                  self.PUT, url, "")
+        return d
+
     def test_POST_upload(self):
         d = self.POST(self.public_url + "/foo", t="upload",
                       file=("new.txt", self.NEWFILE_CONTENTS))
@@ -1066,6 +1109,14 @@ class Web(WebMixin, unittest.TestCase):
                                                       self.NEWFILE_CONTENTS))
         return d
 
+    def test_POST_upload_no_replace_ok(self):
+        d = self.POST(self.public_url + "/foo?replace=false", t="upload",
+                      file=("new.txt", self.NEWFILE_CONTENTS))
+        d.addCallback(lambda res: self.GET(self.public_url + "/foo/new.txt"))
+        d.addCallback(lambda res: self.failUnlessEqual(res,
+                                                       self.NEWFILE_CONTENTS))
+        return d
+
     def test_POST_upload_no_replace_queryarg(self):
         d = self.POST(self.public_url + "/foo?replace=false", t="upload",
                       file=("bar.txt", self.NEWFILE_CONTENTS))
@@ -1515,3 +1566,15 @@ class Web(WebMixin, unittest.TestCase):
             pass
         d.addCallback(_done)
         return d
+
+    def test_bad_method(self):
+        url = self.webish_url + self.public_url + "/foo/bar.txt"
+        d = self.shouldHTTPError2("test_bad_method", 404, "Not Found", None,
+                                  client.getPage, url, method="BOGUS")
+        return d
+
+    def test_short_url(self):
+        url = self.webish_url + "/uri"
+        d = self.shouldHTTPError2("test_short_url", 404, "Not Found", None,
+                                  client.getPage, url, method="DELETE")
+        return d

src/allmydata/webish.py

@@ -906,6 +906,11 @@ class PUTHandler(rend.Page):
         localfile = self._localfile
         localdir = self._localdir
 
+        if t == "upload" and not (localfile or localdir):
+            req.setResponseCode(http.BAD_REQUEST)
+            req.setHeader("content-type", "text/plain")
+            return "t=upload requires localfile= or localdir="
+
         # we must traverse the path, creating new directories as necessary
         d = self._get_or_create_directories(self._node, self._path[:-1])
         name = self._path[-1]
@@ -913,12 +918,11 @@ class PUTHandler(rend.Page):
         if t == "upload":
             if localfile:
                 d.addCallback(self._upload_localfile, localfile, name)
-            elif localdir:
+            else:
+                # localdir
                 # take the last step
                 d.addCallback(self._get_or_create_directories, self._path[-1:])
                 d.addCallback(self._upload_localdir, localdir)
-            else:
-                raise RuntimeError("t=upload requires localfile= or localdir=")
         elif t == "uri":
             d.addCallback(self._attach_uri, req.content, name)
         elif t == "mkdir":
@@ -1063,6 +1067,19 @@ class Manifest(rend.Page):
         ctx.fillSlots("refresh_capability", refresh_cap)
         return ctx.tag
 
+class ChildError:
+    implements(inevow.IResource)
+    def renderHTTP(self, ctx):
+        req = inevow.IRequest(ctx)
+        req.setResponseCode(http.BAD_REQUEST)
+        req.setHeader("content-type", "text/plain")
+        return self.text
+
+def child_error(text):
+    ce = ChildError()
+    ce.text = text
+    return ce, ()
+
 class VDrive(rend.Page):
 
     def __init__(self, node, name):
@@ -1079,13 +1096,6 @@ class VDrive(rend.Page):
         method = req.method
         path = segments
 
-        # when we're pointing at a directory (like /uri/$DIR_URI/my_pix),
-        # Directory.addSlash causes a redirect to /uri/$DIR_URI/my_pix/,
-        # which appears here as ['my_pix', '']. This is supposed to hit the
-        # same Directory as ['my_pix'].
-        if path and path[-1] == '':
-            path = path[:-1]
-
         t = get_arg(req, "t", "")
         localfile = get_arg(req, "localfile", None)
         if localfile is not None:
@@ -1131,13 +1141,13 @@ class VDrive(rend.Page):
                     elif t == "readonly-uri":
                         return FileReadOnlyURI(node), ()
                     else:
-                        raise RuntimeError("bad t=%s" % t)
+                        return child_error("bad t=%s" % t)
                 elif IDirectoryNode.providedBy(node):
                     if t == "download":
                         if localdir:
                             # recursive download to a local directory
                             return LocalDirectoryDownloader(node, localdir), ()
-                        raise RuntimeError("t=download requires localdir=")
+                        return child_error("t=download requires localdir=")
                     elif t == "":
                         # send an HTML representation of the directory
                         return Directory(self.name, node, path), ()
@@ -1152,9 +1162,9 @@ class VDrive(rend.Page):
                     elif t == 'rename-form':
                         return RenameForm(self.name, node, path), ()
                     else:
-                        raise RuntimeError("bad t=%s" % t)
+                        return child_error("bad t=%s" % t)
                 else:
-                    raise RuntimeError("unknown node type")
+                    return child_error("unknown node type")
             d.addCallback(file_or_dir)
         elif method == "POST":
             # the node must exist, and our operation will be performed on the
@@ -1177,10 +1187,6 @@ class VDrive(rend.Page):
             return PUTHandler(self.node, path, t, localfile, localdir, replace), ()
         else:
             return rend.NotFound
-        def _trap_KeyError(f):
-            f.trap(KeyError)
-            return rend.FourOhFour(), ()
-        d.addErrback(_trap_KeyError)
         return d
 
 class URIPUTHandler(rend.Page):