From cf145bd437029d02d9c9d1d93752fa641f438463 Mon Sep 17 00:00:00 2001 From: Haytham Abuelfutuh Date: Wed, 1 Sep 2021 17:37:07 -0700 Subject: [PATCH] Fix gorm wrong error type cast (#48) * Fix gorm wrong error type cast Signed-off-by: Haytham Abuelfutuh * Fix unit tests Signed-off-by: Haytham Abuelfutuh * Support unwrapping errors to detect connection problems Signed-off-by: Haytham Abuelfutuh * cleanup Signed-off-by: Haytham Abuelfutuh --- datacalog/go.sum | 3 -- .../pkg/manager/impl/artifact_manager.go | 2 +- datacalog/pkg/repositories/errors/postgres.go | 28 +++++++++++++------ .../repositories/gormimpl/artifact_test.go | 2 +- .../pkg/repositories/gormimpl/dataset_test.go | 2 +- .../pkg/repositories/gormimpl/tag_test.go | 2 +- 6 files changed, 23 insertions(+), 16 deletions(-) diff --git a/datacalog/go.sum b/datacalog/go.sum index 5742d0d6be..55dc41b760 100644 --- a/datacalog/go.sum +++ b/datacalog/go.sum @@ -434,7 +434,6 @@ github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/lib/pq v1.0.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo= github.com/lib/pq v1.1.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo= -github.com/lib/pq v1.2.0 h1:LXpIM/LZ5xGFhOpXAQUIMM1HdyqzVYM13zNdjCEEcA0= github.com/lib/pq v1.2.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo= github.com/lib/pq v1.3.0 h1:/qkRGz8zljWiDcFvgpwUpwIAPu3r07TDvs3Rws+o/pU= github.com/lib/pq v1.3.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo= @@ -686,7 +685,6 @@ golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8U golang.org/x/crypto v0.0.0-20200323165209-0ec3e9974c59/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20201002170205-7f63de1d35b0/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= -golang.org/x/crypto v0.0.0-20201221181555-eec23a3978ad h1:DN0cp81fZ3njFcrLCytUHRSUkqBjfTo4Tx9RJTWs0EY= golang.org/x/crypto v0.0.0-20201221181555-eec23a3978ad/go.mod h1:jdWPYTVW3xRLrWPugEBEK3UY2ZEsg3UU495nc5E+M+I= golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2 h1:It14KIkyBFYkHkwZ7k45minvA9aorojkyjGk9KJ5B/w= golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2/go.mod h1:T9bdIzuCu7OtxOm1hfPfRQxPLYneinmdGuTeoZ9dtd4= @@ -767,7 +765,6 @@ golang.org/x/net v0.0.0-20201031054903-ff519b6c9102/go.mod h1:sp8m0HH+o8qH0wwXwY golang.org/x/net v0.0.0-20201110031124-69a78807bb2b/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= golang.org/x/net v0.0.0-20201209123823-ac852fbbde11/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20201224014010-6772e930b67b/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= -golang.org/x/net v0.0.0-20210119194325-5f4716e94777 h1:003p0dJM77cxMSyCPFphvZf/Y5/NXf5fzg6ufd1/Oew= golang.org/x/net v0.0.0-20210119194325-5f4716e94777/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20210226172049-e18ecbb05110 h1:qWPm9rbaAMKs8Bq/9LRpbMqxWRVUAQwMI9fVrssnTfw= golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= diff --git a/datacalog/pkg/manager/impl/artifact_manager.go b/datacalog/pkg/manager/impl/artifact_manager.go index 85553d1ac1..7ac97d461b 100644 --- a/datacalog/pkg/manager/impl/artifact_manager.go +++ b/datacalog/pkg/manager/impl/artifact_manager.go @@ -161,7 +161,7 @@ func (m *artifactManager) GetArtifact(ctx context.Context, request *datacatalog. if err != nil { if errors.IsDoesNotExistError(err) { - logger.Warnf(ctx, "Artifact does not exist tag: %+v, err %v", request.GetTagName(), err) + logger.Infof(ctx, "Artifact does not exist tag: %+v, err %v", request.GetTagName(), err) m.systemMetrics.doesNotExistCounter.Inc(ctx) } else { logger.Errorf(ctx, "Unable to retrieve Artifact by tag %v, err: %v", request.GetTagName(), err) diff --git a/datacalog/pkg/repositories/errors/postgres.go b/datacalog/pkg/repositories/errors/postgres.go index bbdd7d595d..39cdddcc55 100644 --- a/datacalog/pkg/repositories/errors/postgres.go +++ b/datacalog/pkg/repositories/errors/postgres.go @@ -1,11 +1,15 @@ package errors import ( + "errors" "fmt" + "reflect" + + "github.com/flyteorg/flytestdlib/logger" "github.com/jackc/pgconn" - "github.com/flyteorg/datacatalog/pkg/errors" + catalogErrors "github.com/flyteorg/datacatalog/pkg/errors" "google.golang.org/grpc/codes" "gorm.io/gorm" ) @@ -22,32 +26,38 @@ type postgresErrorTransformer struct { const ( unexpectedType = "unexpected error type for: %v" uniqueConstraintViolation = "value with matching already exists (%s)" - defaultPgError = "failed database operation with %s" + defaultPgError = "failed database operation with code [%s] and msg [%s]" unsupportedTableOperation = "cannot query with specified table attributes: %s" ) func (p *postgresErrorTransformer) fromGormError(err error) error { switch err.Error() { case gorm.ErrRecordNotFound.Error(): - return errors.NewDataCatalogErrorf(codes.NotFound, "entry not found") + return catalogErrors.NewDataCatalogErrorf(codes.NotFound, "entry not found") default: - return errors.NewDataCatalogErrorf(codes.Internal, unexpectedType, err) + return catalogErrors.NewDataCatalogErrorf(codes.Internal, unexpectedType, err) } } func (p *postgresErrorTransformer) ToDataCatalogError(err error) error { - cErr, ok := err.(ConnectError) + if unwrappedErr := errors.Unwrap(err); unwrappedErr != nil { + err = unwrappedErr + } + + pqError, ok := err.(*pgconn.PgError) if !ok { + logger.InfofNoCtx("Unable to cast to pgconn.PgError. Error type: [%v]", + reflect.TypeOf(err)) return p.fromGormError(err) } - pqError := cErr.Unwrap().(*pgconn.PgError) + switch pqError.Code { case uniqueConstraintViolationCode: - return errors.NewDataCatalogErrorf(codes.AlreadyExists, uniqueConstraintViolation, pqError.Message) + return catalogErrors.NewDataCatalogErrorf(codes.AlreadyExists, uniqueConstraintViolation, pqError.Message) case undefinedTable: - return errors.NewDataCatalogErrorf(codes.InvalidArgument, unsupportedTableOperation, pqError.Message) + return catalogErrors.NewDataCatalogErrorf(codes.InvalidArgument, unsupportedTableOperation, pqError.Message) default: - return errors.NewDataCatalogErrorf(codes.Unknown, fmt.Sprintf(defaultPgError, pqError.Message)) + return catalogErrors.NewDataCatalogErrorf(codes.Unknown, fmt.Sprintf(defaultPgError, pqError.Code, pqError.Message)) } } diff --git a/datacalog/pkg/repositories/gormimpl/artifact_test.go b/datacalog/pkg/repositories/gormimpl/artifact_test.go index 49292fb86c..1bff6f85dd 100644 --- a/datacalog/pkg/repositories/gormimpl/artifact_test.go +++ b/datacalog/pkg/repositories/gormimpl/artifact_test.go @@ -275,7 +275,7 @@ func TestCreateArtifactAlreadyExists(t *testing.T) { assert.Error(t, err) dcErr, ok := err.(apiErrors.DataCatalogError) assert.True(t, ok) - assert.Equal(t, dcErr.Code(), codes.AlreadyExists) + assert.Equal(t, dcErr.Code().String(), codes.AlreadyExists.String()) } func TestListArtifactsWithPartition(t *testing.T) { diff --git a/datacalog/pkg/repositories/gormimpl/dataset_test.go b/datacalog/pkg/repositories/gormimpl/dataset_test.go index 2f2ef8e980..a4904da1db 100644 --- a/datacalog/pkg/repositories/gormimpl/dataset_test.go +++ b/datacalog/pkg/repositories/gormimpl/dataset_test.go @@ -246,7 +246,7 @@ func TestCreateDatasetAlreadyExists(t *testing.T) { assert.Error(t, err) dcErr, ok := err.(datacatalog_error.DataCatalogError) assert.True(t, ok) - assert.Equal(t, dcErr.Code(), codes.AlreadyExists) + assert.Equal(t, dcErr.Code().String(), codes.AlreadyExists.String()) } func TestListDatasets(t *testing.T) { diff --git a/datacalog/pkg/repositories/gormimpl/tag_test.go b/datacalog/pkg/repositories/gormimpl/tag_test.go index ee33417a47..9dbb4270a5 100644 --- a/datacalog/pkg/repositories/gormimpl/tag_test.go +++ b/datacalog/pkg/repositories/gormimpl/tag_test.go @@ -157,5 +157,5 @@ func TestTagAlreadyExists(t *testing.T) { assert.Error(t, err) dcErr, ok := err.(datacatalog_error.DataCatalogError) assert.True(t, ok) - assert.Equal(t, dcErr.Code(), codes.AlreadyExists) + assert.Equal(t, dcErr.Code().String(), codes.AlreadyExists.String()) }