From 25ab5d2bf337e12c0f76ac24d6b24aa7ddd011ec Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 19 Mar 2019 13:36:35 +0100 Subject: [PATCH 1/2] Revert "Revert "Merge branch 'zj-rename-namespace-creates-parent-dir' into 'master'"" This reverts commit 95c04bd25d769e171c8a8532c638543d6c2ff27d. The original commit created regressions in Rails, where this commit reverts it, so later commits can update and fix the behaviour. --- internal/helper/error.go | 9 +- internal/service/namespace/namespace.go | 97 ++++++++++++-------- internal/service/namespace/namespace_test.go | 46 +++++++--- 3 files changed, 99 insertions(+), 53 deletions(-) diff --git a/internal/helper/error.go b/internal/helper/error.go index 41bb907b2c1..8bb2ffe5fbe 100644 --- a/internal/helper/error.go +++ b/internal/helper/error.go @@ -19,9 +19,14 @@ 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) } +// 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) } @@ -30,7 +35,7 @@ func ErrInvalidArgumentf(format string, a ...interface{}) error { return DecorateError(codes.InvalidArgument, fmt.Errorf(format, a...)) } -// ErrPreconditionFailed wraps err with codes.FailedPrecondition, unless err is already a grpc error +// ErrPreconditionFailed wraps error with codes.FailedPrecondition, unless err is already a grpc error func ErrPreconditionFailed(err error) error { return DecorateError(codes.FailedPrecondition, err) } // ErrPreconditionFailedf wraps err with codes.FailedPrecondition, unless err is already a grpc error diff --git a/internal/service/namespace/namespace.go b/internal/service/namespace/namespace.go index afd4caf876a..f1682cee8d2 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 a01fe0b0af4..2469d8cc092 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 { -- GitLab From 73cdfc41cc30624c2c7d9924e816b31581d344a8 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 19 Mar 2019 13:47:51 +0100 Subject: [PATCH 2/2] Renaming RPC returns InvalidArgument if target exists Previously the status code returned when the target directory already existed was Internal. Even before that it was InvalidArgument. Some clients depend on the old behaviour, which is now reverted to. --- internal/service/namespace/namespace.go | 11 ++++++++++- internal/service/namespace/namespace_test.go | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/internal/service/namespace/namespace.go b/internal/service/namespace/namespace.go index f1682cee8d2..abbe9ba889a 100644 --- a/internal/service/namespace/namespace.go +++ b/internal/service/namespace/namespace.go @@ -115,7 +115,8 @@ func directoryExists(storagePath, namespace string) (bool, error) { } func validateRenameNamespaceRequest(req *gitalypb.RenameNamespaceRequest) error { - if _, err := helper.GetStorageByName(req.GetStorageName()); err != nil { + storePath, err := helper.GetStorageByName(req.GetStorageName()) + if err != nil { return err } @@ -126,5 +127,13 @@ func validateRenameNamespaceRequest(req *gitalypb.RenameNamespaceRequest) error return errors.New("to field cannot be empty") } + if found, err := directoryExists(storePath, req.GetTo()); found || err != nil { + if err != nil { + return err + } + + return errors.New("target directory already exists") + } + return nil } diff --git a/internal/service/namespace/namespace_test.go b/internal/service/namespace/namespace_test.go index 2469d8cc092..9a1f5f4f726 100644 --- a/internal/service/namespace/namespace_test.go +++ b/internal/service/namespace/namespace_test.go @@ -271,7 +271,7 @@ func TestRenameNamespace(t *testing.T) { To: "existing", StorageName: "default", }, - errorCode: codes.Internal, + errorCode: codes.InvalidArgument, }, { desc: "non existing to parent directory", -- GitLab