diff options
author | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2019-03-12 17:24:22 +0300 |
---|---|---|
committer | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2019-03-12 17:24:22 +0300 |
commit | 95c04bd25d769e171c8a8532c638543d6c2ff27d (patch) | |
tree | 4094fede27412f69f013f04ecd317567c5eeace7 | |
parent | ce40231195f78eb07c1abe06d88f252af7b2a631 (diff) |
Revert "Merge branch 'zj-rename-namespace-creates-parent-dir' into 'master'"
This reverts merge request !1090
-rw-r--r-- | internal/helper/error.go | 16 | ||||
-rw-r--r-- | internal/service/namespace/namespace.go | 97 | ||||
-rw-r--r-- | internal/service/namespace/namespace_test.go | 46 |
3 files changed, 53 insertions, 106 deletions
diff --git a/internal/helper/error.go b/internal/helper/error.go index 98ef1a23e..c20a1690a 100644 --- a/internal/helper/error.go +++ b/internal/helper/error.go @@ -1,8 +1,6 @@ package helper import ( - "fmt" - "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -19,22 +17,12 @@ func DecorateError(code codes.Code, err error) error { return err } -// ErrInternal wraps err with codes.Internal, unless err is already a grpc error +// ErrInternal wrappes 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 +// ErrInvalidArgument wrappes 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 f1682cee8..afd4caf87 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/filepath" + "path" "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 = helper.ErrInvalidArgumentf("name: cannot be empty") +var noNameError = status.Errorf(codes.InvalidArgument, "Name: cannot be empty") func (s *server) NamespaceExists(ctx context.Context, in *gitalypb.NamespaceExistsRequest) (*gitalypb.NamespaceExistsResponse, error) { storagePath, err := helper.GetStorageByName(in.GetStorageName()) @@ -19,15 +19,19 @@ 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 } - exists, err := directoryExists(storagePath, in.GetName()) - - return &gitalypb.NamespaceExistsResponse{Exists: exists}, helper.ErrInternal(err) + 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 + } } func (s *server) AddNamespace(ctx context.Context, in *gitalypb.AddNamespaceRequest) (*gitalypb.AddNamespaceResponse, error) { @@ -37,35 +41,41 @@ func (s *server) AddNamespace(ctx context.Context, in *gitalypb.AddNamespaceRequ } name := in.GetName() - if name == "" { + if len(name) == 0 { return nil, noNameError } - if err := createDirectory(storagePath, name); err != nil { - return nil, helper.ErrInternal(err) + if err = os.MkdirAll(namespacePath(storagePath, name), 0770); err != nil { + return nil, status.Errorf(codes.Internal, "create directory: %v", 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 } - fromPath, toPath := in.GetFrom(), in.GetTo() + if in.GetFrom() == "" || in.GetTo() == "" { + return nil, status.Errorf(codes.InvalidArgument, "from and to cannot be empty") + } - if err = createDirectory(storagePath, filepath.Dir(toPath)); err != nil { - return nil, helper.ErrInternal(err) + // 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 = os.Rename(namespacePath(storagePath, fromPath), namespacePath(storagePath, 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) } return &gitalypb.RenameNamespaceResponse{}, nil @@ -78,53 +88,18 @@ func (s *server) RemoveNamespace(ctx context.Context, in *gitalypb.RemoveNamespa } // Needed as else we might destroy the whole storage - name := in.GetName() - if name == "" { + if in.GetName() == "" { 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, name)); err != nil { - return nil, helper.ErrInternal(err) + if err = os.RemoveAll(namespacePath(storagePath, in.GetName())); err != nil { + return nil, status.Errorf(codes.Internal, "removal: %v", err) } return &gitalypb.RemoveNamespaceResponse{}, nil } -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 +func namespacePath(storage, ns string) string { + return path.Join(storage, ns) } diff --git a/internal/service/namespace/namespace_test.go b/internal/service/namespace/namespace_test.go index 2469d8cc0..a01fe0b0a 100644 --- a/internal/service/namespace/namespace_test.go +++ b/internal/service/namespace/namespace_test.go @@ -1,6 +1,7 @@ package namespace import ( + "context" "io/ioutil" "os" "testing" @@ -39,7 +40,7 @@ func TestNamespaceExists(t *testing.T) { defer conn.Close() // Create one namespace for testing it exists - ctx, cancel := testhelper.Context() + ctx, cancel := context.WithCancel(context.Background()) defer cancel() _, err := client.AddNamespace(ctx, &gitalypb.AddNamespaceRequest{StorageName: "default", Name: "existing"}) @@ -91,7 +92,7 @@ func TestNamespaceExists(t *testing.T) { for _, tc := range queries { t.Run(tc.desc, func(t *testing.T) { - ctx, cancel := testhelper.Context() + ctx, cancel := context.WithCancel(context.Background()) defer cancel() response, err := client.NamespaceExists(ctx, tc.request) @@ -152,7 +153,7 @@ func TestAddNamespace(t *testing.T) { for _, tc := range queries { t.Run(tc.desc, func(t *testing.T) { - ctx, cancel := testhelper.Context() + ctx, cancel := context.WithCancel(context.Background()) defer cancel() _, err := client.AddNamespace(ctx, tc.request) @@ -177,7 +178,7 @@ func TestRemoveNamespace(t *testing.T) { client, conn := newNamespaceClient(t, serverSocketPath) defer conn.Close() - ctx, cancel := testhelper.Context() + ctx, cancel := context.WithCancel(context.Background()) defer cancel() queries := []struct { @@ -229,7 +230,7 @@ func TestRenameNamespace(t *testing.T) { client, conn := newNamespaceClient(t, serverSocketPath) defer conn.Close() - ctx, cancel := testhelper.Context() + ctx, cancel := context.WithCancel(context.Background()) defer cancel() queries := []struct { @@ -262,7 +263,7 @@ func TestRenameNamespace(t *testing.T) { To: "new-path", StorageName: "default", }, - errorCode: codes.Internal, + errorCode: codes.InvalidArgument, }, { desc: "existing destination namespace", @@ -271,37 +272,20 @@ func TestRenameNamespace(t *testing.T) { To: "existing", StorageName: "default", }, - errorCode: codes.Internal, - }, - { - desc: "non existing to parent directory", - request: &gitalypb.RenameNamespaceRequest{ - From: "existing", - To: "ab/cd/non-existing", - StorageName: "default", - }, - errorCode: codes.OK, + errorCode: codes.InvalidArgument, }, } + _, 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.AddNamespace(ctx, &gitalypb.AddNamespaceRequest{ - StorageName: "default", - Name: "existing", - }) - require.NoError(t, err) + _, err := client.RenameNamespace(ctx, tc.request) - 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 { |