tidy up

FerretDB · Jul 21, 2023 · d1b8ff0 · d1b8ff0
1 parent bee2816
commit d1b8ff0
Show file tree

Hide file tree

Showing 9 changed files with 55 additions and 53 deletions.
diff --git a/internal/handlers/common/aggregations/expression.go b/internal/handlers/common/aggregations/expression.go
@@ -70,23 +70,20 @@ func (e *ExpressionError) Code() ExpressionErrorCode {
 
 // Expression is an expression constructed from field value.
 type Expression struct {
-	*ExpressionOpts
+	opts commonpath.FindValuesOpts
 	path types.Path
 }
 
-// ExpressionOpts represents options used to modify behavior of Expression functions.
-type ExpressionOpts struct {
-	// TODO https://github.com/FerretDB/FerretDB/issues/2348
-
-	// IgnoreArrays disables checking arrays for provided key.
-	// So expression {"$v.foo"} won't match {"v":[{"foo":42}]}
-	IgnoreArrays bool // defaults to false
-}
+// NewExpression creates a new instance by checking expression string.
+// It can take additional opts that specify how path should be searched.
+func NewExpression(expression string, opts *commonpath.FindValuesOpts) (*Expression, error) {
+	if opts == nil {
+		opts = &commonpath.FindValuesOpts{
+			FindArrayIndex: false,
+			SearchArray:    true,
+		}
+	}
 
-// NewExpressionWithOpts creates a new instance by checking expression string.
-// It can take additional opts that specify how expressions should be evaluated.
-func NewExpressionWithOpts(expression string, opts *ExpressionOpts) (*Expression, error) {
-	// TODO https://github.com/FerretDB/FerretDB/issues/2348
 	var val string
 
 	switch {
@@ -122,17 +119,11 @@ func NewExpressionWithOpts(expression string, opts *ExpressionOpts) (*Expression
 	}
 
 	return &Expression{
-		path:           path,
-		ExpressionOpts: opts,
+		path: path,
+		opts: *opts,
 	}, nil
 }
 
-// NewExpression creates a new instance by checking expression string.
-func NewExpression(expression string) (*Expression, error) {
-	// TODO https://github.com/FerretDB/FerretDB/issues/2348
-	return NewExpressionWithOpts(expression, new(ExpressionOpts))
-}
-
 // Evaluate gets the value at the path.
 // It returns error if the path does not exist.
 func (e *Expression) Evaluate(doc *types.Document) (any, error) {
@@ -156,10 +147,7 @@ func (e *Expression) Evaluate(doc *types.Document) (any, error) {
 		}
 	}
 
-	vals, err := commonpath.FindValues(doc, path, &commonpath.FindValuesOpts{
-		IgnoreArrayIndex:   true,
-		IgnoreArrayElement: e.IgnoreArrays,
-	})
+	vals, err := commonpath.FindValues(doc, path, &e.opts)
 	if err != nil {
 		return nil, lazyerrors.Error(err)
 	}

diff --git a/internal/handlers/common/aggregations/operators/accumulators/sum.go b/internal/handlers/common/aggregations/operators/accumulators/sum.go
@@ -49,7 +49,7 @@ func newSum(accumulation *types.Document) (Accumulator, error) {
 		accumulator.number = expr
 	case string:
 		var err error
-		if accumulator.expression, err = aggregations.NewExpression(expr); err != nil {
+		if accumulator.expression, err = aggregations.NewExpression(expr, nil); err != nil {
 			// $sum returns 0 on non-existent field.
 			accumulator.number = int32(0)
 		}

diff --git a/internal/handlers/common/aggregations/operators/type.go b/internal/handlers/common/aggregations/operators/type.go
@@ -107,7 +107,7 @@ func (t *typeOp) Process(doc *types.Document) (any, error) {
 
 		case string:
 			if strings.HasPrefix(param, "$") {
-				expression, err := aggregations.NewExpression(param)
+				expression, err := aggregations.NewExpression(param, nil)
 				if err != nil {
 					return nil, err
 				}

diff --git a/internal/handlers/common/aggregations/stages/group.go b/internal/handlers/common/aggregations/stages/group.go
@@ -174,7 +174,7 @@ func (g *group) groupDocuments(ctx context.Context, in []*types.Document) ([]gro
 		}}, nil
 	}
 
-	expression, err := aggregations.NewExpression(groupKey)
+	expression, err := aggregations.NewExpression(groupKey, nil)
 	if err != nil {
 		var exprErr *aggregations.ExpressionError
 		if !errors.As(err, &exprErr) {

diff --git a/internal/handlers/common/aggregations/stages/unwind.go b/internal/handlers/common/aggregations/stages/unwind.go
@@ -22,6 +22,7 @@ import (
 	"github.com/FerretDB/FerretDB/internal/handlers/common"
 	"github.com/FerretDB/FerretDB/internal/handlers/common/aggregations"
 	"github.com/FerretDB/FerretDB/internal/handlers/commonerrors"
+	"github.com/FerretDB/FerretDB/internal/handlers/commonpath"
 	"github.com/FerretDB/FerretDB/internal/types"
 	"github.com/FerretDB/FerretDB/internal/util/iterator"
 	"github.com/FerretDB/FerretDB/internal/util/lazyerrors"
@@ -54,10 +55,10 @@ func newUnwind(stage *types.Document) (aggregations.Stage, error) {
 			)
 		}
 
-		opts := aggregations.ExpressionOpts{
-			IgnoreArrays: true,
-		}
-		expr, err = aggregations.NewExpressionWithOpts(field, &opts)
+		expr, err = aggregations.NewExpression(field, &commonpath.FindValuesOpts{
+			FindArrayIndex: false,
+			SearchArray:    false,
+		})
 
 		if err != nil {
 			var exprErr *aggregations.ExpressionError

diff --git a/internal/handlers/common/distinct.go b/internal/handlers/common/distinct.go
@@ -108,7 +108,10 @@ func FilterDistinctValues(iter types.DocumentsIterator, key string) (*types.Arra
 		}
 
 		// vals contains all values exist at the suffix of the path
-		vals, err := commonpath.FindValues(doc, path, nil)
+		vals, err := commonpath.FindValues(doc, path, &commonpath.FindValuesOpts{
+			FindArrayIndex: true,
+			SearchArray:    true,
+		})
 		if err != nil {
 			return nil, lazyerrors.Error(err)
 		}

diff --git a/internal/handlers/common/filter.go b/internal/handlers/common/filter.go
@@ -108,7 +108,10 @@ func filterDocumentPair(doc *types.Document, filterKey string, filterValue any)
 		}
 	} else {
 		filterSuffix = path.Suffix()
-		vals, err = commonpath.FindValues(doc, path, nil)
+		vals, err = commonpath.FindValues(doc, path, &commonpath.FindValuesOpts{
+			FindArrayIndex: true,
+			SearchArray:    true,
+		})
 		if err != nil {
 			return false, lazyerrors.Error(err)
 		}

diff --git a/internal/handlers/commonpath/path.go b/internal/handlers/commonpath/path.go
@@ -25,10 +25,14 @@ import (
 
 // FindValuesOpts sets options for FindValues.
 type FindValuesOpts struct {
-	// IgnoreArrayIndex does not check if an array contains an element at the array index
-	IgnoreArrayIndex bool
-	// IgnoreArrayElement does not check if an array contains documents that have the key
-	IgnoreArrayElement bool
+	// FindArrayIndex gets an element at the specified index of an array.
+	// Using path `v.0` and `v` is an array, it returns 0-th index element of the array.
+	// If `v` is not an array, FindArrayIndex has no impact.
+	FindArrayIndex bool
+	// SearchArray searches all document of an array to find documents that contains path key.
+	// Using path `v.foo` and `v` is an array, it returns all document which has key `foo`.
+	// If `v` is not an array, SearchArray has no impact.
+	SearchArray bool
 }
 
 // FindValues goes through each key of the path iteratively on doc to find values
@@ -60,7 +64,7 @@ func FindValues(doc *types.Document, path types.Path, opts *FindValuesOpts) ([]a
 
 				values = append(values, v)
 			case *types.Array:
-				if !opts.IgnoreArrayIndex {
+				if opts.FindArrayIndex {
 					if index, err := strconv.Atoi(key); err == nil {
 						// key is an integer, check if that integer is an index of the array
 						v, err := input.Get(index)
@@ -74,7 +78,7 @@ func FindValues(doc *types.Document, path types.Path, opts *FindValuesOpts) ([]a
 					}
 				}
 
-				if opts.IgnoreArrayElement {
+				if !opts.SearchArray {
 					continue
 				}
 

diff --git a/internal/handlers/commonpath/path_test.go b/internal/handlers/commonpath/path_test.go
@@ -61,35 +61,38 @@ func TestGetPathValue(t *testing.T) {
 		"ArrayIndexDoc": {
 			doc:  arrayDocOne,
 			path: types.NewStaticPath("foo", "0", "bar"),
+			opts: &FindValuesOpts{FindArrayIndex: true},
 			res:  []any{1},
 		},
+		"ArrayIndexDocFindArrayIndexFalse": {
+			doc:  arrayDocOne,
+			path: types.NewStaticPath("foo", "0", "bar"),
+			opts: &FindValuesOpts{FindArrayIndex: false},
+			res:  []any{},
+		},
 		"ArrayIndexScalar": {
 			doc:  arrayScalarThree,
 			path: types.NewStaticPath("foo", "1"),
+			opts: &FindValuesOpts{FindArrayIndex: true},
 			res:  []any{1},
 		},
 		"ArrayDocument": {
 			doc:  arrayDocOne,
 			path: types.NewStaticPath("foo", "bar"),
+			opts: &FindValuesOpts{SearchArray: true},
 			res:  []any{1},
 		},
-		"ArrayDocumentTwo": {
-			doc:  arrayDocTwo,
-			path: types.NewStaticPath("foo", "bar"),
-			opts: &FindValuesOpts{IgnoreArrayIndex: true},
-			res:  []any{1, 2},
-		},
-		"IgnoreArrayIndex": {
+		"ArrayDocumentSearchArrayFalse": {
 			doc:  arrayDocOne,
-			path: types.NewStaticPath("foo", "0", "bar"),
-			opts: &FindValuesOpts{IgnoreArrayIndex: true},
+			path: types.NewStaticPath("foo", "bar"),
+			opts: &FindValuesOpts{SearchArray: false},
 			res:  []any{},
 		},
-		"IgnoreArrayElement": {
-			doc:  arrayDocOne,
+		"ArrayDocumentTwo": {
+			doc:  arrayDocTwo,
 			path: types.NewStaticPath("foo", "bar"),
-			opts: &FindValuesOpts{IgnoreArrayElement: true},
-			res:  []any{},
+			opts: &FindValuesOpts{SearchArray: true},
+			res:  []any{1, 2},
 		},
 	} {
 		name, tc := name, tc