From 32b0543d34e38f4ad9496fc3bc25530dab7813d5 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Thu, 13 Apr 2023 17:12:52 +0900 Subject: [PATCH 01/14] add tests --- integration/findandmodify_compat_test.go | 57 ++++++++++++++++++++++++ internal/handlers/common/update.go | 18 ++++++-- 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/integration/findandmodify_compat_test.go b/integration/findandmodify_compat_test.go index bcdc0d84b68f..87531ec69cdb 100644 --- a/integration/findandmodify_compat_test.go +++ b/integration/findandmodify_compat_test.go @@ -190,6 +190,42 @@ func TestFindAndModifyCompatUpdate(t *testing.T) { }, skipForTigris: "schema validation would fail", }, + "SetForNonExisting": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, + {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, + }, + }, + "SetForExisting": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, + }, + }, + "UnsetForNonExisting": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, + {"update", bson.D{{"$unset", "v"}}}, + }, + }, + "UnsetForExisting": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{{"$unset", "v"}}}, + }, + }, + "UnsetForNonExistingInvalid": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, + {"update", bson.D{{"$unset", "non-existent-field"}}}, + }, + }, + "UnsetForExistingInvalid": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{{"$unset", "non-existent-field"}}}, + }, + }, } testFindAndModifyCompat(t, testCases) @@ -319,6 +355,27 @@ func TestFindAndModifyCompatUpsert(t *testing.T) { {"update", bson.D{{"v", "replaced"}}}, }, }, + "SetInvalidValue": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, + {"upsert", true}, + {"update", bson.D{{"$set", "invalid"}}}, + }, + }, + "SetForNonExisting": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, + {"upsert", true}, + {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, + }, + }, + "SetForExisting": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"upsert", true}, + {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, + }, + }, "UnsetForNonExisting": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, diff --git a/internal/handlers/common/update.go b/internal/handlers/common/update.go index 580a5c584221..54e7a0afdc39 100644 --- a/internal/handlers/common/update.go +++ b/internal/handlers/common/update.go @@ -61,13 +61,13 @@ func UpdateDocument(doc, update *types.Document) (bool, error) { } case "$set": - changed, err = processSetFieldExpression(doc, updateV.(*types.Document), false) + changed, err = processSetFieldExpression(doc, updateV, false) if err != nil { return false, err } case "$setOnInsert": - changed, err = processSetFieldExpression(doc, updateV.(*types.Document), true) + changed, err = processSetFieldExpression(doc, updateV, true) if err != nil { return false, err } @@ -199,9 +199,21 @@ func UpdateDocument(doc, update *types.Document) (bool, error) { // processSetFieldExpression changes document according to $set and $setOnInsert operators. // If the document was changed it returns true. -func processSetFieldExpression(doc, setDoc *types.Document, setOnInsert bool) (bool, error) { +func processSetFieldExpression(doc *types.Document, setVal any, setOnInsert bool) (bool, error) { var changed bool + setDoc, ok := setVal.(*types.Document) + if !ok { + return false, commonerrors.NewCommandErrorMsg( + commonerrors.ErrFailedToParse, + fmt.Sprintf( + "Modifiers operate on fields but we found type string instead. "+ + "For example: {$mod: {: ...}} not {$set: \"%s\"}", + setVal, + ), + ) + } + setDocKeys := setDoc.Keys() sort.Strings(setDocKeys) From 9c32f0ed359ed5aee529d92e0c24969facd961f4 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Fri, 14 Apr 2023 13:09:59 +0900 Subject: [PATCH 02/14] wip --- internal/handlers/common/update.go | 51 ++++++++++++++++++----- internal/handlers/pg/msg_findandmodify.go | 4 ++ 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/internal/handlers/common/update.go b/internal/handlers/common/update.go index 54e7a0afdc39..c1844a058d2a 100644 --- a/internal/handlers/common/update.go +++ b/internal/handlers/common/update.go @@ -33,6 +33,8 @@ import ( // UpdateDocument updates the given document with a series of update operators. // Returns true if document was changed. +// To validate update document, call ValidateUpdateOperators or +// ValidateFindAndModifyOperators before calling this. func UpdateDocument(doc, update *types.Document) (bool, error) { var changed bool var err error @@ -73,16 +75,8 @@ func UpdateDocument(doc, update *types.Document) (bool, error) { } case "$unset": - unsetDoc, ok := updateV.(*types.Document) - if !ok { - return false, commonerrors.NewCommandErrorMsg( - commonerrors.ErrFailedToParse, - fmt.Sprintf("Modifiers operate on fields but we found type string instead. "+ - "For example: {$mod: {: ...}} not {$unset: \"%s\"}", - updateV, - ), - ) - } + // Checked in validateUnsetExpression that updateV is a doc. + unsetDoc := updateV.(*types.Document) for _, key := range unsetDoc.Keys() { var path types.Path @@ -813,8 +807,18 @@ func processCurrentDateFieldExpression(doc *types.Document, currentDateVal any) return changed, nil } -// ValidateUpdateOperators validates update statement. +// ValidateFindAndModifyOperator validates update statement and returns NewCommandErrorMsg upon error. +func ValidateFindAndModifyOperator(update *types.Document) error { + return validateUpdateOperators(update, commonerrors.NewCommandErrorMsg) +} + +// ValidateUpdateOperators validates update statement and returns NewWriteErrorMsg upon error. func ValidateUpdateOperators(update *types.Document) error { + return validateUpdateOperators(update, commonerrors.NewWriteErrorMsg) +} + +// validateUpdateOperators validates update statement. +func validateUpdateOperators(update *types.Document, newErr func(errCode commonerrors.ErrorCode, msg string) error) error { var err error if _, err = HasSupportedUpdateModifiers(update); err != nil { return err @@ -850,6 +854,10 @@ func ValidateUpdateOperators(update *types.Document) error { return err } + if err = validateUnsetExpression(update, newErr); err != nil { + return err + } + unset, err := extractValueFromUpdateOperator("$unset", update) if err != nil { return err @@ -1162,3 +1170,24 @@ func validateCurrentDateExpression(update *types.Document) error { return nil } + +func validateUnsetExpression(update *types.Document, newErr func(errCode commonerrors.ErrorCode, msg string) error) error { + updateExpression, err := update.Get("$unset") + if err != nil { + // the update is not $unset operator, that's find + return nil + } + + _, ok := updateExpression.(*types.Document) + if !ok { + return newErr( + commonerrors.ErrFailedToParse, + fmt.Sprintf("Modifiers operate on fields but we found type string instead. "+ + "For example: {$mod: {: ...}} not {$unset: \"%s\"}", + updateExpression, + ), + ) + } + + return nil +} diff --git a/internal/handlers/pg/msg_findandmodify.go b/internal/handlers/pg/msg_findandmodify.go index 6a26be539ba8..988e5b6f5365 100644 --- a/internal/handlers/pg/msg_findandmodify.go +++ b/internal/handlers/pg/msg_findandmodify.go @@ -48,6 +48,10 @@ func (h *Handler) MsgFindAndModify(ctx context.Context, msg *wire.OpMsg) (*wire. return nil, err } + if err := common.ValidateFindAndModifyOperator(params.Update); err != nil { + return nil, err + } + if params.MaxTimeMS != 0 { ctxWithTimeout, cancel := context.WithTimeout(ctx, time.Duration(params.MaxTimeMS)*time.Millisecond) defer cancel() From b07b02147d1fb8fae99931a333b2879017333248 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Fri, 14 Apr 2023 15:18:52 +0900 Subject: [PATCH 03/14] findAndModify returns CommandError, update returns WriteError --- integration/findandmodify_compat_test.go | 7 --- integration/findandmodify_test.go | 11 ++++ internal/handlers/common/update.go | 65 ++++++++---------------- 3 files changed, 33 insertions(+), 50 deletions(-) diff --git a/integration/findandmodify_compat_test.go b/integration/findandmodify_compat_test.go index 87531ec69cdb..ce326a6a1d98 100644 --- a/integration/findandmodify_compat_test.go +++ b/integration/findandmodify_compat_test.go @@ -369,13 +369,6 @@ func TestFindAndModifyCompatUpsert(t *testing.T) { {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, }, }, - "SetForExisting": { - command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, - {"upsert", true}, - {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, - }, - }, "UnsetForNonExisting": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, diff --git a/integration/findandmodify_test.go b/integration/findandmodify_test.go index 1d6965360a24..6ae0e5aaf48b 100644 --- a/integration/findandmodify_test.go +++ b/integration/findandmodify_test.go @@ -191,6 +191,17 @@ func TestFindAndModifyUpsertComplex(t *testing.T) { }, skipForTigris: "schema validation would fail", }, + "SetForExisting": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"upsert", true}, + {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, + }, + lastErrorObject: bson.D{ + {"n", int32(1)}, + {"updatedExisting", false}, + }, + }, } { name, tc := name, tc t.Run(name, func(t *testing.T) { diff --git a/internal/handlers/common/update.go b/internal/handlers/common/update.go index c1844a058d2a..bc6d220fbb06 100644 --- a/internal/handlers/common/update.go +++ b/internal/handlers/common/update.go @@ -824,76 +824,72 @@ func validateUpdateOperators(update *types.Document, newErr func(errCode commone return err } - currentDate, err := extractValueFromUpdateOperator("$currentDate", update) + currentDate, err := extractValueFromUpdateOperator("$currentDate", update, newErr) if err != nil { return err } - inc, err := extractValueFromUpdateOperator("$inc", update) + inc, err := extractValueFromUpdateOperator("$inc", update, newErr) if err != nil { return err } - max, err := extractValueFromUpdateOperator("$max", update) + max, err := extractValueFromUpdateOperator("$max", update, newErr) if err != nil { return err } - min, err := extractValueFromUpdateOperator("$min", update) + min, err := extractValueFromUpdateOperator("$min", update, newErr) if err != nil { return err } - mul, err := extractValueFromUpdateOperator("$mul", update) + mul, err := extractValueFromUpdateOperator("$mul", update, newErr) if err != nil { return err } - set, err := extractValueFromUpdateOperator("$set", update) + set, err := extractValueFromUpdateOperator("$set", update, newErr) if err != nil { return err } - if err = validateUnsetExpression(update, newErr); err != nil { - return err - } - - unset, err := extractValueFromUpdateOperator("$unset", update) + unset, err := extractValueFromUpdateOperator("$unset", update, newErr) if err != nil { return err } - setOnInsert, err := extractValueFromUpdateOperator("$setOnInsert", update) + setOnInsert, err := extractValueFromUpdateOperator("$setOnInsert", update, newErr) if err != nil { return err } - _, err = extractValueFromUpdateOperator("$rename", update) + _, err = extractValueFromUpdateOperator("$rename", update, newErr) if err != nil { return err } - pop, err := extractValueFromUpdateOperator("$pop", update) + pop, err := extractValueFromUpdateOperator("$pop", update, newErr) if err != nil { return err } - push, err := extractValueFromUpdateOperator("$push", update) + push, err := extractValueFromUpdateOperator("$push", update, newErr) if err != nil { return err } - addToSet, err := extractValueFromUpdateOperator("$addToSet", update) + addToSet, err := extractValueFromUpdateOperator("$addToSet", update, newErr) if err != nil { return err } - pullAll, err := extractValueFromUpdateOperator("$pullAll", update) + pullAll, err := extractValueFromUpdateOperator("$pullAll", update, newErr) if err != nil { return err } - pull, err := extractValueFromUpdateOperator("$pull", update) + pull, err := extractValueFromUpdateOperator("$pull", update, newErr) if err != nil { return err } @@ -1010,7 +1006,7 @@ func checkConflictingChanges(a, b *types.Document) error { // bson.D{{"v", nil}}. // // It also checks for path collisions and returns the error if there's any. -func extractValueFromUpdateOperator(op string, update *types.Document) (*types.Document, error) { +func extractValueFromUpdateOperator(op string, update *types.Document, newErr func(errCode commonerrors.ErrorCode, msg string) error) (*types.Document, error) { if !update.Has(op) { return nil, nil } @@ -1018,15 +1014,19 @@ func extractValueFromUpdateOperator(op string, update *types.Document) (*types.D doc, ok := updateExpression.(*types.Document) if !ok { - return nil, commonerrors.NewWriteErrorMsg( + return nil, newErr( commonerrors.ErrFailedToParse, - "Modifiers operate on fields but we found another type instead", + fmt.Sprintf("Modifiers operate on fields but we found type string instead. "+ + "For example: {$mod: {: ...}} not {%s: \"%s\"}", + op, + updateExpression, + ), ) } duplicate, ok := doc.FindDuplicateKey() if ok { - return nil, commonerrors.NewWriteErrorMsg( + return nil, newErr( commonerrors.ErrConflictingUpdateOperators, fmt.Sprintf( "Updating the path '%[1]s' would create a conflict at '%[1]s'", duplicate, @@ -1170,24 +1170,3 @@ func validateCurrentDateExpression(update *types.Document) error { return nil } - -func validateUnsetExpression(update *types.Document, newErr func(errCode commonerrors.ErrorCode, msg string) error) error { - updateExpression, err := update.Get("$unset") - if err != nil { - // the update is not $unset operator, that's find - return nil - } - - _, ok := updateExpression.(*types.Document) - if !ok { - return newErr( - commonerrors.ErrFailedToParse, - fmt.Sprintf("Modifiers operate on fields but we found type string instead. "+ - "For example: {$mod: {: ...}} not {$unset: \"%s\"}", - updateExpression, - ), - ) - } - - return nil -} From fe544e889e508187151efc77375249ccfd8c7464 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Fri, 14 Apr 2023 15:55:49 +0900 Subject: [PATCH 04/14] return CommandError for more findAndModify cases --- integration/findandmodify_compat_test.go | 15 +++++ internal/handlers/common/findandmodify.go | 2 +- internal/handlers/common/update.go | 77 ++++++++++++----------- internal/handlers/pg/msg_findandmodify.go | 2 +- internal/handlers/pg/msg_update.go | 2 +- internal/handlers/tigris/msg_update.go | 2 +- 6 files changed, 59 insertions(+), 41 deletions(-) diff --git a/integration/findandmodify_compat_test.go b/integration/findandmodify_compat_test.go index ce326a6a1d98..40c0f09730b9 100644 --- a/integration/findandmodify_compat_test.go +++ b/integration/findandmodify_compat_test.go @@ -226,6 +226,21 @@ func TestFindAndModifyCompatUpdate(t *testing.T) { {"update", bson.D{{"$unset", "non-existent-field"}}}, }, }, + "InvalidOperator": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{{"$invalid", "non-existent-field"}}}, + }, + }, + "OperatorConflict": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{ + {"$set", bson.D{{"v", 4}}}, + {"$inc", bson.D{{"v", 4}}}, + }}, + }, + }, } testFindAndModifyCompat(t, testCases) diff --git a/internal/handlers/common/findandmodify.go b/internal/handlers/common/findandmodify.go index a6602b24c02f..f4ad1deffa1f 100644 --- a/internal/handlers/common/findandmodify.go +++ b/internal/handlers/common/findandmodify.go @@ -184,7 +184,7 @@ func GetFindAndModifyParams(doc *types.Document, l *zap.Logger) (*FindAndModifyP ) } - hasUpdateOperators, err := HasSupportedUpdateModifiers(update) + hasUpdateOperators, err := HasSupportedUpdateModifiers(command, update) if err != nil { return nil, err } diff --git a/internal/handlers/common/update.go b/internal/handlers/common/update.go index bc6d220fbb06..beec28e3e1a5 100644 --- a/internal/handlers/common/update.go +++ b/internal/handlers/common/update.go @@ -33,8 +33,7 @@ import ( // UpdateDocument updates the given document with a series of update operators. // Returns true if document was changed. -// To validate update document, call ValidateUpdateOperators or -// ValidateFindAndModifyOperators before calling this. +// To validate update document, call ValidateUpdateOperators before UpdateDocument. func UpdateDocument(doc, update *types.Document) (bool, error) { var changed bool var err error @@ -807,98 +806,88 @@ func processCurrentDateFieldExpression(doc *types.Document, currentDateVal any) return changed, nil } -// ValidateFindAndModifyOperator validates update statement and returns NewCommandErrorMsg upon error. -func ValidateFindAndModifyOperator(update *types.Document) error { - return validateUpdateOperators(update, commonerrors.NewCommandErrorMsg) -} - -// ValidateUpdateOperators validates update statement and returns NewWriteErrorMsg upon error. -func ValidateUpdateOperators(update *types.Document) error { - return validateUpdateOperators(update, commonerrors.NewWriteErrorMsg) -} - // validateUpdateOperators validates update statement. -func validateUpdateOperators(update *types.Document, newErr func(errCode commonerrors.ErrorCode, msg string) error) error { +func ValidateUpdateOperators(command string, update *types.Document) error { var err error - if _, err = HasSupportedUpdateModifiers(update); err != nil { + if _, err = HasSupportedUpdateModifiers(command, update); err != nil { return err } - currentDate, err := extractValueFromUpdateOperator("$currentDate", update, newErr) + currentDate, err := extractValueFromUpdateOperator(command, "$currentDate", update) if err != nil { return err } - inc, err := extractValueFromUpdateOperator("$inc", update, newErr) + inc, err := extractValueFromUpdateOperator(command, "$inc", update) if err != nil { return err } - max, err := extractValueFromUpdateOperator("$max", update, newErr) + max, err := extractValueFromUpdateOperator(command, "$max", update) if err != nil { return err } - min, err := extractValueFromUpdateOperator("$min", update, newErr) + min, err := extractValueFromUpdateOperator(command, "$min", update) if err != nil { return err } - mul, err := extractValueFromUpdateOperator("$mul", update, newErr) + mul, err := extractValueFromUpdateOperator(command, "$mul", update) if err != nil { return err } - set, err := extractValueFromUpdateOperator("$set", update, newErr) + set, err := extractValueFromUpdateOperator(command, "$set", update) if err != nil { return err } - unset, err := extractValueFromUpdateOperator("$unset", update, newErr) + unset, err := extractValueFromUpdateOperator(command, "$unset", update) if err != nil { return err } - setOnInsert, err := extractValueFromUpdateOperator("$setOnInsert", update, newErr) + setOnInsert, err := extractValueFromUpdateOperator(command, "$setOnInsert", update) if err != nil { return err } - _, err = extractValueFromUpdateOperator("$rename", update, newErr) + _, err = extractValueFromUpdateOperator(command, "$rename", update) if err != nil { return err } - pop, err := extractValueFromUpdateOperator("$pop", update, newErr) + pop, err := extractValueFromUpdateOperator(command, "$pop", update) if err != nil { return err } - push, err := extractValueFromUpdateOperator("$push", update, newErr) + push, err := extractValueFromUpdateOperator(command, "$push", update) if err != nil { return err } - addToSet, err := extractValueFromUpdateOperator("$addToSet", update, newErr) + addToSet, err := extractValueFromUpdateOperator(command, "$addToSet", update) if err != nil { return err } - pullAll, err := extractValueFromUpdateOperator("$pullAll", update, newErr) + pullAll, err := extractValueFromUpdateOperator(command, "$pullAll", update) if err != nil { return err } - pull, err := extractValueFromUpdateOperator("$pull", update, newErr) + pull, err := extractValueFromUpdateOperator(command, "$pull", update) if err != nil { return err } - if err = checkConflictingChanges(set, inc); err != nil { + if err = checkConflictingChanges(command, set, inc); err != nil { return err } - if err = checkConflictingOperators( + if err = checkConflictingOperators(command, mul, currentDate, inc, min, max, set, setOnInsert, unset, pop, push, addToSet, pullAll, pull, ); err != nil { return err @@ -918,7 +907,7 @@ func validateUpdateOperators(update *types.Document, newErr func(errCode commone // HasSupportedUpdateModifiers checks that update document contains supported update operators. // If no update operators are found it returns false. // If update document contains unsupported update operators it returns an error. -func HasSupportedUpdateModifiers(update *types.Document) (bool, error) { +func HasSupportedUpdateModifiers(command string, update *types.Document) (bool, error) { for _, updateOp := range update.Keys() { switch updateOp { case // field update operators: @@ -932,12 +921,13 @@ func HasSupportedUpdateModifiers(update *types.Document) (bool, error) { return true, nil default: if strings.HasPrefix(updateOp, "$") { - return false, commonerrors.NewWriteErrorMsg( + return false, newErr( commonerrors.ErrFailedToParse, fmt.Sprintf( "Unknown modifier: %s. Expected a valid update modifier or pipeline-style "+ "update specified as an array", updateOp, ), + command, ) } @@ -948,8 +938,17 @@ func HasSupportedUpdateModifiers(update *types.Document) (bool, error) { return false, nil } +// newErr returns CommandError for findAndModify command, other cases return WriteError. +func newErr(code commonerrors.ErrorCode, msg, command string) error { + if command == "findAndModify" { + return commonerrors.NewCommandErrorMsgWithArgument(code, msg, command) + } + + return commonerrors.NewWriteErrorMsg(code, msg) +} + // checkConflictingOperators checks if there are the same keys in these documents and returns an error, if any. -func checkConflictingOperators(a *types.Document, bs ...*types.Document) error { +func checkConflictingOperators(command string, a *types.Document, bs ...*types.Document) error { if a == nil { return nil } @@ -957,11 +956,12 @@ func checkConflictingOperators(a *types.Document, bs ...*types.Document) error { for _, key := range a.Keys() { for _, b := range bs { if b != nil && b.Has(key) { - return commonerrors.NewWriteErrorMsg( + return newErr( commonerrors.ErrConflictingUpdateOperators, fmt.Sprintf( "Updating the path '%[1]s' would create a conflict at '%[1]s'", key, ), + command, ) } } @@ -971,7 +971,7 @@ func checkConflictingOperators(a *types.Document, bs ...*types.Document) error { } // checkConflictingChanges checks if there are the same keys in these documents and returns an error, if any. -func checkConflictingChanges(a, b *types.Document) error { +func checkConflictingChanges(command string, a, b *types.Document) error { if a == nil { return nil } @@ -981,11 +981,12 @@ func checkConflictingChanges(a, b *types.Document) error { for _, key := range a.Keys() { if b.Has(key) { - return commonerrors.NewWriteErrorMsg( + return newErr( commonerrors.ErrConflictingUpdateOperators, fmt.Sprintf( "Updating the path '%[1]s' would create a conflict at '%[1]s'", key, ), + command, ) } } @@ -1006,7 +1007,7 @@ func checkConflictingChanges(a, b *types.Document) error { // bson.D{{"v", nil}}. // // It also checks for path collisions and returns the error if there's any. -func extractValueFromUpdateOperator(op string, update *types.Document, newErr func(errCode commonerrors.ErrorCode, msg string) error) (*types.Document, error) { +func extractValueFromUpdateOperator(command, op string, update *types.Document) (*types.Document, error) { if !update.Has(op) { return nil, nil } @@ -1021,6 +1022,7 @@ func extractValueFromUpdateOperator(op string, update *types.Document, newErr fu op, updateExpression, ), + command, ) } @@ -1031,6 +1033,7 @@ func extractValueFromUpdateOperator(op string, update *types.Document, newErr fu fmt.Sprintf( "Updating the path '%[1]s' would create a conflict at '%[1]s'", duplicate, ), + command, ) } diff --git a/internal/handlers/pg/msg_findandmodify.go b/internal/handlers/pg/msg_findandmodify.go index 988e5b6f5365..2854e1ba5880 100644 --- a/internal/handlers/pg/msg_findandmodify.go +++ b/internal/handlers/pg/msg_findandmodify.go @@ -48,7 +48,7 @@ func (h *Handler) MsgFindAndModify(ctx context.Context, msg *wire.OpMsg) (*wire. return nil, err } - if err := common.ValidateFindAndModifyOperator(params.Update); err != nil { + if err := common.ValidateUpdateOperators(document.Command(), params.Update); err != nil { return nil, err } diff --git a/internal/handlers/pg/msg_update.go b/internal/handlers/pg/msg_update.go index 448a2b76ce34..1c817112405a 100644 --- a/internal/handlers/pg/msg_update.go +++ b/internal/handlers/pg/msg_update.go @@ -128,7 +128,7 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, } if u != nil { - if err = common.ValidateUpdateOperators(u); err != nil { + if err = common.ValidateUpdateOperators(document.Command(), u); err != nil { return err } } diff --git a/internal/handlers/tigris/msg_update.go b/internal/handlers/tigris/msg_update.go index cda840fe1525..44d857fa00ff 100644 --- a/internal/handlers/tigris/msg_update.go +++ b/internal/handlers/tigris/msg_update.go @@ -106,7 +106,7 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, return nil, err } if u != nil { - if err = common.ValidateUpdateOperators(u); err != nil { + if err = common.ValidateUpdateOperators(document.Command(), u); err != nil { return nil, err } } From 705ed79fd59bfa91bf45b15798d2312ecfe265ab Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Fri, 14 Apr 2023 16:38:23 +0900 Subject: [PATCH 05/14] wip --- integration/findandmodify_compat_test.go | 19 +++++++++++++++++++ integration/findandmodify_test.go | 11 +++++++++++ internal/handlers/common/update.go | 2 +- internal/handlers/pg/msg_findandmodify.go | 6 ++++-- internal/handlers/tigris/msg_findandmodify.go | 6 ++++++ 5 files changed, 41 insertions(+), 3 deletions(-) diff --git a/integration/findandmodify_compat_test.go b/integration/findandmodify_compat_test.go index 40c0f09730b9..a65627e04e99 100644 --- a/integration/findandmodify_compat_test.go +++ b/integration/findandmodify_compat_test.go @@ -190,6 +190,18 @@ func TestFindAndModifyCompatUpdate(t *testing.T) { }, skipForTigris: "schema validation would fail", }, + "NonExistentExistsTrue": { + command: bson.D{ + {"query", bson.D{{"non-existent", bson.D{{"$exists", true}}}}}, + {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, + }, + }, + "NonExistentExistsFalse": { + command: bson.D{ + {"query", bson.D{{"non-existent", bson.D{{"$exists", false}}}}}, + {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, + }, + }, "SetForNonExisting": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, @@ -398,6 +410,13 @@ func TestFindAndModifyCompatUpsert(t *testing.T) { {"update", bson.D{{"$unset", "invalid"}}}, }, }, + "NonExistentExistsFalse": { + command: bson.D{ + {"query", bson.D{{"non-existent", bson.D{{"$exists", false}}}}}, + {"upsert", true}, + {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, + }, + }, } testFindAndModifyCompat(t, testCases) diff --git a/integration/findandmodify_test.go b/integration/findandmodify_test.go index 6ae0e5aaf48b..ac4c462dec37 100644 --- a/integration/findandmodify_test.go +++ b/integration/findandmodify_test.go @@ -202,6 +202,17 @@ func TestFindAndModifyUpsertComplex(t *testing.T) { {"updatedExisting", false}, }, }, + "NonExistentExistsTrue": { + command: bson.D{ + {"query", bson.D{{"non-existent", bson.D{{"$exists", true}}}}}, + {"upsert", true}, + {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, + }, + lastErrorObject: bson.D{ + {"n", int32(1)}, + {"updatedExisting", false}, + }, + }, } { name, tc := name, tc t.Run(name, func(t *testing.T) { diff --git a/internal/handlers/common/update.go b/internal/handlers/common/update.go index beec28e3e1a5..b92bc027fb87 100644 --- a/internal/handlers/common/update.go +++ b/internal/handlers/common/update.go @@ -806,7 +806,7 @@ func processCurrentDateFieldExpression(doc *types.Document, currentDateVal any) return changed, nil } -// validateUpdateOperators validates update statement. +// ValidateUpdateOperators validates update statement. func ValidateUpdateOperators(command string, update *types.Document) error { var err error if _, err = HasSupportedUpdateModifiers(command, update); err != nil { diff --git a/internal/handlers/pg/msg_findandmodify.go b/internal/handlers/pg/msg_findandmodify.go index 2854e1ba5880..4cf9e6dea035 100644 --- a/internal/handlers/pg/msg_findandmodify.go +++ b/internal/handlers/pg/msg_findandmodify.go @@ -48,8 +48,10 @@ func (h *Handler) MsgFindAndModify(ctx context.Context, msg *wire.OpMsg) (*wire. return nil, err } - if err := common.ValidateUpdateOperators(document.Command(), params.Update); err != nil { - return nil, err + if params.Update != nil { + if err := common.ValidateUpdateOperators(document.Command(), params.Update); err != nil { + return nil, err + } } if params.MaxTimeMS != 0 { diff --git a/internal/handlers/tigris/msg_findandmodify.go b/internal/handlers/tigris/msg_findandmodify.go index 0188809cdfe2..136e630730c6 100644 --- a/internal/handlers/tigris/msg_findandmodify.go +++ b/internal/handlers/tigris/msg_findandmodify.go @@ -46,6 +46,12 @@ func (h *Handler) MsgFindAndModify(ctx context.Context, msg *wire.OpMsg) (*wire. return nil, err } + if params.Update != nil { + if err := common.ValidateUpdateOperators(document.Command(), params.Update); err != nil { + return nil, err + } + } + if params.MaxTimeMS != 0 { ctxWithTimeout, cancel := context.WithTimeout(ctx, time.Duration(params.MaxTimeMS)*time.Millisecond) defer cancel() From 2664ef19dbbbd921f78d615cfc8aeb940487d421 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Fri, 14 Apr 2023 16:44:41 +0900 Subject: [PATCH 06/14] remove unnecessary change --- internal/handlers/common/update.go | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/internal/handlers/common/update.go b/internal/handlers/common/update.go index b92bc027fb87..8fb24ef108ff 100644 --- a/internal/handlers/common/update.go +++ b/internal/handlers/common/update.go @@ -62,13 +62,13 @@ func UpdateDocument(doc, update *types.Document) (bool, error) { } case "$set": - changed, err = processSetFieldExpression(doc, updateV, false) + changed, err = processSetFieldExpression(doc, updateV.(*types.Document), false) if err != nil { return false, err } case "$setOnInsert": - changed, err = processSetFieldExpression(doc, updateV, true) + changed, err = processSetFieldExpression(doc, updateV.(*types.Document), true) if err != nil { return false, err } @@ -192,21 +192,9 @@ func UpdateDocument(doc, update *types.Document) (bool, error) { // processSetFieldExpression changes document according to $set and $setOnInsert operators. // If the document was changed it returns true. -func processSetFieldExpression(doc *types.Document, setVal any, setOnInsert bool) (bool, error) { +func processSetFieldExpression(doc, setDoc *types.Document, setOnInsert bool) (bool, error) { var changed bool - setDoc, ok := setVal.(*types.Document) - if !ok { - return false, commonerrors.NewCommandErrorMsg( - commonerrors.ErrFailedToParse, - fmt.Sprintf( - "Modifiers operate on fields but we found type string instead. "+ - "For example: {$mod: {: ...}} not {$set: \"%s\"}", - setVal, - ), - ) - } - setDocKeys := setDoc.Keys() sort.Strings(setDocKeys) From 5185f894adbc2cd74cff2b596f4111049b89d4d9 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Fri, 14 Apr 2023 17:23:25 +0900 Subject: [PATCH 07/14] update more errors --- integration/findandmodify_compat_test.go | 30 ++++++++++ internal/handlers/common/update.go | 56 +++++++++---------- internal/handlers/pg/msg_findandmodify.go | 2 +- internal/handlers/tigris/msg_findandmodify.go | 2 +- 4 files changed, 57 insertions(+), 33 deletions(-) diff --git a/integration/findandmodify_compat_test.go b/integration/findandmodify_compat_test.go index a65627e04e99..9dd40d0ae741 100644 --- a/integration/findandmodify_compat_test.go +++ b/integration/findandmodify_compat_test.go @@ -253,6 +253,36 @@ func TestFindAndModifyCompatUpdate(t *testing.T) { }}, }, }, + "CurrentDateNotDoc": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{{"$currentDate", 1}}}, + }, + }, + "CurrentDateUnknownOption": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{{"$currentDate", bson.D{{"v", bson.D{{"foo", int32(1)}}}}}}}, + }, + }, + "CurrentDateNotDate": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{{"$currentDate", bson.D{{"v", bson.D{{"$type", int32(1)}}}}}}}, + }, + }, + "CurrentDateUnknownType": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{{"$currentDate", bson.D{{"v", bson.D{{"$type", "unknown"}}}}}}}, + }, + }, + "CurrentDateInvalidType": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{{"$currentDate", bson.D{{"v", 1}}}}}, + }, + }, } testFindAndModifyCompat(t, testCases) diff --git a/internal/handlers/common/update.go b/internal/handlers/common/update.go index 8fb24ef108ff..86c162f1d2e4 100644 --- a/internal/handlers/common/update.go +++ b/internal/handlers/common/update.go @@ -74,7 +74,7 @@ func UpdateDocument(doc, update *types.Document) (bool, error) { } case "$unset": - // Checked in validateUnsetExpression that updateV is a doc. + // Checked in processSetFieldExpression that updateV is a doc. unsetDoc := updateV.(*types.Document) for _, key := range unsetDoc.Keys() { @@ -881,7 +881,7 @@ func ValidateUpdateOperators(command string, update *types.Document) error { return err } - if err = validateCurrentDateExpression(update); err != nil { + if err = validateCurrentDateExpression(command, update); err != nil { return err } @@ -909,7 +909,7 @@ func HasSupportedUpdateModifiers(command string, update *types.Document) (bool, return true, nil default: if strings.HasPrefix(updateOp, "$") { - return false, newErr( + return false, newUpdateError( commonerrors.ErrFailedToParse, fmt.Sprintf( "Unknown modifier: %s. Expected a valid update modifier or pipeline-style "+ @@ -926,8 +926,8 @@ func HasSupportedUpdateModifiers(command string, update *types.Document) (bool, return false, nil } -// newErr returns CommandError for findAndModify command, other cases return WriteError. -func newErr(code commonerrors.ErrorCode, msg, command string) error { +// newUpdateError returns CommandError for findAndModify command, WriteError for other commands. +func newUpdateError(code commonerrors.ErrorCode, msg, command string) error { if command == "findAndModify" { return commonerrors.NewCommandErrorMsgWithArgument(code, msg, command) } @@ -944,7 +944,7 @@ func checkConflictingOperators(command string, a *types.Document, bs ...*types.D for _, key := range a.Keys() { for _, b := range bs { if b != nil && b.Has(key) { - return newErr( + return newUpdateError( commonerrors.ErrConflictingUpdateOperators, fmt.Sprintf( "Updating the path '%[1]s' would create a conflict at '%[1]s'", key, @@ -969,7 +969,7 @@ func checkConflictingChanges(command string, a, b *types.Document) error { for _, key := range a.Keys() { if b.Has(key) { - return newErr( + return newUpdateError( commonerrors.ErrConflictingUpdateOperators, fmt.Sprintf( "Updating the path '%[1]s' would create a conflict at '%[1]s'", key, @@ -1003,12 +1003,13 @@ func extractValueFromUpdateOperator(command, op string, update *types.Document) doc, ok := updateExpression.(*types.Document) if !ok { - return nil, newErr( + return nil, newUpdateError( commonerrors.ErrFailedToParse, - fmt.Sprintf("Modifiers operate on fields but we found type string instead. "+ - "For example: {$mod: {: ...}} not {%s: \"%s\"}", + fmt.Sprintf(`Modifiers operate on fields but we found type %[1]s instead. `+ + `For example: {$mod: {: ...}} not {%s: %s}`, + AliasFromType(updateExpression), op, - updateExpression, + types.FormatAnyValue(updateExpression), ), command, ) @@ -1016,7 +1017,7 @@ func extractValueFromUpdateOperator(command, op string, update *types.Document) duplicate, ok := doc.FindDuplicateKey() if ok { - return nil, newErr( + return nil, newUpdateError( commonerrors.ErrConflictingUpdateOperators, fmt.Sprintf( "Updating the path '%[1]s' would create a conflict at '%[1]s'", duplicate, @@ -1100,19 +1101,14 @@ func validateRenameExpression(update *types.Document) error { } // validateCurrentDateExpression validates $currentDate input on correctness. -func validateCurrentDateExpression(update *types.Document) error { +func validateCurrentDateExpression(command string, update *types.Document) error { currentDateTopField, err := update.Get("$currentDate") if err != nil { return nil // it is ok: key is absent } - currentDateExpression, ok := currentDateTopField.(*types.Document) - if !ok { - return commonerrors.NewWriteErrorMsg( - commonerrors.ErrFailedToParse, - "Modifiers operate on fields but we found another type instead", - ) - } + // currentDateExpression is document, checked in processSetFieldExpression. + currentDateExpression := currentDateTopField.(*types.Document) for _, field := range currentDateExpression.Keys() { setValue := must.NotFail(currentDateExpression.Get(field)) @@ -1121,9 +1117,10 @@ func validateCurrentDateExpression(update *types.Document) error { case *types.Document: for _, k := range setValue.Keys() { if k != "$type" { - return commonerrors.NewWriteErrorMsg( + return newUpdateError( commonerrors.ErrBadValue, fmt.Sprintf("Unrecognized $currentDate option: %s", k), + command, ) } } @@ -1133,16 +1130,12 @@ func validateCurrentDateExpression(update *types.Document) error { } currentDateTypeString, ok := currentDateType.(string) - if !ok { - return commonerrors.NewWriteErrorMsg( + if !ok || !slices.Contains([]string{"date", "timestamp"}, currentDateTypeString) { + return newUpdateError( commonerrors.ErrBadValue, - "The '$type' string field is required to be 'date' or 'timestamp'", - ) - } - if !slices.Contains([]string{"date", "timestamp"}, currentDateTypeString) { - return commonerrors.NewWriteErrorMsg( - commonerrors.ErrBadValue, - "The '$type' string field is required to be 'date' or 'timestamp'", + "The '$type' string field is required to be 'date' or 'timestamp': "+ + "{$currentDate: {field : {$type: 'date'}}}", + command, ) } @@ -1150,11 +1143,12 @@ func validateCurrentDateExpression(update *types.Document) error { continue default: - return commonerrors.NewWriteErrorMsg( + return newUpdateError( commonerrors.ErrBadValue, fmt.Sprintf("%s is not valid type for $currentDate. Please use a boolean ('true') "+ "or a $type expression ({$type: 'timestamp/date'}).", AliasFromType(setValue), ), + command, ) } } diff --git a/internal/handlers/pg/msg_findandmodify.go b/internal/handlers/pg/msg_findandmodify.go index 4cf9e6dea035..7b07c22919dd 100644 --- a/internal/handlers/pg/msg_findandmodify.go +++ b/internal/handlers/pg/msg_findandmodify.go @@ -49,7 +49,7 @@ func (h *Handler) MsgFindAndModify(ctx context.Context, msg *wire.OpMsg) (*wire. } if params.Update != nil { - if err := common.ValidateUpdateOperators(document.Command(), params.Update); err != nil { + if err = common.ValidateUpdateOperators(document.Command(), params.Update); err != nil { return nil, err } } diff --git a/internal/handlers/tigris/msg_findandmodify.go b/internal/handlers/tigris/msg_findandmodify.go index 136e630730c6..2a9b63fca527 100644 --- a/internal/handlers/tigris/msg_findandmodify.go +++ b/internal/handlers/tigris/msg_findandmodify.go @@ -47,7 +47,7 @@ func (h *Handler) MsgFindAndModify(ctx context.Context, msg *wire.OpMsg) (*wire. } if params.Update != nil { - if err := common.ValidateUpdateOperators(document.Command(), params.Update); err != nil { + if err = common.ValidateUpdateOperators(document.Command(), params.Update); err != nil { return nil, err } } From 03fd822d29eab84b7eff5dd1e1c8f4a1c107a4af Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Fri, 14 Apr 2023 18:58:36 +0900 Subject: [PATCH 08/14] add test --- integration/findandmodify_compat_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/integration/findandmodify_compat_test.go b/integration/findandmodify_compat_test.go index 9dd40d0ae741..c4795af76665 100644 --- a/integration/findandmodify_compat_test.go +++ b/integration/findandmodify_compat_test.go @@ -352,6 +352,21 @@ func TestFindAndModifyCompatUpsert(t *testing.T) { {"new", true}, }, }, + "UpsertNonExistent": { + command: bson.D{ + {"query", bson.D{{"_id", "non-existent"}}}, + {"upsert", true}, + {"update", bson.D{{"$set", bson.D{{"v", "43"}}}}}, + }, + }, + "UpsertNewNonExistent": { + command: bson.D{ + {"query", bson.D{{"_id", "non-existent"}}}, + {"upsert", true}, + {"update", bson.D{{"$set", bson.D{{"v", "43"}}}}}, + {"new", true}, + }, + }, "UpsertNoSuchDocument": { command: bson.D{ {"query", bson.D{{"_id", "no-such-doc"}}}, From 9e501d5ef96a61050b8e9ffe7d77c56f403ee30f Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Mon, 17 Apr 2023 12:34:17 +0900 Subject: [PATCH 09/14] wip --- integration/findandmodify_compat_test.go | 126 +++++++++++++++++------ integration/findandmodify_test.go | 2 +- internal/handlers/common/update.go | 35 +++---- 3 files changed, 114 insertions(+), 49 deletions(-) diff --git a/integration/findandmodify_compat_test.go b/integration/findandmodify_compat_test.go index c4795af76665..46d254a64c09 100644 --- a/integration/findandmodify_compat_test.go +++ b/integration/findandmodify_compat_test.go @@ -190,6 +190,30 @@ func TestFindAndModifyCompatUpdate(t *testing.T) { }, skipForTigris: "schema validation would fail", }, + "InvalidOperator": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{{"$invalid", "non-existent-field"}}}, + }, + }, + "OperatorConflict": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{ + {"$set", bson.D{{"v", 4}}}, + {"$inc", bson.D{{"v", 4}}}, + }}, + }, + }, + } + + testFindAndModifyCompat(t, testCases) +} + +func TestFindAndModifyCompatUpdateSet(t *testing.T) { + t.Parallel() + + testCases := map[string]findAndModifyCompatTestCase{ "NonExistentExistsTrue": { command: bson.D{ {"query", bson.D{{"non-existent", bson.D{{"$exists", true}}}}}, @@ -202,85 +226,127 @@ func TestFindAndModifyCompatUpdate(t *testing.T) { {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, }, }, - "SetForNonExisting": { + "ExistsTrue": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, }, }, - "SetForExisting": { + "ExistsFalse": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, }, }, - "UnsetForNonExisting": { + } + + testFindAndModifyCompat(t, testCases) +} + +func TestFindAndModifyCompatUpdateUnset(t *testing.T) { + t.Parallel() + + testCases := map[string]findAndModifyCompatTestCase{ + "NonExistentExistsTrue": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, {"update", bson.D{{"$unset", "v"}}}, }, }, - "UnsetForExisting": { + "NonExistentExistsFalse": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, {"update", bson.D{{"$unset", "v"}}}, }, }, - "UnsetForNonExistingInvalid": { + "ExistsTrue": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, {"update", bson.D{{"$unset", "non-existent-field"}}}, }, }, - "UnsetForExistingInvalid": { + "ExistsFalse": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, {"update", bson.D{{"$unset", "non-existent-field"}}}, }, }, - "InvalidOperator": { + } + + testFindAndModifyCompat(t, testCases) +} + +func TestFindAndModifyCompatUpdateCurrentDate(t *testing.T) { + t.Parallel() + + testCases := map[string]findAndModifyCompatTestCase{ + "NotDocument": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, - {"update", bson.D{{"$invalid", "non-existent-field"}}}, + {"query", bson.D{{"_id", "datetime"}}}, + {"update", bson.D{{"$currentDate", 1}}}, }, }, - "OperatorConflict": { + "UnknownOption": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, - {"update", bson.D{ - {"$set", bson.D{{"v", 4}}}, - {"$inc", bson.D{{"v", 4}}}, - }}, + {"query", bson.D{{"_id", "datetime"}}}, + {"update", bson.D{{"$currentDate", bson.D{{"v", bson.D{{"foo", int32(1)}}}}}}}, }, }, - "CurrentDateNotDoc": { + "NotDate": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, - {"update", bson.D{{"$currentDate", 1}}}, + {"query", bson.D{{"_id", "datetime"}}}, + {"update", bson.D{{"$currentDate", bson.D{{"v", bson.D{{"$type", int32(1)}}}}}}}, }, }, - "CurrentDateUnknownOption": { + "UnknownType": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, - {"update", bson.D{{"$currentDate", bson.D{{"v", bson.D{{"foo", int32(1)}}}}}}}, + {"query", bson.D{{"_id", "datetime"}}}, + {"update", bson.D{{"$currentDate", bson.D{{"v", bson.D{{"$type", "unknown"}}}}}}}, }, }, - "CurrentDateNotDate": { + "InvalidType": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, - {"update", bson.D{{"$currentDate", bson.D{{"v", bson.D{{"$type", int32(1)}}}}}}}, + {"query", bson.D{{"_id", "datetime"}}}, + {"update", bson.D{{"$currentDate", bson.D{{"v", 1}}}}}, }, }, - "CurrentDateUnknownType": { + } + + testFindAndModifyCompat(t, testCases) +} + +func TestFindAndModifyCompatUpdateRename(t *testing.T) { + t.Parallel() + + testCases := map[string]findAndModifyCompatTestCase{ + "NotDocument": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, - {"update", bson.D{{"$currentDate", bson.D{{"v", bson.D{{"$type", "unknown"}}}}}}}, + {"query", bson.D{{"_id", "int64"}}}, + {"update", bson.D{{"$rename", 1}}}, }, }, - "CurrentDateInvalidType": { + "NonStringTargetField": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, - {"update", bson.D{{"$currentDate", bson.D{{"v", 1}}}}}, + {"query", bson.D{{"_id", "int64"}}}, + {"update", bson.D{{"$rename", bson.D{{"v", 0}}}}}, + }, + }, + "SameTargetField": { + command: bson.D{ + {"query", bson.D{{"_id", "int64"}}}, + {"update", bson.D{{"$rename", bson.D{{"v", "v"}}}}}, + }, + }, + "DuplicateSource": { + command: bson.D{ + {"query", bson.D{{"_id", "int64"}}}, + {"update", bson.D{{"$rename", bson.D{{"v", "w"}, {"v", "x"}}}}}, + }, + }, + "DuplicateTarget": { + command: bson.D{ + {"query", bson.D{{"_id", "int64"}}}, + {"update", bson.D{{"$rename", bson.D{{"v", "w"}, {"x", "w"}}}}}, }, }, } diff --git a/integration/findandmodify_test.go b/integration/findandmodify_test.go index ac4c462dec37..6f345dd7d112 100644 --- a/integration/findandmodify_test.go +++ b/integration/findandmodify_test.go @@ -191,7 +191,7 @@ func TestFindAndModifyUpsertComplex(t *testing.T) { }, skipForTigris: "schema validation would fail", }, - "SetForExisting": { + "ExistsFalse": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, {"upsert", true}, diff --git a/internal/handlers/common/update.go b/internal/handlers/common/update.go index 86c162f1d2e4..2e8582c4e722 100644 --- a/internal/handlers/common/update.go +++ b/internal/handlers/common/update.go @@ -885,7 +885,7 @@ func ValidateUpdateOperators(command string, update *types.Document) error { return err } - if err = validateRenameExpression(update); err != nil { + if err = validateRenameExpression(command, update); err != nil { return err } @@ -1030,20 +1030,15 @@ func extractValueFromUpdateOperator(command, op string, update *types.Document) } // validateRenameExpression validates $rename input on correctness. -func validateRenameExpression(update *types.Document) error { +func validateRenameExpression(command string, update *types.Document) error { if !update.Has("$rename") { return nil } updateExpression := must.NotFail(update.Get("$rename")) - doc, ok := updateExpression.(*types.Document) - if !ok { - return commonerrors.NewWriteErrorMsg( - commonerrors.ErrFailedToParse, - "Modifiers operate on fields but we found another type instead", - ) - } + // updateExpression is document, checked in processSetFieldExpression. + doc := updateExpression.(*types.Document) iter := doc.Iterator() defer iter.Close() @@ -1062,35 +1057,39 @@ func validateRenameExpression(update *types.Document) error { var vStr string - vStr, ok = v.(string) + vStr, ok := v.(string) if !ok { - return commonerrors.NewWriteErrorMsg( + return newUpdateError( commonerrors.ErrBadValue, - fmt.Sprintf("The 'to' field for $rename must be a string: %s: %v", k, vStr), + fmt.Sprintf("The 'to' field for $rename must be a string: %s: %v", k, v), + command, ) } // disallow fields where key is equal to the target if k == vStr { - return commonerrors.NewWriteErrorMsg( + return newUpdateError( commonerrors.ErrBadValue, - fmt.Sprintf("The source and target field for $rename must differ: %s: %v", k, vStr), + fmt.Sprintf(`The source and target field for $rename must differ: %s: "%[1]s"`, k, vStr), + command, ) } if _, ok = keys[k]; ok { - return commonerrors.NewWriteErrorMsg( + return newUpdateError( commonerrors.ErrConflictingUpdateOperators, - fmt.Sprintf("Updating the '%s' would create a conflict at '%s'", k, k), + fmt.Sprintf("Updating the path '%s' would create a conflict at '%s'", k, k), + command, ) } keys[k] = struct{}{} if _, ok = keys[vStr]; ok { - return commonerrors.NewWriteErrorMsg( + return newUpdateError( commonerrors.ErrConflictingUpdateOperators, - fmt.Sprintf("Updating the '%s' would create a conflict at '%s'", vStr, vStr), + fmt.Sprintf("Updating the path '%s' would create a conflict at '%s'", vStr, vStr), + command, ) } From 5b7bb454b54e7b15522b9a24f4b895e173369081 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Mon, 17 Apr 2023 12:43:31 +0900 Subject: [PATCH 10/14] tests are covered in integration tests --- integration/findandmodify_compat_test.go | 107 ++++++++++++++--------- 1 file changed, 66 insertions(+), 41 deletions(-) diff --git a/integration/findandmodify_compat_test.go b/integration/findandmodify_compat_test.go index 46d254a64c09..e3deac5184fe 100644 --- a/integration/findandmodify_compat_test.go +++ b/integration/findandmodify_compat_test.go @@ -403,36 +403,6 @@ func TestFindAndModifyCompatUpsert(t *testing.T) { t.Parallel() testCases := map[string]findAndModifyCompatTestCase{ - "Upsert": { - command: bson.D{ - {"query", bson.D{{"_id", "double"}}}, - {"update", bson.D{{"$set", bson.D{{"v", 43.13}}}}}, - {"upsert", true}, - }, - }, - "UpsertNew": { - command: bson.D{ - {"query", bson.D{{"_id", "double"}}}, - {"update", bson.D{{"$set", bson.D{{"v", 43.13}}}}}, - {"upsert", true}, - {"new", true}, - }, - }, - "UpsertNonExistent": { - command: bson.D{ - {"query", bson.D{{"_id", "non-existent"}}}, - {"upsert", true}, - {"update", bson.D{{"$set", bson.D{{"v", "43"}}}}}, - }, - }, - "UpsertNewNonExistent": { - command: bson.D{ - {"query", bson.D{{"_id", "non-existent"}}}, - {"upsert", true}, - {"update", bson.D{{"$set", bson.D{{"v", "43"}}}}}, - {"new", true}, - }, - }, "UpsertNoSuchDocument": { command: bson.D{ {"query", bson.D{{"_id", "no-such-doc"}}}, @@ -493,39 +463,94 @@ func TestFindAndModifyCompatUpsert(t *testing.T) { {"update", bson.D{{"v", "replaced"}}}, }, }, - "SetInvalidValue": { + } + + testFindAndModifyCompat(t, testCases) +} + +func TestFindAndModifyCompatUpsertSet(t *testing.T) { + t.Parallel() + + testCases := map[string]findAndModifyCompatTestCase{ + "Upsert": { + command: bson.D{ + {"query", bson.D{{"_id", "double"}}}, + {"update", bson.D{{"$set", bson.D{{"v", 43.13}}}}}, + {"upsert", true}, + }, + }, + "UpsertNew": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, + {"query", bson.D{{"_id", "double"}}}, + {"update", bson.D{{"$set", bson.D{{"v", 43.13}}}}}, {"upsert", true}, - {"update", bson.D{{"$set", "invalid"}}}, + {"new", true}, }, }, - "SetForNonExisting": { + "UpsertNonExistent": { + command: bson.D{ + {"query", bson.D{{"_id", "non-existent"}}}, + {"upsert", true}, + {"update", bson.D{{"$set", bson.D{{"v", "43"}}}}}, + }, + }, + "UpsertNewNonExistent": { + command: bson.D{ + {"query", bson.D{{"_id", "non-existent"}}}, + {"upsert", true}, + {"update", bson.D{{"$set", bson.D{{"v", "43"}}}}}, + {"new", true}, + }, + }, + "NonExistentExistsFalse": { + command: bson.D{ + {"query", bson.D{{"non-existent", bson.D{{"$exists", false}}}}}, + {"upsert", true}, + {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, + }, + }, + "ExistsTrue": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, {"upsert", true}, {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, }, }, - "UnsetForNonExisting": { + } + + testFindAndModifyCompat(t, testCases) +} + +func TestFindAndModifyCompatUpsertUnset(t *testing.T) { + t.Parallel() + + testCases := map[string]findAndModifyCompatTestCase{ + "NonExistentExistsTrue": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, {"upsert", true}, - {"update", bson.D{{"$unset", "invalid"}}}, + {"update", bson.D{{"$unset", "v"}}}, }, }, - "UnsetForExisting": { + "NonExistentExistsFalse": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, {"upsert", true}, - {"update", bson.D{{"$unset", "invalid"}}}, + {"update", bson.D{{"$unset", "v"}}}, }, }, - "NonExistentExistsFalse": { + "ExistsTrue": { command: bson.D{ - {"query", bson.D{{"non-existent", bson.D{{"$exists", false}}}}}, + {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, {"upsert", true}, - {"update", bson.D{{"$set", bson.D{{"v", "foo"}}}}}, + {"update", bson.D{{"$unset", "non-existent-field"}}}, + }, + }, + "ExistsFalse": { + command: bson.D{ + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"upsert", true}, + {"update", bson.D{{"$unset", "non-existent-field"}}}, }, }, } From 144c855d1dd8ab59e5f8344a022b2123c21467c2 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Mon, 17 Apr 2023 13:11:45 +0900 Subject: [PATCH 11/14] add todo --- internal/handlers/common/update.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/handlers/common/update.go b/internal/handlers/common/update.go index 2e8582c4e722..c6c49f946082 100644 --- a/internal/handlers/common/update.go +++ b/internal/handlers/common/update.go @@ -34,6 +34,7 @@ import ( // UpdateDocument updates the given document with a series of update operators. // Returns true if document was changed. // To validate update document, call ValidateUpdateOperators before UpdateDocument. +// TODO findAndModify returns CommandError https://github.com/FerretDB/FerretDB/issues/2440 func UpdateDocument(doc, update *types.Document) (bool, error) { var changed bool var err error From 5d6271d15bf443d2d9ab7e586af59f3547702a5f Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Tue, 18 Apr 2023 12:12:49 +0900 Subject: [PATCH 12/14] update tests --- integration/findandmodify_compat_test.go | 25 ++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/integration/findandmodify_compat_test.go b/integration/findandmodify_compat_test.go index e3deac5184fe..d3c7422b666c 100644 --- a/integration/findandmodify_compat_test.go +++ b/integration/findandmodify_compat_test.go @@ -249,25 +249,31 @@ func TestFindAndModifyCompatUpdateUnset(t *testing.T) { testCases := map[string]findAndModifyCompatTestCase{ "NonExistentExistsTrue": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, + {"query", bson.D{{"non-existent", bson.D{{"$exists", true}}}}}, {"update", bson.D{{"$unset", "v"}}}, }, }, "NonExistentExistsFalse": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"query", bson.D{{"non-existent", bson.D{{"$exists", false}}}}}, {"update", bson.D{{"$unset", "v"}}}, }, }, "ExistsTrue": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, - {"update", bson.D{{"$unset", "non-existent-field"}}}, + {"update", bson.D{{"$unset", "v"}}}, }, }, "ExistsFalse": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"update", bson.D{{"$unset", "v"}}}, + }, + }, + "UnsetNonExistentField": { + command: bson.D{ + {"query", bson.D{{"_id", "double"}}}, {"update", bson.D{{"$unset", "non-existent-field"}}}, }, }, @@ -527,14 +533,14 @@ func TestFindAndModifyCompatUpsertUnset(t *testing.T) { testCases := map[string]findAndModifyCompatTestCase{ "NonExistentExistsTrue": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, + {"query", bson.D{{"non-existent", bson.D{{"$exists", true}}}}}, {"upsert", true}, {"update", bson.D{{"$unset", "v"}}}, }, }, "NonExistentExistsFalse": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"query", bson.D{{"non-existent", bson.D{{"$exists", false}}}}}, {"upsert", true}, {"update", bson.D{{"$unset", "v"}}}, }, @@ -543,13 +549,20 @@ func TestFindAndModifyCompatUpsertUnset(t *testing.T) { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", true}}}}}, {"upsert", true}, - {"update", bson.D{{"$unset", "non-existent-field"}}}, + {"update", bson.D{{"$unset", "v"}}}, }, }, "ExistsFalse": { command: bson.D{ {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, {"upsert", true}, + {"update", bson.D{{"$unset", "v"}}}, + }, + }, + "UnsetNonExistentField": { + command: bson.D{ + {"query", bson.D{{"_id", "double"}}}, + {"upsert", true}, {"update", bson.D{{"$unset", "non-existent-field"}}}, }, }, From 50b30d2829631da852c412d95efd0b25d55bd3ad Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Tue, 18 Apr 2023 12:16:01 +0900 Subject: [PATCH 13/14] wip --- integration/findandmodify_compat_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/integration/findandmodify_compat_test.go b/integration/findandmodify_compat_test.go index d3c7422b666c..45417507092e 100644 --- a/integration/findandmodify_compat_test.go +++ b/integration/findandmodify_compat_test.go @@ -271,9 +271,9 @@ func TestFindAndModifyCompatUpdateUnset(t *testing.T) { {"update", bson.D{{"$unset", "v"}}}, }, }, - "UnsetNonExistentField": { + "ExistsFalseNonExistentField": { command: bson.D{ - {"query", bson.D{{"_id", "double"}}}, + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, {"update", bson.D{{"$unset", "non-existent-field"}}}, }, }, @@ -559,9 +559,9 @@ func TestFindAndModifyCompatUpsertUnset(t *testing.T) { {"update", bson.D{{"$unset", "v"}}}, }, }, - "UnsetNonExistentField": { + "ExistsFalseNonExistentField": { command: bson.D{ - {"query", bson.D{{"_id", "double"}}}, + {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, {"upsert", true}, {"update", bson.D{{"$unset", "non-existent-field"}}}, }, From 2e03d2081ecc29ede2a3059e34fd91f25bfbe49a Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Tue, 18 Apr 2023 12:22:04 +0900 Subject: [PATCH 14/14] use simple example --- integration/findandmodify_compat_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/integration/findandmodify_compat_test.go b/integration/findandmodify_compat_test.go index 45417507092e..d3c7422b666c 100644 --- a/integration/findandmodify_compat_test.go +++ b/integration/findandmodify_compat_test.go @@ -271,9 +271,9 @@ func TestFindAndModifyCompatUpdateUnset(t *testing.T) { {"update", bson.D{{"$unset", "v"}}}, }, }, - "ExistsFalseNonExistentField": { + "UnsetNonExistentField": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"query", bson.D{{"_id", "double"}}}, {"update", bson.D{{"$unset", "non-existent-field"}}}, }, }, @@ -559,9 +559,9 @@ func TestFindAndModifyCompatUpsertUnset(t *testing.T) { {"update", bson.D{{"$unset", "v"}}}, }, }, - "ExistsFalseNonExistentField": { + "UnsetNonExistentField": { command: bson.D{ - {"query", bson.D{{"_id", bson.D{{"$exists", false}}}}}, + {"query", bson.D{{"_id", "double"}}}, {"upsert", true}, {"update", bson.D{{"$unset", "non-existent-field"}}}, },