From 17dd2acb5ca4e2112b015ee55aa963aa8591d402 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Tue, 11 Jul 2023 18:50:38 +0900 Subject: [PATCH 01/14] add test --- integration/update_field_compat_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/integration/update_field_compat_test.go b/integration/update_field_compat_test.go index d8c459470852..69cc2edd0f1d 100644 --- a/integration/update_field_compat_test.go +++ b/integration/update_field_compat_test.go @@ -812,6 +812,11 @@ func TestUpdateFieldCompatSet(t *testing.T) { resultType: emptyResult, skip: "https://github.com/FerretDB/FerretDB/issues/3017", }, + "UpsertQuery": { + filter: bson.D{{"v", bson.D{{"$gt", 3}}}}, + update: bson.D{{"$set", bson.D{{"found", true}}}}, + updateOpts: options.Update().SetUpsert(true), + }, } testUpdateCompat(t, testCases) From 7dd39046ca35e0e4290f8fba1c02997d74a35b54 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 12 Jul 2023 15:08:26 +0900 Subject: [PATCH 02/14] add test --- integration/update_field_compat_test.go | 1 + integration/update_field_test.go | 88 +++++++++++++++++++++++-- 2 files changed, 84 insertions(+), 5 deletions(-) diff --git a/integration/update_field_compat_test.go b/integration/update_field_compat_test.go index 69cc2edd0f1d..1c05e1bef843 100644 --- a/integration/update_field_compat_test.go +++ b/integration/update_field_compat_test.go @@ -21,6 +21,7 @@ import ( "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/bson/primitive" + "go.mongodb.org/mongo-driver/mongo/options" "github.com/FerretDB/FerretDB/integration/shareddata" "github.com/FerretDB/FerretDB/internal/types" diff --git a/integration/update_field_test.go b/integration/update_field_test.go index efc9beda9726..b677385df2d8 100644 --- a/integration/update_field_test.go +++ b/integration/update_field_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/require" "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/mongo" + "go.mongodb.org/mongo-driver/mongo/options" "github.com/FerretDB/FerretDB/integration/setup" "github.com/FerretDB/FerretDB/integration/shareddata" @@ -31,7 +32,7 @@ func TestUpdateFieldSet(t *testing.T) { t.Parallel() for name, tc := range map[string]struct { - id string // optional, defaults to empty + filter bson.D // optional, defaults to bson.D{} update bson.D // required, used for update parameter res *mongo.UpdateResult // optional, expected response from update @@ -39,7 +40,7 @@ func TestUpdateFieldSet(t *testing.T) { skip string // optional, skip test with a specified reason }{ "ArrayNil": { - id: "string", + filter: bson.D{{"_id", "string"}}, update: bson.D{{"$set", bson.D{{"v", bson.A{nil}}}}}, findRes: bson.D{{"_id", "string"}, {"v", bson.A{nil}}}, res: &mongo.UpdateResult{ @@ -49,7 +50,7 @@ func TestUpdateFieldSet(t *testing.T) { }, }, "SetSameValueInt": { - id: "int32", + filter: bson.D{{"_id", "int32"}}, update: bson.D{{"$set", bson.D{{"v", int32(42)}}}}, findRes: bson.D{{"_id", "int32"}, {"v", int32(42)}}, res: &mongo.UpdateResult{ @@ -58,6 +59,26 @@ func TestUpdateFieldSet(t *testing.T) { UpsertedCount: 0, }, }, + "QueryOperatorExistingModified": { + filter: bson.D{{"v", bson.D{{"$eq", 4080}}}}, + update: bson.D{{"$set", bson.D{{"new", "val"}}}}, + res: &mongo.UpdateResult{ + MatchedCount: 1, + ModifiedCount: 1, + UpsertedCount: 0, + }, + findRes: bson.D{{"_id", "int32-1"}, {"v", int32(4080)}, {"new", "val"}}, + }, + "QueryOperatorEmptySet": { + filter: bson.D{{"v", bson.D{{"$eq", 4080}}}}, + update: bson.D{{"$set", bson.D{}}}, + res: &mongo.UpdateResult{ + MatchedCount: 1, + ModifiedCount: 0, + UpsertedCount: 0, + }, + findRes: bson.D{{"_id", "int32-1"}, {"v", int32(4080)}}, + }, } { name, tc := name, tc t.Run(name, func(t *testing.T) { @@ -69,21 +90,78 @@ func TestUpdateFieldSet(t *testing.T) { require.NotNil(t, tc.update, "update should be set") + filter := bson.D{} + if tc.filter != nil { + filter = tc.filter + } + ctx, collection := setup.Setup(t, shareddata.Scalars, shareddata.Composites) - res, err := collection.UpdateOne(ctx, bson.D{{"_id", tc.id}}, tc.update) + res, err := collection.UpdateOne(ctx, filter, tc.update) require.NoError(t, err) require.Equal(t, tc.res, res) var actual bson.D - err = collection.FindOne(ctx, bson.D{{"_id", tc.id}}).Decode(&actual) + err = collection.FindOne(ctx, filter).Decode(&actual) require.NoError(t, err) AssertEqualDocuments(t, tc.findRes, actual) }) } } +func TestUpdateFieldSetUpsert(t *testing.T) { + t.Parallel() + + t.Run("Upsert", func(t *testing.T) { + t.Parallel() + + ctx, collection := setup.Setup(t, shareddata.Scalars, shareddata.Composites) + + updateRes, err := collection.UpdateOne( + ctx, + bson.D{{"v", bson.D{{"$eq", "non-existent"}}}}, + bson.D{{"$set", bson.D{{"new", "val"}}}}, + options.Update().SetUpsert(true), + ) + + require.NoError(t, err) + assert.Equal(t, int64(0), updateRes.MatchedCount) + assert.Equal(t, int64(0), updateRes.ModifiedCount) + assert.Equal(t, int64(1), updateRes.UpsertedCount) + require.NotNil(t, updateRes.UpsertedID) + + var res bson.D + err = collection.FindOne(ctx, bson.D{{"_id", updateRes.UpsertedID}}).Decode(&res) + require.NoError(t, err) + AssertEqualDocuments(t, bson.D{{"_id", updateRes.UpsertedID}, {"new", "val"}}, res) + }) + + t.Run("NoUpsert", func(t *testing.T) { + t.Parallel() + + ctx, collection := setup.Setup(t, shareddata.Scalars, shareddata.Composites) + + updateRes, err := collection.UpdateOne( + ctx, + bson.D{{"v", bson.D{{"$eq", "non-existent"}}}}, + bson.D{{"$set", bson.D{{"new", "val"}}}}, + options.Update().SetUpsert(false), + ) + + require.NoError(t, err) + assert.Equal( + t, + &mongo.UpdateResult{ + MatchedCount: 0, + ModifiedCount: 0, + UpsertedCount: 0, + }, + updateRes, + ) + }) +} + func TestUpdateFieldErrors(t *testing.T) { t.Parallel() From 0920df431c489006f07f4dbc137c84000d7d083a Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 12 Jul 2023 16:48:11 +0900 Subject: [PATCH 03/14] add test --- integration/update_field_test.go | 162 ++++++++++++++++++++++--------- 1 file changed, 118 insertions(+), 44 deletions(-) diff --git a/integration/update_field_test.go b/integration/update_field_test.go index b677385df2d8..d31959128b53 100644 --- a/integration/update_field_test.go +++ b/integration/update_field_test.go @@ -110,56 +110,130 @@ func TestUpdateFieldSet(t *testing.T) { } } -func TestUpdateFieldSetUpsert(t *testing.T) { +func TestUpdateFieldSetUpdateManyUpsert(t *testing.T) { t.Parallel() - t.Run("Upsert", func(t *testing.T) { - t.Parallel() - - ctx, collection := setup.Setup(t, shareddata.Scalars, shareddata.Composites) - - updateRes, err := collection.UpdateOne( - ctx, - bson.D{{"v", bson.D{{"$eq", "non-existent"}}}}, - bson.D{{"$set", bson.D{{"new", "val"}}}}, - options.Update().SetUpsert(true), - ) - - require.NoError(t, err) - assert.Equal(t, int64(0), updateRes.MatchedCount) - assert.Equal(t, int64(0), updateRes.ModifiedCount) - assert.Equal(t, int64(1), updateRes.UpsertedCount) - require.NotNil(t, updateRes.UpsertedID) - - var res bson.D - err = collection.FindOne(ctx, bson.D{{"_id", updateRes.UpsertedID}}).Decode(&res) - require.NoError(t, err) - AssertEqualDocuments(t, bson.D{{"_id", updateRes.UpsertedID}, {"new", "val"}}, res) - }) - - t.Run("NoUpsert", func(t *testing.T) { - t.Parallel() - - ctx, collection := setup.Setup(t, shareddata.Scalars, shareddata.Composites) - - updateRes, err := collection.UpdateOne( - ctx, - bson.D{{"v", bson.D{{"$eq", "non-existent"}}}}, - bson.D{{"$set", bson.D{{"new", "val"}}}}, - options.Update().SetUpsert(false), - ) - - require.NoError(t, err) - assert.Equal( - t, - &mongo.UpdateResult{ + for name, tc := range map[string]struct { //nolint:vet // used for testing only + filter bson.D // optional, defaults to bson.D{} + update bson.D // required, used for update parameter + opts *options.UpdateOptions // optional + + findRes []bson.E // required, expected response from find without _id generated by upsert + skip string // optional, skip test with a specified reason + }{ + "QueryOperator": { + filter: bson.D{{"v", bson.D{{"$lt", 3}}}}, + update: bson.D{{"$set", bson.D{{"new", "val"}}}}, + opts: options.Update().SetUpsert(true), + findRes: []bson.E{{"new", "val"}}, + }, + "NoOperator": { + filter: bson.D{{"v", int32(4080)}}, + update: bson.D{{"$set", bson.D{{"new", "val"}}}}, + opts: options.Update().SetUpsert(true), + findRes: []bson.E{{"new", "val"}}, + }, + } { + name, tc := name, tc + t.Run(name, func(t *testing.T) { + t.Parallel() + + require.NotNil(t, tc.update, "update should be set") + require.NotNil(t, tc.findRes, "findRes should be set") + + ctx, collection := setup.Setup(t, shareddata.Nulls) + + updateRes, err := collection.UpdateMany(ctx, tc.filter, tc.update, tc.opts) + require.NoError(t, err) + assert.Equal(t, int64(0), updateRes.MatchedCount) + assert.Equal(t, int64(0), updateRes.ModifiedCount) + assert.Equal(t, int64(1), updateRes.UpsertedCount) + require.NotNil(t, updateRes.UpsertedID) + + cursor, err := collection.Find(ctx, bson.D{{"_id", updateRes.UpsertedID}}) + require.NoError(t, err) + + var res []bson.D + err = cursor.All(ctx, &res) + require.NoError(t, err) + + expected := bson.D{{"_id", updateRes.UpsertedID}} + for _, e := range tc.findRes { + expected = append(expected, e) + } + + AssertEqualDocumentsSlice(t, []bson.D{expected}, res) + }) + } +} + +func TestUpdateFieldSetUpdateManyNoUpsert(t *testing.T) { + t.Parallel() + + for name, tc := range map[string]struct { //nolint:vet // used for testing only + filter bson.D // optional, defaults to bson.D{} + update bson.D // required, used for update parameter + opts *options.UpdateOptions // optional + providers shareddata.Providers // optional, defaults to shareddata.Nulls + + updateRes *mongo.UpdateResult // optional, expected response from update + findRes []bson.D // required, expected response from find + skip string // optional, skip test with a specified reason + }{ + "QueryOperatorExists": { + filter: bson.D{{"v", bson.D{{"$lt", 3}}}}, + update: bson.D{{"$set", bson.D{{"new", "val"}}}}, + opts: options.Update().SetUpsert(true), + providers: []shareddata.Provider{shareddata.Int32s}, + updateRes: &mongo.UpdateResult{ + MatchedCount: 2, + ModifiedCount: 2, + UpsertedCount: 0, + }, + findRes: []bson.D{ + {{"_id", "int32-min"}, {"v", int32(math.MinInt32)}, {"new", "val"}}, + {{"_id", "int32-zero"}, {"v", int32(0)}, {"new", "val"}}, + }, + }, + "QueryOperatorUpsertFalse": { + filter: bson.D{{"v", int32(4080)}}, + update: bson.D{{"$set", bson.D{{"new", "val"}}}}, + opts: options.Update().SetUpsert(false), + updateRes: &mongo.UpdateResult{ MatchedCount: 0, ModifiedCount: 0, UpsertedCount: 0, }, - updateRes, - ) - }) + findRes: []bson.D{}, + }, + } { + name, tc := name, tc + t.Run(name, func(t *testing.T) { + t.Parallel() + + require.NotNil(t, tc.update, "update should be set") + require.NotNil(t, tc.findRes, "findRes should be set") + + providers := tc.providers + if providers == nil { + providers = []shareddata.Provider{shareddata.Nulls} + } + + ctx, collection := setup.Setup(t, providers...) + + updateRes, err := collection.UpdateMany(ctx, tc.filter, tc.update, tc.opts) + require.NoError(t, err) + assert.Equal(t, tc.updateRes, updateRes) + + cursor, err := collection.Find(ctx, tc.filter, options.Find().SetSort(bson.D{{"_id", 1}})) + require.NoError(t, err) + + var res []bson.D + err = cursor.All(ctx, &res) + require.NoError(t, err) + AssertEqualDocumentsSlice(t, tc.findRes, res) + }) + } } func TestUpdateFieldErrors(t *testing.T) { From d78c277b942630a179511f85037d43b7fb6bcb6b Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 12 Jul 2023 16:51:07 +0900 Subject: [PATCH 04/14] make test pass for mongoDB --- integration/update_field_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/update_field_test.go b/integration/update_field_test.go index d31959128b53..8ab14b2ea91e 100644 --- a/integration/update_field_test.go +++ b/integration/update_field_test.go @@ -131,7 +131,7 @@ func TestUpdateFieldSetUpdateManyUpsert(t *testing.T) { filter: bson.D{{"v", int32(4080)}}, update: bson.D{{"$set", bson.D{{"new", "val"}}}}, opts: options.Update().SetUpsert(true), - findRes: []bson.E{{"new", "val"}}, + findRes: []bson.E{{"v", int32(4080)}, {"new", "val"}}, }, } { name, tc := name, tc From ae27e2453582d10badbdde73d4e989c188e8db19 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 12 Jul 2023 17:11:25 +0900 Subject: [PATCH 05/14] add logic for upsert --- internal/handlers/common/filter.go | 35 +++++++++++++++++++++++ internal/handlers/common/findandmodify.go | 6 ++-- internal/handlers/pg/msg_update.go | 33 +++++++++++++++++++-- 3 files changed, 68 insertions(+), 6 deletions(-) diff --git a/internal/handlers/common/filter.go b/internal/handlers/common/filter.go index 6a40007a4bdc..a123db9223e9 100644 --- a/internal/handlers/common/filter.go +++ b/internal/handlers/common/filter.go @@ -60,6 +60,41 @@ func FilterDocument(doc, filter *types.Document) (bool, error) { } } +// HasQueryOperator recursively checks if filter document contains any operator prefixed with $. +func HasQueryOperator(filter *types.Document) (bool, error) { + iter := filter.Iterator() + defer iter.Close() + + for { + filterKey, filterValue, err := iter.Next() + if err != nil { + if errors.Is(err, iterator.ErrIteratorDone) { + return false, nil + } + + return false, lazyerrors.Error(err) + } + + if strings.HasPrefix(filterKey, "$") { + return true, nil + } + + filterDoc, ok := filterValue.(*types.Document) + if !ok { + continue + } + + hasOperator, err := HasQueryOperator(filterDoc) + if err != nil { + return false, lazyerrors.Error(err) + } + + if hasOperator { + return true, nil + } + } +} + // filterDocumentPair handles a single filter element key/value pair {filterKey: filterValue}. func filterDocumentPair(doc *types.Document, filterKey string, filterValue any) (bool, error) { docs := []*types.Document{doc} diff --git a/internal/handlers/common/findandmodify.go b/internal/handlers/common/findandmodify.go index 8df5014ce9ce..0f41f5acda6c 100644 --- a/internal/handlers/common/findandmodify.go +++ b/internal/handlers/common/findandmodify.go @@ -209,7 +209,7 @@ func prepareDocumentForInsert(params *FindAndModifyParams) (*types.Document, err } if !insert.Has("_id") { - id, err := getUpsertID(params.Query) + id, err := GetUpsertID(params.Query) if err != nil { return nil, err } @@ -253,9 +253,9 @@ func prepareDocumentForUpdate(docs []*types.Document, params *FindAndModifyParam return update, nil } -// getUpsertID gets the _id to use for upsert document. If query contains _id, +// GetUpsertID gets the _id to use for upsert document. If query contains _id, // that _id is assigned unless _id contains operator. Otherwise, it generates an ID. -func getUpsertID(query *types.Document) (any, error) { +func GetUpsertID(query *types.Document) (any, error) { id, err := query.Get("_id") if err != nil { return types.NewObjectID(), nil diff --git a/internal/handlers/pg/msg_update.go b/internal/handlers/pg/msg_update.go index 3fcc3485b81e..f154bfaecbc8 100644 --- a/internal/handlers/pg/msg_update.go +++ b/internal/handlers/pg/msg_update.go @@ -85,12 +85,39 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, continue } - doc := u.Filter.DeepCopy() - if _, err = common.UpdateDocument(document.Command(), doc, u.Update); err != nil { + hasQueryOperators, err := common.HasQueryOperator(u.Filter) + if err != nil { return err } + + var doc *types.Document + if hasQueryOperators { + doc = must.NotFail(types.NewDocument()) + } else { + doc = u.Filter + } + + hasUpdateOperators, err := common.HasSupportedUpdateModifiers(document.Command(), u.Update) + if err != nil { + return err + } + + if hasUpdateOperators { + if _, err = common.UpdateDocument(document.Command(), doc, u.Update); err != nil { + return err + } + } else { + doc = u.Update + } + if !doc.Has("_id") { - doc.Set("_id", types.NewObjectID()) + var id any + id, err = common.GetUpsertID(u.Filter) + if err != nil { + return err + } + + doc.Set("_id", id) } upserted.Append(must.NotFail(types.NewDocument( From 43aeeb5692635b102b9602be9c88d2b96280e892 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 12 Jul 2023 17:14:39 +0900 Subject: [PATCH 06/14] remove unneeded --- integration/update_field_compat_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/integration/update_field_compat_test.go b/integration/update_field_compat_test.go index 1c05e1bef843..d8c459470852 100644 --- a/integration/update_field_compat_test.go +++ b/integration/update_field_compat_test.go @@ -21,7 +21,6 @@ import ( "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/bson/primitive" - "go.mongodb.org/mongo-driver/mongo/options" "github.com/FerretDB/FerretDB/integration/shareddata" "github.com/FerretDB/FerretDB/internal/types" @@ -813,11 +812,6 @@ func TestUpdateFieldCompatSet(t *testing.T) { resultType: emptyResult, skip: "https://github.com/FerretDB/FerretDB/issues/3017", }, - "UpsertQuery": { - filter: bson.D{{"v", bson.D{{"$gt", 3}}}}, - update: bson.D{{"$set", bson.D{{"found", true}}}}, - updateOpts: options.Update().SetUpsert(true), - }, } testUpdateCompat(t, testCases) From 573f7e7b4b330364f1db0f6b7008b872108ae111 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 12 Jul 2023 19:07:11 +0900 Subject: [PATCH 07/14] add more tests --- integration/update_field_test.go | 107 ++++++++++++++++++++++++++++- internal/handlers/pg/msg_update.go | 2 +- 2 files changed, 107 insertions(+), 2 deletions(-) diff --git a/integration/update_field_test.go b/integration/update_field_test.go index 8ab14b2ea91e..f590788575ec 100644 --- a/integration/update_field_test.go +++ b/integration/update_field_test.go @@ -26,6 +26,8 @@ import ( "github.com/FerretDB/FerretDB/integration/setup" "github.com/FerretDB/FerretDB/integration/shareddata" + "github.com/FerretDB/FerretDB/internal/types" + "github.com/FerretDB/FerretDB/internal/util/must" ) func TestUpdateFieldSet(t *testing.T) { @@ -127,7 +129,7 @@ func TestUpdateFieldSetUpdateManyUpsert(t *testing.T) { opts: options.Update().SetUpsert(true), findRes: []bson.E{{"new", "val"}}, }, - "NoOperator": { + "NoQueryOperator": { filter: bson.D{{"v", int32(4080)}}, update: bson.D{{"$set", bson.D{{"new", "val"}}}}, opts: options.Update().SetUpsert(true), @@ -236,6 +238,109 @@ func TestUpdateFieldSetUpdateManyNoUpsert(t *testing.T) { } } +func TestUpdateCommandUpsertErrors(t *testing.T) { + t.Parallel() + + for name, tc := range map[string]struct { //nolint:vet // used for testing only + filter bson.D // optional, defaults to bson.D{} + updates bson.A // required, used for update parameter + + nMatched int32 // optional + nModified int32 // optional + nUpserted int // optional + err *mongo.WriteError // optional, expected error from MongoDB + skip string // optional, skip test with a specified reason + }{ + "NoUpdateOperator": { + filter: bson.D{{"v", bson.D{{"$lt", 3}}}}, + updates: bson.A{ + bson.D{ + {"q", bson.D{{"v", bson.D{{"$lt", 3}}}}}, + {"u", bson.D{{"new", "val"}}}, + {"upsert", true}, + }, + }, + nMatched: int32(1), + nModified: int32(0), + nUpserted: 1, + }, + "NoUpdateEmptyQueryOperator": { + updates: bson.A{ + bson.D{ + {"q", bson.D{}}, + {"u", bson.D{{"new", "val"}}}, + {"upsert", true}, + }, + }, + nMatched: int32(1), + nModified: int32(0), + nUpserted: 0, + }, + "UnknownUpdateOperator": { + updates: bson.A{ + bson.D{ + {"q", bson.D{{"v", bson.D{{"$lt", 3}}}}}, + {"u", bson.D{{"$unknown", bson.D{{"new", "val"}}}}}, + {"upsert", true}, + }, + }, + err: &mongo.WriteError{ + Code: 9, + Message: "Unknown modifier: $unknown. Expected a valid update modifier or pipeline-style update specified as an array", + }, + }, + } { + name, tc := name, tc + t.Run(name, func(t *testing.T) { + t.Parallel() + + require.NotNil(t, tc.updates, "update should be set") + + ctx, collection := setup.Setup(t, shareddata.Nulls) + + var res bson.D + err := collection.Database().RunCommand(ctx, + bson.D{ + {"update", collection.Name()}, + {"updates", tc.updates}, + }).Decode(&res) + + if tc.err != nil { + assert.Nil(t, res) + AssertEqualWriteError(t, *tc.err, err) + + return + } + + doc := ConvertDocument(t, res) + + nMatched, _ := doc.Get("n") + assert.Equal(t, tc.nMatched, nMatched, "unexpected nMatched") + + v, _ := doc.Get("upserted") + upserted, _ := v.(*types.Array) + require.Equal(t, upserted.Len(), tc.nUpserted, "unexpected nUpserted") + + for i := 0; i < upserted.Len(); i++ { + firstElem, _ := must.NotFail(upserted.Get(i)).(*types.Document) + + index, _ := firstElem.Get("index") + assert.Equal(t, int32(0), index) + + // _id is generated, cannot check for exact value so check it is not zero value + id, _ := firstElem.Get("_id") + assert.NotZero(t, id) + } + + nModified, _ := doc.Get("nModified") + assert.Equal(t, nModified, nModified, "nModified") + + resOk, _ := doc.Get("ok") + assert.Equal(t, float64(1), resOk) + }) + } +} + func TestUpdateFieldErrors(t *testing.T) { t.Parallel() diff --git a/internal/handlers/pg/msg_update.go b/internal/handlers/pg/msg_update.go index f154bfaecbc8..1dc8c1cd9727 100644 --- a/internal/handlers/pg/msg_update.go +++ b/internal/handlers/pg/msg_update.go @@ -87,7 +87,7 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, hasQueryOperators, err := common.HasQueryOperator(u.Filter) if err != nil { - return err + return lazyerrors.Error(err) } var doc *types.Document From 8880fb7f45d3ea38b314216f43fb6ddbfadd360a Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 12 Jul 2023 19:14:45 +0900 Subject: [PATCH 08/14] copy paste sqlite --- internal/handlers/pg/msg_update.go | 4 +-- internal/handlers/sqlite/msg_update.go | 35 ++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/internal/handlers/pg/msg_update.go b/internal/handlers/pg/msg_update.go index 1dc8c1cd9727..01ef38b5d75a 100644 --- a/internal/handlers/pg/msg_update.go +++ b/internal/handlers/pg/msg_update.go @@ -112,8 +112,8 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, if !doc.Has("_id") { var id any - id, err = common.GetUpsertID(u.Filter) - if err != nil { + + if id, err = common.GetUpsertID(u.Filter); err != nil { return err } diff --git a/internal/handlers/sqlite/msg_update.go b/internal/handlers/sqlite/msg_update.go index 8f0f479afc10..f42ef20af697 100644 --- a/internal/handlers/sqlite/msg_update.go +++ b/internal/handlers/sqlite/msg_update.go @@ -123,15 +123,40 @@ func (h *Handler) updateDocument(ctx context.Context, params *common.UpdatesPara continue } - doc := u.Filter.DeepCopy() - if _, err = common.UpdateDocument("update", doc, u.Update); err != nil { + hasQueryOperators, err := common.HasQueryOperator(u.Filter) + if err != nil { return 0, 0, nil, lazyerrors.Error(err) } - if !doc.Has("_id") { - doc.Set("_id", types.NewObjectID()) + var doc *types.Document + if hasQueryOperators { + doc = must.NotFail(types.NewDocument()) + } else { + doc = u.Filter + } + + hasUpdateOperators, err := common.HasSupportedUpdateModifiers("update", u.Update) + if err != nil { + return 0, 0, nil, err } + if hasUpdateOperators { + if _, err = common.UpdateDocument("update", doc, u.Update); err != nil { + return 0, 0, nil, err + } + } else { + doc = u.Update + } + + if !doc.Has("_id") { + var id any + + if id, err = common.GetUpsertID(u.Filter); err != nil { + return 0, 0, nil, err + } + + doc.Set("_id", id) + } upserted.Append(must.NotFail(types.NewDocument( "index", int32(0), // TODO "_id", must.NotFail(doc.Get("_id")), @@ -142,7 +167,7 @@ func (h *Handler) updateDocument(ctx context.Context, params *common.UpdatesPara iter := must.NotFail(types.NewArray(doc)).Iterator() defer iter.Close() - _, err := db.Collection(params.Collection).Insert(ctx, &backends.InsertParams{ + _, err = db.Collection(params.Collection).Insert(ctx, &backends.InsertParams{ Iter: iter, }) if err != nil { From d98b3e0d4dbdbd5d5bb0b54e456e04e10eca461d Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 12 Jul 2023 19:22:04 +0900 Subject: [PATCH 09/14] add todo --- internal/handlers/pg/msg_findandmodify.go | 1 + internal/handlers/pg/msg_update.go | 1 + internal/handlers/sqlite/msg_update.go | 1 + 3 files changed, 3 insertions(+) diff --git a/internal/handlers/pg/msg_findandmodify.go b/internal/handlers/pg/msg_findandmodify.go index 372e89736d9d..359a4800973e 100644 --- a/internal/handlers/pg/msg_findandmodify.go +++ b/internal/handlers/pg/msg_findandmodify.go @@ -125,6 +125,7 @@ func (h *Handler) MsgFindAndModify(ctx context.Context, msg *wire.OpMsg) (*wire. return nil } + // TODO: https://github.com/FerretDB/FerretDB/issues/3040 var upsert *types.Document if params.HasUpdateOperators { diff --git a/internal/handlers/pg/msg_update.go b/internal/handlers/pg/msg_update.go index 01ef38b5d75a..9d11ce2c076b 100644 --- a/internal/handlers/pg/msg_update.go +++ b/internal/handlers/pg/msg_update.go @@ -85,6 +85,7 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, continue } + // TODO: https://github.com/FerretDB/FerretDB/issues/3040 hasQueryOperators, err := common.HasQueryOperator(u.Filter) if err != nil { return lazyerrors.Error(err) diff --git a/internal/handlers/sqlite/msg_update.go b/internal/handlers/sqlite/msg_update.go index f42ef20af697..c4fa26337e6d 100644 --- a/internal/handlers/sqlite/msg_update.go +++ b/internal/handlers/sqlite/msg_update.go @@ -123,6 +123,7 @@ func (h *Handler) updateDocument(ctx context.Context, params *common.UpdatesPara continue } + // TODO: https://github.com/FerretDB/FerretDB/issues/3040 hasQueryOperators, err := common.HasQueryOperator(u.Filter) if err != nil { return 0, 0, nil, lazyerrors.Error(err) From 546aa0d1a9e8c8ec9000952897c37d779b8bab7e Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Thu, 13 Jul 2023 13:41:48 +0900 Subject: [PATCH 10/14] more tests around index --- integration/update_field_test.go | 97 ++++++++++++++++++++++---- internal/handlers/common/filter.go | 8 +-- internal/handlers/pg/msg_update.go | 2 +- internal/handlers/sqlite/msg_update.go | 2 +- 4 files changed, 89 insertions(+), 20 deletions(-) diff --git a/integration/update_field_test.go b/integration/update_field_test.go index f590788575ec..21e1250a6e2b 100644 --- a/integration/update_field_test.go +++ b/integration/update_field_test.go @@ -135,6 +135,12 @@ func TestUpdateFieldSetUpdateManyUpsert(t *testing.T) { opts: options.Update().SetUpsert(true), findRes: []bson.E{{"v", int32(4080)}, {"new", "val"}}, }, + "QueryOperatorIDQuery": { + filter: bson.D{{"_id", bson.D{{"$eq", 1}}}}, + update: bson.D{{"$set", bson.D{{"new", "val"}}}}, + opts: options.Update().SetUpsert(true), + findRes: []bson.E{{"new", "val"}}, + }, } { name, tc := name, tc t.Run(name, func(t *testing.T) { @@ -238,49 +244,96 @@ func TestUpdateFieldSetUpdateManyNoUpsert(t *testing.T) { } } -func TestUpdateCommandUpsertErrors(t *testing.T) { +func TestUpdateCommandUpsert(t *testing.T) { t.Parallel() for name, tc := range map[string]struct { //nolint:vet // used for testing only - filter bson.D // optional, defaults to bson.D{} updates bson.A // required, used for update parameter nMatched int32 // optional nModified int32 // optional nUpserted int // optional + findRes []bson.D // required, expected response from find without _id generated by upsert err *mongo.WriteError // optional, expected error from MongoDB skip string // optional, skip test with a specified reason }{ "NoUpdateOperator": { - filter: bson.D{{"v", bson.D{{"$lt", 3}}}}, updates: bson.A{ bson.D{ {"q", bson.D{{"v", bson.D{{"$lt", 3}}}}}, - {"u", bson.D{{"new", "val"}}}, + {"u", bson.D{{"updateV", "val"}}}, {"upsert", true}, }, }, nMatched: int32(1), nModified: int32(0), nUpserted: 1, + findRes: []bson.D{{{"v", nil}}, {{"updateV", "val"}}}, }, - "NoUpdateEmptyQueryOperator": { + "NoUpdateOperatorEmptyQuery": { updates: bson.A{ bson.D{ {"q", bson.D{}}, - {"u", bson.D{{"new", "val"}}}, + {"u", bson.D{{"updateV", "val"}}}, {"upsert", true}, }, }, nMatched: int32(1), - nModified: int32(0), + nModified: int32(1), nUpserted: 0, }, + "NoQueryOperator": { + updates: bson.A{ + bson.D{ + {"q", bson.D{{"queryV", "v"}}}, + {"u", bson.D{{"$set", bson.D{{"updateV", "val"}}}}}, + {"upsert", true}, + }, + }, + nMatched: int32(1), + nModified: int32(0), + nUpserted: 1, + findRes: []bson.D{{{"v", nil}}, {{"queryV", "v"}, {"updateV", "val"}}}, + }, + "NoUpdateOperatorNoQueryOperator": { + updates: bson.A{ + bson.D{ + {"q", bson.D{{"queryV", "val"}}}, + {"u", bson.D{{"updateV", "val"}}}, + {"upsert", true}, + }, + }, + nMatched: int32(1), + nModified: int32(0), + nUpserted: 1, + findRes: []bson.D{{{"v", nil}}, {{"updateV", "val"}}}, + }, + "MultipleUpserts": { + updates: bson.A{ + bson.D{ + {"q", bson.D{{"v", bson.D{{"$gt", 3}}}}}, + {"u", bson.D{{"updateV", "greater"}}}, + {"upsert", true}, + }, + bson.D{ + {"q", bson.D{{"v", bson.D{{"$lt", 3}}}}}, + {"u", bson.D{{"updateV", "less"}}}, + {"upsert", true}, + }, + }, + nMatched: int32(2), + nModified: int32(0), + nUpserted: 2, + findRes: []bson.D{ + {{"v", nil}}, + {{"updateV", "greater"}}, + {{"updateV", "less"}}}, + }, "UnknownUpdateOperator": { updates: bson.A{ bson.D{ {"q", bson.D{{"v", bson.D{{"$lt", 3}}}}}, - {"u", bson.D{{"$unknown", bson.D{{"new", "val"}}}}}, + {"u", bson.D{{"$unknown", bson.D{{"v", "val"}}}}}, {"upsert", true}, }, }, @@ -317,26 +370,42 @@ func TestUpdateCommandUpsertErrors(t *testing.T) { nMatched, _ := doc.Get("n") assert.Equal(t, tc.nMatched, nMatched, "unexpected nMatched") + nModified, _ := doc.Get("nModified") + assert.Equal(t, tc.nModified, nModified, "unexpected nModified") + + resOk, _ := doc.Get("ok") + assert.Equal(t, float64(1), resOk) + v, _ := doc.Get("upserted") upserted, _ := v.(*types.Array) - require.Equal(t, upserted.Len(), tc.nUpserted, "unexpected nUpserted") + require.Equal(t, tc.nUpserted, upserted.Len(), "unexpected nUpserted") for i := 0; i < upserted.Len(); i++ { firstElem, _ := must.NotFail(upserted.Get(i)).(*types.Document) index, _ := firstElem.Get("index") - assert.Equal(t, int32(0), index) + assert.Equal(t, int32(i), index, "unexpected index") // _id is generated, cannot check for exact value so check it is not zero value id, _ := firstElem.Get("_id") assert.NotZero(t, id) } - nModified, _ := doc.Get("nModified") - assert.Equal(t, nModified, nModified, "nModified") + cursor, err := collection.Find(ctx, bson.D{}, options.Find().SetSort(bson.D{{"v", 1}, {"_id", 1}})) + require.NoError(t, err) - resOk, _ := doc.Get("ok") - assert.Equal(t, float64(1), resOk) + var findRes []bson.D + err = cursor.All(ctx, &findRes) + require.NoError(t, err) + + for i, elem := range tc.findRes { + doc := ConvertDocument(t, elem) + + actualDoc := ConvertDocument(t, findRes[i]) + _ = actualDoc.Remove("_id") + + require.Equal(t, doc, actualDoc) + } }) } } diff --git a/internal/handlers/common/filter.go b/internal/handlers/common/filter.go index a123db9223e9..ffc81bfaead5 100644 --- a/internal/handlers/common/filter.go +++ b/internal/handlers/common/filter.go @@ -66,7 +66,7 @@ func HasQueryOperator(filter *types.Document) (bool, error) { defer iter.Close() for { - filterKey, filterValue, err := iter.Next() + k, v, err := iter.Next() if err != nil { if errors.Is(err, iterator.ErrIteratorDone) { return false, nil @@ -75,16 +75,16 @@ func HasQueryOperator(filter *types.Document) (bool, error) { return false, lazyerrors.Error(err) } - if strings.HasPrefix(filterKey, "$") { + if strings.HasPrefix(k, "$") { return true, nil } - filterDoc, ok := filterValue.(*types.Document) + doc, ok := v.(*types.Document) if !ok { continue } - hasOperator, err := HasQueryOperator(filterDoc) + hasOperator, err := HasQueryOperator(doc) if err != nil { return false, lazyerrors.Error(err) } diff --git a/internal/handlers/pg/msg_update.go b/internal/handlers/pg/msg_update.go index 9d11ce2c076b..065d96eaa82b 100644 --- a/internal/handlers/pg/msg_update.go +++ b/internal/handlers/pg/msg_update.go @@ -122,7 +122,7 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, } upserted.Append(must.NotFail(types.NewDocument( - "index", int32(0), // TODO + "index", int32(upserted.Len()), "_id", must.NotFail(doc.Get("_id")), ))) diff --git a/internal/handlers/sqlite/msg_update.go b/internal/handlers/sqlite/msg_update.go index c4fa26337e6d..2ae9091f90e4 100644 --- a/internal/handlers/sqlite/msg_update.go +++ b/internal/handlers/sqlite/msg_update.go @@ -159,7 +159,7 @@ func (h *Handler) updateDocument(ctx context.Context, params *common.UpdatesPara doc.Set("_id", id) } upserted.Append(must.NotFail(types.NewDocument( - "index", int32(0), // TODO + "index", int32(upserted.Len()), "_id", must.NotFail(doc.Get("_id")), ))) From 730da36fcfdeada4c5cbf22dd5d13574f0018997 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Thu, 13 Jul 2023 15:05:09 +0900 Subject: [PATCH 11/14] use compat test instead of integration test --- integration/update_compat_test.go | 123 ++++++++++++++++++++++++ integration/update_field_compat_test.go | 32 ++++++ integration/update_field_test.go | 107 ++------------------- 3 files changed, 162 insertions(+), 100 deletions(-) diff --git a/integration/update_compat_test.go b/integration/update_compat_test.go index d4585f7623c5..2acd6f731d90 100644 --- a/integration/update_compat_test.go +++ b/integration/update_compat_test.go @@ -155,6 +155,129 @@ func testUpdateCompat(t *testing.T, testCases map[string]updateCompatTestCase) { } } +// testUpdateManyCompatTestCase describes update compatibility test case. +type testUpdateManyCompatTestCase struct { //nolint:vet // used for testing only + update bson.D // required if replace is nil + filter bson.D // defaults to bson.D{{"_id", id}} + updateOpts *options.UpdateOptions // defaults to nil + resultType compatTestCaseResultType // defaults to nonEmptyResult + providers []shareddata.Provider // defaults to shareddata.AllProviders() + + skip string // skips test if non-empty +} + +// testUpdateManyCompat tests update compatibility test cases. +func testUpdateManyCompat(t *testing.T, testCases map[string]testUpdateManyCompatTestCase) { + t.Helper() + + for name, tc := range testCases { + name, tc := name, tc + t.Run(name, func(t *testing.T) { + t.Helper() + + if tc.skip != "" { + t.Skip(tc.skip) + } + + t.Parallel() + + providers := shareddata.AllProviders() + if tc.providers != nil { + providers = tc.providers + } + + s := setup.SetupCompatWithOpts(t, &setup.SetupCompatOpts{ + Providers: providers, + AddNonExistentCollection: true, + }) + ctx, targetCollections, compatCollections := s.Ctx, s.TargetCollections, s.CompatCollections + + update := tc.update + require.NotNil(t, update, "`update` must be set") + + var nonEmptyResults bool + for i := range targetCollections { + targetCollection := targetCollections[i] + compatCollection := compatCollections[i] + t.Run(targetCollection.Name(), func(t *testing.T) { + t.Helper() + + allDocs := FindAll(t, ctx, targetCollection) + + for _, doc := range allDocs { + id, _ := ConvertDocument(t, doc).Get("_id") + require.NotNil(t, id) + + t.Run(fmt.Sprint(id), func(t *testing.T) { + t.Helper() + + filter := tc.filter + if tc.filter == nil { + filter = bson.D{{"_id", id}} + } + + var targetUpdateRes, compatUpdateRes *mongo.UpdateResult + var targetErr, compatErr error + + targetUpdateRes, targetErr = targetCollection.UpdateMany(ctx, filter, update, tc.updateOpts) + compatUpdateRes, compatErr = compatCollection.UpdateMany(ctx, filter, update, tc.updateOpts) + + if targetErr != nil { + t.Logf("Target error: %v", targetErr) + t.Logf("Compat error: %v", compatErr) + + // error messages are intentionally not compared + AssertMatchesWriteError(t, compatErr, targetErr) + + return + } + require.NoError(t, compatErr, "compat error; target returned no error") + + if pointer.Get(targetUpdateRes).ModifiedCount > 0 || pointer.Get(compatUpdateRes).ModifiedCount > 0 { + nonEmptyResults = true + } + + assert.Equal(t, compatUpdateRes, targetUpdateRes) + + opts := options.Find().SetSort(bson.D{{"_id", 1}}) + targetCursor, targetErr := targetCollection.Find(ctx, bson.D{}, opts) + compatCursor, compatErr := compatCollection.Find(ctx, bson.D{}, opts) + + if targetCursor != nil { + defer targetCursor.Close(ctx) + } + if compatCursor != nil { + defer compatCursor.Close(ctx) + } + + require.NoError(t, targetErr) + require.NoError(t, compatErr) + + targetRes := FetchAll(t, ctx, targetCursor) + compatRes := FetchAll(t, ctx, compatCursor) + + AssertEqualDocumentsSlice(t, compatRes, targetRes) + + if len(targetRes) > 0 || len(compatRes) > 0 { + nonEmptyResults = true + } + }) + } + }) + } + + switch tc.resultType { + case nonEmptyResult: + assert.True(t, nonEmptyResults, "expected non-empty results (some documents should be modified)") + case emptyResult: + assert.False(t, nonEmptyResults, "expected empty results (no documents should be modified)") + default: + t.Fatalf("unknown result type %v", tc.resultType) + } + }) + } +} + // updateCommandCompatTestCase describes update command compatibility test case. type updateCommandCompatTestCase struct { multi any // defaults to false, if true updates multiple documents diff --git a/integration/update_field_compat_test.go b/integration/update_field_compat_test.go index d8c459470852..0e1d9e216fbc 100644 --- a/integration/update_field_compat_test.go +++ b/integration/update_field_compat_test.go @@ -21,6 +21,7 @@ import ( "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/bson/primitive" + "go.mongodb.org/mongo-driver/mongo/options" "github.com/FerretDB/FerretDB/integration/shareddata" "github.com/FerretDB/FerretDB/internal/types" @@ -817,6 +818,37 @@ func TestUpdateFieldCompatSet(t *testing.T) { testUpdateCompat(t, testCases) } +func TestUpdateManyFieldCompatSet(t *testing.T) { + t.Parallel() + + testCases := map[string]testUpdateManyCompatTestCase{ + "QueryOperatorExists": { + filter: bson.D{{"v", bson.D{{"$lt", 3}}}}, + update: bson.D{{"$set", bson.D{{"new", "val"}}}}, + updateOpts: options.Update().SetUpsert(true), + // only use providers contain filter match, no match results in + // upsert with generated ID which is tested in integration test + providers: []shareddata.Provider{shareddata.Scalars, shareddata.Int32s, shareddata.Doubles}, + }, + "QueryOperatorUpsertFalse": { + filter: bson.D{{"v", int32(4080)}}, + update: bson.D{{"$set", bson.D{{"new", "val"}}}}, + updateOpts: options.Update().SetUpsert(false), + }, + "QueryOperatorExistingModified": { + filter: bson.D{{"v", bson.D{{"$eq", 4080}}}}, + update: bson.D{{"$set", bson.D{{"new", "val"}}}}, + updateOpts: options.Update().SetUpsert(false), + }, + "QueryOperatorEmptySet": { + filter: bson.D{{"v", bson.D{{"$eq", 4080}}}}, + update: bson.D{{"$set", bson.D{}}}, + updateOpts: options.Update().SetUpsert(false), + }, + } + testUpdateManyCompat(t, testCases) +} + func TestUpdateFieldCompatSetArray(t *testing.T) { t.Parallel() diff --git a/integration/update_field_test.go b/integration/update_field_test.go index 21e1250a6e2b..d048f7b0e987 100644 --- a/integration/update_field_test.go +++ b/integration/update_field_test.go @@ -34,7 +34,7 @@ func TestUpdateFieldSet(t *testing.T) { t.Parallel() for name, tc := range map[string]struct { - filter bson.D // optional, defaults to bson.D{} + id string // optional, defaults to empty update bson.D // required, used for update parameter res *mongo.UpdateResult // optional, expected response from update @@ -42,7 +42,7 @@ func TestUpdateFieldSet(t *testing.T) { skip string // optional, skip test with a specified reason }{ "ArrayNil": { - filter: bson.D{{"_id", "string"}}, + id: "string", update: bson.D{{"$set", bson.D{{"v", bson.A{nil}}}}}, findRes: bson.D{{"_id", "string"}, {"v", bson.A{nil}}}, res: &mongo.UpdateResult{ @@ -52,7 +52,7 @@ func TestUpdateFieldSet(t *testing.T) { }, }, "SetSameValueInt": { - filter: bson.D{{"_id", "int32"}}, + id: "int32", update: bson.D{{"$set", bson.D{{"v", int32(42)}}}}, findRes: bson.D{{"_id", "int32"}, {"v", int32(42)}}, res: &mongo.UpdateResult{ @@ -61,26 +61,6 @@ func TestUpdateFieldSet(t *testing.T) { UpsertedCount: 0, }, }, - "QueryOperatorExistingModified": { - filter: bson.D{{"v", bson.D{{"$eq", 4080}}}}, - update: bson.D{{"$set", bson.D{{"new", "val"}}}}, - res: &mongo.UpdateResult{ - MatchedCount: 1, - ModifiedCount: 1, - UpsertedCount: 0, - }, - findRes: bson.D{{"_id", "int32-1"}, {"v", int32(4080)}, {"new", "val"}}, - }, - "QueryOperatorEmptySet": { - filter: bson.D{{"v", bson.D{{"$eq", 4080}}}}, - update: bson.D{{"$set", bson.D{}}}, - res: &mongo.UpdateResult{ - MatchedCount: 1, - ModifiedCount: 0, - UpsertedCount: 0, - }, - findRes: bson.D{{"_id", "int32-1"}, {"v", int32(4080)}}, - }, } { name, tc := name, tc t.Run(name, func(t *testing.T) { @@ -92,20 +72,15 @@ func TestUpdateFieldSet(t *testing.T) { require.NotNil(t, tc.update, "update should be set") - filter := bson.D{} - if tc.filter != nil { - filter = tc.filter - } - ctx, collection := setup.Setup(t, shareddata.Scalars, shareddata.Composites) - res, err := collection.UpdateOne(ctx, filter, tc.update) + res, err := collection.UpdateOne(ctx, bson.D{{"_id", tc.id}}, tc.update) require.NoError(t, err) require.Equal(t, tc.res, res) var actual bson.D - err = collection.FindOne(ctx, filter).Decode(&actual) + err = collection.FindOne(ctx, bson.D{{"_id", tc.id}}).Decode(&actual) require.NoError(t, err) AssertEqualDocuments(t, tc.findRes, actual) }) @@ -175,75 +150,6 @@ func TestUpdateFieldSetUpdateManyUpsert(t *testing.T) { } } -func TestUpdateFieldSetUpdateManyNoUpsert(t *testing.T) { - t.Parallel() - - for name, tc := range map[string]struct { //nolint:vet // used for testing only - filter bson.D // optional, defaults to bson.D{} - update bson.D // required, used for update parameter - opts *options.UpdateOptions // optional - providers shareddata.Providers // optional, defaults to shareddata.Nulls - - updateRes *mongo.UpdateResult // optional, expected response from update - findRes []bson.D // required, expected response from find - skip string // optional, skip test with a specified reason - }{ - "QueryOperatorExists": { - filter: bson.D{{"v", bson.D{{"$lt", 3}}}}, - update: bson.D{{"$set", bson.D{{"new", "val"}}}}, - opts: options.Update().SetUpsert(true), - providers: []shareddata.Provider{shareddata.Int32s}, - updateRes: &mongo.UpdateResult{ - MatchedCount: 2, - ModifiedCount: 2, - UpsertedCount: 0, - }, - findRes: []bson.D{ - {{"_id", "int32-min"}, {"v", int32(math.MinInt32)}, {"new", "val"}}, - {{"_id", "int32-zero"}, {"v", int32(0)}, {"new", "val"}}, - }, - }, - "QueryOperatorUpsertFalse": { - filter: bson.D{{"v", int32(4080)}}, - update: bson.D{{"$set", bson.D{{"new", "val"}}}}, - opts: options.Update().SetUpsert(false), - updateRes: &mongo.UpdateResult{ - MatchedCount: 0, - ModifiedCount: 0, - UpsertedCount: 0, - }, - findRes: []bson.D{}, - }, - } { - name, tc := name, tc - t.Run(name, func(t *testing.T) { - t.Parallel() - - require.NotNil(t, tc.update, "update should be set") - require.NotNil(t, tc.findRes, "findRes should be set") - - providers := tc.providers - if providers == nil { - providers = []shareddata.Provider{shareddata.Nulls} - } - - ctx, collection := setup.Setup(t, providers...) - - updateRes, err := collection.UpdateMany(ctx, tc.filter, tc.update, tc.opts) - require.NoError(t, err) - assert.Equal(t, tc.updateRes, updateRes) - - cursor, err := collection.Find(ctx, tc.filter, options.Find().SetSort(bson.D{{"_id", 1}})) - require.NoError(t, err) - - var res []bson.D - err = cursor.All(ctx, &res) - require.NoError(t, err) - AssertEqualDocumentsSlice(t, tc.findRes, res) - }) - } -} - func TestUpdateCommandUpsert(t *testing.T) { t.Parallel() @@ -327,7 +233,8 @@ func TestUpdateCommandUpsert(t *testing.T) { findRes: []bson.D{ {{"v", nil}}, {{"updateV", "greater"}}, - {{"updateV", "less"}}}, + {{"updateV", "less"}}, + }, }, "UnknownUpdateOperator": { updates: bson.A{ From 3ad5d5822b4333fa0459b20afc77d99d663d032d Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Thu, 13 Jul 2023 15:10:11 +0900 Subject: [PATCH 12/14] update test name --- integration/update_field_compat_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration/update_field_compat_test.go b/integration/update_field_compat_test.go index 0e1d9e216fbc..5c662b2bf88e 100644 --- a/integration/update_field_compat_test.go +++ b/integration/update_field_compat_test.go @@ -818,7 +818,7 @@ func TestUpdateFieldCompatSet(t *testing.T) { testUpdateCompat(t, testCases) } -func TestUpdateManyFieldCompatSet(t *testing.T) { +func TestUpdateFieldCompatSetMulti(t *testing.T) { t.Parallel() testCases := map[string]testUpdateManyCompatTestCase{ @@ -835,7 +835,7 @@ func TestUpdateManyFieldCompatSet(t *testing.T) { update: bson.D{{"$set", bson.D{{"new", "val"}}}}, updateOpts: options.Update().SetUpsert(false), }, - "QueryOperatorExistingModified": { + "QueryOperatorModified": { filter: bson.D{{"v", bson.D{{"$eq", 4080}}}}, update: bson.D{{"$set", bson.D{{"new", "val"}}}}, updateOpts: options.Update().SetUpsert(false), From 3523bbf38f216ac340aabddcd26c426e36276f32 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Thu, 13 Jul 2023 15:44:25 +0900 Subject: [PATCH 13/14] remove unnecessary code --- internal/handlers/common/findandmodify.go | 6 +++--- internal/handlers/pg/msg_update.go | 8 +------- internal/handlers/sqlite/msg_update.go | 8 +------- 3 files changed, 5 insertions(+), 17 deletions(-) diff --git a/internal/handlers/common/findandmodify.go b/internal/handlers/common/findandmodify.go index 0f41f5acda6c..8df5014ce9ce 100644 --- a/internal/handlers/common/findandmodify.go +++ b/internal/handlers/common/findandmodify.go @@ -209,7 +209,7 @@ func prepareDocumentForInsert(params *FindAndModifyParams) (*types.Document, err } if !insert.Has("_id") { - id, err := GetUpsertID(params.Query) + id, err := getUpsertID(params.Query) if err != nil { return nil, err } @@ -253,9 +253,9 @@ func prepareDocumentForUpdate(docs []*types.Document, params *FindAndModifyParam return update, nil } -// GetUpsertID gets the _id to use for upsert document. If query contains _id, +// getUpsertID gets the _id to use for upsert document. If query contains _id, // that _id is assigned unless _id contains operator. Otherwise, it generates an ID. -func GetUpsertID(query *types.Document) (any, error) { +func getUpsertID(query *types.Document) (any, error) { id, err := query.Get("_id") if err != nil { return types.NewObjectID(), nil diff --git a/internal/handlers/pg/msg_update.go b/internal/handlers/pg/msg_update.go index 065d96eaa82b..17239fe78198 100644 --- a/internal/handlers/pg/msg_update.go +++ b/internal/handlers/pg/msg_update.go @@ -112,13 +112,7 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, } if !doc.Has("_id") { - var id any - - if id, err = common.GetUpsertID(u.Filter); err != nil { - return err - } - - doc.Set("_id", id) + doc.Set("_id", types.NewObjectID()) } upserted.Append(must.NotFail(types.NewDocument( diff --git a/internal/handlers/sqlite/msg_update.go b/internal/handlers/sqlite/msg_update.go index 2ae9091f90e4..705ace4232a9 100644 --- a/internal/handlers/sqlite/msg_update.go +++ b/internal/handlers/sqlite/msg_update.go @@ -150,13 +150,7 @@ func (h *Handler) updateDocument(ctx context.Context, params *common.UpdatesPara } if !doc.Has("_id") { - var id any - - if id, err = common.GetUpsertID(u.Filter); err != nil { - return 0, 0, nil, err - } - - doc.Set("_id", id) + doc.Set("_id", types.NewObjectID()) } upserted.Append(must.NotFail(types.NewDocument( "index", int32(upserted.Len()), From d12fbc6d35cc3f176426377e63269b8dfd97bd35 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Thu, 13 Jul 2023 16:12:54 +0900 Subject: [PATCH 14/14] add todo --- integration/update_field_compat_test.go | 17 +++++++++++++++++ internal/handlers/pg/msg_update.go | 1 + internal/handlers/sqlite/msg_update.go | 1 + 3 files changed, 19 insertions(+) diff --git a/integration/update_field_compat_test.go b/integration/update_field_compat_test.go index 5c662b2bf88e..15e6694ef2b5 100644 --- a/integration/update_field_compat_test.go +++ b/integration/update_field_compat_test.go @@ -230,6 +230,22 @@ func TestUpdateFieldCompatIncComplex(t *testing.T) { testUpdateCompat(t, testCases) } +func TestUpdateFieldCompatIncMulti(t *testing.T) { + t.Parallel() + + testCases := map[string]testUpdateManyCompatTestCase{ + "InvalidInc": { + filter: bson.D{{"v", bson.D{{"$eq", "non-existent"}}}}, + update: bson.D{{"$inc", bson.D{{"v", 1}}}}, + updateOpts: options.Update().SetUpsert(true), + providers: []shareddata.Provider{shareddata.Scalars}, + skip: "https://github.com/FerretDB/FerretDB/issues/3044", + }, + } + + testUpdateManyCompat(t, testCases) +} + func TestUpdateFieldCompatMax(t *testing.T) { t.Parallel() @@ -846,6 +862,7 @@ func TestUpdateFieldCompatSetMulti(t *testing.T) { updateOpts: options.Update().SetUpsert(false), }, } + testUpdateManyCompat(t, testCases) } diff --git a/internal/handlers/pg/msg_update.go b/internal/handlers/pg/msg_update.go index 17239fe78198..52d91f49ae0b 100644 --- a/internal/handlers/pg/msg_update.go +++ b/internal/handlers/pg/msg_update.go @@ -104,6 +104,7 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, } if hasUpdateOperators { + // TODO: https://github.com/FerretDB/FerretDB/issues/3044 if _, err = common.UpdateDocument(document.Command(), doc, u.Update); err != nil { return err } diff --git a/internal/handlers/sqlite/msg_update.go b/internal/handlers/sqlite/msg_update.go index 705ace4232a9..d5c9f8fff2a7 100644 --- a/internal/handlers/sqlite/msg_update.go +++ b/internal/handlers/sqlite/msg_update.go @@ -142,6 +142,7 @@ func (h *Handler) updateDocument(ctx context.Context, params *common.UpdatesPara } if hasUpdateOperators { + // TODO: https://github.com/FerretDB/FerretDB/issues/3044 if _, err = common.UpdateDocument("update", doc, u.Update); err != nil { return 0, 0, nil, err }