Merge pull request #3602 from aduffeck/fix/search-trashed-path-collision

fix(search): keep trashed and live folders at the same path apart

services/search/pkg/bleve/batch.go

@@ -92,26 +92,53 @@ func (b *Batch) setDeleted(id string, deleted bool) error {
 
 func (b *Batch) Purge(id string, onlyDeleted bool) error {
 	return b.withSizeLimit(func() error {
-		return b.forSelfAndDescendants(id, func(resource *search.Resource) error {
-			if onlyDeleted && !resource.Deleted {
-				return nil
-			}
+		purge := func(resource *search.Resource) error {
 			b.batch.Delete(resource.ID)
 			return nil
-		})
+		}
+		if !onlyDeleted {
+			return b.forSelfAndDescendants(id, purge)
+		}
+
+		root, err := searchResourceByID(id, b.index)
+		if err != nil {
+			return err
+		}
+		// the Path term matches the root too: a trashed root is purged, a live one kept
+		return forEachMatch(trashedTreeQuery(root), b.index, b.pushing(purge))
 	})
 }
 
-// fn sees the root first; the root's original path drives the descendant lookup
+// fn sees the root first; the root's original path and trash state drive the
+// descendant lookup. A trashed folder keeps its path, so a live folder can take
+// the same one: the descendants are the ones in the root's trash state.
 func (b *Batch) forSelfAndDescendants(id string, fn func(*search.Resource) error) error {
 	root, err := searchResourceByID(id, b.index)
 	if err != nil {
 		return err
 	}
-	rootID, rootPath := root.RootID, root.Path
+	// built before fn changes the root's path or trash state
+	q := treeQuery(root)
 	isContainer := root.Type == uint64(storageProvider.ResourceType_RESOURCE_TYPE_CONTAINER)
 
-	apply := func(resource *search.Resource) error {
+	apply := b.pushing(fn)
+	if err := apply(root); err != nil {
+		return err
+	}
+	if !isContainer {
+		return nil
+	}
+	return forEachMatch(q, b.index, func(resource *search.Resource) error {
+		if resource.ID == id {
+			return nil
+		}
+		return apply(resource)
+	})
+}
+
+// pushing wraps fn to push the batch whenever it is full
+func (b *Batch) pushing(fn func(*search.Resource) error) func(*search.Resource) error {
+	return func(resource *search.Resource) error {
 		if err := fn(resource); err != nil {
 			return err
 		}
@@ -120,19 +147,6 @@ func (b *Batch) forSelfAndDescendants(id string, fn func(*search.Resource) error
 		}
 		return nil
 	}
-
-	if err := apply(root); err != nil {
-		return err
-	}
-	if !isContainer {
-		return nil
-	}
-	return forEachResourceByPath(rootID, rootPath, b.index, func(resource *search.Resource) error {
-		if resource.ID == id {
-			return nil
-		}
-		return apply(resource)
-	})
 }
 
 func (b *Batch) Push() error {

services/search/pkg/bleve/descendants_test.go

@@ -23,7 +23,7 @@ func indexResources(idx bleve.Index, resources ...search.Resource) {
 	Expect(idx.Batch(batch)).To(Succeed())
 }
 
-var _ = Describe("forEachResourceByPath", func() {
+var _ = Describe("forEachMatch", func() {
 	var idx bleve.Index
 
 	BeforeEach(func() {

services/search/pkg/bleve/index.go

@@ -15,6 +15,7 @@ import (
 	"github.com/blevesearch/bleve/v2/analysis/token/lowercase"
 	"github.com/blevesearch/bleve/v2/analysis/tokenizer/unicode"
 	"github.com/blevesearch/bleve/v2/mapping"
+	"github.com/blevesearch/bleve/v2/search/query"
 
 	"github.com/opencloud-eu/opencloud/pkg/log"
 	"github.com/opencloud-eu/opencloud/services/search/pkg/bleve/hierarchy"
@@ -265,17 +266,31 @@ func searchResourceByID(id string, index bleve.Index) (*search.Resource, error)
 // memory (about 7 MB per 5k hits) for fewer rescans
 var descendantPageSize = 20_000
 
-// forEachResourceByPath streams the folder at lookupPath and its descendants
-// (the folder term matches both, see PathAnalyzer); paged by id so memory is
-// bounded by the page and fn may write to the index between pages
-func forEachResourceByPath(rootID string, lookupPath string, index bleve.Index, fn func(*search.Resource) error) error {
-	rootQuery := bleve.NewTermQuery(rootID)
-	rootQuery.SetField("RootID")
-	pathQuery := bleve.NewTermQuery(lookupPath)
-	pathQuery.SetField("Path")
+// treeQuery matches root and its descendants that share root's trash state: a
+// trashed folder keeps its path, so a live folder can take the same one
+func treeQuery(root *search.Resource) query.Query {
+	return bleve.NewConjunctionQuery(
+		&query.TermQuery{FieldVal: "RootID", Term: root.RootID},
+		&query.TermQuery{FieldVal: "Path", Term: root.Path},
+		&query.BoolFieldQuery{FieldVal: "Deleted", Bool: root.Deleted},
+	)
+}
 
+// trashedTreeQuery matches the trashed resources at and below root's path, even
+// if root itself is live
+func trashedTreeQuery(root *search.Resource) query.Query {
+	return bleve.NewConjunctionQuery(
+		&query.TermQuery{FieldVal: "RootID", Term: root.RootID},
+		&query.TermQuery{FieldVal: "Path", Term: root.Path},
+		&query.BoolFieldQuery{FieldVal: "Deleted", Bool: true},
+	)
+}
+
+// forEachMatch streams the resources matching q; paged by id so memory is
+// bounded by the page and fn may write to the index between pages
+func forEachMatch(q query.Query, index bleve.Index, fn func(*search.Resource) error) error {
 	pageSize := descendantPageSize
-	bleveReq := bleve.NewSearchRequest(bleve.NewConjunctionQuery(rootQuery, pathQuery))
+	bleveReq := bleve.NewSearchRequest(q)
 	bleveReq.Size = pageSize
 	bleveReq.Fields = []string{"*"}
 	bleveReq.SortBy([]string{"_id"})

services/search/pkg/opensearch/batch.go

@@ -168,14 +168,14 @@ func (b *Batch) Purge(id string, onlyDeleted bool) error {
 			return fmt.Errorf("failed to get resource: %w", err)
 		}
 
-		// scope to the resource's space: the same path exists in other spaces
+		// scope to the resource's space: the same path exists in other spaces;
+		// and to its trash state: a trashed folder keeps its path, so a live
+		// folder can take the same one (onlyDeleted takes the trashed ones only)
 		query := osu.NewBoolQuery().Must(
 			osu.NewTermQuery[string]("RootID").Value(resource.RootID),
 			osu.NewTermQuery[string]("Path").Value(resource.Path),
+			osu.NewTermQuery[bool]("Deleted").Value(resource.Deleted || onlyDeleted),
 		)
-		if onlyDeleted {
-			query.Must(osu.NewTermQuery[bool]("Deleted").Value(true))
-		}
 
 		req, err := osu.BuildDocumentDeleteByQueryReq(
 			opensearchgoAPI.DocumentDeleteByQueryReq{

services/search/pkg/opensearch/opensearch.go

@@ -57,9 +57,12 @@ func updateSelfAndDescendants(ctx context.Context, client *opensearchgoAPI.Clien
 				Refresh:           conversions.ToPointer(true),
 			},
 		},
+		// a trashed folder keeps its path, so a live folder can take the same one:
+		// the descendants are the ones in the resource's trash state
 		osu.NewBoolQuery().
 			Must(osu.NewTermQuery[string]("RootID").Value(resource.RootID)).
-			Must(osu.NewTermQuery[string]("Path").Value(resource.Path)),
+			Must(osu.NewTermQuery[string]("Path").Value(resource.Path)).
+			Must(osu.NewTermQuery[bool]("Deleted").Value(resource.Deleted)),
 		osu.UpdateByQueryBodyParams{
 			Script: scriptProvider(resource),
 		},

services/search/pkg/parity/README.md

@@ -488,6 +488,10 @@ Fixtures:
 
 - `parent`, ID = 1$1!2, folder
 - `child.pdf`, ID = 1$1!3, Path = ./parent/child.pdf
+- `a`, ID = 1$1!trashed, folder, deleted
+- `x.txt`, ID = 1$1!trashed-x, Path = ./a/x.txt, deleted
+- `a`, ID = 1$1!live, folder
+- `y.txt`, ID = 1$1!live-y, Path = ./a/y.txt
 
 | Case | Query | expected | bleve | OpenSearch | same? |
 |---|---|---|---|---|---|
@@ -499,6 +503,8 @@ Fixtures:
 | PURGE-02 | removes the tree, then `DocCount()` | 0 | 0 | 0 | ✅ |
 | PURGE-03 | takes only the deleted ones when it is told to, then `name:"*parent*"` | parent | parent | parent | ✅ |
 | PURGE-03 | takes only the deleted ones when it is told to, then `name:"*child*"` | no match | no match | no match | ✅ |
+| PURGE-04 | leaves the live folder that took the trashed one's path alone, then `path:"./a"` | a, y.txt | a, y.txt | a, y.txt | ✅ |
+| PURGE-04 | leaves the live folder that took the trashed one's path alone, then `DocCount()` | 2 | 2 | 2 | ✅ |
 
 ### purgespace
 
@@ -535,6 +541,10 @@ Fixtures:
 - `x.txt`, ID = 1$1!6, Path = ./big2/x.txt
 - `odd name (1)`, ID = 1$1!7, folder
 - `f:x+y.txt`, ID = 1$1!8, Path = ./odd name (1)/f:x+y.txt
+- `a`, ID = 1$1!trashed, folder, deleted
+- `x.txt`, ID = 1$1!trashed-x, Path = ./a/x.txt, deleted
+- `a`, ID = 1$1!live, folder
+- `y.txt`, ID = 1$1!live-y, Path = ./a/y.txt
 
 | Case | Query | expected | bleve | OpenSearch | same? |
 |---|---|---|---|---|---|
@@ -546,6 +556,8 @@ Fixtures:
 | MOVE-03 | leaves a sibling folder that shares the prefix alone, then `path:"./big2"` | x.txt | x.txt | x.txt | ✅ |
 | MOVE-04 | carries the descendants of a path with special characters, then `path:"./odd name (2)"` | f:x+y.txt, odd name (2) | f:x+y.txt, odd name (2) | f:x+y.txt, odd name (2) | ✅ |
 | MOVE-04 | carries the descendants of a path with special characters, then `path:"./odd name (1)"` | no match | no match | no match | ✅ |
+| MOVE-05 | leaves a trashed folder that shares the path where it is, then `path:"./a"` | a, x.txt | a, x.txt | a, x.txt | ✅ |
+| MOVE-05 | leaves a trashed folder that shares the path where it is, then `path:"./b"` | b, y.txt | b, y.txt | b, y.txt | ✅ |
 
 ### rootscope
 

services/search/pkg/parity/fixtures_test.go

@@ -101,6 +101,20 @@ func fixtureTree() (parent, child search.Resource) {
 	return parent, child
 }
 
+// fixtureSamePath is a trashed folder and the live folder that took its path,
+// with a file in each.
+func fixtureSamePath() (trashed, live search.Resource, all []search.Resource) {
+	trashed = fixtureFolder("a", withID("1$1!trashed"), isDeleted())
+	live = fixtureFolder("a", withID("1$1!live"))
+
+	return trashed, live, []search.Resource{
+		trashed,
+		fixtureDoc("x.txt", withID("1$1!trashed-x"), withParent(trashed.ID), withPath("./a/x.txt"), isDeleted()),
+		live,
+		fixtureDoc("y.txt", withID("1$1!live-y"), withParent(live.ID), withPath("./a/y.txt")),
+	}
+}
+
 func treeIsLeft(names ...string) []expectation {
 	left := func(name string) []string {
 		if slices.Contains(names, name) {

services/search/pkg/parity/lifecycle_move_test.go

@@ -11,6 +11,7 @@ func moveLifecycle() lifecycleGroup {
 	inSibling := fixtureDoc("x.txt", withID("1$1!6"), withParent("1$1!big2"), withPath("./big2/x.txt"))
 	odd := fixtureFolder("odd name (1)", withID("1$1!7"))
 	inOdd := fixtureDoc("f:x+y.txt", withID("1$1!8"), withParent(odd.ID), withPath("./odd name (1)/f:x+y.txt"))
+	trashed, live, samePath := fixtureSamePath()
 
 	return lifecycleGroup{
 		name:     "move",
@@ -56,6 +57,22 @@ func moveLifecycle() lifecycleGroup {
 					{`path:"./odd name (1)"`, nil},
 				},
 			},
+			{
+				id: 5, title: "leaves a trashed folder that shares the path where it is",
+				fixtures: samePath,
+				do: func(e search.Engine) error {
+					if err := e.Move(live.ID, live.ParentID, "./b"); err != nil {
+						return err
+					}
+
+					// results only show the trashed folder once it is back
+					return e.Restore(trashed.ID)
+				},
+				expect: []expectation{
+					{`path:"./a"`, []string{"a", "x.txt"}},
+					{`path:"./b"`, []string{"b", "y.txt"}},
+				},
+			},
 		},
 	}
 }

services/search/pkg/parity/lifecycle_purge_test.go

@@ -7,6 +7,7 @@ import (
 
 func purgeLifecycle() lifecycleGroup {
 	parent, child := fixtureTree()
+	trashed, _, samePath := fixtureSamePath()
 
 	return lifecycleGroup{
 		name:     "purge",
@@ -35,6 +36,13 @@ func purgeLifecycle() lifecycleGroup {
 				},
 				expect: treeIsLeft("parent"),
 			},
+			{
+				id: 4, title: "leaves the live folder that took the trashed one's path alone",
+				fixtures:     samePath,
+				do:           func(e search.Engine) error { return e.Purge(trashed.ID, false) },
+				expect:       []expectation{{`path:"./a"`, []string{"a", "y.txt"}}},
+				wantDocCount: conversions.ToPointer(uint64(2)),
+			},
 		},
 	}
 }