From e45973d1a2ce2e236c7e8af902a356b15079d6d8 Mon Sep 17 00:00:00 2001 From: Arthur Clement Date: Mon, 22 Nov 2021 21:46:31 +1100 Subject: [PATCH 1/7] add optimistic locking --- field.go | 21 ++++++++++++++++++++- model.go | 11 +++++++++-- model_test.go | 11 ++++++++++- operation.go | 10 +++++++++- 4 files changed, 48 insertions(+), 5 deletions(-) diff --git a/field.go b/field.go index 378e59d..31e5fd5 100644 --- a/field.go +++ b/field.go @@ -18,6 +18,10 @@ type DateFields struct { UpdatedAt time.Time `json:"updated_at" bson:"updated_at"` } +type VersionField struct { + Version int `json:"version" bson:"version"` +} + // PrepareID method prepares the ID value to be used for filtering // e.g convert hex-string ID value to bson.ObjectId func (f *IDField) PrepareID(id interface{}) (interface{}, error) { @@ -39,6 +43,21 @@ func (f *IDField) SetID(id interface{}) { f.ID = id.(primitive.ObjectID) } +// GetVersion returns the model version field +func (f *VersionField) GetVersion() interface{} { + return f.Version +} + +// GetVersionFieldName returns the field name holding the version field (has to match the bson tag) +func (f *VersionField) GetVersionFieldName() string { + return "version" +} + +// SetVersion returns the model version field +func (f *VersionField) IncrementVersion() { + f.Version++ +} + //-------------------------------- // DateField methods //-------------------------------- @@ -51,7 +70,7 @@ func (f *DateFields) Creating() error { return nil } -// Saving hook is used here to set the `updated_at` field +// Saving hook is used here to set the `updated_at` field // value when creating or updateing a model. // TODO: get context as param the next version(4). func (f *DateFields) Saving() error { diff --git a/model.go b/model.go index 8a85dfa..d6b84ee 100644 --- a/model.go +++ b/model.go @@ -26,10 +26,17 @@ type Model interface { SetID(id interface{}) } +type Versionable interface { + GetVersion() interface{} + GetVersionFieldName() string + IncrementVersion() +} + // DefaultModel struct contains a model's default fields. type DefaultModel struct { - IDField `bson:",inline"` - DateFields `bson:",inline"` + IDField `bson:",inline"` + DateFields `bson:",inline"` + VersionField `bson:",inline"` } // Creating function calls the inner fields' defined hooks diff --git a/model_test.go b/model_test.go index 835ec4b..4f70eb8 100644 --- a/model_test.go +++ b/model_test.go @@ -1,10 +1,11 @@ package mgm_test import ( + "testing" + "github.com/kamva/mgm/v3/internal/util" "github.com/stretchr/testify/require" "go.mongodb.org/mongo-driver/bson/primitive" - "testing" ) func TestPrepareInvalidId(t *testing.T) { @@ -23,3 +24,11 @@ func TestPrepareId(t *testing.T) { require.Equal(t, val.(primitive.ObjectID), id) util.AssertErrIsNil(t, err) } + +func TestVersion(t *testing.T) { + d := &Doc{} + require.Equal(t, 0, d.GetVersion()) + require.Equal(t, "version", d.GetVersionFieldName()) + d.IncrementVersion() + require.Equal(t, 1, d.GetVersion()) +} diff --git a/operation.go b/operation.go index f12a75c..c08ca34 100644 --- a/operation.go +++ b/operation.go @@ -2,6 +2,7 @@ package mgm import ( "context" + "github.com/kamva/mgm/v3/field" "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/mongo/options" @@ -35,7 +36,14 @@ func update(ctx context.Context, c *Collection, model Model, opts ...*options.Up return err } - res, err := c.UpdateOne(ctx, bson.M{field.ID: model.GetID()}, bson.M{"$set": model}, opts...) + query := bson.M{field.ID: model.GetID()} + modelVersionable, isVersionable := model.(Versionable) + if isVersionable { + query[modelVersionable.GetVersionFieldName()] = modelVersionable.GetVersion() + modelVersionable.IncrementVersion() + } + + res, err := c.UpdateOne(ctx, query, bson.M{"$set": model}, opts...) if err != nil { return err From 4204335754344da0dd9900a2dfbef034462c195d Mon Sep 17 00:00:00 2001 From: Arthur Clement Date: Mon, 22 Nov 2021 22:20:55 +1100 Subject: [PATCH 2/7] add optimistic locking --- operation.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/operation.go b/operation.go index c08ca34..ecab8bf 100644 --- a/operation.go +++ b/operation.go @@ -2,6 +2,7 @@ package mgm import ( "context" + "fmt" "github.com/kamva/mgm/v3/field" "go.mongodb.org/mongo-driver/bson" @@ -45,6 +46,10 @@ func update(ctx context.Context, c *Collection, model Model, opts ...*options.Up res, err := c.UpdateOne(ctx, query, bson.M{"$set": model}, opts...) + if isVersionable && res.MatchedCount == 0 { + return fmt.Errorf("versioning error : document %v %v with version %v could not be found", c.Name(), model.GetID(), modelVersionable.GetVersion()) + } + if err != nil { return err } From 6ea90e66bcf59d073e94fc85f96af260fe484054 Mon Sep 17 00:00:00 2001 From: Arthur Clement Date: Mon, 22 Nov 2021 22:28:39 +1100 Subject: [PATCH 3/7] add optimistic locking --- operation.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/operation.go b/operation.go index ecab8bf..c165bd4 100644 --- a/operation.go +++ b/operation.go @@ -46,14 +46,14 @@ func update(ctx context.Context, c *Collection, model Model, opts ...*options.Up res, err := c.UpdateOne(ctx, query, bson.M{"$set": model}, opts...) - if isVersionable && res.MatchedCount == 0 { - return fmt.Errorf("versioning error : document %v %v with version %v could not be found", c.Name(), model.GetID(), modelVersionable.GetVersion()) - } - if err != nil { return err } + if isVersionable && res.MatchedCount == 0 { + return fmt.Errorf("versioning error : document %v %v with version %v could not be found", c.Name(), model.GetID(), modelVersionable.GetVersion()) + } + return callToAfterUpdateHooks(ctx, res, model) } From b312abca8137d4137f915a4a2fa5babd265ee4da Mon Sep 17 00:00:00 2001 From: Arthur Clement Date: Tue, 23 Nov 2021 18:31:38 +1100 Subject: [PATCH 4/7] handle adding add optimistic locking to documents created without it --- operation.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/operation.go b/operation.go index c165bd4..1e4c00f 100644 --- a/operation.go +++ b/operation.go @@ -39,8 +39,13 @@ func update(ctx context.Context, c *Collection, model Model, opts ...*options.Up query := bson.M{field.ID: model.GetID()} modelVersionable, isVersionable := model.(Versionable) + var currentVersion interface{} if isVersionable { - query[modelVersionable.GetVersionFieldName()] = modelVersionable.GetVersion() + currentVersion = modelVersionable.GetVersion() + //handle adding versionning to documents that were created without versionning + if currentVersion != 0 && currentVersion != nil { + query[modelVersionable.GetVersionFieldName()] = currentVersion + } modelVersionable.IncrementVersion() } @@ -51,7 +56,7 @@ func update(ctx context.Context, c *Collection, model Model, opts ...*options.Up } if isVersionable && res.MatchedCount == 0 { - return fmt.Errorf("versioning error : document %v %v with version %v could not be found", c.Name(), model.GetID(), modelVersionable.GetVersion()) + return fmt.Errorf("versioning error : document %v %v with version %v could not be found", c.Name(), model.GetID(), currentVersion) } return callToAfterUpdateHooks(ctx, res, model) From 9cb9fe3dc5e9deeb5b3b86395071833a6a7f2bc6 Mon Sep 17 00:00:00 2001 From: Arthur Clement Date: Wed, 24 Nov 2021 10:08:53 +1100 Subject: [PATCH 5/7] handle adding add optimistic locking to documents created without it --- operation.go | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/operation.go b/operation.go index 1e4c00f..f6de426 100644 --- a/operation.go +++ b/operation.go @@ -3,6 +3,7 @@ package mgm import ( "context" "fmt" + "time" "github.com/kamva/mgm/v3/field" "go.mongodb.org/mongo-driver/bson" @@ -42,10 +43,26 @@ func update(ctx context.Context, c *Collection, model Model, opts ...*options.Up var currentVersion interface{} if isVersionable { currentVersion = modelVersionable.GetVersion() - //handle adding versionning to documents that were created without versionning - if currentVersion != 0 && currentVersion != nil { + + //Handle adding versionning for documents that were created without it + //In this case, the version field would be of zero value and not exist in the DB + //Add $or exists false in the query condition for this case + isCurrentVersionZero := false + switch c := currentVersion.(type) { + case string: + isCurrentVersionZero = c == "" + case int: + isCurrentVersionZero = c == 0 + case time.Time: + isCurrentVersionZero = c.IsZero() + } + + if isCurrentVersionZero { + query["$or"] = bson.A{bson.M{modelVersionable.GetVersionFieldName(): currentVersion}, bson.M{modelVersionable.GetVersionFieldName(): bson.M{"$exists": false}}} + } else { query[modelVersionable.GetVersionFieldName()] = currentVersion } + modelVersionable.IncrementVersion() } From 5d2c58228c8f881b5a3303a8579d6e69236469b1 Mon Sep 17 00:00:00 2001 From: Arthur Clement Date: Wed, 8 Dec 2021 14:47:14 +1100 Subject: [PATCH 6/7] PR feedback : dont include version field by default, rename to _v, have specific error type --- errors.go | 7 +++++++ field.go | 9 +++++++-- model.go | 6 +++--- model_test.go | 2 +- operation.go | 35 ++++++++++++----------------------- testhelpers_test.go | 4 +++- 6 files changed, 33 insertions(+), 30 deletions(-) create mode 100644 errors.go diff --git a/errors.go b/errors.go new file mode 100644 index 0000000..8b98f32 --- /dev/null +++ b/errors.go @@ -0,0 +1,7 @@ +package mgm + +import ( + "errors" +) + +var ErrVersionning = errors.New("versionning error") diff --git a/field.go b/field.go index 31e5fd5..78c39c2 100644 --- a/field.go +++ b/field.go @@ -19,7 +19,7 @@ type DateFields struct { } type VersionField struct { - Version int `json:"version" bson:"version"` + Version int `json:"_v" bson:"_v"` } // PrepareID method prepares the ID value to be used for filtering @@ -50,7 +50,7 @@ func (f *VersionField) GetVersion() interface{} { // GetVersionFieldName returns the field name holding the version field (has to match the bson tag) func (f *VersionField) GetVersionFieldName() string { - return "version" + return "_v" } // SetVersion returns the model version field @@ -58,6 +58,11 @@ func (f *VersionField) IncrementVersion() { f.Version++ } +// Determines whether the version field is in its zero value +func (f *VersionField) IsVersionZero() bool { + return f.Version == 0 +} + //-------------------------------- // DateField methods //-------------------------------- diff --git a/model.go b/model.go index d6b84ee..c637390 100644 --- a/model.go +++ b/model.go @@ -30,13 +30,13 @@ type Versionable interface { GetVersion() interface{} GetVersionFieldName() string IncrementVersion() + IsVersionZero() bool } // DefaultModel struct contains a model's default fields. type DefaultModel struct { - IDField `bson:",inline"` - DateFields `bson:",inline"` - VersionField `bson:",inline"` + IDField `bson:",inline"` + DateFields `bson:",inline"` } // Creating function calls the inner fields' defined hooks diff --git a/model_test.go b/model_test.go index 4f70eb8..ee24bff 100644 --- a/model_test.go +++ b/model_test.go @@ -28,7 +28,7 @@ func TestPrepareId(t *testing.T) { func TestVersion(t *testing.T) { d := &Doc{} require.Equal(t, 0, d.GetVersion()) - require.Equal(t, "version", d.GetVersionFieldName()) + require.Equal(t, "_v", d.GetVersionFieldName()) d.IncrementVersion() require.Equal(t, 1, d.GetVersion()) } diff --git a/operation.go b/operation.go index f6de426..22764bd 100644 --- a/operation.go +++ b/operation.go @@ -3,7 +3,6 @@ package mgm import ( "context" "fmt" - "time" "github.com/kamva/mgm/v3/field" "go.mongodb.org/mongo-driver/bson" @@ -39,31 +38,21 @@ func update(ctx context.Context, c *Collection, model Model, opts ...*options.Up } query := bson.M{field.ID: model.GetID()} - modelVersionable, isVersionable := model.(Versionable) - var currentVersion interface{} - if isVersionable { - currentVersion = modelVersionable.GetVersion() - - //Handle adding versionning for documents that were created without it - //In this case, the version field would be of zero value and not exist in the DB - //Add $or exists false in the query condition for this case - isCurrentVersionZero := false - switch c := currentVersion.(type) { - case string: - isCurrentVersionZero = c == "" - case int: - isCurrentVersionZero = c == 0 - case time.Time: - isCurrentVersionZero = c.IsZero() - } - if isCurrentVersionZero { - query["$or"] = bson.A{bson.M{modelVersionable.GetVersionFieldName(): currentVersion}, bson.M{modelVersionable.GetVersionFieldName(): bson.M{"$exists": false}}} + var v interface{} + vmodel, isVersionable := model.(Versionable) + if isVersionable { + v = vmodel.GetVersion() + if vmodel.IsVersionZero() { + query["$or"] = bson.A{ + bson.M{vmodel.GetVersionFieldName(): v}, + bson.M{vmodel.GetVersionFieldName(): bson.M{"$exists": false}}, + } } else { - query[modelVersionable.GetVersionFieldName()] = currentVersion + query[vmodel.GetVersionFieldName()] = v } - modelVersionable.IncrementVersion() + vmodel.IncrementVersion() } res, err := c.UpdateOne(ctx, query, bson.M{"$set": model}, opts...) @@ -73,7 +62,7 @@ func update(ctx context.Context, c *Collection, model Model, opts ...*options.Up } if isVersionable && res.MatchedCount == 0 { - return fmt.Errorf("versioning error : document %v %v with version %v could not be found", c.Name(), model.GetID(), currentVersion) + return fmt.Errorf("document %v %v with version %v could not be found %w", c.Name(), model.GetID(), v, ErrVersionning) } return callToAfterUpdateHooks(ctx, res, model) diff --git a/testhelpers_test.go b/testhelpers_test.go index 1243558..bb2616a 100644 --- a/testhelpers_test.go +++ b/testhelpers_test.go @@ -1,11 +1,12 @@ package mgm_test import ( + "testing" + "github.com/kamva/mgm/v3" "github.com/kamva/mgm/v3/internal/util" "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/mongo/options" - "testing" ) func setupDefConnection() { @@ -43,6 +44,7 @@ func findDoc(t *testing.T) *Doc { type Doc struct { mgm.DefaultModel `bson:",inline"` + mgm.VersionField `bson:",inline"` Name string `bson:"name"` Age int `bson:"age"` From eabaab66c16279af164441014f8e82a450981d0b Mon Sep 17 00:00:00 2001 From: Arthur Clement Date: Tue, 14 Dec 2021 12:17:28 +1100 Subject: [PATCH 7/7] PR feedback : - Name version field name Version_ to avoid collisions. - In operation.update(), capture Version before hooks in case updatedAt is used as versionable field - In operation.create(), if model is versionable and zero, increment version so it gets initialized in DB - Rename GetVersionFieldName to GetVersionBsonFieldName to make it clear it is the bson field name. - Rename GetVersion() to Version() as it is more idiomatic --- field.go | 8 ++++---- model.go | 4 ++-- operation.go | 23 +++++++++++++++++------ 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/field.go b/field.go index 78c39c2..dd297a8 100644 --- a/field.go +++ b/field.go @@ -19,7 +19,7 @@ type DateFields struct { } type VersionField struct { - Version int `json:"_v" bson:"_v"` + Version_ int `json:"_v" bson:"_v"` } // PrepareID method prepares the ID value to be used for filtering @@ -45,7 +45,7 @@ func (f *IDField) SetID(id interface{}) { // GetVersion returns the model version field func (f *VersionField) GetVersion() interface{} { - return f.Version + return f.Version_ } // GetVersionFieldName returns the field name holding the version field (has to match the bson tag) @@ -55,12 +55,12 @@ func (f *VersionField) GetVersionFieldName() string { // SetVersion returns the model version field func (f *VersionField) IncrementVersion() { - f.Version++ + f.Version_++ } // Determines whether the version field is in its zero value func (f *VersionField) IsVersionZero() bool { - return f.Version == 0 + return f.Version_ == 0 } //-------------------------------- diff --git a/model.go b/model.go index c637390..c84345b 100644 --- a/model.go +++ b/model.go @@ -27,8 +27,8 @@ type Model interface { } type Versionable interface { - GetVersion() interface{} - GetVersionFieldName() string + Version() interface{} + GetVersionBsonFieldName() string IncrementVersion() IsVersionZero() bool } diff --git a/operation.go b/operation.go index 22764bd..2a14354 100644 --- a/operation.go +++ b/operation.go @@ -15,6 +15,12 @@ func create(ctx context.Context, c *Collection, model Model, opts ...*options.In return err } + //If this model is versionable and its version value is zero, increment it to initialize it in the db + vmodel, isVersionable := model.(Versionable) + if isVersionable && vmodel.IsVersionZero() { + vmodel.IncrementVersion() + } + res, err := c.InsertOne(ctx, model, opts...) if err != nil { @@ -32,6 +38,14 @@ func first(ctx context.Context, c *Collection, filter interface{}, model Model, } func update(ctx context.Context, c *Collection, model Model, opts ...*options.UpdateOptions) error { + + //Get current version before calling hooks that could alter it + var v interface{} + vmodel, isVersionable := model.(Versionable) + if isVersionable { + v = vmodel.Version() + } + // Call to saving hook if err := callToBeforeUpdateHooks(ctx, model); err != nil { return err @@ -39,17 +53,14 @@ func update(ctx context.Context, c *Collection, model Model, opts ...*options.Up query := bson.M{field.ID: model.GetID()} - var v interface{} - vmodel, isVersionable := model.(Versionable) if isVersionable { - v = vmodel.GetVersion() if vmodel.IsVersionZero() { query["$or"] = bson.A{ - bson.M{vmodel.GetVersionFieldName(): v}, - bson.M{vmodel.GetVersionFieldName(): bson.M{"$exists": false}}, + bson.M{vmodel.GetVersionBsonFieldName(): v}, + bson.M{vmodel.GetVersionBsonFieldName(): bson.M{"$exists": false}}, } } else { - query[vmodel.GetVersionFieldName()] = v + query[vmodel.GetVersionBsonFieldName()] = v } vmodel.IncrementVersion()