Skip to content

Commit

Permalink
Refactor types.NewPathFromString to return (Path, error) (#1994)
Browse files Browse the repository at this point in the history 
Closes #1954.
  • Loading branch information
Dmitry authored Feb 20, 2023
1 parent b20778f commit a62245e
Show file tree
Hide file tree
Showing 13 changed files with 188 additions and 83 deletions.
2 changes: 1 addition & 1 deletion integration/commands_diagnostic_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ func TestCommandsDiagnosticValidate(t *testing.T) {

actual.Remove("keysPerIndex")
actual.Remove("indexDetails")
testutil.CompareAndSetByPathNum(t, expected, actual, 18, types.NewPathFromString("nrecords"))
testutil.CompareAndSetByPathNum(t, expected, actual, 18, types.NewStaticPath("nrecords"))
testutil.AssertEqual(t, expected, actual)
}

Expand Down
14 changes: 7 additions & 7 deletions integration/update_field_compat_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,34 +52,34 @@ func TestUpdateFieldCompatCurrentDate(t *testing.T) {
},
"BoolTrue": {
update: bson.D{{"$currentDate", bson.D{{"v", true}}}},
paths: []types.Path{types.NewPathFromString("v")},
paths: []types.Path{types.NewStaticPath("v")},
},
"BoolTwoTrue": {
update: bson.D{{"$currentDate", bson.D{{"v", true}, {"nonexistent", true}}}},
paths: []types.Path{
types.NewPathFromString("v"),
types.NewPathFromString("nonexistent"),
types.NewStaticPath("v"),
types.NewStaticPath("nonexistent"),
},
},
"BoolFalse": {
update: bson.D{{"$currentDate", bson.D{{"v", false}}}},
paths: []types.Path{types.NewPathFromString("v")},
paths: []types.Path{types.NewStaticPath("v")},
},
"Int32": {
update: bson.D{{"$currentDate", bson.D{{"v", int32(1)}}}},
resultType: emptyResult,
},
"Timestamp": {
update: bson.D{{"$currentDate", bson.D{{"v", bson.D{{"$type", "timestamp"}}}}}},
paths: []types.Path{types.NewPathFromString("v")},
paths: []types.Path{types.NewStaticPath("v")},
},
"TimestampCapitalised": {
update: bson.D{{"$currentDate", bson.D{{"v", bson.D{{"$type", "Timestamp"}}}}}},
resultType: emptyResult,
},
"Date": {
update: bson.D{{"$currentDate", bson.D{{"v", bson.D{{"$type", "date"}}}}}},
paths: []types.Path{types.NewPathFromString("v")},
paths: []types.Path{types.NewStaticPath("v")},
},
"WrongType": {
update: bson.D{{"$currentDate", bson.D{{"v", bson.D{{"$type", bson.D{{"abcd", int32(1)}}}}}}}},
Expand All @@ -88,7 +88,7 @@ func TestUpdateFieldCompatCurrentDate(t *testing.T) {
"NoField": {
update: bson.D{{"$currentDate", bson.D{{"nonexistent", bson.D{{"$type", "date"}}}}}},
paths: []types.Path{
types.NewPathFromString("nonexistent"),
types.NewStaticPath("nonexistent"),
},
},
"UnrecognizedOption": {
Expand Down
7 changes: 6 additions & 1 deletion internal/handlers/common/distinct.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,12 @@ func FilterDistinctValues(docs []*types.Document, key string) (*types.Array, err
for _, doc := range docs {
var val any

val, err := doc.GetByPath(types.NewPathFromString(key))
path, err := types.NewPathFromString(key)
if err != nil {
return nil, lazyerrors.Error(err)
}

val, err = doc.GetByPath(path)
if err != nil {
continue
}
Expand Down
7 changes: 6 additions & 1 deletion internal/handlers/common/filter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"golang.org/x/exp/slices"

"github.com/FerretDB/FerretDB/internal/types"
"github.com/FerretDB/FerretDB/internal/util/lazyerrors"
"github.com/FerretDB/FerretDB/internal/util/must"
)

Expand Down Expand Up @@ -56,7 +57,11 @@ func FilterDocument(doc, filter *types.Document) (bool, error) {
func filterDocumentPair(doc *types.Document, filterKey string, filterValue any) (bool, error) {
if strings.ContainsRune(filterKey, '.') {
// {field1./.../.fieldN: filterValue}
path := types.NewPathFromString(filterKey)
path, err := types.NewPathFromString(filterKey)
if err != nil {
return false, lazyerrors.Error(err)
}

// we pass the path without the last key because we want {fieldN: *someValue*}, not just *someValue*
docValue, err := doc.GetByPath(path.TrimSuffix())
if err != nil {
Expand Down
100 changes: 66 additions & 34 deletions internal/handlers/common/update.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

"golang.org/x/exp/slices"

"github.com/FerretDB/FerretDB/internal/handlers/commonerrors"
"github.com/FerretDB/FerretDB/internal/types"
"github.com/FerretDB/FerretDB/internal/util/iterator"
"github.com/FerretDB/FerretDB/internal/util/lazyerrors"
Expand Down Expand Up @@ -75,7 +76,13 @@ func UpdateDocument(doc, update *types.Document) (bool, error) {
unsetDoc := updateV.(*types.Document)

for _, key := range unsetDoc.Keys() {
path := types.NewPathFromString(key)
var path types.Path

path, err = types.NewPathFromString(key)
if err != nil {
return false, lazyerrors.Error(err)
}

if doc.HasByPath(path) {
doc.RemoveByPath(path)
changed = true
Expand Down Expand Up @@ -175,7 +182,10 @@ func processSetFieldExpression(doc, setDoc *types.Document, setOnInsert bool) (b
}
}

path := types.NewPathFromString(setKey)
path, err := types.NewPathFromString(setKey)
if err != nil {
return false, lazyerrors.Error(err)
}

if doc.HasByPath(path) {
docValue := must.NotFail(doc.GetByPath(path))
Expand All @@ -190,8 +200,8 @@ func processSetFieldExpression(doc, setDoc *types.Document, setOnInsert bool) (b
}

if err := doc.SetByPath(path, setValue); err != nil {
return false, NewWriteErrorMsg(
ErrUnsuitableValueType,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrUnsuitableValueType,
err.Error(),
)
}
Expand Down Expand Up @@ -219,8 +229,15 @@ func processRenameFieldExpression(doc *types.Document, update *types.Document) (
// this is covered in validateRenameExpression
renameValue := renameRawValue.(string)

sourcePath := types.NewPathFromString(key)
targetPath := types.NewPathFromString(renameValue)
sourcePath, err := types.NewPathFromString(key)
if err != nil {
return changed, lazyerrors.Error(err)
}

targetPath, err := types.NewPathFromString(renameValue)
if err != nil {
return changed, lazyerrors.Error(err)
}

// Get value to move
val, err := doc.GetByPath(sourcePath)
Expand All @@ -234,7 +251,7 @@ func processRenameFieldExpression(doc *types.Document, update *types.Document) (
continue
}

return changed, NewWriteErrorMsg(ErrUnsuitableValueType, dpe.Error())
return changed, commonerrors.NewWriteErrorMsg(commonerrors.ErrUnsuitableValueType, dpe.Error())
}

// Remove old document
Expand Down Expand Up @@ -262,23 +279,29 @@ func processIncFieldExpression(doc *types.Document, updateV any) (bool, error) {
for _, incKey := range incDoc.Keys() {
incValue := must.NotFail(incDoc.Get(incKey))

path := types.NewPathFromString(incKey)
var path types.Path
var err error

path, err = types.NewPathFromString(incKey)
if err != nil {
return false, lazyerrors.Error(err)
}

if !doc.HasByPath(path) {
// ensure incValue is a valid number type.
switch incValue.(type) {
case float64, int32, int64:
default:
return false, NewWriteErrorMsg(
ErrTypeMismatch,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrTypeMismatch,
fmt.Sprintf(`Cannot increment with non-numeric argument: {%s: %#v}`, incKey, incValue),
)
}

// $inc sets the field if it does not exist.
if err := doc.SetByPath(path, incValue); err != nil {
return false, NewWriteErrorMsg(
ErrUnsuitableValueType,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrUnsuitableValueType,
err.Error(),
)
}
Expand All @@ -288,7 +311,12 @@ func processIncFieldExpression(doc *types.Document, updateV any) (bool, error) {
continue
}

docValue, err := doc.GetByPath(types.NewPathFromString(incKey))
path, err = types.NewPathFromString(incKey)
if err != nil {
return false, lazyerrors.Error(err)
}

docValue, err := doc.GetByPath(path)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -449,8 +477,8 @@ func processMinFieldExpression(doc *types.Document, updateV any) (bool, error) {
func processMulFieldExpression(doc *types.Document, updateV any) (bool, error) {
mulDoc, ok := updateV.(*types.Document)
if !ok {
return false, NewWriteErrorMsg(
ErrFailedToParse,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrFailedToParse,
fmt.Sprintf(`Modifiers operate on fields but we found type %[1]s instead. `+
`For example: {$mod: {<field>: ...}} not {$rename: %[1]s}`,
AliasFromType(updateV),
Expand All @@ -463,7 +491,13 @@ func processMulFieldExpression(doc *types.Document, updateV any) (bool, error) {
for _, mulKey := range mulDoc.Keys() {
mulValue := must.NotFail(mulDoc.Get(mulKey))

path := types.NewPathFromString(mulKey)
var path types.Path
var err error

path, err = types.NewPathFromString(mulKey)
if err != nil {
return false, lazyerrors.Error(err)
}

if !doc.HasByPath(path) {
// $mul sets the field to zero if the field does not exist.
Expand All @@ -475,16 +509,16 @@ func processMulFieldExpression(doc *types.Document, updateV any) (bool, error) {
case int64:
mulValue = int64(0)
default:
return false, NewWriteErrorMsg(
ErrTypeMismatch,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrTypeMismatch,
fmt.Sprintf(`Cannot multiply with non-numeric argument: {%s: %#v}`, mulKey, mulValue),
)
}

err := doc.SetByPath(path, mulValue)
if err != nil {
return false, NewWriteErrorMsg(
ErrUnsuitableValueType,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrUnsuitableValueType,
err.Error(),
)
}
Expand All @@ -494,8 +528,6 @@ func processMulFieldExpression(doc *types.Document, updateV any) (bool, error) {
continue
}

var err error

docValue, err := doc.GetByPath(path)
if err != nil {
return false, err
Expand All @@ -507,8 +539,8 @@ func processMulFieldExpression(doc *types.Document, updateV any) (bool, error) {
switch {
case err == nil:
if multiplied, ok := multiplied.(float64); ok && math.IsInf(multiplied, 0) {
return false, NewCommandErrorMsg(
ErrBadValue,
return false, commonerrors.NewCommandErrorMsg(
commonerrors.ErrBadValue,
fmt.Sprintf("update produces invalid value: { %q: %f } "+
"(update operations that produce infinity values are not allowed)", path, multiplied,
),
Expand All @@ -517,8 +549,8 @@ func processMulFieldExpression(doc *types.Document, updateV any) (bool, error) {

err = doc.SetByPath(path, multiplied)
if err != nil {
return false, NewWriteErrorMsg(
ErrUnsuitableValueType,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrUnsuitableValueType,
fmt.Sprintf(`Cannot create field in element {%s: %v}`, path.Prefix(), docValue),
)
}
Expand All @@ -535,8 +567,8 @@ func processMulFieldExpression(doc *types.Document, updateV any) (bool, error) {
continue

case errors.Is(err, errUnexpectedLeftOpType):
return false, NewWriteErrorMsg(
ErrTypeMismatch,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrTypeMismatch,
fmt.Sprintf(
`Cannot multiply with non-numeric argument: {%s: %#v}`,
mulKey,
Expand All @@ -549,8 +581,8 @@ func processMulFieldExpression(doc *types.Document, updateV any) (bool, error) {
k = path.Suffix()
}

return false, NewWriteErrorMsg(
ErrTypeMismatch,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrTypeMismatch,
fmt.Sprintf(
`Cannot apply $mul to a value of non-numeric type. `+
`{_id: %s} has the field '%s' of non-numeric type %s`,
Expand All @@ -560,17 +592,17 @@ func processMulFieldExpression(doc *types.Document, updateV any) (bool, error) {
),
)
case errors.Is(err, errLongExceeded):
return false, NewWriteErrorMsg(
ErrBadValue,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrBadValue,
fmt.Sprintf(
`Failed to apply $mul operations to current value ((NumberLong)%d) for document {_id: "%s"}`,
docValue,
must.NotFail(doc.Get("_id")),
),
)
case errors.Is(err, errIntExceeded):
return false, NewWriteErrorMsg(
ErrBadValue,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrBadValue,
fmt.Sprintf(
`Failed to apply $mul operations to current value ((NumberInt)%d) for document {_id: "%s"}`,
docValue,
Expand Down
28 changes: 20 additions & 8 deletions internal/handlers/common/update_array_operators.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,23 @@ func processPopArrayUpdateExpression(doc *types.Document, update *types.Document

popValue, err := GetWholeNumberParam(popValueRaw)
if err != nil {
return false, NewWriteErrorMsg(ErrFailedToParse, fmt.Sprintf(`Expected a number in: %s: "%v"`, key, popValueRaw))
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrFailedToParse,
fmt.Sprintf(`Expected a number in: %s: "%v"`, key, popValueRaw),
)
}

if popValue != 1 && popValue != -1 {
return false, NewWriteErrorMsg(ErrFailedToParse, fmt.Sprintf("$pop expects 1 or -1, found: %d", popValue))
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrFailedToParse,
fmt.Sprintf("$pop expects 1 or -1, found: %d", popValue),
)
}

path := types.NewPathFromString(key)
path, err := types.NewPathFromString(key)
if err != nil {
return false, lazyerrors.Error(err)
}

val, err := doc.GetByPath(path)
if err != nil {
Expand Down Expand Up @@ -150,13 +159,16 @@ func processPushArrayUpdateExpression(doc *types.Document, update *types.Documen
return false, lazyerrors.Error(err)
}

path := types.NewPathFromString(key)
path, err := types.NewPathFromString(key)
if err != nil {
return false, lazyerrors.Error(err)
}

// If the path does not exist, create a new array and set it.
if !doc.HasByPath(path) {
if err = doc.SetByPath(path, types.MakeArray(1)); err != nil {
return false, NewWriteErrorMsg(
ErrUnsuitableValueType,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrUnsuitableValueType,
err.Error(),
)
}
Expand All @@ -169,8 +181,8 @@ func processPushArrayUpdateExpression(doc *types.Document, update *types.Documen

array, ok := val.(*types.Array)
if !ok {
return false, NewWriteErrorMsg(
ErrBadValue,
return false, commonerrors.NewWriteErrorMsg(
commonerrors.ErrBadValue,
fmt.Sprintf(
"The field '%s' must be an array but is of type '%s' in document {_id: %s}",
key, AliasFromType(val), must.NotFail(doc.Get("_id")),
Expand Down
Loading

0 comments on commit a62245e

Please sign in to comment.