Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return command error from findAndModify #2646

Merged
merged 17 commits into from
May 19, 2023
Merged

Return command error from findAndModify #2646

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
7ed9193
lower case findandmodify works
chilagrow May 16, 2023
cafc324
set returns command error for findAndModify
chilagrow May 17, 2023
833d344
returns command error for findAndModify
chilagrow May 17, 2023
9bd8961
merge from main
chilagrow May 17, 2023
7c24d6c
inc returns command error for findAndModify
chilagrow May 17, 2023
0fe2e5f
max returns command error for findAndModify
chilagrow May 17, 2023
1a34783
min returns command error for findAndModify
chilagrow May 17, 2023
5a1691f
mul returns command error for findAndModify
chilagrow May 17, 2023
ac24108
clean up
chilagrow May 17, 2023
6860223
add comments
chilagrow May 17, 2023
f38ee13
Merge branch 'main' into issue-1903-findandmodify-error
AlekSi May 17, 2023
4c3e926
remove js driver specific test
chilagrow May 18, 2023
9222059
remove unused code
chilagrow May 18, 2023
c005d0a
add uncovered test case for update with upsert
chilagrow May 19, 2023
748d3af
use compat when we can
chilagrow May 19, 2023
e05dbd0
fix non compiling code
chilagrow May 19, 2023
2106ffa
Merge branch 'main' into issue-1903-findandmodify-error
May 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
set returns command error for findAndModify
  • Loading branch information
@chilagrow
chilagrow committed May 17, 2023
commit cafc3245f026e51035a69c6afd0d447275d8b55b
12 changes: 12 additions & 0 deletions integration/findandmodify_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

"github.com/FerretDB/FerretDB/integration/setup"
"github.com/FerretDB/FerretDB/integration/shareddata"
"github.com/FerretDB/FerretDB/internal/handlers/commonerrors"
)

