From 11127c5b67a10a46f60c1c1c53a2c2639b7914e1 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Mon, 20 Nov 2017 18:43:46 -0500 Subject: [PATCH 1/1] 12483: Make webdav rename/remove work. Tidy up code. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- sdk/go/arvados/collection_fs.go | 79 +++++++++++++++++----------- sdk/go/arvados/collection_fs_test.go | 10 ++-- services/keep-web/cadaver_test.go | 65 +++++++++++++++++++++++ services/keep-web/handler.go | 5 +- services/keep-web/handler_test.go | 4 +- services/keep-web/webdav.go | 48 +++++++++++++++-- 6 files changed, 168 insertions(+), 43 deletions(-) diff --git a/sdk/go/arvados/collection_fs.go b/sdk/go/arvados/collection_fs.go index 4c97af5712..3f5fb5ea20 100644 --- a/sdk/go/arvados/collection_fs.go +++ b/sdk/go/arvados/collection_fs.go @@ -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 diff --git a/sdk/go/arvados/collection_fs_test.go b/sdk/go/arvados/collection_fs_test.go index 07d00be05b..dabb884a57 100644 --- a/sdk/go/arvados/collection_fs_test.go +++ b/sdk/go/arvados/collection_fs_test.go @@ -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 diff --git a/services/keep-web/cadaver_test.go b/services/keep-web/cadaver_test.go index eab4a70688..991e92368f 100644 --- a/services/keep-web/cadaver_test.go +++ b/services/keep-web/cadaver_test.go @@ -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) diff --git a/services/keep-web/handler.go b/services/keep-web/handler.go index f6c12c3d85..f10cb5977b 100644 --- a/services/keep-web/handler.go +++ b/services/keep-web/handler.go @@ -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 diff --git a/services/keep-web/handler_test.go b/services/keep-web/handler_test.go index 6add8043e0..85a8e500d4 100644 --- a/services/keep-web/handler_test.go +++ b/services/keep-web/handler_test.go @@ -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) diff --git a/services/keep-web/webdav.go b/services/keep-web/webdav.go index 7798a69e3c..1a306e37e9 100644 --- a/services/keep-web/webdav.go +++ b/services/keep-web/webdav.go @@ -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) } -- 2.30.2