diff --git a/services/search/pkg/bleve/batch.go b/services/search/pkg/bleve/batch.go index 62bf4301f0..41510485a4 100644 --- a/services/search/pkg/bleve/batch.go +++ b/services/search/pkg/bleve/batch.go @@ -56,7 +56,7 @@ func (b *Batch) Move(id, parentID, location string) error { return b.withSizeLimit(func() error { nextPath := utils.MakeRelativePath(location) var currentPath string - return b.forSelfAndDescendants(id, func(resource *search.Resource) error { + return b.forSelfAndDescendants(id, false, func(resource *search.Resource) error { if resource.ID == id { currentPath = resource.Path resource.Path = nextPath @@ -84,7 +84,7 @@ func (b *Batch) Restore(id string) error { } func (b *Batch) setDeleted(id string, deleted bool) error { - return b.forSelfAndDescendants(id, func(resource *search.Resource) error { + return b.forSelfAndDescendants(id, false, func(resource *search.Resource) error { resource.Deleted = deleted return b.indexResource(resource.ID, *resource) }) @@ -92,23 +92,23 @@ 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 - } + return b.forSelfAndDescendants(id, onlyDeleted, func(resource *search.Resource) error { b.batch.Delete(resource.ID) return nil }) }) } -// fn sees the root first; the root's original path drives the descendant lookup -func (b *Batch) forSelfAndDescendants(id string, fn func(*search.Resource) error) error { +// 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, or the +// trashed ones with onlyDeleted, which skips a live root as well. +func (b *Batch) forSelfAndDescendants(id string, onlyDeleted bool, fn func(*search.Resource) error) error { root, err := searchResourceByID(id, b.index) if err != nil { return err } - rootID, rootPath := root.RootID, root.Path + rootID, rootPath, deleted := root.RootID, root.Path, root.Deleted || onlyDeleted isContainer := root.Type == uint64(storageProvider.ResourceType_RESOURCE_TYPE_CONTAINER) apply := func(resource *search.Resource) error { @@ -121,13 +121,15 @@ func (b *Batch) forSelfAndDescendants(id string, fn func(*search.Resource) error return nil } - if err := apply(root); err != nil { - return err + if root.Deleted || !onlyDeleted { + if err := apply(root); err != nil { + return err + } } if !isContainer { return nil } - return forEachResourceByPath(rootID, rootPath, b.index, func(resource *search.Resource) error { + return forEachResourceByPath(rootID, rootPath, deleted, b.index, func(resource *search.Resource) error { if resource.ID == id { return nil } diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index 8182beeaef..3bf0b2c900 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -266,16 +266,19 @@ func searchResourceByID(id string, index bleve.Index) (*search.Resource, error) 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 { +// (the folder term matches both, see PathAnalyzer) whose Deleted flag equals +// deleted; 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, deleted bool, index bleve.Index, fn func(*search.Resource) error) error { rootQuery := bleve.NewTermQuery(rootID) rootQuery.SetField("RootID") pathQuery := bleve.NewTermQuery(lookupPath) pathQuery.SetField("Path") + deletedQuery := bleve.NewBoolFieldQuery(deleted) + deletedQuery.SetField("Deleted") pageSize := descendantPageSize - bleveReq := bleve.NewSearchRequest(bleve.NewConjunctionQuery(rootQuery, pathQuery)) + bleveReq := bleve.NewSearchRequest(bleve.NewConjunctionQuery(rootQuery, pathQuery, deletedQuery)) bleveReq.Size = pageSize bleveReq.Fields = []string{"*"} bleveReq.SortBy([]string{"_id"}) diff --git a/services/search/pkg/opensearch/batch.go b/services/search/pkg/opensearch/batch.go index 5df34d78b7..9d53bce1bb 100644 --- a/services/search/pkg/opensearch/batch.go +++ b/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{ diff --git a/services/search/pkg/opensearch/opensearch.go b/services/search/pkg/opensearch/opensearch.go index ca8ec7841d..01e34d3302 100644 --- a/services/search/pkg/opensearch/opensearch.go +++ b/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), }, diff --git a/services/search/pkg/parity/README.md b/services/search/pkg/parity/README.md index 6558e96847..186a48badb 100644 --- a/services/search/pkg/parity/README.md +++ b/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 diff --git a/services/search/pkg/parity/fixtures_test.go b/services/search/pkg/parity/fixtures_test.go index 330fddc3dd..7f1569ebbe 100644 --- a/services/search/pkg/parity/fixtures_test.go +++ b/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) { diff --git a/services/search/pkg/parity/lifecycle_move_test.go b/services/search/pkg/parity/lifecycle_move_test.go index 3a27d91b12..ba44634e14 100644 --- a/services/search/pkg/parity/lifecycle_move_test.go +++ b/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"}}, + }, + }, }, } } diff --git a/services/search/pkg/parity/lifecycle_purge_test.go b/services/search/pkg/parity/lifecycle_purge_test.go index 841d02c53d..bc7e096260 100644 --- a/services/search/pkg/parity/lifecycle_purge_test.go +++ b/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)), + }, }, } }