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

[aduffeck]

A trashed folder keeps its path in the index, so a live folder can take the same path. Move/delete/restore/purge looked up descendants by path only. As a result, purging the trashed ./a removed the live ./a tree, and moving the live ./a took the trashed tree with it. Both engines now also match the root's trash state.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage 74.42% diff coverage · +0.01% coverage variation

Metric	Results
Coverage variation	✅ +0.01% coverage variation (-1.00%)
Diff coverage	✅ 74.42% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (74462d3)	89037	21582	24.24%
Head commit (b1efb8c)	89056 (+19)	21599 (+17)	24.25% (+0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3602)	43	32	74.42%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[dschmidt]

Instead of adding the deleted / onlyDeleted bools to forEachResourceByPath and forSelfAndDescendants, should the caller pass the query into forEachResourceByPath? It would then only do the paging, and each caller adds the Deleted filter it needs.

Roughly, with three small new helpers:

• treeQuery(root): the RootID + Path conjunction that forEachResourceByPath builds inline today
• deletedQuery(deleted): the Deleted bool field query
• b.pushing(fn): the current apply closure from forSelfAndDescendants, pulled out so Purge can use it too

// forSelfAndDescendants(id, fn): back to the signature from main
q := treeQuery(root)
q.AddQuery(deletedQuery(root.Deleted))
forEachResourceByPath(q, b.index, fn)

// Purge(id, true): doesn't go through forSelfAndDescendants
q := treeQuery(root)
q.AddQuery(deletedQuery(true)) // also leaves out the live root, since the Path term matches the folder itself
forEachResourceByPath(q, b.index, b.pushing(purge))

In forSelfAndDescendants the query has to be built before fn(root) runs, just like rootPath today.

Personally I really dislike boolean parameters as I think it's really hard to reason about what they are doing

[dschmidt]

great, looks much better to me now 👍

[aduffeck]

@dschmidt 👍 I hear you about the bool parameter. I wasn't comfortable with the idea of a treeQuery which spans the live tree AND the trash unless amended with another query though, so my treeQuery now consideres the root deleted state and then there's a trashedTreeQuery which only considers the trash instead.

thanks for the review!

[dschmidt]

Sounds very good, thanks for taking my review into account 🙂

[v-scharf]

I think we need port it to stable because this case especially bad for opensearch

steps:

• alan creates foo/alan-file.txt
• mary creates foo/mary-file.txt
• mary deletes foo and purge it in trashbin
• alan search by foo by alan-file

Actual: 7.2.4 - search result is empty. dev (main branch) - alan can see folder and file