Skip to content

Commit

Permalink
Reorganize and fix update/upsert logic (#4069)
Browse files Browse the repository at this point in the history 
Closes #3040.
Closes #3017.
Closes #3856.
Closes #2630.
Closes #3843.
  • Loading branch information
@wazir-ahmed
wazir-ahmed authored Feb 21, 2024
1 parent a1dc6c7 commit 4956f29
Show file tree
Hide file tree
Showing 13 changed files with 608 additions and 545 deletions.
86 changes: 78 additions & 8 deletions integration/findandmodify_compat_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"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/setup"
Expand Down Expand Up @@ -119,6 +120,24 @@ func TestFindAndModifyCompatErrors(t *testing.T) {
},
resultType: emptyResult,
},
"DuplicateID": {
command: bson.D{
{"query", bson.D{{"non-existent", "val"}}},
{"update", bson.D{{"_id", "int32"}, {"v", int32(43)}}},
{"upsert", true},
},
providers: []shareddata.Provider{shareddata.Int32s},
resultType: emptyResult,
},
"InvalidID": {
command: bson.D{
{"query", bson.D{{"non-existent", "val"}}},
{"update", bson.D{{"_id", primitive.Regex{Pattern: "[a-z]*[0-9]"}}, {"v", int32(43)}}},
{"upsert", true},
},
providers: []shareddata.Provider{shareddata.Int32s},
resultType: emptyResult,
},
}

