From ad4c8ad11238c01bca3fd81b7cce642322fcef10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Fri, 25 Sep 2026 15:31:31 +0200 Subject: [PATCH] fix(search): keep trashed and live folders at the same path apart A trashed folder keeps its path in the index, so a new folder can take the same one. The descendant lookup behind move, delete, restore and purge matched on space and path only: purging the trashed folder took the live folder and its descendants out of the index, and moving the live folder carried the trashed tree along. Both engines now also match the root's trash state; purge with onlyDeleted keeps matching the trashed ones only. --- services/search/pkg/bleve/batch.go | 26 ++++++++++--------- services/search/pkg/bleve/index.go | 11 +++++--- services/search/pkg/opensearch/batch.go | 8 +++--- services/search/pkg/opensearch/opensearch.go | 5 +++- services/search/pkg/parity/README.md | 12 +++++++++ services/search/pkg/parity/fixtures_test.go | 14 ++++++++++ .../search/pkg/parity/lifecycle_move_test.go | 17 ++++++++++++ .../search/pkg/parity/lifecycle_purge_test.go | 8 ++++++ 8 files changed, 80 insertions(+), 21 deletions(-) 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)), + }, }, } }