From a064eacfa953bc75c2ddbc143b0c5a8c48b08246 Mon Sep 17 00:00:00 2001 From: Carsten Rietz Date: Tue, 10 Dec 2024 09:13:42 +0100 Subject: [PATCH 1/7] docstore/memdocstore: #3508 allow nested slices query --- docstore/memdocstore/codec.go | 2 +- docstore/memdocstore/mem.go | 48 +++++++++++++++++++++----------- docstore/memdocstore/mem_test.go | 44 +++++++++++++++++++++++++++++ docstore/memdocstore/query.go | 23 +++++++++++---- 4 files changed, 95 insertions(+), 22 deletions(-) diff --git a/docstore/memdocstore/codec.go b/docstore/memdocstore/codec.go index 68c4d1db85..c2c5baca59 100644 --- a/docstore/memdocstore/codec.go +++ b/docstore/memdocstore/codec.go @@ -104,7 +104,7 @@ func decodeDoc(m storedDoc, ddoc driver.Document, fps [][]string) error { // (We don't need the key field because ddoc must already have it.) m2 = map[string]interface{}{} for _, fp := range fps { - val, err := getAtFieldPath(m, fp) + val, err := getAtFieldPath(m, fp, false) if err != nil { if gcerrors.Code(err) == gcerrors.NotFound { continue diff --git a/docstore/memdocstore/mem.go b/docstore/memdocstore/mem.go index c4ecf83178..aaf8134ce3 100644 --- a/docstore/memdocstore/mem.go +++ b/docstore/memdocstore/mem.go @@ -68,6 +68,12 @@ type Options struct { // When the collection is closed, its contents are saved to the file. Filename string + // AllowNestedSlicesQuery allows querying with nested slices. + // This makes the memdocstore more compatible with MongoDB, + // but other providers may not support this feature. + // See https://github.com/google/go-cloud/pull/3511 for more details. + AllowNestedSlicesQuery bool + // Call this function when the collection is closed. // For internal use only. onClose func() @@ -399,16 +405,34 @@ func (c *collection) checkRevision(arg driver.Document, current storedDoc) error // getAtFieldPath gets the value of m at fp. It returns an error if fp is invalid // (see getParentMap). -func getAtFieldPath(m map[string]interface{}, fp []string) (interface{}, error) { - m2, err := getParentMap(m, fp, false) - if err != nil { - return nil, err +func getAtFieldPath(m map[string]interface{}, fp []string, nested bool) (result interface{}, err error) { + + var get func(m interface{}, name string) interface{} + get = func(m interface{}, name string) interface{} { + switch concrete := m.(type) { + case map[string]interface{}: + return concrete[name] + case []interface{}: + if !nested { + return nil + } + result := []interface{}{} + for _, e := range concrete { + result = append(result, get(e, name)) + } + return result + } + return nil } - v, ok := m2[fp[len(fp)-1]] - if ok { - return v, nil + result = m + for _, k := range fp { + next := get(result, k) + if next == nil { + return nil, gcerr.Newf(gcerr.NotFound, nil, "field %s not found", strings.Join(fp, ".")) + } + result = next } - return nil, gcerr.Newf(gcerr.NotFound, nil, "field %s not found", fp) + return result, nil } // setAtFieldPath sets m's value at fp to val. It creates intermediate maps as @@ -422,14 +446,6 @@ func setAtFieldPath(m map[string]interface{}, fp []string, val interface{}) erro return nil } -// Delete the value from m at the given field path, if it exists. -func deleteAtFieldPath(m map[string]interface{}, fp []string) { - m2, _ := getParentMap(m, fp, false) // ignore error - if m2 != nil { - delete(m2, fp[len(fp)-1]) - } -} - // getParentMap returns the map that directly contains the given field path; // that is, the value of m at the field path that excludes the last component // of fp. If a non-map is encountered along the way, an InvalidArgument error is diff --git a/docstore/memdocstore/mem_test.go b/docstore/memdocstore/mem_test.go index 9a843202d6..29796ccf98 100644 --- a/docstore/memdocstore/mem_test.go +++ b/docstore/memdocstore/mem_test.go @@ -16,6 +16,7 @@ package memdocstore import ( "context" + "io" "os" "path/filepath" "testing" @@ -131,6 +132,49 @@ func TestUpdateAtomic(t *testing.T) { } } +func TestQueryNested(t *testing.T) { + ctx := context.Background() + + count := func(iter *docstore.DocumentIterator) (c int) { + doc := docmap{} + for { + if err := iter.Next(ctx, doc); err != nil { + if err == io.EOF { + break + } + t.Fatal(err) + } + c++ + } + return c + } + + dc, err := newCollection(drivertest.KeyField, nil, &Options{AllowNestedSlicesQuery: true}) + if err != nil { + t.Fatal(err) + } + coll := docstore.NewCollection(dc) + defer coll.Close() + + doc := docmap{drivertest.KeyField: "TestQueryNested", + "list": []any{docmap{"a": "A"}}, + "map": docmap{"b": "B"}, + dc.RevisionField(): nil, + } + if err := coll.Put(ctx, doc); err != nil { + t.Fatal(err) + } + + got := count(coll.Query().Where("list.a", "=", "A").Get(ctx)) + if got != 1 { + t.Errorf("got %v docs when filtering by list.a, want 1", got) + } + got = count(coll.Query().Where("map.b", "=", "B").Get(ctx)) + if got != 1 { + t.Errorf("got %v docs when filtering by map.b, want 1", got) + } +} + func TestSortDocs(t *testing.T) { newDocs := func() []storedDoc { return []storedDoc{ diff --git a/docstore/memdocstore/query.go b/docstore/memdocstore/query.go index 419017b993..9a660ffe50 100644 --- a/docstore/memdocstore/query.go +++ b/docstore/memdocstore/query.go @@ -37,7 +37,7 @@ func (c *collection) RunGetQuery(_ context.Context, q *driver.Query) (driver.Doc var resultDocs []storedDoc for _, doc := range c.docs { - if filtersMatch(q.Filters, doc) { + if filtersMatch(q.Filters, doc, c.opts.AllowNestedSlicesQuery) { resultDocs = append(resultDocs, doc) } } @@ -74,17 +74,17 @@ func (c *collection) RunGetQuery(_ context.Context, q *driver.Query) (driver.Doc }, nil } -func filtersMatch(fs []driver.Filter, doc storedDoc) bool { +func filtersMatch(fs []driver.Filter, doc storedDoc, nested bool) bool { for _, f := range fs { - if !filterMatches(f, doc) { + if !filterMatches(f, doc, nested) { return false } } return true } -func filterMatches(f driver.Filter, doc storedDoc) bool { - docval, err := getAtFieldPath(doc, f.FieldPath) +func filterMatches(f driver.Filter, doc storedDoc, nested bool) bool { + docval, err := getAtFieldPath(doc, f.FieldPath, nested) // missing or bad field path => no match if err != nil { return false @@ -138,6 +138,19 @@ func compare(x1, x2 interface{}) (int, bool) { } return -1, true } + if v1.Kind() == reflect.Slice { + for i := 0; i < v1.Len(); i++ { + if c, ok := compare(x2, v1.Index(i).Interface()); ok { + if !ok { + return 0, false + } + if c == 0 { + return 0, true + } + } + } + return -1, true + } if v1.Kind() == reflect.String && v2.Kind() == reflect.String { return strings.Compare(v1.String(), v2.String()), true } From 500e50e52fd9ffab53bbb9ea1404eb1837b2d74a Mon Sep 17 00:00:00 2001 From: Carsten Rietz Date: Thu, 12 Dec 2024 15:23:21 +0100 Subject: [PATCH 2/7] docstore/memdocstore: #3508 first review renamed option to AllowNestedSliceQueries --- docstore/memdocstore/mem.go | 17 ++++++++--------- docstore/memdocstore/mem_test.go | 25 ++++++++++++++++++++++++- docstore/memdocstore/query.go | 5 +++-- 3 files changed, 35 insertions(+), 12 deletions(-) diff --git a/docstore/memdocstore/mem.go b/docstore/memdocstore/mem.go index aaf8134ce3..54387b45a0 100644 --- a/docstore/memdocstore/mem.go +++ b/docstore/memdocstore/mem.go @@ -68,11 +68,11 @@ type Options struct { // When the collection is closed, its contents are saved to the file. Filename string - // AllowNestedSlicesQuery allows querying with nested slices. + // AllowNestedSliceQueries allows querying with nested slices. // This makes the memdocstore more compatible with MongoDB, // but other providers may not support this feature. // See https://github.com/google/go-cloud/pull/3511 for more details. - AllowNestedSlicesQuery bool + AllowNestedSliceQueries bool // Call this function when the collection is closed. // For internal use only. @@ -405,18 +405,17 @@ func (c *collection) checkRevision(arg driver.Document, current storedDoc) error // getAtFieldPath gets the value of m at fp. It returns an error if fp is invalid // (see getParentMap). -func getAtFieldPath(m map[string]interface{}, fp []string, nested bool) (result interface{}, err error) { - - var get func(m interface{}, name string) interface{} - get = func(m interface{}, name string) interface{} { +func getAtFieldPath(m map[string]any, fp []string, nested bool) (result any, err error) { + var get func(m any, name string) any + get = func(m any, name string) any { switch concrete := m.(type) { - case map[string]interface{}: + case map[string]any: return concrete[name] - case []interface{}: + case []any: if !nested { return nil } - result := []interface{}{} + var result []any for _, e := range concrete { result = append(result, get(e, name)) } diff --git a/docstore/memdocstore/mem_test.go b/docstore/memdocstore/mem_test.go index 29796ccf98..d1d36da496 100644 --- a/docstore/memdocstore/mem_test.go +++ b/docstore/memdocstore/mem_test.go @@ -149,7 +149,7 @@ func TestQueryNested(t *testing.T) { return c } - dc, err := newCollection(drivertest.KeyField, nil, &Options{AllowNestedSlicesQuery: true}) + dc, err := newCollection(drivertest.KeyField, nil, &Options{AllowNestedSliceQueries: true}) if err != nil { t.Fatal(err) } @@ -159,6 +159,9 @@ func TestQueryNested(t *testing.T) { doc := docmap{drivertest.KeyField: "TestQueryNested", "list": []any{docmap{"a": "A"}}, "map": docmap{"b": "B"}, + "listOfMaps": []any{docmap{"id": "1"}, docmap{"id": "2"}, docmap{"id": "3"}}, + "mapOfLists": docmap{"ids": []any{"1", "2", "3"}}, + "deep": []any{docmap{"nesting": []any{docmap{"of": docmap{"elements": "yes"}}}}}, dc.RevisionField(): nil, } if err := coll.Put(ctx, doc); err != nil { @@ -169,10 +172,30 @@ func TestQueryNested(t *testing.T) { if got != 1 { t.Errorf("got %v docs when filtering by list.a, want 1", got) } + got = count(coll.Query().Where("list.a", "=", "missing").Get(ctx)) + if got != 0 { + t.Errorf("got %v docs when filtering by list.a, want 0", got) + } got = count(coll.Query().Where("map.b", "=", "B").Get(ctx)) if got != 1 { t.Errorf("got %v docs when filtering by map.b, want 1", got) } + got = count(coll.Query().Where("listOfMaps.id", "=", "1").Get(ctx)) + if got != 1 { + t.Errorf("got %v docs when filtering by listOfMaps.id, want 1", got) + } + got = count(coll.Query().Where("listOfMaps.id", "=", "123").Get(ctx)) + if got != 0 { + t.Errorf("got %v docs when filtering by listOfMaps.id, want 0", got) + } + got = count(coll.Query().Where("mapOfLists.ids", "=", "1").Get(ctx)) + if got != 1 { + t.Errorf("got %v docs when filtering by listOfMaps.1, want 1", got) + } + got = count(coll.Query().Where("deep.nesting.of.elements", "=", "yes").Get(ctx)) + if got != 1 { + t.Errorf("got %v docs when filtering by deep.nesting.of.elements, want 1", got) + } } func TestSortDocs(t *testing.T) { diff --git a/docstore/memdocstore/query.go b/docstore/memdocstore/query.go index 9a660ffe50..ee0a08f8a4 100644 --- a/docstore/memdocstore/query.go +++ b/docstore/memdocstore/query.go @@ -37,7 +37,7 @@ func (c *collection) RunGetQuery(_ context.Context, q *driver.Query) (driver.Doc var resultDocs []storedDoc for _, doc := range c.docs { - if filtersMatch(q.Filters, doc, c.opts.AllowNestedSlicesQuery) { + if filtersMatch(q.Filters, doc, c.opts.AllowNestedSliceQueries) { resultDocs = append(resultDocs, doc) } } @@ -138,6 +138,8 @@ func compare(x1, x2 interface{}) (int, bool) { } return -1, true } + // for AllowNestedSliceQueries + // when querying for x2 in the document and x1 is a list of values we only need one value to match if v1.Kind() == reflect.Slice { for i := 0; i < v1.Len(); i++ { if c, ok := compare(x2, v1.Index(i).Interface()); ok { @@ -149,7 +151,6 @@ func compare(x1, x2 interface{}) (int, bool) { } } } - return -1, true } if v1.Kind() == reflect.String && v2.Kind() == reflect.String { return strings.Compare(v1.String(), v2.String()), true From a046f08fda66369946846f18939db97a08165319 Mon Sep 17 00:00:00 2001 From: Carsten Rietz Date: Fri, 6 Jun 2025 07:51:00 +0200 Subject: [PATCH 3/7] docstore/memdocstore: #3508 fixed >= and <= operators not working when comparing in nested structures --- docstore/memdocstore/mem_test.go | 72 +++++++++++++++++++++++++++++--- docstore/memdocstore/query.go | 30 +++++++++---- 2 files changed, 89 insertions(+), 13 deletions(-) diff --git a/docstore/memdocstore/mem_test.go b/docstore/memdocstore/mem_test.go index d1d36da496..58d5f60b0b 100644 --- a/docstore/memdocstore/mem_test.go +++ b/docstore/memdocstore/mem_test.go @@ -162,6 +162,7 @@ func TestQueryNested(t *testing.T) { "listOfMaps": []any{docmap{"id": "1"}, docmap{"id": "2"}, docmap{"id": "3"}}, "mapOfLists": docmap{"ids": []any{"1", "2", "3"}}, "deep": []any{docmap{"nesting": []any{docmap{"of": docmap{"elements": "yes"}}}}}, + "listOfLists": []any{docmap{"items": []any{docmap{"price": 10}}}}, dc.RevisionField(): nil, } if err := coll.Put(ctx, doc); err != nil { @@ -180,14 +181,10 @@ func TestQueryNested(t *testing.T) { if got != 1 { t.Errorf("got %v docs when filtering by map.b, want 1", got) } - got = count(coll.Query().Where("listOfMaps.id", "=", "1").Get(ctx)) + got = count(coll.Query().Where("listOfMaps.id", "=", "2").Get(ctx)) if got != 1 { t.Errorf("got %v docs when filtering by listOfMaps.id, want 1", got) } - got = count(coll.Query().Where("listOfMaps.id", "=", "123").Get(ctx)) - if got != 0 { - t.Errorf("got %v docs when filtering by listOfMaps.id, want 0", got) - } got = count(coll.Query().Where("mapOfLists.ids", "=", "1").Get(ctx)) if got != 1 { t.Errorf("got %v docs when filtering by listOfMaps.1, want 1", got) @@ -196,6 +193,71 @@ func TestQueryNested(t *testing.T) { if got != 1 { t.Errorf("got %v docs when filtering by deep.nesting.of.elements, want 1", got) } + got = count(coll.Query().Where("listOfLists.items.price", "=", 10).Get(ctx)) + if got != 1 { + t.Errorf("got %v docs when filtering by listOfLists.items.price, want 1", got) + } + got = count(coll.Query().Where("listOfLists.items.price", "<=", 20).Get(ctx)) + if got != 1 { + t.Errorf("got %v docs when filtering by listOfLists.items.price, want 1", got) + } + + // GreaterOrEqual + // this is not extracted as the setup and count functions also would be needed to be duplicated + doc = docmap{drivertest.KeyField: "CheapItems", + "items": []any{docmap{"price": 10}, docmap{"price": 1}}, + dc.RevisionField(): nil, + } + if err := coll.Put(ctx, doc); err != nil { + t.Fatal(err) + } + doc = docmap{drivertest.KeyField: "ExpensivItems", + "items": []any{docmap{"price": 50}, docmap{"price": 100}}, + dc.RevisionField(): nil, + } + if err := coll.Put(ctx, doc); err != nil { + t.Fatal(err) + } + + got = count(coll.Query().Where("items.price", "=", 1).Get(ctx)) + if got != 1 { + t.Errorf("got %v docs when filtering by items.price = 12, want 1", got) + } + got = count(coll.Query().Where("items.price", "=", 5).Get(ctx)) + if got != 0 { + t.Errorf("got %v docs when filtering by items.price = 5, want 0", got) + } + + got = count(coll.Query().Where("items.price", ">=", 1).Get(ctx)) + if got != 2 { + t.Errorf("got %v docs when filtering by items.price >= 1, want 2", got) + } + got = count(coll.Query().Where("items.price", ">=", 5).Get(ctx)) + if got != 2 { + t.Errorf("got %v docs when filtering by items.price >= 5, want 2", got) + } + got = count(coll.Query().Where("items.price", ">=", 10).Get(ctx)) + if got != 2 { + t.Errorf("got %v docs when filtering by items.price >= 10, want 2", got) + } + + got = count(coll.Query().Where("items.price", "<=", 50).Get(ctx)) + if got != 2 { + t.Errorf("got %v docs when filtering by items.price <= 50, want 2", got) + } + + doc = docmap{drivertest.KeyField: "CheapItems", + "items": []any{docmap{"price": 10}, docmap{"price": 1}}, + dc.RevisionField(): nil, + } + if err := coll.Put(ctx, doc); err != nil { + t.Fatal(err) + } + + got = count(coll.Query().Where("items.price", "<=", 50).Get(ctx)) + if got != 2 { + t.Errorf("got %v docs when filtering by items.price <= 50, want 2", got) + } } func TestSortDocs(t *testing.T) { diff --git a/docstore/memdocstore/query.go b/docstore/memdocstore/query.go index ee0a08f8a4..860f7141d7 100644 --- a/docstore/memdocstore/query.go +++ b/docstore/memdocstore/query.go @@ -89,7 +89,7 @@ func filterMatches(f driver.Filter, doc storedDoc, nested bool) bool { if err != nil { return false } - c, ok := compare(docval, f.Value) + c, ok := compare(docval, f.Value, f.Op) if !ok { return false } @@ -120,37 +120,51 @@ func applyComparison(op string, c int) bool { } } -func compare(x1, x2 interface{}) (int, bool) { +func compare(x1, x2 any, op string) (int, bool) { v1 := reflect.ValueOf(x1) v2 := reflect.ValueOf(x2) - // this is for in/not-in queries. + // for in/not-in queries. otherwise this should only be reached with AllowNestedSliceQueries set // return 0 if x1 is in slice x2, -1 if not. if v2.Kind() == reflect.Slice { - for i := 0; i < v2.Len(); i++ { - if c, ok := compare(x1, v2.Index(i).Interface()); ok { + for i := range v2.Len() { + if c, ok := compare(x1, v2.Index(i).Interface(), op); ok { if !ok { return 0, false } if c == 0 { return 0, true } + if op != "in" && op != "not-in" { + return c, true + } } } return -1, true } // for AllowNestedSliceQueries // when querying for x2 in the document and x1 is a list of values we only need one value to match + // the comparison value depends on the operator if v1.Kind() == reflect.Slice { - for i := 0; i < v1.Len(); i++ { - if c, ok := compare(x2, v1.Index(i).Interface()); ok { + v2Greater := false + v2Less := false + for i := range v1.Len() { + if c, ok := compare(x2, v1.Index(i).Interface(), op); ok { if !ok { return 0, false } if c == 0 { return 0, true } + v2Greater = v2Greater || c > 0 + v2Less = v2Less || c < 0 } } + if op[0] == '>' && v2Less { + return 1, true + } else if op[0] == '<' && v2Greater { + return -1, true + } + return 0, false } if v1.Kind() == reflect.String && v2.Kind() == reflect.String { return strings.Compare(v1.String(), v2.String()), true @@ -174,7 +188,7 @@ func compare(x1, x2 interface{}) (int, bool) { func sortDocs(docs []storedDoc, field string, asc bool) { sort.Slice(docs, func(i, j int) bool { - c, ok := compare(docs[i][field], docs[j][field]) + c, ok := compare(docs[i][field], docs[j][field], ">") if !ok { return false } From 586fbea383c2721c15c943df4d0e0d944bcb5828 Mon Sep 17 00:00:00 2001 From: Carsten Rietz Date: Fri, 6 Jun 2025 08:55:55 +0200 Subject: [PATCH 4/7] docstore/memdocstore: #3508 allow to enable AllowNestedSliceQueries per url parameter --- docstore/memdocstore/urls.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docstore/memdocstore/urls.go b/docstore/memdocstore/urls.go index 84d3258aeb..5f99a00157 100644 --- a/docstore/memdocstore/urls.go +++ b/docstore/memdocstore/urls.go @@ -65,8 +65,9 @@ func (o *URLOpener) OpenCollectionURL(ctx context.Context, u *url.URL) (*docstor } options := &Options{ - RevisionField: q.Get("revision_field"), - Filename: q.Get("filename"), + RevisionField: q.Get("revision_field"), + Filename: q.Get("filename"), + AllowNestedSliceQueries: q.Get("allow_nested_slice_queries") == "true", onClose: func() { o.mu.Lock() delete(o.collections, collName) @@ -75,6 +76,7 @@ func (o *URLOpener) OpenCollectionURL(ctx context.Context, u *url.URL) (*docstor } q.Del("revision_field") q.Del("filename") + q.Del("allow_nested_slice_queries") for param := range q { return nil, fmt.Errorf("open collection %v: invalid query parameter %q", u, param) } From 6178743e84ad784fa283cc5a9d236bb47ca713d4 Mon Sep 17 00:00:00 2001 From: Carsten Rietz Date: Fri, 6 Jun 2025 14:47:24 +0200 Subject: [PATCH 5/7] docstore/memdocstore: #3508 fixed slices in slices are now found with operators --- docstore/memdocstore/mem.go | 10 +++++++++- docstore/memdocstore/mem_test.go | 10 +++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/docstore/memdocstore/mem.go b/docstore/memdocstore/mem.go index 54387b45a0..95ff247b00 100644 --- a/docstore/memdocstore/mem.go +++ b/docstore/memdocstore/mem.go @@ -417,7 +417,15 @@ func getAtFieldPath(m map[string]any, fp []string, nested bool) (result any, err } var result []any for _, e := range concrete { - result = append(result, get(e, name)) + next := get(e, name) + // if we have slices within slices the compare function does not see the nested slices + // changing the compare function to be recursive also is more effort than flattening the slices here + sliced, ok := next.([]any) + if ok { + result = append(result, sliced...) + } else { + result = append(result, next) + } } return result } diff --git a/docstore/memdocstore/mem_test.go b/docstore/memdocstore/mem_test.go index 58d5f60b0b..291e2124a1 100644 --- a/docstore/memdocstore/mem_test.go +++ b/docstore/memdocstore/mem_test.go @@ -162,7 +162,7 @@ func TestQueryNested(t *testing.T) { "listOfMaps": []any{docmap{"id": "1"}, docmap{"id": "2"}, docmap{"id": "3"}}, "mapOfLists": docmap{"ids": []any{"1", "2", "3"}}, "deep": []any{docmap{"nesting": []any{docmap{"of": docmap{"elements": "yes"}}}}}, - "listOfLists": []any{docmap{"items": []any{docmap{"price": 10}}}}, + "listOfLists": []any{docmap{"items": []any{docmap{"price": 10}, docmap{"price": 20}}}}, dc.RevisionField(): nil, } if err := coll.Put(ctx, doc); err != nil { @@ -201,6 +201,14 @@ func TestQueryNested(t *testing.T) { if got != 1 { t.Errorf("got %v docs when filtering by listOfLists.items.price, want 1", got) } + got = count(coll.Query().Where("listOfLists.items.price", "=", 10).Get(ctx)) + if got != 1 { + t.Errorf("got %v docs when filtering by listOfLists.items.price = 10, want 1", got) + } + got = count(coll.Query().Where("listOfLists.items.price", "=", 20).Get(ctx)) + if got != 1 { + t.Errorf("got %v docs when filtering by listOfLists.items.price = 20, want 1", got) + } // GreaterOrEqual // this is not extracted as the setup and count functions also would be needed to be duplicated From be1bdae3f94be9a8f0e23a6bd9ea19f06188f4d1 Mon Sep 17 00:00:00 2001 From: Carsten Rietz Date: Tue, 1 Jul 2025 11:57:10 +0200 Subject: [PATCH 6/7] docstore/memdocstore: #3508 second review --- docstore/memdocstore/mem.go | 8 +- docstore/memdocstore/mem_test.go | 237 +++++++++++++++---------------- docstore/memdocstore/query.go | 8 +- 3 files changed, 124 insertions(+), 129 deletions(-) diff --git a/docstore/memdocstore/mem.go b/docstore/memdocstore/mem.go index 95ff247b00..8e88e64b0c 100644 --- a/docstore/memdocstore/mem.go +++ b/docstore/memdocstore/mem.go @@ -68,10 +68,11 @@ type Options struct { // When the collection is closed, its contents are saved to the file. Filename string - // AllowNestedSliceQueries allows querying with nested slices. + // AllowNestedSliceQueries allows querying into nested slices. + // If true queries for a field path which points to a slice will return + // true if any element of the slice has a value that validates with the operator. // This makes the memdocstore more compatible with MongoDB, // but other providers may not support this feature. - // See https://github.com/google/go-cloud/pull/3511 for more details. AllowNestedSliceQueries bool // Call this function when the collection is closed. @@ -404,6 +405,7 @@ func (c *collection) checkRevision(arg driver.Document, current storedDoc) error } // getAtFieldPath gets the value of m at fp. It returns an error if fp is invalid +// if nested is true compare against all elements of a slice, see AllowNestedSliceQueries // (see getParentMap). func getAtFieldPath(m map[string]any, fp []string, nested bool) (result any, err error) { var get func(m any, name string) any @@ -419,7 +421,7 @@ func getAtFieldPath(m map[string]any, fp []string, nested bool) (result any, err for _, e := range concrete { next := get(e, name) // if we have slices within slices the compare function does not see the nested slices - // changing the compare function to be recursive also is more effort than flattening the slices here + // changing the compare function to be recursive would be more effort than flattening the slices here sliced, ok := next.([]any) if ok { result = append(result, sliced...) diff --git a/docstore/memdocstore/mem_test.go b/docstore/memdocstore/mem_test.go index 291e2124a1..3d8d617c94 100644 --- a/docstore/memdocstore/mem_test.go +++ b/docstore/memdocstore/mem_test.go @@ -135,20 +135,6 @@ func TestUpdateAtomic(t *testing.T) { func TestQueryNested(t *testing.T) { ctx := context.Background() - count := func(iter *docstore.DocumentIterator) (c int) { - doc := docmap{} - for { - if err := iter.Next(ctx, doc); err != nil { - if err == io.EOF { - break - } - t.Fatal(err) - } - c++ - } - return c - } - dc, err := newCollection(drivertest.KeyField, nil, &Options{AllowNestedSliceQueries: true}) if err != nil { t.Fatal(err) @@ -156,115 +142,128 @@ func TestQueryNested(t *testing.T) { coll := docstore.NewCollection(dc) defer coll.Close() - doc := docmap{drivertest.KeyField: "TestQueryNested", - "list": []any{docmap{"a": "A"}}, - "map": docmap{"b": "B"}, - "listOfMaps": []any{docmap{"id": "1"}, docmap{"id": "2"}, docmap{"id": "3"}}, - "mapOfLists": docmap{"ids": []any{"1", "2", "3"}}, - "deep": []any{docmap{"nesting": []any{docmap{"of": docmap{"elements": "yes"}}}}}, - "listOfLists": []any{docmap{"items": []any{docmap{"price": 10}, docmap{"price": 20}}}}, - dc.RevisionField(): nil, - } - if err := coll.Put(ctx, doc); err != nil { - t.Fatal(err) - } - - got := count(coll.Query().Where("list.a", "=", "A").Get(ctx)) - if got != 1 { - t.Errorf("got %v docs when filtering by list.a, want 1", got) - } - got = count(coll.Query().Where("list.a", "=", "missing").Get(ctx)) - if got != 0 { - t.Errorf("got %v docs when filtering by list.a, want 0", got) - } - got = count(coll.Query().Where("map.b", "=", "B").Get(ctx)) - if got != 1 { - t.Errorf("got %v docs when filtering by map.b, want 1", got) - } - got = count(coll.Query().Where("listOfMaps.id", "=", "2").Get(ctx)) - if got != 1 { - t.Errorf("got %v docs when filtering by listOfMaps.id, want 1", got) - } - got = count(coll.Query().Where("mapOfLists.ids", "=", "1").Get(ctx)) - if got != 1 { - t.Errorf("got %v docs when filtering by listOfMaps.1, want 1", got) - } - got = count(coll.Query().Where("deep.nesting.of.elements", "=", "yes").Get(ctx)) - if got != 1 { - t.Errorf("got %v docs when filtering by deep.nesting.of.elements, want 1", got) - } - got = count(coll.Query().Where("listOfLists.items.price", "=", 10).Get(ctx)) - if got != 1 { - t.Errorf("got %v docs when filtering by listOfLists.items.price, want 1", got) - } - got = count(coll.Query().Where("listOfLists.items.price", "<=", 20).Get(ctx)) - if got != 1 { - t.Errorf("got %v docs when filtering by listOfLists.items.price, want 1", got) - } - got = count(coll.Query().Where("listOfLists.items.price", "=", 10).Get(ctx)) - if got != 1 { - t.Errorf("got %v docs when filtering by listOfLists.items.price = 10, want 1", got) - } - got = count(coll.Query().Where("listOfLists.items.price", "=", 20).Get(ctx)) - if got != 1 { - t.Errorf("got %v docs when filtering by listOfLists.items.price = 20, want 1", got) - } - - // GreaterOrEqual - // this is not extracted as the setup and count functions also would be needed to be duplicated - doc = docmap{drivertest.KeyField: "CheapItems", - "items": []any{docmap{"price": 10}, docmap{"price": 1}}, - dc.RevisionField(): nil, - } - if err := coll.Put(ctx, doc); err != nil { - t.Fatal(err) - } - doc = docmap{drivertest.KeyField: "ExpensivItems", - "items": []any{docmap{"price": 50}, docmap{"price": 100}}, - dc.RevisionField(): nil, - } - if err := coll.Put(ctx, doc); err != nil { - t.Fatal(err) - } - - got = count(coll.Query().Where("items.price", "=", 1).Get(ctx)) - if got != 1 { - t.Errorf("got %v docs when filtering by items.price = 12, want 1", got) - } - got = count(coll.Query().Where("items.price", "=", 5).Get(ctx)) - if got != 0 { - t.Errorf("got %v docs when filtering by items.price = 5, want 0", got) - } - - got = count(coll.Query().Where("items.price", ">=", 1).Get(ctx)) - if got != 2 { - t.Errorf("got %v docs when filtering by items.price >= 1, want 2", got) - } - got = count(coll.Query().Where("items.price", ">=", 5).Get(ctx)) - if got != 2 { - t.Errorf("got %v docs when filtering by items.price >= 5, want 2", got) - } - got = count(coll.Query().Where("items.price", ">=", 10).Get(ctx)) - if got != 2 { - t.Errorf("got %v docs when filtering by items.price >= 10, want 2", got) + // Set up test documents + testDocs := []docmap{{ + drivertest.KeyField: "TestQueryNested", + "list": []any{docmap{"a": "A"}}, + "map": docmap{"b": "B"}, + "listOfMaps": []any{docmap{"id": "1"}, docmap{"id": "2"}, docmap{"id": "3"}}, + "mapOfLists": docmap{"ids": []any{"1", "2", "3"}}, + "deep": []any{docmap{"nesting": []any{docmap{"of": docmap{"elements": "yes"}}}}}, + "listOfLists": []any{docmap{"items": []any{docmap{"price": 10}, docmap{"price": 20}}}}, + dc.RevisionField(): nil, + }, { + drivertest.KeyField: "CheapItems", + "items": []any{docmap{"price": 10}, docmap{"price": 1}}, + dc.RevisionField(): nil, + }, { + drivertest.KeyField: "ExpensiveItems", + "items": []any{docmap{"price": 50}, docmap{"price": 100}}, + dc.RevisionField(): nil, + }} + + for _, testDoc := range testDocs { + err = coll.Put(ctx, testDoc) + if err != nil { + t.Fatal(err) + } } - got = count(coll.Query().Where("items.price", "<=", 50).Get(ctx)) - if got != 2 { - t.Errorf("got %v docs when filtering by items.price <= 50, want 2", got) - } + tests := []struct { + name string + where []any + wantKeys []string + }{ + { + name: "list field match", + where: []any{"list.a", "=", "A"}, + wantKeys: []string{"TestQueryNested"}, + }, { + name: "list field no match", + where: []any{"list.a", "=", "missing"}, + }, { + name: "map field match", + where: []any{"map.b", "=", "B"}, + wantKeys: []string{"TestQueryNested"}, + }, { + name: "list of maps field match", + where: []any{"listOfMaps.id", "=", "2"}, + wantKeys: []string{"TestQueryNested"}, + }, { + name: "map of lists field match", + where: []any{"mapOfLists.ids", "=", "1"}, + wantKeys: []string{"TestQueryNested"}, + }, { + name: "deep nested field match", + where: []any{"deep.nesting.of.elements", "=", "yes"}, + wantKeys: []string{"TestQueryNested"}, + }, { + name: "list of lists exact price 10", + where: []any{"listOfLists.items.price", "=", 10}, + wantKeys: []string{"TestQueryNested"}, + }, { + name: "list of lists exact price 20", + where: []any{"listOfLists.items.price", "=", 20}, + wantKeys: []string{"TestQueryNested"}, + }, { + name: "list of lists price less than or equal to 20", + where: []any{"listOfLists.items.price", "<=", 20}, + wantKeys: []string{"TestQueryNested"}, + }, { + name: "items price equals 1", + where: []any{"items.price", "=", 1}, + wantKeys: []string{"CheapItems"}, + }, { + name: "items price equals 5 (no match)", + where: []any{"items.price", "=", 5}, + }, { + name: "items price greater than or equal to 1", + where: []any{"items.price", ">=", 1}, + wantKeys: []string{"CheapItems", "ExpensiveItems"}, + }, { + name: "items price greater than or equal to 5", + where: []any{"items.price", ">=", 5}, + wantKeys: []string{"CheapItems", "ExpensiveItems"}, + }, { + name: "items price greater than or equal to 10", + where: []any{"items.price", ">=", 10}, + wantKeys: []string{"CheapItems", "ExpensiveItems"}, + }, { + name: "items price less than or equal to 50", + where: []any{"items.price", "<=", 50}, + wantKeys: []string{"CheapItems", "ExpensiveItems"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + iter := coll.Query().Where(docstore.FieldPath(tc.where[0].(string)), tc.where[1].(string), tc.where[2]).Get(ctx) + var got []docmap + for { + doc := docmap{} + err := iter.Next(ctx, doc) + if err != nil { + if err == io.EOF { + break + } + t.Fatal(err) + } + got = append(got, doc) + } - doc = docmap{drivertest.KeyField: "CheapItems", - "items": []any{docmap{"price": 10}, docmap{"price": 1}}, - dc.RevisionField(): nil, - } - if err := coll.Put(ctx, doc); err != nil { - t.Fatal(err) - } + // Extract keys from results + var gotKeys []string + for _, d := range got { + if key, ok := d[drivertest.KeyField].(string); ok { + gotKeys = append(gotKeys, key) + } + } - got = count(coll.Query().Where("items.price", "<=", 50).Get(ctx)) - if got != 2 { - t.Errorf("got %v docs when filtering by items.price <= 50, want 2", got) + diff := cmp.Diff(gotKeys, tc.wantKeys) + if diff != "" { + t.Errorf("query results mismatch (-got +want):\n%s", diff) + } + }) } } diff --git a/docstore/memdocstore/query.go b/docstore/memdocstore/query.go index 860f7141d7..551ab010d6 100644 --- a/docstore/memdocstore/query.go +++ b/docstore/memdocstore/query.go @@ -128,9 +128,6 @@ func compare(x1, x2 any, op string) (int, bool) { if v2.Kind() == reflect.Slice { for i := range v2.Len() { if c, ok := compare(x1, v2.Index(i).Interface(), op); ok { - if !ok { - return 0, false - } if c == 0 { return 0, true } @@ -141,7 +138,7 @@ func compare(x1, x2 any, op string) (int, bool) { } return -1, true } - // for AllowNestedSliceQueries + // See Options.AllowNestedSliceQueries // when querying for x2 in the document and x1 is a list of values we only need one value to match // the comparison value depends on the operator if v1.Kind() == reflect.Slice { @@ -149,9 +146,6 @@ func compare(x1, x2 any, op string) (int, bool) { v2Less := false for i := range v1.Len() { if c, ok := compare(x2, v1.Index(i).Interface(), op); ok { - if !ok { - return 0, false - } if c == 0 { return 0, true } From cd9090afec683e00ae9f378bc6159e92a83c11b1 Mon Sep 17 00:00:00 2001 From: Carsten Rietz Date: Fri, 4 Jul 2025 08:13:10 +0200 Subject: [PATCH 7/7] docstore/memdocstore: #3508 third review fixed comment style --- docstore/memdocstore/mem.go | 14 +++++++------- docstore/memdocstore/mem_test.go | 2 ++ docstore/memdocstore/query.go | 8 ++++---- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/docstore/memdocstore/mem.go b/docstore/memdocstore/mem.go index 8e88e64b0c..0077e76fcc 100644 --- a/docstore/memdocstore/mem.go +++ b/docstore/memdocstore/mem.go @@ -404,24 +404,24 @@ func (c *collection) checkRevision(arg driver.Document, current storedDoc) error return nil } -// getAtFieldPath gets the value of m at fp. It returns an error if fp is invalid -// if nested is true compare against all elements of a slice, see AllowNestedSliceQueries +// getAtFieldPath gets the value of m at fp. It returns an error if fp is invalid. +// If nested is true compare against all elements of a slice, see AllowNestedSliceQueries // (see getParentMap). func getAtFieldPath(m map[string]any, fp []string, nested bool) (result any, err error) { var get func(m any, name string) any get = func(m any, name string) any { - switch concrete := m.(type) { + switch m := m.(type) { case map[string]any: - return concrete[name] + return m[name] case []any: if !nested { return nil } var result []any - for _, e := range concrete { + for _, e := range m { next := get(e, name) - // if we have slices within slices the compare function does not see the nested slices - // changing the compare function to be recursive would be more effort than flattening the slices here + // If we have slices within slices the compare function does not see the nested slices. + // Changing the compare function to be recursive would be more effort than flattening the slices here. sliced, ok := next.([]any) if ok { result = append(result, sliced...) diff --git a/docstore/memdocstore/mem_test.go b/docstore/memdocstore/mem_test.go index 3d8d617c94..c32e627e24 100644 --- a/docstore/memdocstore/mem_test.go +++ b/docstore/memdocstore/mem_test.go @@ -19,6 +19,7 @@ import ( "io" "os" "path/filepath" + "slices" "testing" "github.com/google/go-cmp/cmp" @@ -258,6 +259,7 @@ func TestQueryNested(t *testing.T) { gotKeys = append(gotKeys, key) } } + slices.Sort(gotKeys) diff := cmp.Diff(gotKeys, tc.wantKeys) if diff != "" { diff --git a/docstore/memdocstore/query.go b/docstore/memdocstore/query.go index 551ab010d6..24830e6ed2 100644 --- a/docstore/memdocstore/query.go +++ b/docstore/memdocstore/query.go @@ -123,8 +123,8 @@ func applyComparison(op string, c int) bool { func compare(x1, x2 any, op string) (int, bool) { v1 := reflect.ValueOf(x1) v2 := reflect.ValueOf(x2) - // for in/not-in queries. otherwise this should only be reached with AllowNestedSliceQueries set - // return 0 if x1 is in slice x2, -1 if not. + // For in/not-in queries. Otherwise this should only be reached with AllowNestedSliceQueries set. + // Return 0 if x1 is in slice x2, -1 if not. if v2.Kind() == reflect.Slice { for i := range v2.Len() { if c, ok := compare(x1, v2.Index(i).Interface(), op); ok { @@ -139,8 +139,8 @@ func compare(x1, x2 any, op string) (int, bool) { return -1, true } // See Options.AllowNestedSliceQueries - // when querying for x2 in the document and x1 is a list of values we only need one value to match - // the comparison value depends on the operator + // When querying for x2 in the document and x1 is a list of values we only need one value to match + // the comparison value depends on the operator. if v1.Kind() == reflect.Slice { v2Greater := false v2Less := false