webdav: factor out a moveFiles function, and make the tests call that

instead of FileSystem.Rename directly.

Dir.Rename's behavior wrt overwriting existing files and directories is
OS-dependent.

Fixes golang/go#9786

Change-Id: If42728caa6f0f38f8e3d6b1fcdda8c2d272080d6
Reviewed-on: https://go-review.googlesource.com/4341
Reviewed-by: Nick Cooper <[email protected]>
Reviewed-by: Alex Brainman <[email protected]>

Author: nigeltao

webdav/file.go

@@ -30,6 +30,10 @@ func slashClean(name string) string {
 //
 // Each method has the same semantics as the os package's function of the same
 // name.
+//
+// Note that the os.Rename documentation says that "OS-specific restrictions
+// might apply". In particular, whether or not renaming a file or directory
+// overwriting another existing file or directory is an error is OS-dependent.
 type FileSystem interface {
 	Mkdir(name string, perm os.FileMode) error
 	OpenFile(name string, flag int, perm os.FileMode) (File, error)
@@ -548,6 +552,36 @@ func (f *memFile) Write(p []byte) (int, error) {
 	return lenp, nil
 }
 
+// moveFiles moves files and/or directories from src to dst.
+//
+// See section 9.9.4 for when various HTTP status codes apply.
+func moveFiles(fs FileSystem, src, dst string, overwrite bool) (status int, err error) {
+	created := false
+	if _, err := fs.Stat(dst); err != nil {
+		if !os.IsNotExist(err) {
+			return http.StatusForbidden, err
+		}
+		created = true
+	} else if overwrite {
+		// Section 9.9.3 says that "If a resource exists at the destination
+		// and the Overwrite header is "T", then prior to performing the move,
+		// the server must perform a DELETE with "Depth: infinity" on the
+		// destination resource.
+		if err := fs.RemoveAll(dst); err != nil {
+			return http.StatusForbidden, err
+		}
+	} else {
+		return http.StatusPreconditionFailed, os.ErrExist
+	}
+	if err := fs.Rename(src, dst); err != nil {
+		return http.StatusForbidden, err
+	}
+	if created {
+		return http.StatusCreated, nil
+	}
+	return http.StatusNoContent, nil
+}
+
 // copyFiles copies files and/or directories from src to dst.
 //
 // See section 9.8.5 for when various HTTP status codes apply.

webdav/file_test.go

@@ -321,44 +321,41 @@ func testFS(t *testing.T, fs FileSystem) {
 		"  stat /d want dir",
 		"  stat /d/m want dir",
 		"  find / /a /b /d /d/m",
-		"rename /b /b want ok",
-		"  stat /b want 2",
-		"  stat /c want errNotExist",
-		"rename /b /c want ok",
+		"move__ o=F /b /c want ok",
 		"  stat /b want errNotExist",
 		"  stat /c want 2",
 		"  stat /d/m want dir",
 		"  stat /d/n want errNotExist",
 		"  find / /a /c /d /d/m",
-		"rename /d/m /d/n want ok",
+		"move__ o=F /d/m /d/n want ok",
 		"create /d/n/q QQQQ want ok",
 		"  stat /d/m want errNotExist",
 		"  stat /d/n want dir",
 		"  stat /d/n/q want 4",
-		"rename /d /d/n/z want err",
-		"rename /c /d/n/q want ok",
+		"move__ o=F /d /d/n/z want err",
+		"move__ o=T /c /d/n/q want ok",
 		"  stat /c want errNotExist",
 		"  stat /d/n/q want 2",
 		"  find / /a /d /d/n /d/n/q",
 		"create /d/n/r RRRRR want ok",
 		"mk-dir /u want ok",
 		"mk-dir /u/v want ok",
-		"rename /d/n /u want err",
+		"move__ o=F /d/n /u want errExist",
 		"create /t TTTTTT want ok",
-		"rename /d/n /t want err",
+		"move__ o=F /d/n /t want errExist",
 		"rm-all /t want ok",
-		"rename /d/n /t want ok",
+		"move__ o=F /d/n /t want ok",
 		"  stat /d want dir",
 		"  stat /d/n want errNotExist",
 		"  stat /d/n/r want errNotExist",
 		"  stat /t want dir",
 		"  stat /t/q want 2",
 		"  stat /t/r want 5",
 		"  find / /a /d /t /t/q /t/r /u /u/v",
-		"rename /t / want err",
-		"rename /t /u/v want ok",
+		"move__ o=F /t / want errExist",
+		"move__ o=T /t /u/v want ok",
 		"  stat /u/v/r want 5",
-		"rename / /z want err",
+		"move__ o=F / /z want err",
 		"  find / /a /d /u /u/v /u/v/q /u/v/r",
 		"  stat /a want 1",
 		"  stat /b want errNotExist",
@@ -445,13 +442,13 @@ func testFS(t *testing.T, fs FileSystem) {
 				t.Fatalf("test case #%d %q:\ngot  %s\nwant %s", i, tc, got, want)
 			}
 
-		case "copy__", "mk-dir", "rename", "rm-all", "stat":
+		case "copy__", "mk-dir", "move__", "rm-all", "stat":
 			nParts := 3
 			switch op {
 			case "copy__":
 				nParts = 6
-			case "rename":
-				nParts = 4
+			case "move__":
+				nParts = 5
 			}
 			parts := strings.Split(arg, " ")
 			if len(parts) != nParts {
@@ -461,18 +458,15 @@ func testFS(t *testing.T, fs FileSystem) {
 			got, opErr := "", error(nil)
 			switch op {
 			case "copy__":
-				overwrite, depth := false, 0
-				if parts[0] == "o=T" {
-					overwrite = true
-				}
+				depth := 0
 				if parts[1] == "d=∞" {
 					depth = infiniteDepth
 				}
-				_, opErr = copyFiles(fs, parts[2], parts[3], overwrite, depth, 0)
+				_, opErr = copyFiles(fs, parts[2], parts[3], parts[0] == "o=T", depth, 0)
 			case "mk-dir":
 				opErr = fs.Mkdir(parts[0], 0777)
-			case "rename":
-				opErr = fs.Rename(parts[0], parts[1])
+			case "move__":
+				_, opErr = moveFiles(fs, parts[1], parts[2], parts[0] == "o=T")
 			case "rm-all":
 				opErr = fs.RemoveAll(parts[0])
 			case "stat":

webdav/webdav.go

@@ -247,36 +247,7 @@ func (h *Handler) handleCopyMove(w http.ResponseWriter, r *http.Request) (status
 			return http.StatusBadRequest, errInvalidDepth
 		}
 	}
-
-	created := false
-	if _, err := h.FileSystem.Stat(dst); err != nil {
-		if !os.IsNotExist(err) {
-			return http.StatusForbidden, err
-		}
-		created = true
-	} else {
-		switch r.Header.Get("Overwrite") {
-		case "T":
-			// Section 9.9.3 says that "If a resource exists at the destination
-			// and the Overwrite header is "T", then prior to performing the move,
-			// the server must perform a DELETE with "Depth: infinity" on the
-			// destination resource.
-			if err := h.FileSystem.RemoveAll(dst); err != nil {
-				return http.StatusForbidden, err
-			}
-		case "F":
-			return http.StatusPreconditionFailed, os.ErrExist
-		default:
-			return http.StatusBadRequest, errInvalidOverwrite
-		}
-	}
-	if err := h.FileSystem.Rename(src, dst); err != nil {
-		return http.StatusForbidden, err
-	}
-	if created {
-		return http.StatusCreated, nil
-	}
-	return http.StatusNoContent, nil
+	return moveFiles(h.FileSystem, src, dst, r.Header.Get("Overwrite") == "T")
 }
 
 func (h *Handler) handleLock(w http.ResponseWriter, r *http.Request) (retStatus int, retErr error) {
@@ -450,7 +421,6 @@ var (
 	errInvalidIfHeader         = errors.New("webdav: invalid If header")
 	errInvalidLockInfo         = errors.New("webdav: invalid lock info")
 	errInvalidLockToken        = errors.New("webdav: invalid lock token")
-	errInvalidOverwrite        = errors.New("webdav: invalid overwrite")
 	errInvalidPropfind         = errors.New("webdav: invalid propfind")
 	errInvalidResponse         = errors.New("webdav: invalid response")
 	errInvalidTimeout          = errors.New("webdav: invalid timeout")