func TestFindAndModifyEmptyCollectionName(t *testing.T) {
Expand Down Expand Up @@ -137,6 +138,17 @@ func TestFindAndModifyErrors(t *testing.T) {
},
altMessage: "Performing an update on the path '_id' would modify the immutable field '_id'",
},
"SetUnsuitableValueType": {
command: bson.D{
{"update", bson.D{{"$set", bson.D{{"v.v.foo", "foo"}}}}},
},
err: &mongo.CommandError{
Code: 28,
Name: "PathNotViable",
Message: "Plan executor error during findAndModify :: caused by :: Cannot create field 'foo' in element {v: \"foo\"}",
},
altMessage: "Cannot create field 'foo' in element {v: { v: \"foo\" }}",
},
} {
name, tc := name, tc
t.Run(name, func(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions internal/handlers/common/findandmodify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ func prepareDocumentForInsert(params *FindAndModifyParams) (*types.Document, err
insert := must.NotFail(types.NewDocument())

if params.HasUpdateOperators {
if _, err := UpdateDocument(insert, params.Update); err != nil {
if _, err := UpdateDocument("findAndModify", insert, params.Update); err != nil {
return nil, err
}
} else {
Expand All @@ -224,7 +224,7 @@ func prepareDocumentForUpdate(docs []*types.Document, params *FindAndModifyParam
update := docs[0].DeepCopy()

if params.HasUpdateOperators {
if _, err := UpdateDocument(update, params.Update); err != nil {
if _, err := UpdateDocument("findAndModify", update, params.Update); err != nil {
return nil, err
}

Expand Down
14 changes: 6 additions & 8 deletions internal/handlers/common/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import (
// Returns true if document was changed.
// To validate update document, must call ValidateUpdateOperators before calling UpdateDocument.
// TODO findAndModify returns CommandError https://github.com/FerretDB/FerretDB/issues/2440
func UpdateDocument(doc, update *types.Document) (bool, error) {
rumyantseva marked this conversation as resolved.
Show resolved Hide resolved
func UpdateDocument(command string, doc, update *types.Document) (bool, error) {
var changed bool
var err error

Expand Down Expand Up @@ -64,13 +64,13 @@ func UpdateDocument(doc, update *types.Document) (bool, error) {
}

case "$set":
changed, err = processSetFieldExpression(doc, updateV.(*types.Document), false)
changed, err = processSetFieldExpression(command, doc, updateV.(*types.Document), false)
if err != nil {
return false, err
}

case "$setOnInsert":
changed, err = processSetFieldExpression(doc, updateV.(*types.Document), true)
changed, err = processSetFieldExpression(command, doc, updateV.(*types.Document), true)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -194,7 +194,7 @@ 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(command string, doc, setDoc *types.Document, setOnInsert bool) (bool, error) {
var changed bool

setDocKeys := setDoc.Keys()
Expand Down Expand Up @@ -230,10 +230,7 @@ func processSetFieldExpression(doc, setDoc *types.Document, setOnInsert bool) (b
}

if err := doc.SetByPath(path, setValue); err != nil {
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrUnsuitableValueType,
err.Error(),
)
return false, newUpdateError(commonerrors.ErrUnsuitableValueType, err.Error(), command)
}

changed = true
Expand Down Expand Up @@ -906,6 +903,7 @@ func HasSupportedUpdateModifiers(command string, update *types.Document) (bool,

// newUpdateError returns CommandError for findAndModify command, WriteError for other commands.
func newUpdateError(code commonerrors.ErrorCode, msg, command string) error {
// Depending on the driver, the command may be camel case or lower case.
if strings.EqualFold(command, "findAndModify") {
return commonerrors.NewCommandErrorMsgWithArgument(code, msg, command)
}
Expand Down
2 changes: 1 addition & 1 deletion internal/handlers/commonerrors/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ const (
ErrIndexNotFound = ErrorCode(27) // IndexNotFound

// ErrUnsuitableValueType indicates that field could not be created for given value.
ErrUnsuitableValueType = ErrorCode(28) // UnsuitableValueType
ErrUnsuitableValueType = ErrorCode(28) // PathNotViable

// ErrConflictingUpdateOperators indicates that $set, $inc or $setOnInsert were used together.
ErrConflictingUpdateOperators = ErrorCode(40) // ConflictingUpdateOperators
Expand Down
122 changes: 61 additions & 61 deletions internal/handlers/commonerrors/errorcode_string.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions internal/handlers/commonparams/extract_params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ func ExtractParams(doc *types.Document, command string, value any, l *zap.Logger
lookup := key

// If the key is the same as the command name, then it is a collection name.
// Depending on the driver, the key may be camel case or lower case for a collection name.
if strings.EqualFold(key, command) {
rumyantseva marked this conversation as resolved.
Show resolved Hide resolved
rumyantseva marked this conversation as resolved.
Show resolved Hide resolved
lookup = "collection"
}
Expand Down Expand Up @@ -179,6 +180,7 @@ func lookupFieldTag(key string, value *reflect.Value) (*int, *tagOptions, error)

optionsList := strings.Split(tag, ",")

// Depending on the driver, the key may be camel case or lower case for a collection name.
if !strings.EqualFold(optionsList[0], key) {
continue
}
Expand Down Expand Up @@ -410,6 +412,7 @@ func checkAllRequiredFieldsPopulated(v *reflect.Value, command string, keys []st
continue
}

// Depending on the driver, the key may be camel case or lower case for a collection name.
if !slices.Contains(keys, key) && !slices.Contains(keys, strings.ToLower(key)) {
return commonerrors.NewCommandErrorMsgWithArgument(
commonerrors.ErrMissingField,
Expand Down
2 changes: 1 addition & 1 deletion internal/handlers/pg/msg_findandmodify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func (h *Handler) MsgFindAndModify(ctx context.Context, msg *wire.OpMsg) (*wire.

if params.HasUpdateOperators {
upsert = resDocs[0].DeepCopy()
_, err = common.UpdateDocument(upsert, params.Update)
_, err = common.UpdateDocument(document.Command(), upsert, params.Update)
if err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions internal/handlers/pg/msg_update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg,
}

doc := u.Filter.DeepCopy()
if _, err = common.UpdateDocument(doc, u.Update); err != nil {
if _, err = common.UpdateDocument(document.Command(), doc, u.Update); err != nil {
return err
}
if !doc.Has("_id") {
Expand Down Expand Up @@ -113,7 +113,7 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg,
matched += int32(len(resDocs))

for _, doc := range resDocs {
changed, err := common.UpdateDocument(doc, u.Update)
changed, err := common.UpdateDocument(document.Command(), doc, u.Update)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion internal/handlers/tigris/msg_findandmodify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func (h *Handler) MsgFindAndModify(ctx context.Context, msg *wire.OpMsg) (*wire.

if params.HasUpdateOperators {
upsert = resDocs[0].DeepCopy()
if _, err = common.UpdateDocument(upsert, params.Update); err != nil {
if _, err = common.UpdateDocument(document.Command(), upsert, params.Update); err != nil {
return nil, err
}

Expand Down
4 changes: 2 additions & 2 deletions internal/handlers/tigris/msg_update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg,
}

doc := u.Filter.DeepCopy()
if _, err = common.UpdateDocument(doc, u.Update); err != nil {
if _, err = common.UpdateDocument(document.Command(), doc, u.Update); err != nil {
return nil, err
}
if !doc.Has("_id") {
Expand All @@ -95,7 +95,7 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg,
matched += int32(len(resDocs))

for _, doc := range resDocs {
changed, err := common.UpdateDocument(doc, u.Update)
changed, err := common.UpdateDocument(document.Command(), doc, u.Update)
if err != nil {
return nil, err
}
Expand Down