more comment

FerretDB · chilagrow · Aug 4, 2023 · Jul 20, 2023 · Jul 20, 2023 · Jul 20, 2023
commit 2885c85a0ec5d2e2d77ca6fb1b8bbaff46fd3868
@@ -79,8 +79,8 @@ type Expression struct {
 func NewExpression(expression string, opts *commonpath.FindValuesOpts) (*Expression, error) {
 	if opts == nil {
 		opts = &commonpath.FindValuesOpts{
-			FindArrayIndex:  false,
-			FindArrayValues: true,
+			FindArrayIndex:     false,
+			FindArrayDocuments: true,
 		}
 	}
 

@@ -56,8 +56,8 @@ func newUnwind(stage *types.Document) (aggregations.Stage, error) {
 		}
 
 		expr, err = aggregations.NewExpression(field, &commonpath.FindValuesOpts{
-			FindArrayIndex:  false,
-			FindArrayValues: false,
+			FindArrayIndex:     false,
+			FindArrayDocuments: false,
 		})
 
 		if err != nil {

@@ -109,8 +109,8 @@ 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, &commonpath.FindValuesOpts{
-			FindArrayIndex:  true,
-			FindArrayValues: true,
+			FindArrayIndex:     true,
+			FindArrayDocuments: true,
 		})
 		if err != nil {
 			return nil, lazyerrors.Error(err)

@@ -109,8 +109,8 @@ func filterDocumentPair(doc *types.Document, filterKey string, filterValue any)
 	} else {
 		filterSuffix = path.Suffix()
 		vals, err = commonpath.FindValues(doc, path, &commonpath.FindValuesOpts{
-			FindArrayIndex:  true,
-			FindArrayValues: true,
+			FindArrayIndex:     true,
+			FindArrayDocuments: true,
 		})
 		if err != nil {
 			return false, lazyerrors.Error(err)

@@ -26,28 +26,31 @@ import (
 
 // FindValuesOpts sets options for FindValues.
 type FindValuesOpts struct {
-	// If FindArrayValues is true, it iterates the array to find documents containing the key.
-	// If FindArrayValues is false, it does not find any value from the array.
+	// If FindArrayDocuments is true, it iterates the array to find documents that have path.
+	// If FindArrayDocuments is false, it does not find documents from the array.
 	// Using path `v.foo` and `v` is an array:
-	// 	- with FindArrayValues true, it returns documents' values of key `foo`;
-	//  - with FindArrayValues false, it returns an empty array.
-	// If `v` is not an array, FindArrayValues has no impact.
-	FindArrayValues bool
-	// If FindArrayIndex is true, it uses indexes to find a value of the array.
-	// If FindArrayIndex is false, it does use indexes on the path.
+	// 	- with FindArrayDocuments true, it finds values of `foo` of found documents;
+	//  - with FindArrayDocuments false, it returns an empty array.
+	// If `v` is not an array, FindArrayDocuments has no impact.
+	FindArrayDocuments bool
+	// If FindArrayIndex is true, it finds value at index of an array.
+	// If FindArrayIndex is false, it does not find value at index of an array.
 	// Using path `v.0` and `v` is an array:
-	//  - with FindArrayIndex true, it returns 0-th index value of the array;
+	//  - with FindArrayIndex true, it finds 0-th index value of the array;
 	//  - with FindArrayIndex false, it returns empty array.
 	// If `v` is not an array, FindArrayIndex has no impact.
 	FindArrayIndex bool
 }
 
-// FindValues iterates path elements, at each path element it adds to next values to iterate:
-//   - if it is a document and has the key;
-//   - if it is an array, FindArrayIndex is true and finds value at index;
-//   - if it is an array, FindArrayValues is true and finds values.
+// FindValues returns values found at path. Unlike document.GetByPath it may find multiple values.
 //
-// It returns values or an empty array.
+// It iterates path elements, at each path element it adds to next values to iterate:
+//   - if it is a document and has path, it adds the document field value to next values;
+//   - if it is an array, FindArrayIndex is true and finds value at index, it adds value to next values;
+//   - if it is an array, FindArrayDocuments is true and documents in the array have path,
+//     it adds field value of all documents that have path to next values.
+//
+// It returns next values after iterating path elements.
 func FindValues(doc *types.Document, path types.Path, opts *FindValuesOpts) ([]any, error) {
 	if opts == nil {
 		opts = new(FindValuesOpts)
@@ -71,15 +74,14 @@ func FindValues(doc *types.Document, path types.Path, opts *FindValuesOpts) ([]a
 			case *types.Array:
 				if opts.FindArrayIndex {
 					res, err := findArrayIndex(next, p)
-					if err != nil {
-						return nil, lazyerrors.Error(err)
+					if err == nil {
+						values = append(values, res)
+						continue
 					}
-
-					values = append(values, res...)
 				}
 
-				if opts.FindArrayValues {
-					res, err := findArrayValues(next, p)
+				if opts.FindArrayDocuments {
+					res, err := findArrayDocuments(next, p)
 					if err != nil {
 						return nil, lazyerrors.Error(err)
 					}
@@ -98,28 +100,27 @@ func FindValues(doc *types.Document, path types.Path, opts *FindValuesOpts) ([]a
 	return nextValues, nil
 }
 
-// findArrayIndex checks if key can be used as an index of array and returns
-// the value found at that index.
-// Empty array is returned if the key cannot be used as an index
-// or key is not an existing index of array.
-func findArrayIndex(array *types.Array, key string) ([]any, error) {
-	index, err := strconv.Atoi(key)
+// findArrayIndex returns the value found at array index.
+// Error is returned if index is not a number or index does not exist in array.
+func findArrayIndex(array *types.Array, index string) (any, error) {
+	i, err := strconv.Atoi(index)
 	if err != nil {
-		return []any{}, nil
+		return nil, lazyerrors.Error(err)
 	}
 
-	// key is an integer, check if that integer is an index of the array
-	v, _ := array.Get(index)
-	if v != nil {
-		return []any{v}, nil
+	v, err := array.Get(i)
+	if err != nil {
+		return nil, lazyerrors.Error(err)
 	}
 
-	return []any{}, nil
+	return v, nil
 }
 
-// findArrayValues finds document fields containing the key and returns the document field value.
-// Empty array is returned if no document containing the key was found.
-func findArrayValues(array *types.Array, key string) ([]any, error) {
+// findArrayDocuments iterates array to find documents that have documentKey.
+// It returns field values of found documents at documentKey.
+// Multiple documents with documentKey may exist in array, hence an array is returned.
+// Empty array is returned if no document was found.
+func findArrayDocuments(array *types.Array, documentKey string) ([]any, error) {
 	iter := array.Iterator()
 	defer iter.Close()
 
@@ -140,8 +141,7 @@ func findArrayValues(array *types.Array, key string) ([]any, error) {
 			continue
 		}
 
-		v, _ = doc.Get(key)
-		if v != nil {
+		if v, _ = doc.Get(documentKey); v != nil {
 			res = append(res, v)
 		}
 	}

@@ -42,107 +42,107 @@ func TestGetPathValue(t *testing.T) {
 				doc:  array,
 				path: types.NewStaticPath("foo", "1", "bar"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  true,
-					FindArrayValues: true,
+					FindArrayIndex:     true,
+					FindArrayDocuments: true,
 				},
 				res: []any{1},
 			},
 			"PathNested2": {
 				doc:  array,
 				path: types.NewStaticPath("foo", "1", "bar"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  true,
-					FindArrayValues: false,
+					FindArrayIndex:     true,
+					FindArrayDocuments: false,
 				},
 				res: []any{1},
 			},
 			"PathNested3": {
 				doc:  array,
 				path: types.NewStaticPath("foo", "1", "bar"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  false,
-					FindArrayValues: true,
+					FindArrayIndex:     false,
+					FindArrayDocuments: true,
 				},
 				res: []any{},
 			},
 			"PathNested4": {
 				doc:  array,
 				path: types.NewStaticPath("foo", "1", "bar"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  false,
-					FindArrayValues: false,
+					FindArrayIndex:     false,
+					FindArrayDocuments: false,
 				},
 				res: []any{},
 			},
 			"PathIndexDotNotation1": {
 				doc:  array,
 				path: types.NewStaticPath("foo", "1"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  true,
-					FindArrayValues: true,
+					FindArrayIndex:     true,
+					FindArrayDocuments: true,
 				},
 				res: []any{must.NotFail(types.NewDocument("bar", 1))},
 			},
 			"PathIndexDotNotation2": {
 				doc:  array,
 				path: types.NewStaticPath("foo", "1"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  true,
-					FindArrayValues: false,
+					FindArrayIndex:     true,
+					FindArrayDocuments: false,
 				},
 				res: []any{must.NotFail(types.NewDocument("bar", 1))},
 			},
 			"PathIndexDotNotation3": {
 				doc:  array,
 				path: types.NewStaticPath("foo", "1"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  false,
-					FindArrayValues: true,
+					FindArrayIndex:     false,
+					FindArrayDocuments: true,
 				},
 				res: []any{},
 			},
 			"PathIndexDotNotation4": {
 				doc:  array,
 				path: types.NewStaticPath("foo", "1"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  false,
-					FindArrayValues: false,
+					FindArrayIndex:     false,
+					FindArrayDocuments: false,
 				},
 				res: []any{},
 			},
 			"PathDotNotation1": {
 				doc:  array,
 				path: types.NewStaticPath("foo", "bar"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  true,
-					FindArrayValues: true,
+					FindArrayIndex:     true,
+					FindArrayDocuments: true,
 				},
 				res: []any{0, 1},
 			},
 			"PathDotNotation2": {
 				doc:  array,
 				path: types.NewStaticPath("foo", "bar"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  true,
-					FindArrayValues: false,
+					FindArrayIndex:     true,
+					FindArrayDocuments: false,
 				},
 				res: []any{},
 			},
 			"PathDotNotation3": {
 				doc:  array,
 				path: types.NewStaticPath("foo", "bar"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  false,
-					FindArrayValues: true,
+					FindArrayIndex:     false,
+					FindArrayDocuments: true,
 				},
 				res: []any{0, 1},
 			},
 			"PathDotNotation4": {
 				doc:  array,
 				path: types.NewStaticPath("foo", "bar"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  false,
-					FindArrayValues: false,
+					FindArrayIndex:     false,
+					FindArrayDocuments: false,
 				},
 				res: []any{},
 			},
@@ -176,35 +176,35 @@ func TestGetPathValue(t *testing.T) {
 				doc:  doc,
 				path: types.NewStaticPath("foo", "bar"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  true,
-					FindArrayValues: true,
+					FindArrayIndex:     true,
+					FindArrayDocuments: true,
 				},
 				res: []any{0},
 			},
 			"PathDotNotation2": {
 				doc:  doc,
 				path: types.NewStaticPath("foo", "bar"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  true,
-					FindArrayValues: false,
+					FindArrayIndex:     true,
+					FindArrayDocuments: false,
 				},
 				res: []any{0},
 			},
 			"PathDotNotation3": {
 				doc:  doc,
 				path: types.NewStaticPath("foo", "bar"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  false,
-					FindArrayValues: true,
+					FindArrayIndex:     false,
+					FindArrayDocuments: true,
 				},
 				res: []any{0},
 			},
 			"PathDotNotation4": {
 				doc:  doc,
 				path: types.NewStaticPath("foo", "bar"),
 				opts: &FindValuesOpts{
-					FindArrayIndex:  false,
-					FindArrayValues: false,
+					FindArrayIndex:     false,
+					FindArrayDocuments: false,
 				},
 				res: []any{0},
 			},