12483: Make webdav rename/remove work. Tidy up code.
authorTom Clegg <tclegg@veritasgenetics.com>
Mon, 20 Nov 2017 23:43:46 +0000 (18:43 -0500)
committerTom Clegg <tclegg@veritasgenetics.com>
Mon, 20 Nov 2017 23:48:55 +0000 (18:48 -0500)
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

sdk/go/arvados/collection_fs.go
sdk/go/arvados/collection_fs_test.go
services/keep-web/cadaver_test.go
services/keep-web/handler.go
services/keep-web/handler_test.go
services/keep-web/webdav.go

index 4c97af5712c34dccd409150107224799f1bf1d0b..3f5fb5ea20e8f386b2042c94329d7d9ea6b0b537 100644 (file)
@@ -735,8 +735,7 @@ func (dn *dirnode) marshalManifest(prefix string) (string, error) {
        sort.Strings(names)
 
        for _, name := range names {
-               node := dn.inodes[name]
-               switch node := node.(type) {
+               switch node := dn.inodes[name].(type) {
                case *dirnode:
                        subdir, err := node.marshalManifest(prefix + "/" + name)
                        if err != nil {
@@ -793,14 +792,14 @@ func (dn *dirnode) marshalManifest(prefix string) (string, error) {
 }
 
 func (dn *dirnode) loadManifest(txt string) error {
-       // FIXME: faster
        var dirname string
        streams := strings.Split(txt, "\n")
        if streams[len(streams)-1] != "" {
                return fmt.Errorf("line %d: no trailing newline", len(streams))
        }
-       var extents []storedExtent
-       for i, stream := range streams[:len(streams)-1] {
+       streams = streams[:len(streams)-1]
+       extents := []storedExtent{}
+       for i, stream := range streams {
                lineno := i + 1
                var anyFileTokens bool
                var pos int64
@@ -863,7 +862,7 @@ func (dn *dirnode) loadManifest(txt string) error {
                                // situation might be rare anyway)
                                extIdx, pos = 0, 0
                        }
-                       for next := int64(0); extIdx < len(extents); extIdx, pos = extIdx+1, next {
+                       for next := int64(0); extIdx < len(extents); extIdx++ {
                                e := extents[extIdx]
                                next = pos + int64(e.Len())
                                if next <= offset || e.Len() == 0 {
@@ -890,6 +889,8 @@ func (dn *dirnode) loadManifest(txt string) error {
                                })
                                if next > offset+length {
                                        break
+                               } else {
+                                       pos = next
                                }
                        }
                        if extIdx == len(extents) && pos < offset+length {
@@ -910,36 +911,35 @@ func (dn *dirnode) loadManifest(txt string) error {
 // only safe to call from loadManifest -- no locking
 func (dn *dirnode) createFileAndParents(path string) (fn *filenode, err error) {
        names := strings.Split(path, "/")
-       if basename := names[len(names)-1]; basename == "" || basename == "." || basename == ".." {
+       basename := names[len(names)-1]
+       if basename == "" || basename == "." || basename == ".." {
                err = fmt.Errorf("invalid filename")
                return
        }
-       var node inode = dn
-       for i, name := range names {
-               dn, ok := node.(*dirnode)
-               if !ok {
-                       err = ErrFileExists
-                       return
-               }
-               if name == "" || name == "." {
-                       continue
-               }
-               if name == ".." {
-                       node = dn.parent
-                       continue
-               }
-               node, ok = dn.inodes[name]
-               if !ok {
-                       if i == len(names)-1 {
-                               fn = dn.newFilenode(name, 0755)
+       for _, name := range names[:len(names)-1] {
+               switch name {
+               case "", ".":
+               case "..":
+                       dn = dn.parent
+               default:
+                       switch node := dn.inodes[name].(type) {
+                       case nil:
+                               dn = dn.newDirnode(name, 0755)
+                       case *dirnode:
+                               dn = node
+                       case *filenode:
+                               err = ErrFileExists
                                return
                        }
-                       node = dn.newDirnode(name, 0755)
                }
        }
-       var ok bool
-       if fn, ok = node.(*filenode); !ok {
-               err = ErrInvalidArgument
+       switch node := dn.inodes[basename].(type) {
+       case nil:
+               fn = dn.newFilenode(basename, 0755)
+       case *filenode:
+               fn = node
+       case *dirnode:
+               err = ErrIsDirectory
        }
        return
 }
@@ -957,11 +957,17 @@ func (dn *dirnode) Mkdir(name string, perm os.FileMode) error {
 }
 
 func (dn *dirnode) Remove(name string) error {
-       return dn.remove(name, false)
+       return dn.remove(strings.TrimRight(name, "/"), false)
 }
 
 func (dn *dirnode) RemoveAll(name string) error {
-       return dn.remove(name, true)
+       err := dn.remove(strings.TrimRight(name, "/"), true)
+       if os.IsNotExist(err) {
+               // "If the path does not exist, RemoveAll returns
+               // nil." (see "os" pkg)
+               err = nil
+       }
+       return err
 }
 
 func (dn *dirnode) remove(name string, recursive bool) error {
@@ -1054,9 +1060,20 @@ func (dn *dirnode) Rename(oldname, newname string) error {
                        }
                }
        } else {
+               if newdn.inodes == nil {
+                       newdn.inodes = make(map[string]inode)
+               }
                newdn.fileinfo.size++
        }
        newdn.inodes[newname] = oldinode
+       switch n := oldinode.(type) {
+       case *dirnode:
+               n.parent = newdn
+       case *filenode:
+               n.parent = newdn
+       default:
+               panic(fmt.Sprintf("bad inode type %T", n))
+       }
        delete(olddn.inodes, oldname)
        olddn.fileinfo.size--
        return nil
index 07d00be05b672cb9192355266d8bfa3aa50ab2b2..dabb884a5750580d1f50920362fcce8d9b1fd7c4 100644 (file)
@@ -560,14 +560,14 @@ func (s *CollectionFSSuite) TestRemove(c *check.C) {
        c.Assert(err, check.IsNil)
        err = fs.Mkdir("dir1/dir2", 0755)
        c.Assert(err, check.IsNil)
+       err = fs.Mkdir("dir1/dir3", 0755)
+       c.Assert(err, check.IsNil)
 
        err = fs.Remove("dir0")
        c.Check(err, check.IsNil)
        err = fs.Remove("dir0")
        c.Check(err, check.Equals, os.ErrNotExist)
 
-       err = fs.Remove("dir1/dir2/")
-       c.Check(err, check.Equals, ErrInvalidArgument)
        err = fs.Remove("dir1/dir2/.")
        c.Check(err, check.Equals, ErrInvalidArgument)
        err = fs.Remove("dir1/dir2/..")
@@ -576,10 +576,12 @@ func (s *CollectionFSSuite) TestRemove(c *check.C) {
        c.Check(err, check.Equals, ErrDirectoryNotEmpty)
        err = fs.Remove("dir1/dir2/../../../dir1")
        c.Check(err, check.Equals, ErrDirectoryNotEmpty)
+       err = fs.Remove("dir1/dir3/")
+       c.Check(err, check.IsNil)
        err = fs.RemoveAll("dir1")
        c.Check(err, check.IsNil)
        err = fs.RemoveAll("dir1")
-       c.Check(err, check.Equals, os.ErrNotExist)
+       c.Check(err, check.IsNil)
 }
 
 func (s *CollectionFSSuite) TestRename(c *check.C) {
@@ -630,7 +632,7 @@ func (s *CollectionFSSuite) TestRename(c *check.C) {
                                // oldname does not exist
                                err := fs.Rename(
                                        fmt.Sprintf("dir%d/dir%d/missing", i, j),
-                                       fmt.Sprintf("dir%d/irelevant", outer-i-1))
+                                       fmt.Sprintf("dir%d/dir%d/file%d", outer-i-1, j, j))
                                c.Check(err, check.ErrorMatches, `.*does not exist`)
 
                                // newname parent dir does not exist
index eab4a70688b03ac16912498bc1baa06a76dda1a8..991e92368f6b2026dc15a333ce7d61df23c74b09 100644 (file)
@@ -95,6 +95,71 @@ func (s *IntegrationSuite) TestWebdavWithCadaver(c *check.C) {
                        match: `(?ms).*succeeded.*`,
                        data:  testdata,
                },
+               {
+                       path:  writePath,
+                       cmd:   "move testfile newdir0\n",
+                       match: `(?ms).*Moving .* succeeded.*`,
+               },
+               {
+                       // Strangely, webdav deletes dst if you do
+                       // "move nonexistent dst" -- otherwise we
+                       // would repeat the above "move testfile
+                       // newdir0" here.
+                       path:  writePath,
+                       cmd:   "move testfile nonexistentdir\n",
+                       match: `(?ms).*Moving .* failed.*`,
+               },
+               {
+                       path:  writePath,
+                       cmd:   "ls\n",
+                       match: `(?ms).*newdir0.* 0 .*`,
+               },
+               {
+                       path:  writePath,
+                       cmd:   "move newdir0/testfile emptyfile/bogus/\n",
+                       match: `(?ms).*Moving .* failed.*`,
+               },
+               {
+                       path:  writePath,
+                       cmd:   "mkcol newdir1\n",
+                       match: `(?ms).*Creating .* succeeded.*`,
+               },
+               {
+                       path:  writePath,
+                       cmd:   "move newdir0/testfile newdir1\n",
+                       match: `(?ms).*Moving .* succeeded.*`,
+               },
+               {
+                       path:  writePath,
+                       cmd:   "put '" + localfile.Name() + "' newdir1/testfile1\n",
+                       match: `(?ms).*Uploading .* succeeded.*`,
+               },
+               {
+                       path:  writePath,
+                       cmd:   "mkcol newdir2\n",
+                       match: `(?ms).*Creating .* succeeded.*`,
+               },
+               {
+                       path:  writePath,
+                       cmd:   "put '" + localfile.Name() + "' newdir2/testfile2\n",
+                       match: `(?ms).*Uploading .* succeeded.*`,
+               },
+               {
+                       path:  writePath,
+                       cmd:   "get newdir2/testfile2 '" + checkfile.Name() + "'\n",
+                       match: `(?ms).*succeeded.*`,
+                       data:  testdata,
+               },
+               {
+                       path:  writePath,
+                       cmd:   "rmcol newdir2\n",
+                       match: `(?ms).*Deleting collection .* succeeded.*`,
+               },
+               {
+                       path:  writePath,
+                       cmd:   "get newdir2/testfile2 '" + checkfile.Name() + "'\n",
+                       match: `(?ms).*Downloading .* failed.*`,
+               },
        } {
                c.Logf("%s %+v", "http://"+s.testServer.Addr, trial)
 
index f6c12c3d85558550bd485dc3dc9353ad3c15520f..f10cb5977b6dda2d89d83b385d2e5ede934ab697 100644 (file)
@@ -98,10 +98,13 @@ func (h *handler) serveStatus(w http.ResponseWriter, r *http.Request) {
 
 var (
        webdavMethod = map[string]bool{
+               "DELETE":   true,
+               "MKCOL":    true,
                "MOVE":     true,
                "OPTIONS":  true,
                "PROPFIND": true,
                "PUT":      true,
+               "RMCOL":    true,
        }
        browserMethod = map[string]bool{
                "GET":  true,
@@ -149,7 +152,7 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
                        return
                }
                w.Header().Set("Access-Control-Allow-Headers", "Authorization, Content-Type, Range")
-               w.Header().Set("Access-Control-Allow-Methods", "GET, MOVE, OPTIONS, POST, PROPFIND, PUT")
+               w.Header().Set("Access-Control-Allow-Methods", "DELETE, GET, MKCOL, MOVE, OPTIONS, POST, PROPFIND, PUT, RMCOL")
                w.Header().Set("Access-Control-Allow-Origin", "*")
                w.Header().Set("Access-Control-Max-Age", "86400")
                statusCode = http.StatusOK
index 6add8043e0f0e512aa9f4aa2033cd833547ea932..85a8e500d42397401187332aedd9bf884c93e5ac 100644 (file)
@@ -45,12 +45,12 @@ func (s *UnitSuite) TestCORSPreflight(c *check.C) {
        c.Check(resp.Code, check.Equals, http.StatusOK)
        c.Check(resp.Body.String(), check.Equals, "")
        c.Check(resp.Header().Get("Access-Control-Allow-Origin"), check.Equals, "*")
-       c.Check(resp.Header().Get("Access-Control-Allow-Methods"), check.Equals, "GET, MOVE, OPTIONS, POST, PROPFIND, PUT")
+       c.Check(resp.Header().Get("Access-Control-Allow-Methods"), check.Equals, "DELETE, GET, MKCOL, MOVE, OPTIONS, POST, PROPFIND, PUT, RMCOL")
        c.Check(resp.Header().Get("Access-Control-Allow-Headers"), check.Equals, "Authorization, Content-Type, Range")
 
        // Check preflight for a disallowed request
        resp = httptest.NewRecorder()
-       req.Header.Set("Access-Control-Request-Method", "DELETE")
+       req.Header.Set("Access-Control-Request-Method", "MAKE-COFFEE")
        h.ServeHTTP(resp, req)
        c.Check(resp.Body.String(), check.Equals, "")
        c.Check(resp.Code, check.Equals, http.StatusMethodNotAllowed)
index 7798a69e3cb3ee441436d3940fa1d6e7b892340a..1a306e37e9d662ad62047dbb507f9e58e80d7698 100644 (file)
@@ -10,6 +10,8 @@ import (
        "fmt"
        prand "math/rand"
        "os"
+       "path"
+       "strings"
        "sync/atomic"
        "time"
 
@@ -27,19 +29,45 @@ var (
 
 // webdavFS implements a webdav.FileSystem by wrapping an
 // arvados.CollectionFilesystem.
+//
+// Collections don't preserve empty directories, so Mkdir is
+// effectively a no-op, and we need to make parent dirs spring into
+// existence automatically so sequences like "mkcol foo; put foo/bar"
+// work as expected.
 type webdavFS struct {
        collfs arvados.CollectionFileSystem
        update func() error
 }
 
+func (fs *webdavFS) makeparents(name string) {
+       dir, name := path.Split(name)
+       if dir == "" || dir == "/" {
+               return
+       }
+       dir = dir[:len(dir)-1]
+       fs.makeparents(dir)
+       fs.collfs.Mkdir(dir, 0755)
+}
+
 func (fs *webdavFS) Mkdir(ctx context.Context, name string, perm os.FileMode) error {
-       return fs.collfs.Mkdir(name, 0755)
+       if fs.update == nil {
+               return errReadOnly
+       }
+       name = strings.TrimRight(name, "/")
+       fs.makeparents(name)
+       if err := fs.collfs.Mkdir(name, 0755); err != nil {
+               return err
+       }
+       return fs.update()
 }
 
 func (fs *webdavFS) OpenFile(ctx context.Context, name string, flag int, perm os.FileMode) (f webdav.File, err error) {
        writing := flag&(os.O_WRONLY|os.O_RDWR) != 0
-       if writing && fs.update == nil {
-               return nil, errReadOnly
+       if writing {
+               if fs.update == nil {
+                       return nil, errReadOnly
+               }
+               fs.makeparents(name)
        }
        f, err = fs.collfs.OpenFile(name, flag, perm)
        if writing && err == nil {
@@ -49,17 +77,27 @@ func (fs *webdavFS) OpenFile(ctx context.Context, name string, flag int, perm os
 }
 
 func (fs *webdavFS) RemoveAll(ctx context.Context, name string) error {
-       return fs.collfs.RemoveAll(name)
+       if err := fs.collfs.RemoveAll(name); err != nil {
+               return err
+       }
+       return fs.update()
 }
 
 func (fs *webdavFS) Rename(ctx context.Context, oldName, newName string) error {
        if fs.update == nil {
                return errReadOnly
        }
-       return fs.collfs.Rename(oldName, newName)
+       fs.makeparents(newName)
+       if err := fs.collfs.Rename(oldName, newName); err != nil {
+               return err
+       }
+       return fs.update()
 }
 
 func (fs *webdavFS) Stat(ctx context.Context, name string) (os.FileInfo, error) {
+       if fs.update != nil {
+               fs.makeparents(name)
+       }
        return fs.collfs.Stat(name)
 }