testFindAndModifyCompat(t, testCases)
Expand Down Expand Up @@ -337,14 +356,12 @@ func TestFindAndModifyCompatUpdateSet(t *testing.T) {
command: bson.D{
{"update", bson.D{{"$set", bson.D{{"_id", "int32"}}}}},
},
skip: "https://github.com/FerretDB/FerretDB/issues/3017",
},
"UpdateExistingID": {
command: bson.D{
{"query", bson.D{{"_id", "int32"}}},
{"update", bson.D{{"$set", bson.D{{"_id", "int32-1"}}}}},
},
skip: "https://github.com/FerretDB/FerretDB/issues/3017",
},
"UpdateSameID": {
command: bson.D{
Expand Down Expand Up @@ -666,14 +683,12 @@ func TestFindAndModifyCompatUpsertSet(t *testing.T) {
{"update", bson.D{{"$set", bson.D{{"_id", "double"}}}}},
},
resultType: emptyResult, // _id must be an immutable field
skip: "https://github.com/FerretDB/FerretDB/issues/3017",
},
"UpsertIDNoQuery": {
command: bson.D{
{"upsert", true},
{"update", bson.D{{"$set", bson.D{{"_id", "int32"}, {"v", int32(2)}}}}},
},
skip: "https://github.com/FerretDB/FerretDB/issues/3017",
},
"UpsertExistingID": {
command: bson.D{
Expand All @@ -682,7 +697,6 @@ func TestFindAndModifyCompatUpsertSet(t *testing.T) {
{"update", bson.D{{"$set", bson.D{{"_id", "int32-1"}, {"v", int32(2)}}}}},
},
resultType: emptyResult,
skip: "https://github.com/FerretDB/FerretDB/issues/3017",
},
"UpsertSameID": {
command: bson.D{
Expand All @@ -697,7 +711,6 @@ func TestFindAndModifyCompatUpsertSet(t *testing.T) {
{"upsert", true},
{"update", bson.D{{"$set", bson.D{{"new", "val"}}}}},
},
skip: "https://github.com/FerretDB/FerretDB/issues/3856",
},
"UpsertQueryOperatorMixed": {
command: bson.D{
Expand All @@ -709,7 +722,20 @@ func TestFindAndModifyCompatUpsertSet(t *testing.T) {
{"upsert", true},
{"update", bson.D{{"$set", bson.D{{"new", "val"}}}}},
},
skip: "https://github.com/FerretDB/FerretDB/issues/3856",
},
"UpsertQueryObject": {
command: bson.D{
{"query", bson.D{{"_id", "non-existent"}, {"v", bson.D{{"k1", "v1"}}}}},
{"upsert", true},
{"update", bson.D{{"$set", bson.D{{"new", "val"}}}}},
},
},
"UpsertQueryObjectNested": {
command: bson.D{
{"query", bson.D{{"_id", "non-existent"}, {"v", bson.D{{"k1", "v1"}, {"k2", bson.D{{"k21", "v21"}}}}}}},
{"upsert", true},
{"update", bson.D{{"$set", bson.D{{"new", "val"}}}}},
},
},
}

Expand Down Expand Up @@ -853,6 +879,50 @@ func TestFindAndModifyCompatRemove(t *testing.T) {
testFindAndModifyCompat(t, testCases)
}

func TestFindAndModifyCompatReplacementDoc(t *testing.T) {
t.Parallel()

testCases := map[string]findAndModifyCompatTestCase{
"Basic": {
command: bson.D{
{"update", bson.D{{"v", int32(43)}}},
},
},
"EmptyDoc": {
command: bson.D{
{"update", bson.D{}},
},
},
"FilterAndUpsertTrue": {
command: bson.D{
{"query", bson.D{{"_id", "non-existent"}}},
{"update", bson.D{{"v", int32(43)}}},
{"upsert", true},
},
},
"WithUpdateOp": {
command: bson.D{
{"update", bson.D{{"v", int32(43)}, {"$set", bson.D{{"test", int32(0)}}}}},
},
resultType: emptyResult,
},
"SameId": {
command: bson.D{
{"query", bson.D{{"_id", "int32"}}},
{"update", bson.D{{"_id", "int32"}, {"v", int32(43)}}},
},
},
"DifferentId": {
command: bson.D{
{"query", bson.D{{"_id", "int32"}}},
{"update", bson.D{{"_id", "non-existent"}, {"v", int32(43)}}},
},
},
}

testFindAndModifyCompat(t, testCases)
}

// findAndModifyCompatTestCase describes findAndModify compatibility test case.
type findAndModifyCompatTestCase struct {
command bson.D
Expand Down Expand Up @@ -915,7 +985,7 @@ func testFindAndModifyCompat(t *testing.T, testCases map[string]findAndModifyCom
t.Logf("Compat error: %v", compatErr)

// error messages are intentionally not compared
AssertMatchesCommandError(t, compatErr, targetErr)
AssertMatchesError(t, compatErr, targetErr)

return
}
Expand Down
1 change: 0 additions & 1 deletion integration/findandmodify_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ func TestFindAndModifyCommandErrors(t *testing.T) {
"Performing an update on the path '_id' would modify the immutable field '_id'",
},
altMessage: "Performing an update on the path '_id' would modify the immutable field '_id'",
skip: "https://github.com/FerretDB/FerretDB/issues/3017",
},
"RenameEmptyFieldName": {
command: bson.D{
Expand Down
17 changes: 17 additions & 0 deletions integration/integration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,23 @@ func AssertEqualWriteError(t testtb.TB, expected mongo.WriteError, actual error)
return assert.Equal(t, expected, a)
}

// AssertMatchesError asserts that both errors are of same type and
// are equal in value, except the message and Raw part.
func AssertMatchesError(t testtb.TB, expected, actual error) {
t.Helper()

switch expected := expected.(type) { //nolint:errorlint // do not inspect error chain
case mongo.CommandError:
AssertMatchesCommandError(t, expected, actual)
case mongo.WriteException:
AssertMatchesWriteError(t, expected, actual)
case mongo.BulkWriteException:
AssertMatchesBulkException(t, expected, actual)
default:
t.Fatalf("unknown error type %T, expected one of [CommandError, WriteException, BulkWriteException]", expected)
}
}

// AssertMatchesCommandError asserts that both errors are equal CommandErrors,
// except messages (and ignoring the Raw part).
func AssertMatchesCommandError(t testtb.TB, expected, actual error) {
Expand Down
67 changes: 49 additions & 18 deletions integration/update_compat_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,12 +128,12 @@ func testUpdateCompat(t *testing.T, testCases map[string]updateCompatTestCase) {
require.NoError(t, compatErr, "compat error; target returned no error")
}

if pointer.Get(targetUpdateRes).ModifiedCount > 0 || pointer.Get(compatUpdateRes).ModifiedCount > 0 {
assert.Equal(t, compatUpdateRes, targetUpdateRes)

if pointer.Get(targetUpdateRes).ModifiedCount > 0 || pointer.Get(targetUpdateRes).UpsertedCount > 0 {
nonEmptyResults = true
}

assert.Equal(t, compatUpdateRes, targetUpdateRes)

var targetFindRes, compatFindRes bson.D
require.NoError(t, targetCollection.FindOne(ctx, bson.D{{"_id", id}}).Decode(&targetFindRes))
require.NoError(t, compatCollection.FindOne(ctx, bson.D{{"_id", id}}).Decode(&compatFindRes))
Expand Down Expand Up @@ -226,12 +226,12 @@ func testUpdateManyCompat(t *testing.T, testCases map[string]testUpdateManyCompa
}
require.NoError(t, compatErr, "compat error; target returned no error")

if pointer.Get(targetUpdateRes).ModifiedCount > 0 || pointer.Get(compatUpdateRes).ModifiedCount > 0 {
assert.Equal(t, compatUpdateRes, targetUpdateRes)

if pointer.Get(targetUpdateRes).ModifiedCount > 0 || pointer.Get(targetUpdateRes).UpsertedCount > 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)
Expand All @@ -250,10 +250,6 @@ func testUpdateManyCompat(t *testing.T, testCases map[string]testUpdateManyCompa
compatRes := FetchAll(t, ctx, compatCursor)

AssertEqualDocumentsSlice(t, compatRes, targetRes)

if len(targetRes) > 0 || len(compatRes) > 0 {
nonEmptyResults = true
}
})
}

Expand All @@ -272,6 +268,7 @@ func testUpdateManyCompat(t *testing.T, testCases map[string]testUpdateManyCompa
// updateCommandCompatTestCase describes update command compatibility test case.
type updateCommandCompatTestCase struct {
multi any // defaults to false, if true updates multiple documents
upsert bool // defaults to false
update bson.D // required
filter bson.D // defaults to bson.D{{"_id", id}}
resultType compatTestCaseResultType // defaults to nonEmptyResult
Expand Down Expand Up @@ -302,6 +299,9 @@ func testUpdateCommandCompat(t *testing.T, testCases map[string]updateCommandCom
require.NotNil(t, update, "`update` must be set")

multi := tc.multi
if multi == nil {
multi = false
}

var nonEmptyResults bool
for i := range targetCollections {
Expand Down Expand Up @@ -330,6 +330,7 @@ func testUpdateCommandCompat(t *testing.T, testCases map[string]updateCommandCom
{"q", filter},
{"u", update},
{"multi", multi},
{"upsert", tc.upsert},
}}},
}

Expand All @@ -339,6 +340,7 @@ func testUpdateCommandCompat(t *testing.T, testCases map[string]updateCommandCom
{"q", filter},
{"u", update},
{"multi", multi},
{"upsert", tc.upsert},
}}},
}

Expand All @@ -352,8 +354,8 @@ func testUpdateCommandCompat(t *testing.T, testCases map[string]updateCommandCom
t.Logf("Target error: %v", targetErr)
t.Logf("Compat error: %v", compatErr)

// error messages are intentionally not compared
AssertMatchesCommandError(t, compatErr, targetErr)
// compare error type and error code
AssertMatchesError(t, compatErr, targetErr)
} else {
require.NoError(t, compatErr, "compat error; target returned no error")
}
Expand Down Expand Up @@ -610,13 +612,11 @@ func TestUpdateCompat(t *testing.T) {
filter: bson.D{{"non-existent", "no-match"}},
replace: bson.D{{"_id", "new"}},
replaceOpts: options.Replace().SetUpsert(true),
resultType: emptyResult,
},
"UpdateNonExistentUpsert": {
filter: bson.D{{"_id", "non-existent"}},
update: bson.D{{"$set", bson.D{{"v", int32(42)}}}},
updateOpts: options.Update().SetUpsert(true),
resultType: emptyResult,
},
}

Expand Down Expand Up @@ -667,14 +667,13 @@ func TestUpdateCompatMultiFlagCommand(t *testing.T) {
resultType: emptyResult,
},
"TrueEmptyDocument": {
update: bson.D{},
multi: true,
skip: "https://github.com/FerretDB/FerretDB/issues/2630",
update: bson.D{},
multi: true,
resultType: emptyResult,
},
"FalseEmptyDocument": {
update: bson.D{},
multi: false,
skip: "https://github.com/FerretDB/FerretDB/issues/3843",
},
}

Expand Down Expand Up @@ -716,3 +715,35 @@ func TestReplaceKeepOrderCompat(t *testing.T) {

assert.Equal(t, compatDoc, targetDoc)
}

func TestUpdateCompatReplacementDoc(t *testing.T) {
t.Parallel()

testCases := map[string]updateCommandCompatTestCase{
"Basic": {
update: bson.D{{"v", int32(43)}},
},
"EmptyDoc": {
update: bson.D{},
},
"FilterAndUpsertTrue": {
filter: bson.D{{"_id", "non-existent"}},
update: bson.D{{"v", int32(43)}},
upsert: true,
},
"WithUpdateOp": {
update: bson.D{{"v", int32(43)}, {"$set", bson.D{{"test", int32(0)}}}},
resultType: emptyResult,
},
"SameId": {
filter: bson.D{{"_id", "int32"}},
update: bson.D{{"_id", "int32"}, {"v", int32(43)}},
},
"DifferentId": {
filter: bson.D{{"_id", "int32"}},
update: bson.D{{"_id", "non-existent"}, {"v", int32(43)}},
},
}

testUpdateCommandCompat(t, testCases)
}
Loading

0 comments on commit 4956f29

Please sign in to comment.