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 1/2] 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)), + }, }, } } From b1efb8c6160e7e0f416c53f867d78101b1a5a36a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Mon, 28 Sep 2026 08:28:52 +0200 Subject: [PATCH 2/2] Pass the query into the bleve descendant walk Instead of deleted/onlyDeleted bool parameters, the caller now builds the query and forEachMatch only does the paging. --- services/search/pkg/bleve/batch.go | 60 +++++++++++-------- services/search/pkg/bleve/descendants_test.go | 2 +- services/search/pkg/bleve/index.go | 36 +++++++---- 3 files changed, 61 insertions(+), 37 deletions(-) diff --git a/services/search/pkg/bleve/batch.go b/services/search/pkg/bleve/batch.go index 41510485a4..022e75d86d 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, false, func(resource *search.Resource) error { + return b.forSelfAndDescendants(id, 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, false, func(resource *search.Resource) error { + return b.forSelfAndDescendants(id, func(resource *search.Resource) error { resource.Deleted = deleted return b.indexResource(resource.ID, *resource) }) @@ -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, onlyDeleted, func(resource *search.Resource) error { + 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 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 { +// 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, deleted := root.RootID, root.Path, root.Deleted || onlyDeleted + // 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,21 +147,6 @@ func (b *Batch) forSelfAndDescendants(id string, onlyDeleted bool, fn func(*sear } return nil } - - if root.Deleted || !onlyDeleted { - if err := apply(root); err != nil { - return err - } - } - if !isContainer { - return nil - } - return forEachResourceByPath(rootID, rootPath, deleted, b.index, func(resource *search.Resource) error { - if resource.ID == id { - return nil - } - return apply(resource) - }) } func (b *Batch) Push() error { diff --git a/services/search/pkg/bleve/descendants_test.go b/services/search/pkg/bleve/descendants_test.go index 8247e273f4..c317160b72 100644 --- a/services/search/pkg/bleve/descendants_test.go +++ b/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() { diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index 3bf0b2c900..7f72e03ba2 100644 --- a/services/search/pkg/bleve/index.go +++ b/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,20 +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) 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") +// 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, deletedQuery)) + bleveReq := bleve.NewSearchRequest(q) bleveReq.Size = pageSize bleveReq.Fields = []string{"*"} bleveReq.SortBy([]string{"_id"})