From e800254393fe8f2b1dfb350112bf9b030a802088 Mon Sep 17 00:00:00 2001 From: Soulou Date: Thu, 21 Nov 2024 10:47:07 +0100 Subject: [PATCH] package(mongo) Do not break API to handle validation errors with internal errors Fixes #998 --- mongo/CHANGELOG.md | 2 +- mongo/document/base.go | 12 ++- mongo/document/document.go | 126 +++++++++++++++--------------- mongo/document/document_test.go | 133 ++++++++++++++++++++++++++++++++ 4 files changed, 205 insertions(+), 68 deletions(-) create mode 100644 mongo/document/document_test.go diff --git a/mongo/CHANGELOG.md b/mongo/CHANGELOG.md index 3c3ea864..54c6b23f 100644 --- a/mongo/CHANGELOG.md +++ b/mongo/CHANGELOG.md @@ -2,7 +2,7 @@ ## To be Released -* [BREAKING CHANGE]: feat(mongo/document/validation): Add distinction between internal and validation errors ([PR#552](/~https://github.com/Scalingo/go-utils/pull/552)). +* feat(mongo/document/validation): Add distinction between internal and validation errors, introduce `ValidateWithInternalError` method which will be used in priority. * feat(pagination): Add `QueryFunc` parameter to be able to customize the query builder and user `WhereUnsopedQuery` for instance ## 1.3.2 diff --git a/mongo/document/base.go b/mongo/document/base.go index b26064ea..8675272b 100644 --- a/mongo/document/base.go +++ b/mongo/document/base.go @@ -39,6 +39,10 @@ func (d *Base) setUpdatedAt(t time.Time) { d.UpdatedAt = t } +func (d *Base) getUpdatedAt() time.Time { + return d.UpdatedAt +} + func (d Base) scope(query bson.M) bson.M { return query } @@ -47,6 +51,10 @@ func (d *Base) destroy(ctx context.Context, collection string) error { return ReallyDestroy(ctx, collection, d) } -func (d *Base) Validate(_ context.Context) (*errors.ValidationErrors, error) { - return nil, nil +func (d *Base) Validate(_ context.Context) *errors.ValidationErrors { + return nil +} + +func (d *Base) ValidateWithInternalError(_ context.Context) (*errors.ValidationErrors, error) { + return nil, ErrValidateNoInternalErrorFunc } diff --git a/mongo/document/document.go b/mongo/document/document.go index ae8f22b1..11e1797d 100644 --- a/mongo/document/document.go +++ b/mongo/document/document.go @@ -2,6 +2,7 @@ package document import ( "context" + stderrors "errors" "fmt" "time" @@ -22,6 +23,7 @@ type document interface { ensureID() ensureCreatedAt() setUpdatedAt(time.Time) + getUpdatedAt() time.Time Validable } @@ -46,8 +48,18 @@ type Closer interface { Close() } +var ErrValidateNoInternalErrorFunc = stderrors.New("no validation returning an internal error has been implemented") + type Validable interface { - Validate(ctx context.Context) (*errors.ValidationErrors, error) + // Validate will be used if no ValidateWithInternalError is defined on a document + // It is not useful to have both defined on a document, only ValidationWithInternalError + // would be used in this case + Validate(ctx context.Context) *errors.ValidationErrors + + // ValidateWithInternalError will be used in priority if defined on a document + // It will be called for all modifying operations (Create, Save, Update) + // If it returns an internal error, the validation error will be nil. + ValidateWithInternalError(ctx context.Context) (*errors.ValidationErrors, error) } var _ Validable = &Base{} @@ -55,55 +67,66 @@ var _ Validable = &Base{} // Create inserts the document in the database, returns an error if document // already exists and set CreatedAt timestamp func Create(ctx context.Context, collectionName string, doc document) error { - log := logger.Get(ctx).WithFields(logrus.Fields{ - "collection": collectionName, - "doc_id": doc.getID().Hex(), + return save(ctx, collectionName, doc, func(ctx context.Context, collectionName string, doc document) error { + log := logger.Get(ctx) + c := mongo.Session(log).Clone().DB("").C(collectionName) + defer c.Database.Session.Close() + log.WithFields(logrus.Fields{ + "collection": collectionName, + "doc_id": doc.getID().Hex(), + }).Debugf("save '%v'", collectionName) + return c.Insert(doc) }) - doc.ensureID() - doc.ensureCreatedAt() - doc.setUpdatedAt(time.Now()) +} - validationErrors, err := doc.Validate(ctx) - if err != nil { - log.WithError(err).Error("Internal error while validating the document") +func Save(ctx context.Context, collectionName string, doc document) error { + return save(ctx, collectionName, doc, func(ctx context.Context, collectionName string, doc document) error { + log := logger.Get(ctx) + c := mongo.Session(log).Clone().DB("").C(collectionName) + defer c.Database.Session.Close() + log.WithFields(logrus.Fields{ + "collection": collectionName, + "doc_id": doc.getID().Hex(), + }).Debugf("save '%v'", collectionName) + _, err := c.UpsertId(doc.getID(), doc) return err + }) +} + +func Update(ctx context.Context, collectionName string, update bson.M, doc document) error { + return save(ctx, collectionName, doc, func(ctx context.Context, collectionName string, doc document) error { + log := logger.Get(ctx) + c := mongo.Session(log).Clone().DB("").C(collectionName) + defer c.Database.Session.Close() + + if _, ok := update["$set"]; ok { + update["$set"].(bson.M)["updated_at"] = doc.getUpdatedAt() + } + + log.WithFields(logrus.Fields{ + "collection": collectionName, + "doc_id": doc.getID().Hex(), + }).Debugf("update %v", collectionName) + return c.UpdateId(doc.getID(), update) + }) +} + +func save(ctx context.Context, collectionName string, doc document, saveFunc func(context.Context, string, document) error) error { + validationErrors, err := doc.ValidateWithInternalError(ctx) + if err == ErrValidateNoInternalErrorFunc { + validationErrors = doc.Validate(ctx) + } else if err != nil { + return errors.Wrap(ctx, err, "fail to validate document") } if validationErrors != nil { return validationErrors } - c := mongo.Session(log).Clone().DB("").C(collectionName) - defer c.Database.Session.Close() - log.WithFields(logrus.Fields{ - "collection": collectionName, - "doc_id": doc.getID().Hex(), - }).Debugf("save '%v'", collectionName) - return c.Insert(doc) -} - -func Save(ctx context.Context, collectionName string, doc document) error { - log := logger.Get(ctx) doc.ensureID() doc.ensureCreatedAt() doc.setUpdatedAt(time.Now()) - validationErrors, err := doc.Validate(ctx) - if err != nil { - log.WithError(err).Error("Internal error while validating the document") - return err - } - if validationErrors != nil { - return validationErrors - } - - c := mongo.Session(log).Clone().DB("").C(collectionName) - defer c.Database.Session.Close() - log.WithFields(logrus.Fields{ - "collection": collectionName, - "doc_id": doc.getID().Hex(), - }).Debugf("save '%v'", collectionName) - _, err = c.UpsertId(doc.getID(), doc) - return err + return saveFunc(ctx, collectionName, doc) } // Destroy really deletes @@ -263,33 +286,6 @@ func WhereIterUnscoped(ctx context.Context, collectionName string, query bson.M, return nil } -func Update(ctx context.Context, collectionName string, update bson.M, doc document) error { - log := logger.Get(ctx) - c := mongo.Session(log).Clone().DB("").C(collectionName) - defer c.Database.Session.Close() - - now := time.Now() - doc.setUpdatedAt(now) - if _, ok := update["$set"]; ok { - update["$set"].(bson.M)["updated_at"] = now - } - - validationErrors, err := doc.Validate(ctx) - if err != nil { - log.WithError(err).Error("Internal error while validating the document") - return err - } - if validationErrors != nil { - return validationErrors - } - - log.WithFields(logrus.Fields{ - "collection": collectionName, - "doc_id": doc.getID().Hex(), - }).Debugf("update %v", collectionName) - return c.UpdateId(doc.getID(), update) -} - func EnsureParanoidIndices(ctx context.Context, collectionNames ...string) { log := logger.Get(ctx) diff --git a/mongo/document/document_test.go b/mongo/document/document_test.go new file mode 100644 index 00000000..2cc25ef5 --- /dev/null +++ b/mongo/document/document_test.go @@ -0,0 +1,133 @@ +package document + +import ( + "context" + "testing" + + "github.com/Scalingo/go-utils/errors/v2" + "github.com/Scalingo/go-utils/logger" + "github.com/Scalingo/go-utils/mongo" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/mgo.v2" +) + +type unvalidatedDocument struct { + Base `bson:",inline"` +} + +const testDocuments = "test_documents" + +type validatedDocument struct { + Base `bson:",inline"` + Valid bool `bson:"valid" json:"valid"` +} + +func (d *validatedDocument) Validate(ctx context.Context) *errors.ValidationErrors { + verr := errors.NewValidationErrorsBuilder() + if !d.Valid { + verr.Set("valid", "must be true") + } + return verr.Build() +} + +func buildValidatedDocument(valid bool) *validatedDocument { + return &validatedDocument{ + Valid: valid, + } +} + +type validatedWithInternalErrorDocument struct { + validatedDocument `bson:",inline"` + InternalError string `bson:"internal_error" json:"internal_error"` +} + +func (d *validatedWithInternalErrorDocument) ValidateWithInternalError(ctx context.Context) (*errors.ValidationErrors, error) { + if d.InternalError != "" { + return nil, errors.New(ctx, d.InternalError) + } + return d.validatedDocument.Validate(ctx), nil +} + +func buildValidatedWithInternalErrorDocument(valid bool, internalError string) *validatedWithInternalErrorDocument { + return &validatedWithInternalErrorDocument{ + validatedDocument: *buildValidatedDocument(valid), + InternalError: internalError, + } +} + +func TestDocument_Create(t *testing.T) { + t.Cleanup(func() { + coll := mongo.Session(logger.Default()).Clone().DB("").C(testDocuments) + err := coll.DropCollection() + // Handle case when collection does not exist + var queryErr *mgo.QueryError + if !errors.As(err, &queryErr) || queryErr.Message != "ns not found" { + require.NoError(t, err) + } + }) + t.Run("without validation", func(t *testing.T) { + t.Run("it should create the document", func(t *testing.T) { + d := &unvalidatedDocument{} + err := Create(context.Background(), testDocuments, d) + require.NoError(t, err) + assert.NotEmpty(t, d.ID) + assert.NotEmpty(t, d.CreatedAt) + }) + }) + + t.Run("with simple validation", func(t *testing.T) { + t.Run("with a valid document, it should create it", func(t *testing.T) { + d := buildValidatedDocument(true) + err := Create(context.Background(), testDocuments, d) + require.NoError(t, err) + assert.NotEmpty(t, d.ID) + assert.NotEmpty(t, d.CreatedAt) + }) + + t.Run("with an invalid document, it should return a validation error", func(t *testing.T) { + d := buildValidatedDocument(false) + err := Create(context.Background(), testDocuments, d) + require.Error(t, err) + require.IsType(t, &errors.ValidationErrors{}, err) + assert.Empty(t, d.ID) + assert.Empty(t, d.CreatedAt) + }) + }) + + t.Run("with internal error validation", func(t *testing.T) { + t.Run("with a valid document, it should create it", func(t *testing.T) { + d := buildValidatedWithInternalErrorDocument(true, "") + err := Create(context.Background(), testDocuments, d) + require.NoError(t, err) + assert.NotEmpty(t, d.ID) + assert.NotEmpty(t, d.CreatedAt) + }) + t.Run("with an invalid document, it should return a validation error", func(t *testing.T) { + d := buildValidatedWithInternalErrorDocument(false, "") + err := Create(context.Background(), testDocuments, d) + require.Error(t, err) + require.IsType(t, &errors.ValidationErrors{}, err) + assert.Empty(t, d.ID) + assert.Empty(t, d.CreatedAt) + }) + + t.Run("with a validation returning an internal error, it should forward internal error", func(t *testing.T) { + d := buildValidatedWithInternalErrorDocument(true, "internal error when validating") + err := Create(context.Background(), testDocuments, d) + require.Error(t, err) + assert.Contains(t, err.Error(), "internal error when validating") + assert.Empty(t, d.ID) + assert.Empty(t, d.CreatedAt) + }) + + t.Run("with a validation returning an internal error and a validation error, it should return the internal error", func(t *testing.T) { + d := buildValidatedWithInternalErrorDocument(false, "internal error when validating") + err := Create(context.Background(), testDocuments, d) + require.Error(t, err) + assert.Contains(t, err.Error(), "internal error when validating") + assert.Empty(t, d.ID) + assert.Empty(t, d.CreatedAt) + }) + }) +}