feat: Add filtered collection truncate - #5097
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds targeted local document purging by document ID with optional history pruning. The operation spans storage cleanup, transactions, authorization, HTTP, CLI, JavaScript, cgo, and integration tests. ChangesDocument purge
Estimated code review effort: 5 (Critical) | ~90 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested labels: Suggested reviewers: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5097 +/- ##
===========================================
+ Coverage 76.29% 76.40% +0.11%
===========================================
Files 645 647 +2
Lines 51122 51555 +433
===========================================
+ Hits 39001 39387 +386
- Misses 8840 8849 +9
- Partials 3281 3319 +38
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 17 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
813ef44 to
6f15733
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/collection_purge_docs.go`:
- Around line 30-32: Update the Long help text for the collection purge command
to clarify that --prune-history removes reachable blocks only when they are no
longer shared with other documents; shared history blocks are retained until
their final owning document is purged. Apply the same clarification to the
corresponding help text referenced by the additional location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 24fee146-880a-4bae-9697-6b7efde010d3
📒 Files selected for processing (23)
cbindings/collection_purge.gocbindings/wrapper_collection.gocli/cli.gocli/collection_purge_docs.goclient/collection.goclient/mocks/collection.godocs/website/references/http/openapi.jsonhttp/client_collection.gohttp/handler_collection.gointernal/db/collection_index.gointernal/db/collection_purge.gointernal/db/collection_purge_conflict_test.gointernal/db/collection_purge_test.gointernal/db/collection_truncate.gointernal/db/document_delete.gointernal/db/id/document.gointernal/db/id/document_test.gointernal/db/txn_collection.gojs/client_collection.gotests/action/purge_docs.gotests/clients/cli/wrapper_collection.gotests/clients/js/wrapper_collection.gotests/integration/collection/purge/simple_test.go
🚧 Files skipped from review as they are similar to previous changes (21)
- tests/clients/cli/wrapper_collection.go
- tests/clients/js/wrapper_collection.go
- cli/cli.go
- js/client_collection.go
- cbindings/collection_purge.go
- http/client_collection.go
- internal/db/collection_index.go
- tests/integration/collection/purge/simple_test.go
- internal/db/txn_collection.go
- internal/db/id/document.go
- http/handler_collection.go
- client/mocks/collection.go
- client/collection.go
- docs/website/references/http/openapi.json
- tests/action/purge_docs.go
- internal/db/collection_purge.go
- internal/db/collection_truncate.go
- internal/db/document_delete.go
- internal/db/id/document_test.go
- internal/db/collection_purge_test.go
- internal/db/collection_purge_conflict_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (52)
- GitHub Check: Check vulnerabilities job
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Start binary job
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Check mocks job
- GitHub Check: Lint GoLang job
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage JS job
- GitHub Check: Test coverage secondary index job
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Test coverage leveldb job
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage signed docs job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Test coverage telemetry job
- GitHub Check: Test coverage view job
- GitHub Check: Build dependencies job
- GitHub Check: Test coverage encryption job
- GitHub Check: Build, install, and test Debian package
- GitHub Check: Test Limited Resource job
- GitHub Check: Check http documentation job
- GitHub Check: Check data format changes job
- GitHub Check: Check wizard health job
- GitHub Check: Test NPX/JS build job
- GitHub Check: Test macos job
- GitHub Check: Build and test Linux C shared library
- GitHub Check: Check cli documentation job
- GitHub Check: Validate containerfile job
- GitHub Check: Lint GoLang job
⚠️ CI failures not shown inline (2)
GitHub Actions: Validate Title Workflow / Validate title job: feat: add targeted document purge
Conclusion: failure
##[group]Run ./tools/scripts/validate-conventional-style.sh "feat: add targeted document purge"
�[36;1m./tools/scripts/validate-conventional-style.sh "feat: add targeted document purge"�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
Info: label = [feat]
Info: description = [ add targeted document purge]
Error: First character after the label is not an uppercase alphabet.
##[error]Process completed with exit code 7.
GitHub Actions: Validate Title Workflow / 0_Validate title job.txt: feat: add targeted document purge
Conclusion: failure
##[group]Run ./tools/scripts/validate-conventional-style.sh "feat: add targeted document purge"
�[36;1m./tools/scripts/validate-conventional-style.sh "feat: add targeted document purge"�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
Info: label = [feat]
Info: description = [ add targeted document purge]
Error: First character after the label is not an uppercase alphabet.
##[error]Process completed with exit code 7.
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-03-27T08:24:16.083Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4661
File: http/middleware.go:93-93
Timestamp: 2026-03-27T08:24:16.083Z
Learning: When handling `client.ErrNotAuthorizedToPerformOperation` in the defradb repository, treat it as an unauthenticated/missing-identity condition (not insufficient permissions). Map it to HTTP 401 Unauthorized rather than 403 Forbidden; this mapping is intentional and consistent with prior behavior (pre-PR `#4661`).
Applied to files:
cli/collection_purge_docs.gocbindings/wrapper_collection.go
📚 Learning: 2026-06-18T16:47:47.339Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: When reviewing this repository’s Go code, do not flag `sync.WaitGroup.Go()` (e.g., `wg.Go(fn)`) as a compilation error. This method exists starting in Go 1.25; it should be treated as valid as long as the repo’s configured/minimum Go toolchain version is 1.25+ (e.g., per `go.mod` and/or CI). If the project is building with an older Go version, then it may be appropriate to flag it.
Applied to files:
cli/collection_purge_docs.gocbindings/wrapper_collection.go
📚 Learning: 2026-06-18T16:47:47.339Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: In sourcenetwork/defradb code reviews, do not raise findings that are likely compiler/type-check errors (e.g., code that would fail `go test`/`go build`) because CI pipelines will catch compilation failures. Only flag compilation-related problems if there is a clear, verifiable issue beyond what CI will already surface (e.g., a mismatched build tag or missing generated artifact that CI would not cover).
Applied to files:
cli/collection_purge_docs.gocbindings/wrapper_collection.go
🔇 Additional comments (2)
cli/collection_purge_docs.go (1)
37-54: LGTM!cbindings/wrapper_collection.go (1)
26-34: LGTM!Also applies to: 335-389
iverc
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/website/references/cli/defradb_client_collection_purge-docs.md`:
- Line 14: Update the fenced code blocks in defradb_client_collection_purge
documentation by adding language identifiers: use console for the command
synopsis and text for the option listings, including the additional referenced
fences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d64a2897-d6f1-4807-9411-d8fcb525013f
📒 Files selected for processing (8)
cli/collection_purge_docs.goclient/mocks/collection.godocs/website/references/cli/defradb_client_collection.mddocs/website/references/cli/defradb_client_collection_purge-docs.mdinternal/db/collection_index.gointernal/db/collection_purge.gointernal/db/collection_purge_test.gointernal/db/collection_truncate.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/db/collection_purge.go
- cli/collection_purge_docs.go
- client/mocks/collection.go
📜 Review details
⏰ Context from checks skipped due to timeout. (52)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage secondary index job
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage leveldb job
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage view job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage encryption job
- GitHub Check: Test coverage signed docs job
- GitHub Check: Test coverage telemetry job
- GitHub Check: Test coverage JS job
- GitHub Check: Check data format changes job
- GitHub Check: Test NPX/JS build job
- GitHub Check: Test Limited Resource job
- GitHub Check: Build, install, and test Debian package
- GitHub Check: Check wizard health job
- GitHub Check: Lint GoLang job
- GitHub Check: Check mocks job
- GitHub Check: Test macos job
- GitHub Check: Validate containerfile job
- GitHub Check: Check http documentation job
- GitHub Check: Check cli documentation job
- GitHub Check: Check vulnerabilities job
- GitHub Check: Build and test Linux C shared library
- GitHub Check: Start binary job
- GitHub Check: Build dependencies job
- GitHub Check: Lint GoLang job
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-03-27T08:24:16.083Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4661
File: http/middleware.go:93-93
Timestamp: 2026-03-27T08:24:16.083Z
Learning: When handling `client.ErrNotAuthorizedToPerformOperation` in the defradb repository, treat it as an unauthenticated/missing-identity condition (not insufficient permissions). Map it to HTTP 401 Unauthorized rather than 403 Forbidden; this mapping is intentional and consistent with prior behavior (pre-PR `#4661`).
Applied to files:
internal/db/collection_index.gointernal/db/collection_purge_test.gointernal/db/collection_truncate.go
📚 Learning: 2026-06-18T16:47:47.339Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: When reviewing this repository’s Go code, do not flag `sync.WaitGroup.Go()` (e.g., `wg.Go(fn)`) as a compilation error. This method exists starting in Go 1.25; it should be treated as valid as long as the repo’s configured/minimum Go toolchain version is 1.25+ (e.g., per `go.mod` and/or CI). If the project is building with an older Go version, then it may be appropriate to flag it.
Applied to files:
internal/db/collection_index.gointernal/db/collection_purge_test.gointernal/db/collection_truncate.go
📚 Learning: 2026-06-18T16:47:47.339Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: In sourcenetwork/defradb code reviews, do not raise findings that are likely compiler/type-check errors (e.g., code that would fail `go test`/`go build`) because CI pipelines will catch compilation failures. Only flag compilation-related problems if there is a clear, verifiable issue beyond what CI will already surface (e.g., a mismatched build tag or missing generated artifact that CI would not cover).
Applied to files:
internal/db/collection_index.gointernal/db/collection_purge_test.gointernal/db/collection_truncate.go
🪛 markdownlint-cli2 (0.23.1)
docs/website/references/cli/defradb_client_collection_purge-docs.md
[warning] 14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (5)
docs/website/references/cli/defradb_client_collection.md (1)
40-40: LGTM!internal/db/collection_purge_test.go (1)
204-221: LGTM!internal/db/collection_truncate.go (2)
529-679: LGTM on the purge-specific ownership/traversal logic (deleteBlockMapping,deleteBlock, and the DAG-walk fallback at Lines 573 and 619) — the write-vs-read-snapshot split for owner checks is well reasoned and covered by the new tests.
494-528: 🗄️ Data Integrity & IntegrationCaller-txn block-visibility fallback is not needed for collection-wide truncate.
Collection-wide truncate clears document data/headstore keys before walking collection commit blocks, and these deletes run outside the explicit transaction via
corekv.SetCtxTxn(ctx, nil).deleteBlocksis called withdocID == "", so per-document uncommitted owner edges cannot be present; if a newly created committed block were encountered, it would already be removed in the earlier hard delete pass.> Likely an incorrect or invalid review comment.internal/db/collection_index.go (1)
348-391: 🎯 Functional CorrectnessNo change needed.
The soft-delete path calls
deleteIndexedDocWithID(..., false)before marking the document deleted, so purge’s early return for already-deleted documents does not leave index entries behind.
|
Final scale and correctness verification:
Verified internal/db and internal/db/id package tests, the purge integration matrix, focused race tests, and golangci-lint with 0 new issues. |
|
CI note:
Base coverage: https://github.com/sourcenetwork/defradb/actions/runs/30392652964/job/90391332793 |
shahzadlone
left a comment
shahzadlone
left a comment
There was a problem hiding this comment.
Thanks for this PR, I have done the first round of review and left some comments. I think there is some work that needs to be done before we can merge this.
In addition to the comments I noticed a pattern where you piggy-backed/used some truncate code for the purge code, is this purposeful? For example rather than defining a new NAC ACP permission using the truncate one or also the use of options.TruncateCollectionOptions over a tailored option for Purge. Please find and sort any and all such actions that I may not have caught where this same pattern might be repeated.
One additional nitpick: The parent collection command synopsis in cli/collection.go still lists add, describe, patch, set-active, delete, and truncate, but omits purge-docs. The regenerated parent documentation preserves that omission. Please include the new command so it is discoverable from the parent help.
|
suggestion: I don't see a linked issue and the PR description is not using the DefraDB PR description template style, will appreciate if you can adhere to that please :). More info: https://github.com/sourcenetwork/defradb?tab=contributing-ov-file#-link-with-relevant-issues |
|
Addressed the review in dba27a2 and 48ee8e9:
Local validation is green across Go, HTTP, CLI, C, and WASM purge flows, plus both pinned lint configurations. |
|
Addressed the review round in
I also added issue #5117 and updated the PR body to the repository template with the operation limitations. Focused tests, race tests, lint, generated CLI docs, and generated OpenAPI checks pass locally. Fresh CI is running now. |
|
Follow-up CI fix: removed the test-only alternate collection identity. Purge tests now follow the normal collection lookup on every client; unauthorized callers fail at The targeted NAC tests pass through Go, HTTP, CLI, and C, and the full client matrix is green. The remaining vulnerability check is also failing on |
|
Addressed the review items and pushed the follow-up fixes. Purge now has its own options and NAC permission, the parent help/docs include purge-docs, and the SourceHub ACP and secondary-index matrices are green. The vulnerability job is failing the same way on develop; LevelDB and macOS are still queued. Ready for another pass. |
|
Stacked #5119 so CI now runs against the fixes for the three default-branch failures. The LevelDB hang was a nested-transaction regression from #5101: the exact failing test went from timing out after 45 seconds to passing in 1.65 seconds, and the purge flow now passes on LevelDB as well. All review threads are resolved and this is ready for another look. |
|
The first full rerun exposed one more baseline test-harness issue: transactional DeleteIndex was waiting for background cleanup before its transaction committed. #5119 now gives it the same transaction guard already used by NewIndex. The full LevelDB transaction package and 10 repeated runs of the failing test pass locally; the follow-up is stacked here for the new CI run. |
|
The remaining deterministic LevelDB failures had the same teardown cause: integration cases left explicit transactions open, then node shutdown waited on an index worker blocked acquiring LevelDB transaction access. The harness now discards those transactions before closing nodes. All three formerly hanging cases pass three consecutive runs; their full packages and the purge tests pass under the race detector. The Wazero failure was a separate P2P sync timeout, and the two affected sync cases passed 40 combined consecutive local executions, so no product change was made for it. Fresh CI is running. |
|
Stacked the verified peer-sync baseline from #5119. The latest The shared update fixes the underlying peer-ID bookkeeping and network-readiness races, and adds a bounded macOS retry that still fails on data races, multiple failures, or a repeated failure. On the stacked branch, P2P internals, network info, document and branchable sync, and purge integration all pass together under shuffled race execution. Fresh CI is running. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/db/collection_purge_leveldb_test.go (1)
41-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClose the database on the timeout path too.
db.Close()runs only when the operation returns. If the operation blocks, theselecttakes the timeout branch and the LevelDB handle stays open for the rest of the package run, while the operation goroutine keeps running.t.TempDir()cleanup then removes files that LevelDB still holds open, which can add a confusing secondary failure on top of the real hang report.Register the close in
newLevelDBForPurgeTestwitht.Cleanupso it runs on every path, and remove the close from this helper.♻️ Proposed cleanup handling
func newLevelDBForPurgeTest(t *testing.T, ctx context.Context) *DB { t.Helper() rootstore, err := leveldb.NewDatastore(t.TempDir(), nil) require.NoError(t, err) adminInfo, err := acpDB.NewNACInfo(ctx, "", false) require.NoError(t, err) db, err := newDB(ctx, rootstore, adminInfo) require.NoError(t, err) + t.Cleanup(func() { db.Close() }) return db } func requireLevelDBOperationCompletes(t *testing.T, db *DB, operation func() error) { t.Helper() done := make(chan error, 1) go func() { done <- operation() }() select { case err := <-done: - db.Close() require.NoError(t, err) case <-time.After(5 * time.Second): t.Fatal("operation blocked while opening a nested LevelDB transaction") } }The
dbparameter then becomes unused; drop it from the helper signature and from both call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/collection_purge_leveldb_test.go` around lines 41 - 56, Register db.Close with t.Cleanup in newLevelDBForPurgeTest so the database closes on both completion and timeout paths. Remove db.Close from requireLevelDBOperationCompletes, drop its now-unused db parameter, and update both call sites to pass only the testing handle and operation.internal/db/collection_purge.go (1)
275-318: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid re-scanning the whole searchable-encryption prefix on each chunk.
In the non-transactional path, the loop breaks after
hardDeleteChunkSizematched keys and then restarts the iterator at the collection-widekeys.DatastoreSEprefix. Every pass re-reads all non-matching entries that were already inspected. For a collection with N searchable-encryption records and M matching records, the total scan cost is about(M / hardDeleteChunkSize) * N.Resume iteration after the last inspected key instead of restarting from the prefix.
♻️ Suggested resumable-cursor approach
ds := datastore.NewMultistore(c.db.rootstore, c.db.lockSet, c.db.blockStoreChunkSize).Datastore() _, transactional := corekv.TryGetCtxTxn(ctx) + var resumeAfter []byte for { iter, err := ds.Iterator(ctx, datastore.IterOptions{ Prefix: keys.DatastoreSE{CollectionShortID: shortID}, KeysOnly: true, + // Continue after the last key inspected in the previous chunk. + StartAfter: resumeAfter, })Set
resumeAfterto a copy ofiter.Key()for the last inspected entry before you break out of the inner loop. Use the option name thatdatastore.IterOptionsexposes for an exclusive start bound.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/collection_purge.go` around lines 275 - 318, Update the purge loop around the datastore iterator to retain a copy of the last inspected iterator key when the non-transactional chunk limit is reached, then pass it as the exclusive resume bound in the next datastore.IterOptions call. Preserve the initial collection prefix, transactional behavior, deletion flow, and completion handling while ensuring subsequent chunks do not rescan previously inspected entries.internal/db/collection_purge_test.go (1)
135-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the transaction-limit failure without matching a Badger message string.
Badger already exposes
badgerds.ErrTxnTooBigfor this condition, so the test should assert that sentinel instead of"Txn is too big".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/collection_purge_test.go` at line 135, Update the transaction-limit assertion in the purge test to verify the returned error matches Badger’s badgerds.ErrTxnTooBig sentinel, replacing the brittle message-string check while preserving the existing failure assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/acp/nac/purge_document_test.go`:
- Around line 58-63: Update
tests/integration/acp/nac/purge_document_test.go:58-63 to retain or rename the
missing-identity preflight case and add a separate caller with collection-get
but without purge-document, asserting the purge-specific denial; at 82-87 grant
collection-get to the wrong identity while withholding purge-document and add
the corresponding successful grant case. Update
tests/integration/acp/nac/relation_admin/purge_document_test.go:35-40 so the
initial relation grants only collection-get, then grant the permission
authorizing purge and verify success.
In `@tests/integration/searchable_encryption/purge_test.go`:
- Around line 62-79: Update the purge test’s post-purge requests so the empty
searchable-encryption result is queried from node 1, matching the node targeted
by PurgeDocs. Add a separate node-0 assertion that queries the same record and
confirms it remains available, preserving the local, non-replicated purge
behavior.
---
Nitpick comments:
In `@internal/db/collection_purge_leveldb_test.go`:
- Around line 41-56: Register db.Close with t.Cleanup in newLevelDBForPurgeTest
so the database closes on both completion and timeout paths. Remove db.Close
from requireLevelDBOperationCompletes, drop its now-unused db parameter, and
update both call sites to pass only the testing handle and operation.
In `@internal/db/collection_purge_test.go`:
- Line 135: Update the transaction-limit assertion in the purge test to verify
the returned error matches Badger’s badgerds.ErrTxnTooBig sentinel, replacing
the brittle message-string check while preserving the existing failure
assertion.
In `@internal/db/collection_purge.go`:
- Around line 275-318: Update the purge loop around the datastore iterator to
retain a copy of the last inspected iterator key when the non-transactional
chunk limit is reached, then pass it as the exclusive resume bound in the next
datastore.IterOptions call. Preserve the initial collection prefix,
transactional behavior, deletion flow, and completion handling while ensuring
subsequent chunks do not rescan previously inspected entries.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ead081c4-5dcc-4b2b-ad8b-5b428434a86f
📒 Files selected for processing (50)
.github/known-vulnerabilities.txt.github/workflows/check-vulnerabilities.yml.github/workflows/test-macos.ymlMakefileacp/types/types.gocbindings/collection_purge.gocbindings/wrapper_collection.gocli/collection.gocli/collection_purge_docs.gocli/test/integration/p2p/replicator_list_test.gocli/wizard/unit_test.goclient/collection.goclient/mocks/collection.goclient/options/collection.godocs/website/references/cli/defradb_client_collection.mddocs/website/references/cli/defradb_client_collection_purge-docs.mdgo.modhttp/client_collection.gohttp/errors.gohttp/errors_test.gohttp/handler_collection.gointernal/db/collection_index.gointernal/db/collection_purge.gointernal/db/collection_purge_conflict_test.gointernal/db/collection_purge_leveldb_test.gointernal/db/collection_purge_test.gointernal/db/collection_truncate.gointernal/db/db.gointernal/db/db_test.gointernal/db/errors.gointernal/db/id/document.gointernal/db/id/document_test.gointernal/db/p2p/p2p.gointernal/db/p2p/p2p_test.gointernal/db/p2p/sync_branchable_col.gointernal/db/p2p/sync_doc.gointernal/db/txn_collection.gojs/client_collection.gotests/action/active_peers.gotests/action/delete_index.gotests/action/purge_docs.gotests/clients/cli/wrapper_collection.gotests/clients/js/wrapper_collection.gotests/integration/acp/dac/purge_test.gotests/integration/acp/nac/purge_document_test.gotests/integration/acp/nac/relation_admin/purge_document_test.gotests/integration/net/info/connect_peers_test.gotests/integration/net/sync/documents_test.gotests/integration/searchable_encryption/purge_test.gotests/integration/utils.go
💤 Files with no reviewable changes (1)
- tests/integration/net/info/connect_peers_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/clients/cli/wrapper_collection.go
- docs/website/references/cli/defradb_client_collection_purge-docs.md
- js/client_collection.go
- cli/collection_purge_docs.go
- internal/db/txn_collection.go
- docs/website/references/cli/defradb_client_collection.md
- http/handler_collection.go
- cbindings/wrapper_collection.go
- internal/db/id/document_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check data format changes job
- GitHub Check: Test macos job
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-03-27T08:24:16.083Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4661
File: http/middleware.go:93-93
Timestamp: 2026-03-27T08:24:16.083Z
Learning: When handling `client.ErrNotAuthorizedToPerformOperation` in the defradb repository, treat it as an unauthenticated/missing-identity condition (not insufficient permissions). Map it to HTTP 401 Unauthorized rather than 403 Forbidden; this mapping is intentional and consistent with prior behavior (pre-PR `#4661`).
Applied to files:
cli/test/integration/p2p/replicator_list_test.gohttp/errors_test.gocli/collection.gointernal/db/p2p/sync_branchable_col.gointernal/db/collection_purge_leveldb_test.gointernal/db/errors.gotests/action/delete_index.gotests/integration/acp/nac/relation_admin/purge_document_test.gotests/integration/acp/nac/purge_document_test.gotests/integration/net/sync/documents_test.gohttp/errors.gointernal/db/collection_purge_conflict_test.gointernal/db/db.gotests/action/purge_docs.gointernal/db/p2p/sync_doc.gointernal/db/db_test.gocbindings/collection_purge.gointernal/db/p2p/p2p.goacp/types/types.goclient/options/collection.gohttp/client_collection.gotests/integration/searchable_encryption/purge_test.goclient/collection.gotests/integration/acp/dac/purge_test.gointernal/db/p2p/p2p_test.gotests/action/active_peers.gotests/integration/utils.gotests/clients/js/wrapper_collection.gointernal/db/collection_purge.gointernal/db/collection_truncate.goclient/mocks/collection.gocli/wizard/unit_test.gointernal/db/collection_purge_test.gointernal/db/collection_index.gointernal/db/id/document.go
📚 Learning: 2026-06-18T16:47:47.339Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: When reviewing this repository’s Go code, do not flag `sync.WaitGroup.Go()` (e.g., `wg.Go(fn)`) as a compilation error. This method exists starting in Go 1.25; it should be treated as valid as long as the repo’s configured/minimum Go toolchain version is 1.25+ (e.g., per `go.mod` and/or CI). If the project is building with an older Go version, then it may be appropriate to flag it.
Applied to files:
cli/test/integration/p2p/replicator_list_test.gohttp/errors_test.gocli/collection.gointernal/db/p2p/sync_branchable_col.gointernal/db/collection_purge_leveldb_test.gointernal/db/errors.gotests/action/delete_index.gotests/integration/acp/nac/relation_admin/purge_document_test.gotests/integration/acp/nac/purge_document_test.gotests/integration/net/sync/documents_test.gohttp/errors.gointernal/db/collection_purge_conflict_test.gointernal/db/db.gotests/action/purge_docs.gointernal/db/p2p/sync_doc.gointernal/db/db_test.gocbindings/collection_purge.gointernal/db/p2p/p2p.goacp/types/types.goclient/options/collection.gohttp/client_collection.gotests/integration/searchable_encryption/purge_test.goclient/collection.gotests/integration/acp/dac/purge_test.gointernal/db/p2p/p2p_test.gotests/action/active_peers.gotests/integration/utils.gotests/clients/js/wrapper_collection.gointernal/db/collection_purge.gointernal/db/collection_truncate.goclient/mocks/collection.gocli/wizard/unit_test.gointernal/db/collection_purge_test.gointernal/db/collection_index.gointernal/db/id/document.go
📚 Learning: 2026-06-18T16:47:47.339Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: In sourcenetwork/defradb code reviews, do not raise findings that are likely compiler/type-check errors (e.g., code that would fail `go test`/`go build`) because CI pipelines will catch compilation failures. Only flag compilation-related problems if there is a clear, verifiable issue beyond what CI will already surface (e.g., a mismatched build tag or missing generated artifact that CI would not cover).
Applied to files:
cli/test/integration/p2p/replicator_list_test.gohttp/errors_test.gocli/collection.gointernal/db/p2p/sync_branchable_col.gointernal/db/collection_purge_leveldb_test.gointernal/db/errors.gotests/action/delete_index.gotests/integration/acp/nac/relation_admin/purge_document_test.gotests/integration/acp/nac/purge_document_test.gotests/integration/net/sync/documents_test.gohttp/errors.gointernal/db/collection_purge_conflict_test.gointernal/db/db.gotests/action/purge_docs.gointernal/db/p2p/sync_doc.gointernal/db/db_test.gocbindings/collection_purge.gointernal/db/p2p/p2p.goacp/types/types.goclient/options/collection.gohttp/client_collection.gotests/integration/searchable_encryption/purge_test.goclient/collection.gotests/integration/acp/dac/purge_test.gointernal/db/p2p/p2p_test.gotests/action/active_peers.gotests/integration/utils.gotests/clients/js/wrapper_collection.gointernal/db/collection_purge.gointernal/db/collection_truncate.goclient/mocks/collection.gocli/wizard/unit_test.gointernal/db/collection_purge_test.gointernal/db/collection_index.gointernal/db/id/document.go
📚 Learning: 2026-06-06T20:14:54.888Z
Learnt from: fredcarle
Repo: sourcenetwork/defradb PR: 4879
File: cli/test/integration/p2p/disconnect_test.go:46-67
Timestamp: 2026-06-06T20:14:54.888Z
Learning: In sourcenetwork/defradb, tests under cli/test/integration/p2p/ are intentionally limited to CLI command-level validation (e.g., argument parsing, command error handling, and user-facing error messages). Do not add assertions that require actual P2P connection state (e.g., verifying peers after connect/disconnect). For end-to-end connection-state verification (connect → disconnect → assert active peers are empty), add/extend tests in tests/integration/net/info/ (e.g., TestNetInfoDisconnectSinglePeer / TestNetInfoDisconnectMultiplePeers).
Applied to files:
cli/test/integration/p2p/replicator_list_test.go
📚 Learning: 2026-08-04T14:08:52.524Z
Learnt from: ChrisBQu
Repo: sourcenetwork/defradb PR: 5099
File: tests/clients/java/wrapper_collection.go:228-271
Timestamp: 2026-08-04T14:08:52.524Z
Learning: In DefraDB client batch-document insertion paths, return successfully when the document list is empty before JSON marshaling. In Go, marshaling a nil slice produces `null`, which must not be sent as an add-document payload; apply equivalent empty-input handling in the C bindings path.
Applied to files:
tests/clients/js/wrapper_collection.go
🪛 GitHub Check: codecov/patch
internal/db/p2p/p2p.go
[warning] 290-290: internal/db/p2p/p2p.go#L290
Added line #L290 was not covered by tests
[warning] 297-297: internal/db/p2p/p2p.go#L297
Added line #L297 was not covered by tests
internal/db/collection_purge.go
[warning] 54-54: internal/db/collection_purge.go#L54
Added line #L54 was not covered by tests
[warning] 73-73: internal/db/collection_purge.go#L73
Added line #L73 was not covered by tests
[warning] 81-81: internal/db/collection_purge.go#L81
Added line #L81 was not covered by tests
[warning] 87-87: internal/db/collection_purge.go#L87
Added line #L87 was not covered by tests
[warning] 90-90: internal/db/collection_purge.go#L90
Added line #L90 was not covered by tests
[warning] 98-98: internal/db/collection_purge.go#L98
Added line #L98 was not covered by tests
[warning] 106-106: internal/db/collection_purge.go#L106
Added line #L106 was not covered by tests
[warning] 132-132: internal/db/collection_purge.go#L132
Added line #L132 was not covered by tests
[warning] 157-157: internal/db/collection_purge.go#L157
Added line #L157 was not covered by tests
[warning] 160-160: internal/db/collection_purge.go#L160
Added line #L160 was not covered by tests
[warning] 189-189: internal/db/collection_purge.go#L189
Added line #L189 was not covered by tests
[warning] 195-195: internal/db/collection_purge.go#L195
Added line #L195 was not covered by tests
[warning] 202-202: internal/db/collection_purge.go#L202
Added line #L202 was not covered by tests
[warning] 221-221: internal/db/collection_purge.go#L221
Added line #L221 was not covered by tests
[warning] 230-230: internal/db/collection_purge.go#L230
Added line #L230 was not covered by tests
[warning] 240-240: internal/db/collection_purge.go#L240
Added line #L240 was not covered by tests
[warning] 250-250: internal/db/collection_purge.go#L250
Added line #L250 was not covered by tests
[warning] 259-259: internal/db/collection_purge.go#L259
Added line #L259 was not covered by tests
[warning] 270-270: internal/db/collection_purge.go#L270
Added line #L270 was not covered by tests
[warning] 281-281: internal/db/collection_purge.go#L281
Added line #L281 was not covered by tests
[warning] 289-289: internal/db/collection_purge.go#L289
Added line #L289 was not covered by tests
[warning] 296-296: internal/db/collection_purge.go#L296
Added line #L296 was not covered by tests
[warning] 301-302: internal/db/collection_purge.go#L301-L302
Added lines #L301 - L302 were not covered by tests
[warning] 307-307: internal/db/collection_purge.go#L307
Added line #L307 was not covered by tests
[warning] 312-312: internal/db/collection_purge.go#L312
Added line #L312 was not covered by tests
internal/db/collection_truncate.go
[warning] 300-303: internal/db/collection_truncate.go#L300-L303
Added lines #L300 - L303 were not covered by tests
[warning] 529-529: internal/db/collection_truncate.go#L529
Added line #L529 was not covered by tests
[warning] 535-535: internal/db/collection_truncate.go#L535
Added line #L535 was not covered by tests
[warning] 573-573: internal/db/collection_truncate.go#L573
Added line #L573 was not covered by tests
[warning] 587-587: internal/db/collection_truncate.go#L587
Added line #L587 was not covered by tests
internal/db/collection_index.go
[warning] 399-399: internal/db/collection_index.go#L399
Added line #L399 was not covered by tests
[warning] 404-404: internal/db/collection_index.go#L404
Added line #L404 was not covered by tests
[warning] 407-407: internal/db/collection_index.go#L407
Added line #L407 was not covered by tests
internal/db/id/document.go
[warning] 280-280: internal/db/id/document.go#L280
Added line #L280 was not covered by tests
[warning] 287-287: internal/db/id/document.go#L287
Added line #L287 was not covered by tests
[warning] 294-294: internal/db/id/document.go#L294
Added line #L294 was not covered by tests
[warning] 302-302: internal/db/id/document.go#L302
Added line #L302 was not covered by tests
[warning] 315-315: internal/db/id/document.go#L315
Added line #L315 was not covered by tests
[warning] 333-333: internal/db/id/document.go#L333
Added line #L333 was not covered by tests
🔇 Additional comments (51)
internal/db/db.go (1)
25-25: LGTM!Also applies to: 120-120, 267-291
internal/db/db_test.go (1)
21-21: LGTM!Also applies to: 75-109
go.mod (1)
3-3: LGTM!.github/known-vulnerabilities.txt (1)
1-6: LGTM!.github/workflows/check-vulnerabilities.yml (1)
50-55: LGTM!cli/wizard/unit_test.go (1)
250-287: LGTM!Also applies to: 316-325, 358-376, 424-446, 495-517, 568-590, 653-695
internal/db/collection_index.go (3)
14-14: LGTM!Also applies to: 32-32
117-124: LGTM!
350-410: LGTM!internal/db/id/document.go (2)
202-237: LGTM!
267-351: LGTM!http/client_collection.go (1)
224-264: LGTM!http/errors.go (1)
174-188: LGTM!http/errors_test.go (1)
65-78: LGTM!cli/collection.go (1)
22-28: LGTM!tests/integration/acp/nac/purge_document_test.go (1)
22-43: LGTM!tests/clients/js/wrapper_collection.go (1)
160-176: LGTM!tests/action/purge_docs.go (1)
62-62: LGTM!tests/integration/acp/dac/purge_test.go (1)
21-83: LGTM!tests/integration/net/sync/documents_test.go (1)
54-59: LGTM!Also applies to: 130-135, 210-215, 377-382
cbindings/collection_purge.go (1)
29-82: LGTM!tests/integration/utils.go (1)
304-304: LGTM!Also applies to: 366-366, 700-707, 843-848, 1006-1015, 1029-1029, 1057-1057, 1262-1301, 2394-2394
Makefile (1)
71-71: LGTM!Also applies to: 327-327, 386-394
.github/workflows/test-macos.yml (1)
79-84: LGTM!cli/test/integration/p2p/replicator_list_test.go (1)
15-15: LGTM!Also applies to: 135-135
tests/action/delete_index.go (1)
105-107: LGTM!tests/action/active_peers.go (1)
15-17: LGTM!Also applies to: 26-29, 73-85
internal/db/p2p/p2p_test.go (1)
19-51: LGTM!internal/db/p2p/p2p.go (1)
24-24: LGTM!Also applies to: 287-303
internal/db/p2p/sync_branchable_col.go (1)
93-100: LGTM!internal/db/p2p/sync_doc.go (1)
97-104: LGTM!client/collection.go (1)
193-204: LGTM!client/options/collection.go (1)
438-465: LGTM!internal/db/collection_purge.go (6)
41-64: LGTM!
66-111: LGTM!
138-210: LGTM!
212-262: LGTM!
345-345: LGTM!
113-136: 🩺 Stability & AvailabilityNo change needed. Committing a chunk transaction does not release the collection lock because lock lifecycle callbacks are attached to
lockTxn; the derived chunk transaction has the same ID but does not reuse the same lock-set entry or callbacks.internal/db/errors.go (1)
109-109: LGTM!Also applies to: 245-245
acp/types/types.go (2)
231-232: LGTM!
125-125: 🔒 Security & PrivacyAuthorization Bypass (CWE-863): Incorrect Authorization
Reachability: External
Verify that the permission constant and the permission-name list stay index-aligned.
NodePurgeDocumentPermis appended at the end of the constant block, and"purge-document"is appended at the end ofRequiredResourcePermissionsForNode. If the constant-to-name conversion uses the constant's ordinal as an index into this slice, the two orderings must match for every entry. A mismatch would map the purge permission to a different policy permission and authorize purge under the wrong rule.Confirm how the node permission constant is converted to its resource permission name.
#!/bin/bash # Confirm the ordering of node permission constants and how they map to names. set -euo pipefail rg -nP -B4 -A60 'NodeBypassDACPerm|NodeResourcePermission\b' acp/types/types.go | head -120 rg -nP -C6 'RequiredResourcePermissionsForNode' --type=go rg -nP -C8 '\bfunc .*(String|Name)\s*\(\).*(string)' acp/types/types.go rg -nP -C6 'NodePurgeDocumentPerm' --type=goAlso applies to: 182-182
client/mocks/collection.go (1)
1063-1138: LGTM!internal/db/collection_purge_test.go (2)
109-134: LGTM!Also applies to: 136-154
156-183: LGTM!Also applies to: 185-238, 240-311, 313-355, 542-552, 585-611
internal/db/collection_purge_conflict_test.go (1)
25-57: LGTM!internal/db/collection_truncate.go (3)
300-307: LGTM!
470-569: LGTM!
571-586: 🩺 Stability & AvailabilityVerify
DeleteBlockis idempotent before relying on purge resumption.
deleteBlocksdeletes encryption blocks before post-order blocks, then releases owner mappings. If a laterDeleteBlockcall fails andDeleteBlockpropagates missing CIDs instead of ignoring them, a resumed purge can callDeleteBlockagain on an already-deleted encryption or signature CID and return early before cleaning remaining blocks. Confirm the v0.3.1github.com/sourcenetwork/corekv/blockstoreimplementation ignores missing CIDs, or add localipld.ErrNotFound/corekv.ErrNotFoundhandling at these calls.internal/db/collection_purge_leveldb_test.go (2)
29-39: LGTM!
58-124: LGTM!
|
Addressed the final review pass in 50ac61e: the SE cleanup scan now resumes within the collection range, block cleanup explicitly ignores missing blocks during retries, LevelDB cleanup runs on timeout, and both Badger limit checks use the sentinel. The direct NAC test also verifies the exact @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/db/collection_purge_test.go (1)
141-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that a denied purge does not modify the document.
Lines 141-143 only verify the returned error. A purge that deletes the document before returning
client.ErrNotAuthorizedToPerformOperationwould pass this test. Before granting the admin relationship, assert that the owner can still finddoc.ID().Proposed test assertion
require.ErrorIs(t, err, client.ErrNotAuthorizedToPerformOperation) require.ErrorContains(t, err, "Permission: purge-document") + exists, err := col.ExistsDocument( + ctx, + doc.ID(), + options.ExistsDocument().SetIdentity(owner), + ) + require.NoError(t, err) + require.True(t, exists) _, err = db.AddNACActorRelationship(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/collection_purge_test.go` around lines 141 - 143, Extend the denied-purge test around the existing error assertions to verify the document remains unchanged: before granting the admin relationship, use the owner client to find doc.ID() and assert that lookup succeeds. Keep the existing authorization and permission error checks intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/db/collection_purge_test.go`:
- Around line 141-143: Extend the denied-purge test around the existing error
assertions to verify the document remains unchanged: before granting the admin
relationship, use the owner client to find doc.ID() and assert that lookup
succeeds. Keep the existing authorization and permission error checks intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 33b0cf01-4c5a-4909-9d9d-a0d26d121a2a
📒 Files selected for processing (10)
internal/db/collection_purge.gointernal/db/collection_purge_leveldb_test.gointernal/db/collection_purge_test.gointernal/db/collection_truncate.gointernal/db/collection_truncate_test.gointernal/db/p2p/p2p_test.gointernal/keys/datastore_se.gointernal/keys/key.gotests/integration/acp/nac/purge_document_test.gotests/integration/searchable_encryption/purge_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/db/collection_purge_leveldb_test.go
- tests/integration/searchable_encryption/purge_test.go
- tests/integration/acp/nac/purge_document_test.go
- internal/db/collection_truncate.go
- internal/db/collection_purge.go
📜 Review details
⏰ Context from checks skipped due to timeout. (51)
- GitHub Check: Test macos job
- GitHub Check: Test Limited Resource job
- GitHub Check: Check data format changes job
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Check mocks job
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage view job
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage signed docs job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Test coverage telemetry job
- GitHub Check: Test coverage leveldb job
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Start binary job
- GitHub Check: Test coverage JS job
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage secondary index job
- GitHub Check: Test coverage encryption job
- GitHub Check: Build, install, and test Debian package
- GitHub Check: Validate containerfile job
- GitHub Check: Check vulnerabilities job
- GitHub Check: Build dependencies job
- GitHub Check: Lint GoLang job
- GitHub Check: Check wizard health job
- GitHub Check: Check cli documentation job
- GitHub Check: Build and test Linux C shared library
- GitHub Check: Check http documentation job
- GitHub Check: Lint GoLang job
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-03-27T08:24:16.083Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4661
File: http/middleware.go:93-93
Timestamp: 2026-03-27T08:24:16.083Z
Learning: When handling `client.ErrNotAuthorizedToPerformOperation` in the defradb repository, treat it as an unauthenticated/missing-identity condition (not insufficient permissions). Map it to HTTP 401 Unauthorized rather than 403 Forbidden; this mapping is intentional and consistent with prior behavior (pre-PR `#4661`).
Applied to files:
internal/keys/datastore_se.gointernal/keys/key.gointernal/db/collection_truncate_test.gointernal/db/p2p/p2p_test.gointernal/db/collection_purge_test.go
📚 Learning: 2026-06-18T16:47:47.339Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: When reviewing this repository’s Go code, do not flag `sync.WaitGroup.Go()` (e.g., `wg.Go(fn)`) as a compilation error. This method exists starting in Go 1.25; it should be treated as valid as long as the repo’s configured/minimum Go toolchain version is 1.25+ (e.g., per `go.mod` and/or CI). If the project is building with an older Go version, then it may be appropriate to flag it.
Applied to files:
internal/keys/datastore_se.gointernal/keys/key.gointernal/db/collection_truncate_test.gointernal/db/p2p/p2p_test.gointernal/db/collection_purge_test.go
📚 Learning: 2026-06-18T16:47:47.339Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: In sourcenetwork/defradb code reviews, do not raise findings that are likely compiler/type-check errors (e.g., code that would fail `go test`/`go build`) because CI pipelines will catch compilation failures. Only flag compilation-related problems if there is a clear, verifiable issue beyond what CI will already surface (e.g., a mismatched build tag or missing generated artifact that CI would not cover).
Applied to files:
internal/keys/datastore_se.gointernal/keys/key.gointernal/db/collection_truncate_test.gointernal/db/p2p/p2p_test.gointernal/db/collection_purge_test.go
🔇 Additional comments (8)
internal/db/collection_truncate_test.go (1)
15-58: LGTM!internal/keys/datastore_se.go (1)
36-36: LGTM!Also applies to: 76-78
internal/keys/key.go (1)
31-47: LGTM!internal/db/p2p/p2p_test.go (1)
25-37: LGTM!Also applies to: 39-52, 54-61, 63-71
internal/db/collection_purge_test.go (4)
25-25: LGTM!Also applies to: 34-34
60-109: LGTM!
159-204: LGTM!
290-339: LGTM!
AndrewSisley
left a comment
AndrewSisley
left a comment
There was a problem hiding this comment.
Hey Ivan, I'm not sure we want to expose this behaviour in this format - I've opened a discussion on discord, please do not merge this until the team has reached a decision.
fc0e2d8 to
4d47281
Compare
iverc
changed the title
AndrewSisley
left a comment
AndrewSisley
left a comment
There was a problem hiding this comment.
Is looking good, thank you very much Ivan! Just a handful of smaller requests from me before merge.
Thank you so much, @AndrewSisley . Addressing the feedback! |
32a10f6 to
bc4065d
Compare
AndrewSisley
left a comment
AndrewSisley
left a comment
There was a problem hiding this comment.
Looks great to me, thanks for all the extra effort on this one Ivan.
Approving now, but please make sure PruneHistory is removed before merge (for ref. discussed over discord), and please expand the documentation on the filter option.
Thank you! Removed |
iverc
added
feature
59fe922 to
b408367
Compare
Has an approval and seems to block some things
|
Bug bash results: |
4 participants
Relevant issue(s)
Resolves #5117
Description
Adds an optional filter to the existing collection truncate operation so callers can permanently remove matching documents from the local node without introducing another delete command. Truncating without a filter keeps the existing collection-wide behavior.
Filtered truncate removes the selected document data and associated local state, including heads, indexes, ID mappings, signatures, encryption blocks, and searchable-encryption records. The optional
pruneHistorysetting also removes history blocks that are not shared by another document.The filter is supported by the Go, HTTP, CLI, and C APIs, and GraphQL now exposes
truncate_<Collection>mutations. No new dependencies are added.Limitations:
Tasks
How has this been tested?
Specify the platform(s) on which this was tested: