Merge branch 'zj-rename-namespace-creates-parent-dir' into 'master'

RenameNamespace RPC creates parent directories for the new location

See merge request gitlab-org/gitaly!1090

4 files changed, 111 insertions, 53 deletions

diff --git a/changelogs/unreleased/zj-rename-namespace-creates-parent-dir.yml b/changelogs/unreleased/zj-rename-namespace-creates-parent-dir.yml
new file mode 100644
index 000000000..5f422d318
--- /dev/null
+++ b/changelogs/unreleased/zj-rename-namespace-creates-parent-dir.yml
@@ -0,0 +1,5 @@
+---
+title: RenameNamespace RPC creates parent directories for the new location
+merge_request: 1090
+author:
+type: changed
diff --git a/internal/helper/error.go b/internal/helper/error.go
index c20a1690a..98ef1a23e 100644
--- a/internal/helper/error.go
+++ b/internal/helper/error.go
@@ -1,6 +1,8 @@
 package helper
 
 import (
+	"fmt"
+
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -17,12 +19,22 @@ func DecorateError(code codes.Code, err error) error {
 	return err
 }
 
-// ErrInternal wrappes err with codes.Internal, unless err is already a grpc error
+// ErrInternal wraps err with codes.Internal, unless err is already a grpc error
 func ErrInternal(err error) error { return DecorateError(codes.Internal, err) }
 
-// ErrInvalidArgument wrappes err with codes.InvalidArgument, unless err is already a grpc error
+// ErrInternalf wraps err with codes.Internal, unless err is already a grpc error
+func ErrInternalf(format string, a ...interface{}) error {
+	return DecorateError(codes.Internal, fmt.Errorf(format, a...))
+}
+
+// ErrInvalidArgument wraps err with codes.InvalidArgument, unless err is already a grpc error
 func ErrInvalidArgument(err error) error { return DecorateError(codes.InvalidArgument, err) }
 
+// ErrInvalidArgumentf wraps err with codes.InvalidArgument, unless err is already a grpc error
+func ErrInvalidArgumentf(format string, a ...interface{}) error {
+	return DecorateError(codes.InvalidArgument, fmt.Errorf(format, a...))
+}
+
 // ErrPreconditionFailed wraps error with codes.FailedPrecondition, unless err is already a grpc error
 func ErrPreconditionFailed(err error) error { return DecorateError(codes.FailedPrecondition, err) }
 
diff --git a/internal/service/namespace/namespace.go b/internal/service/namespace/namespace.go
index afd4caf87..f1682cee8 100644
--- a/internal/service/namespace/namespace.go
+++ b/internal/service/namespace/namespace.go
@@ -2,16 +2,16 @@ package namespace
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"os"
-	"path"
+	"path/filepath"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
-var noNameError = status.Errorf(codes.InvalidArgument, "Name: cannot be empty")
+var noNameError = helper.ErrInvalidArgumentf("name: cannot be empty")
 
 func (s *server) NamespaceExists(ctx context.Context, in *gitalypb.NamespaceExistsRequest) (*gitalypb.NamespaceExistsResponse, error) {
 	storagePath, err := helper.GetStorageByName(in.GetStorageName())
@@ -19,19 +19,15 @@ func (s *server) NamespaceExists(ctx context.Context, in *gitalypb.NamespaceExis
 		return nil, err
 	}
 
-	// This case should return an error, as else we'd actually say the path exists as the
-	// storage exists
+	// This case should return an error, as else we'd actually say the path
+	// exists as the storage exists
 	if in.GetName() == "" {
 		return nil, noNameError
 	}
 
-	if fi, err := os.Stat(namespacePath(storagePath, in.GetName())); os.IsNotExist(err) {
-		return &gitalypb.NamespaceExistsResponse{Exists: false}, nil
-	} else if err != nil {
-		return nil, status.Errorf(codes.Internal, "could not stat the directory: %v", err)
-	} else {
-		return &gitalypb.NamespaceExistsResponse{Exists: fi.IsDir()}, nil
-	}
+	exists, err := directoryExists(storagePath, in.GetName())
+
+	return &gitalypb.NamespaceExistsResponse{Exists: exists}, helper.ErrInternal(err)
 }
 
 func (s *server) AddNamespace(ctx context.Context, in *gitalypb.AddNamespaceRequest) (*gitalypb.AddNamespaceResponse, error) {
@@ -41,41 +37,35 @@ func (s *server) AddNamespace(ctx context.Context, in *gitalypb.AddNamespaceRequ
 	}
 
 	name := in.GetName()
-	if len(name) == 0 {
+	if name == "" {
 		return nil, noNameError
 	}
 
-	if err = os.MkdirAll(namespacePath(storagePath, name), 0770); err != nil {
-		return nil, status.Errorf(codes.Internal, "create directory: %v", err)
+	if err := createDirectory(storagePath, name); err != nil {
+		return nil, helper.ErrInternal(err)
 	}
 
 	return &gitalypb.AddNamespaceResponse{}, nil
 }
 
 func (s *server) RenameNamespace(ctx context.Context, in *gitalypb.RenameNamespaceRequest) (*gitalypb.RenameNamespaceResponse, error) {
+	if err := validateRenameNamespaceRequest(in); err != nil {
+		return nil, helper.ErrInvalidArgument(err)
+	}
+
 	storagePath, err := helper.GetStorageByName(in.GetStorageName())
 	if err != nil {
 		return nil, err
 	}
 
-	if in.GetFrom() == "" || in.GetTo() == "" {
-		return nil, status.Errorf(codes.InvalidArgument, "from and to cannot be empty")
-	}
+	fromPath, toPath := in.GetFrom(), in.GetTo()
 
-	// No need to check if the from path exists, if it doesn't, we'd later get an
-	// os.LinkError
-	toExistsCheck := &gitalypb.NamespaceExistsRequest{StorageName: in.StorageName, Name: in.GetTo()}
-	if exists, err := s.NamespaceExists(ctx, toExistsCheck); err != nil {
-		return nil, err
-	} else if exists.Exists {
-		return nil, status.Errorf(codes.InvalidArgument, "to directory %s already exists", in.GetTo())
+	if err = createDirectory(storagePath, filepath.Dir(toPath)); err != nil {
+		return nil, helper.ErrInternal(err)
 	}
 
-	err = os.Rename(namespacePath(storagePath, in.GetFrom()), namespacePath(storagePath, in.GetTo()))
-	if _, ok := err.(*os.LinkError); ok {
-		return nil, status.Errorf(codes.InvalidArgument, "from directory %s not found", in.GetFrom())
-	} else if err != nil {
-		return nil, status.Errorf(codes.Internal, "rename: %v", err)
+	if err = os.Rename(namespacePath(storagePath, fromPath), namespacePath(storagePath, toPath)); err != nil {
+		return nil, helper.ErrInternal(err)
 	}
 
 	return &gitalypb.RenameNamespaceResponse{}, nil
@@ -88,18 +78,53 @@ func (s *server) RemoveNamespace(ctx context.Context, in *gitalypb.RemoveNamespa
 	}
 
 	// Needed as else we might destroy the whole storage
-	if in.GetName() == "" {
+	name := in.GetName()
+	if name == "" {
 		return nil, noNameError
 	}
 
 	// os.RemoveAll is idempotent by itself
 	// No need to check if the directory exists, or not
-	if err = os.RemoveAll(namespacePath(storagePath, in.GetName())); err != nil {
-		return nil, status.Errorf(codes.Internal, "removal: %v", err)
+	if err = os.RemoveAll(namespacePath(storagePath, name)); err != nil {
+		return nil, helper.ErrInternal(err)
 	}
 	return &gitalypb.RemoveNamespaceResponse{}, nil
 }
 
-func namespacePath(storage, ns string) string {
-	return path.Join(storage, ns)
+func namespacePath(storagePath, ns string) string {
+	return filepath.Join(storagePath, ns)
+}
+
+func createDirectory(storagePath, namespace string) error {
+	return os.MkdirAll(namespacePath(storagePath, namespace), 0755)
+}
+
+func directoryExists(storagePath, namespace string) (bool, error) {
+	fi, err := os.Stat(namespacePath(storagePath, namespace))
+	if os.IsNotExist(err) {
+		return false, nil
+	} else if err != nil {
+		return false, err
+	}
+
+	if !fi.IsDir() {
+		return false, fmt.Errorf("expected directory, found file %s", namespace)
+	}
+
+	return true, nil
+}
+
+func validateRenameNamespaceRequest(req *gitalypb.RenameNamespaceRequest) error {
+	if _, err := helper.GetStorageByName(req.GetStorageName()); err != nil {
+		return err
+	}
+
+	if req.GetFrom() == "" {
+		return errors.New("from field cannot be empty")
+	}
+	if req.GetTo() == "" {
+		return errors.New("to field cannot be empty")
+	}
+
+	return nil
 }
diff --git a/internal/service/namespace/namespace_test.go b/internal/service/namespace/namespace_test.go
index a01fe0b0a..2469d8cc0 100644
--- a/internal/service/namespace/namespace_test.go
+++ b/internal/service/namespace/namespace_test.go
@@ -1,7 +1,6 @@
 package namespace
 
 import (
-	"context"
 	"io/ioutil"
 	"os"
 	"testing"
@@ -40,7 +39,7 @@ func TestNamespaceExists(t *testing.T) {
 	defer conn.Close()
 
 	// Create one namespace for testing it exists
-	ctx, cancel := context.WithCancel(context.Background())
+	ctx, cancel := testhelper.Context()
 	defer cancel()
 
 	_, err := client.AddNamespace(ctx, &gitalypb.AddNamespaceRequest{StorageName: "default", Name: "existing"})
@@ -92,7 +91,7 @@ func TestNamespaceExists(t *testing.T) {
 
 	for _, tc := range queries {
 		t.Run(tc.desc, func(t *testing.T) {
-			ctx, cancel := context.WithCancel(context.Background())
+			ctx, cancel := testhelper.Context()
 			defer cancel()
 			response, err := client.NamespaceExists(ctx, tc.request)
 
@@ -153,7 +152,7 @@ func TestAddNamespace(t *testing.T) {
 
 	for _, tc := range queries {
 		t.Run(tc.desc, func(t *testing.T) {
-			ctx, cancel := context.WithCancel(context.Background())
+			ctx, cancel := testhelper.Context()
 			defer cancel()
 
 			_, err := client.AddNamespace(ctx, tc.request)
@@ -178,7 +177,7 @@ func TestRemoveNamespace(t *testing.T) {
 	client, conn := newNamespaceClient(t, serverSocketPath)
 	defer conn.Close()
 
-	ctx, cancel := context.WithCancel(context.Background())
+	ctx, cancel := testhelper.Context()
 	defer cancel()
 
 	queries := []struct {
@@ -230,7 +229,7 @@ func TestRenameNamespace(t *testing.T) {
 	client, conn := newNamespaceClient(t, serverSocketPath)
 	defer conn.Close()
 
-	ctx, cancel := context.WithCancel(context.Background())
+	ctx, cancel := testhelper.Context()
 	defer cancel()
 
 	queries := []struct {
@@ -263,7 +262,7 @@ func TestRenameNamespace(t *testing.T) {
 				To:          "new-path",
 				StorageName: "default",
 			},
-			errorCode: codes.InvalidArgument,
+			errorCode: codes.Internal,
 		},
 		{
 			desc: "existing destination namespace",
@@ -272,20 +271,37 @@ func TestRenameNamespace(t *testing.T) {
 				To:          "existing",
 				StorageName: "default",
 			},
-			errorCode: codes.InvalidArgument,
+			errorCode: codes.Internal,
+		},
+		{
+			desc: "non existing to parent directory",
+			request: &gitalypb.RenameNamespaceRequest{
+				From:        "existing",
+				To:          "ab/cd/non-existing",
+				StorageName: "default",
+			},
+			errorCode: codes.OK,
 		},
 	}
 
-	_, err := client.AddNamespace(ctx, &gitalypb.AddNamespaceRequest{
-		StorageName: "default",
-		Name:        "existing",
-	})
-	require.NoError(t, err)
-
 	for _, tc := range queries {
 		t.Run(tc.desc, func(t *testing.T) {
-			_, err := client.RenameNamespace(ctx, tc.request)
+			_, err := client.AddNamespace(ctx, &gitalypb.AddNamespaceRequest{
+				StorageName: "default",
+				Name:        "existing",
+			})
+			require.NoError(t, err)
 
+			defer client.RemoveNamespace(ctx, &gitalypb.RemoveNamespaceRequest{
+				StorageName: tc.request.StorageName,
+				Name:        tc.request.To,
+			})
+
+			_, err = client.RenameNamespace(ctx, tc.request)
+
+			if tc.errorCode != helper.GrpcCode(err) {
+				t.Fatal(err)
+			}
 			require.Equal(t, tc.errorCode, helper.GrpcCode(err))
 
 			if tc.errorCode == codes.OK {