feat: Add syntax delete collection cmd#4688
feat: Add syntax delete collection cmd#4688shahzadlone wants to merge 3 commits intosourcenetwork:developfrom
Conversation
shahzadlone
force-pushed
the
lone/feat/syntax-sugar-collection-delete
branch
from
0a765fb to
8b0c800
Compare
📝 WalkthroughWalkthroughAdds end-to-end collection deletion: CLI command, HTTP endpoint, C/JS bindings, client interfaces, core DB deletion logic, options/errors, and comprehensive integration and NAC authorization tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/CLI/HTTP
participant Handler as Handler / CWrapper
participant Auth as ACP Gate
participant DB as DB Store
participant Txn as Transaction
participant Coll as Collection Lookup
Client->>Handler: DeleteCollection(ctx, name, opts)
Handler->>Auth: Check NodePatchCollectionPerm (identity from ctx)
Auth-->>Handler: Authorized / Denied
alt Denied
Handler-->>Client: Permission error (formatted)
else Authorized
Handler->>DB: DeleteCollection(ctx, name)
DB->>Txn: ensureContextTxn(ctx)
Txn-->>DB: txn ready
DB->>Coll: getCollectionByName(name)
Coll-->>DB: found / not found
alt Not found
DB-->>Handler: collection not found error
else Found
DB->>DB: deleteCollectionVersions()
DB->>DB: loadCollectionDefinitions()
DB->>Txn: Commit()
Txn-->>DB: committed
DB-->>Handler: success
Handler-->>Client: 200 OK / nil
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Out-of-scope changes (relative to linked issue
|
| Code Change | Explanation |
|---|---|
internal/db/store.go — new DeleteCollection implementation |
The linked issue requested a CLI syntax sugar command only; adding core DB deletion logic exceeds that single objective. |
http/handler_store.go, http/client.go — new HTTP endpoint and client methods |
HTTP API additions go beyond the CLI-only objective. |
cbindings/*.go, js/*.go — new C and JS bindings |
Language bindings and transaction propagation are broader platform integrations not required by the CLI syntax sugar request. |
Possibly related PRs
- fix: Fix broken transaction behavior #4587 — related to transaction-aware C binding patterns (attachTxnFromPointer) used by the new C binding.
- feat: Improve NotAuthorizedToPerformOperation errors #4373 — related to permission-denied error formatting exercised by the new NAC authorization tests.
Suggested labels
area/datastore
Suggested reviewers
- AndrewSisley
- ChrisBQu
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 @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/integration/collection/delete/delete_collection_test.go (1)
288-297: Avoid hard-coding the collection version ID in test patch paths.The literal CID on Line 293 makes this test fragile against unrelated schema-hash/version-ID evolution. Prefer deriving the target version ID dynamically from a prior
GetCollectionsstep before issuing the remove patch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/collection/delete/delete_collection_test.go` around lines 288 - 297, The test currently hard-codes a collection version CID inside the Patch JSON in action.PatchCollection which makes it brittle; change the test to derive the target version ID from the earlier GetCollections call (use the response from the GetCollections step that lists versions) and inject that dynamic ID into the remove patch payload string instead of the literal "bafyreic...". Locate the action.PatchCollection usage in delete_collection_test.go and replace the static path with a formatted patch that uses the version ID extracted from the GetCollections response (the variable holding that response or its versions list), ensuring the patch path is built at runtime before calling action.PatchCollection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/website/references/cli/defradb_client_collection_delete.md`:
- Around line 15-56: The fenced code blocks on this page (the command usage
block "defradb client collection delete --name <name> [flags]", the Examples
block showing "defradb client collection delete --name Users", the short Options
block listing "-h, --help" and "-n, --name", and the larger "Options inherited
from parent commands" block) lack language identifiers and trigger MD040; add
appropriate languages to each fence (e.g., use ```bash for the command and
example blocks and ```text for the options blocks) so every triple-backtick
block specifies a language.
In `@docs/website/references/cli/defradb_client_collection.md`:
- Line 45: Update the synopsis text that currently reads "Add, describe, patch,
set-active, and truncate collections" to include the delete operation (e.g.,
"Add, describe, patch, set-active, delete, and truncate collections") so the SEE
ALSO entry for defradb_client_collection_delete is reflected in the synopsis;
locate and edit the synopsis string in the defradb_client_collection.md content
to add the word "delete" in the comma-separated list.
In `@docs/website/references/http/openapi.json`:
- Around line 1108-1117: The OpenAPI spec for the DeleteCollection operation
(DELETE /collections) is missing a 404 response; update the operation's
"responses" block for DeleteCollection to include a "404" entry (e.g., "404": {
"$ref": "#/components/responses/not_found" } or inline
{"description":"Collection not found","content":{...}}) so that attempts to
delete a non-existent collection are properly documented; locate the
DeleteCollection operation in openapi.json and add the 404 response reference or
define a new components/responses/not_found and reference it.
In `@http/handler_store.go`:
- Around line 840-843: The OpenAPI spec for the deleteCollection operation is
missing 401 and 404 responses; update the responses map on the deleteCollection
operation (where deleteCollection.Responses is set) to include entries for "401"
and "404" (using the same errorResponse schema or distinct responses as
appropriate) so the documented responses match the handler's httpStatusFromError
mappings; also ensure any logic that converts
client.ErrNotAuthorizedToPerformOperation to a 401 (not 403) is reflected in the
spec.
---
Nitpick comments:
In `@tests/integration/collection/delete/delete_collection_test.go`:
- Around line 288-297: The test currently hard-codes a collection version CID
inside the Patch JSON in action.PatchCollection which makes it brittle; change
the test to derive the target version ID from the earlier GetCollections call
(use the response from the GetCollections step that lists versions) and inject
that dynamic ID into the remove patch payload string instead of the literal
"bafyreic...". Locate the action.PatchCollection usage in
delete_collection_test.go and replace the static path with a formatted patch
that uses the version ID extracted from the GetCollections response (the
variable holding that response or its versions list), ensuring the patch path is
built at runtime before calling action.PatchCollection.
🪄 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
Run ID: 88ee985d-2594-4670-ad78-0a010dcebc71
📒 Files selected for processing (30)
cbindings/delete_collection.gocbindings/wrapper.gocbindings/wrapper_tx.gocli/cli.gocli/collection_delete.goclient/db.goclient/errors.goclient/mocks/txn.goclient/options/store.godocs/website/references/cli/defradb_client_collection.mddocs/website/references/cli/defradb_client_collection_delete.mddocs/website/references/http/openapi.jsonhttp/client.gohttp/client_tx.gohttp/handler_store.gointernal/db/store.gointernal/db/txn.gojs/client.gojs/client_store.gojs/client_tx.gotests/action/delete_collection.gotests/clients/cli/wrapper.gotests/clients/cli/wrapper_tx.gotests/clients/http/wrapper.gotests/clients/http/wrapper_tx.gotests/clients/js/wrapper.gotests/clients/js/wrapper_tx.gotests/integration/acp/nac/delete_collection_test.gotests/integration/acp/nac/relation_admin/delete_collection_test.gotests/integration/collection/delete/delete_collection_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (46)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Test coverage telemetry job
- GitHub Check: Test coverage JS job
- GitHub Check: Test coverage view job
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage secondary index job
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage leveldb job
- GitHub Check: Test coverage encryption job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Test macos job
- GitHub Check: Test Limited Resource job
- GitHub Check: Check data format changes job
- GitHub Check: Check http documentation job
- GitHub Check: Check cli documentation job
- GitHub Check: Check vulnerabilities job
- GitHub Check: Test NPX/JS build job
- GitHub Check: Validate containerfile job
- GitHub Check: Start binary job
- GitHub Check: Lint GoLang job
- GitHub Check: Check wizard health job
🧰 Additional context used
🧠 Learnings (1)
📚 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/cli.goclient/errors.gojs/client.gotests/clients/http/wrapper.gotests/clients/js/wrapper_tx.gohttp/client_tx.gocbindings/wrapper_tx.gointernal/db/txn.gotests/clients/js/wrapper.gotests/clients/cli/wrapper_tx.gojs/client_store.gotests/clients/http/wrapper_tx.gohttp/client.gojs/client_tx.goclient/db.gotests/clients/cli/wrapper.goclient/options/store.gocli/collection_delete.gocbindings/delete_collection.gointernal/db/store.gocbindings/wrapper.gotests/action/delete_collection.gotests/integration/acp/nac/relation_admin/delete_collection_test.goclient/mocks/txn.gotests/integration/acp/nac/delete_collection_test.gohttp/handler_store.gotests/integration/collection/delete/delete_collection_test.go
🪛 markdownlint-cli2 (0.22.0)
docs/website/references/cli/defradb_client_collection_delete.md
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (27)
cli/cli.go (1)
150-150: LGTM!The new
MakeCollectionDeleteCommandis correctly wired into the collection command tree, following the alphabetical ordering convention used throughout the file.client/errors.go (1)
101-101: LGTM!The new sentinel error follows the naming convention of
ErrIndexNameRequiredand provides a clear, descriptive message for validation failures.client/options/store.go (1)
536-563: LGTM!The
DeleteCollectionOptionstype and its builder follow the exact same pattern as the existingPatchCollectionOptions(lines 507-534), maintaining consistency across the options API surface.client/mocks/txn.go (1)
1105-1175: LGTM!This is auto-generated mock code from mockery that follows the established patterns for other transaction methods. The generated mock correctly handles the variadic options parameter.
js/client.go (1)
44-44: LGTM!The new
deleteCollectionJavaScript binding is correctly added using the samegoji.Asyncwrapper pattern as other methods.internal/db/txn.go (1)
226-233: LGTM!The
DeleteCollectionmethod follows the established transaction pattern: it initializes the context with the transaction viaInitContext(which sets the transaction for downstream detection) and delegates to the DB layer. This is consistent withPatchCollectionand other transaction methods.tests/clients/js/wrapper_tx.go (1)
114-117: LGTM!The
DeleteCollectionmethod on the test transaction wrapper follows the same pattern asPatchCollectionand other methods: it sets the transaction on the context and delegates to the underlying wrapper.tests/clients/http/wrapper.go (1)
295-301: DeleteCollection wrapper delegation looks correct.This follows the established test-wrapper pattern and correctly forwards context/options to the HTTP client.
js/client_store.go (1)
64-77: JS deleteCollection bridge method is implemented consistently.Argument parsing, context extraction, identity propagation, and DB delegation all match the existing client-store conventions.
http/client_tx.go (1)
146-153: Transaction-scoped DeleteCollection forwarding is correct.The txn context is set before delegation, consistent with the rest of the transaction client methods.
tests/clients/cli/wrapper.go (1)
426-438: CLI wrapper for collection delete looks good.Command construction and identity option propagation are consistent with existing wrapper patterns.
tests/clients/js/wrapper.go (1)
334-343: JS test wrapper DeleteCollection integration is correct.The method properly applies optional identity and delegates to the registered JS bridge operation.
tests/clients/http/wrapper_tx.go (1)
125-132: HTTP transaction wrapper DeleteCollection method is correct.Txn context wiring and delegation are implemented consistently with neighboring methods.
tests/clients/cli/wrapper_tx.go (1)
131-138: CLI transaction DeleteCollection forwarding looks good.Context scoping and delegation are aligned with the existing transaction wrapper design.
cbindings/wrapper_tx.go (1)
136-143: C bindings transaction DeleteCollection method is consistent and correct.Txn context propagation and delegation follow the established wrapper contract.
http/client.go (1)
190-209: Looks good — client delete wiring is consistent.Identity propagation, query-param encoding, and DELETE request construction all match existing client patterns.
client/db.go (1)
214-224: API contract addition is clear and consistent.The new
DeleteCollectionmethod and documentation fit the existingStoresurface well.js/client_tx.go (1)
43-43: JS transaction binding fordeleteCollectionis correctly wired.Registration, argument extraction, option identity propagation, and delegation are all aligned with the existing method patterns.
Also applies to: 115-128
cbindings/wrapper.go (1)
33-33: C binding integration for collection delete looks solid.The extern declaration and wrapper method are consistent with existing C-wrapper call patterns and resource handling.
Also applies to: 699-717
internal/db/store.go (1)
248-285: Delete flow in store layer is well-structured.Permission check, transactional deletion, definition reload, and commit sequencing are consistent with the DB mutation patterns in this file.
http/handler_store.go (1)
145-172: Handler and route wiring forDELETE /collectionsare correct.Validation, txn-aware dispatch, identity option propagation, and route registration all look consistent.
Also applies to: 1026-1026
cli/collection_delete.go (1)
22-52: CLI delete command wiring looks correct.Validation, identity propagation, and delete invocation are all clean and aligned with the new syntax-sugar behavior.
cbindings/delete_collection.go (1)
27-48: DeleteCollection c-binding flow is consistent and safe.Identity/transaction context handling and store call/error propagation are implemented in the expected order.
tests/action/delete_collection.go (1)
59-88: Test action implementation is solid.The node/transaction branching and identity-aware delete execution are correctly structured for integration coverage.
tests/integration/acp/nac/delete_collection_test.go (1)
24-204: NAC gate coverage is well-targeted.These cases provide good client-specific coverage for authorized, missing-identity, and wrong-identity delete attempts.
tests/integration/acp/nac/relation_admin/delete_collection_test.go (1)
24-127: Admin-relation delete tests look good.The deny-then-grant-then-allow sequence is clear and validates the intended NAC relation behavior.
tests/integration/collection/delete/delete_collection_test.go (1)
25-400: Great end-to-end deletion coverage.This suite exercises the key behavioral paths (head-only delete semantics, data constraints, and transaction behavior) thoroughly.
| ``` | ||
| defradb client collection delete --name <name> [flags] | ||
| ``` | ||
|
|
||
| ### Examples | ||
|
|
||
| ``` | ||
| delete a collection by name: | ||
| defradb client collection delete --name Users | ||
| ``` | ||
|
|
||
| ### Options | ||
|
|
||
| ``` | ||
| -h, --help help for delete | ||
| -n, --name string Collection name to delete | ||
| ``` | ||
|
|
||
| ### Options inherited from parent commands | ||
|
|
||
| ``` | ||
| --collection-id string Collection ID | ||
| --get-inactive Get inactive collections as well as active | ||
| -i, --identity string Hex formatted private key used to authenticate with ACP | ||
| --keyring-backend string Keyring backend to use. Options are file or system (default "file") | ||
| --keyring-namespace string Service name to use when using the system backend (default "defradb") | ||
| --keyring-path string Path to store encrypted keys when using the file backend (default "keys") | ||
| --log-format string Log format to use. Options are text or json (default "text") | ||
| --log-level string Log level to use. Options are debug, info, error, fatal (default "info") | ||
| --log-output string Log output path. Options are stderr or stdout. (default "stderr") | ||
| --log-overrides string Logger config overrides. Format <name>,<key>=<val>,...;<name>,... | ||
| --log-source Include source location in logs | ||
| --log-stacktrace Include stacktrace in error and fatal logs | ||
| --no-keyring Disable the keyring and generate ephemeral keys | ||
| --no-log-color Disable colored log output | ||
| --rootdir string Directory for persistent data (default: $HOME/.defradb) | ||
| --secret-file string Path to the file containing secrets (default ".env") | ||
| --source-hub-address string The SourceHub address authorized by the client to make SourceHub transactions on behalf of the actor | ||
| --tx uint Transaction ID | ||
| --url string URL of HTTP endpoint to listen on or connect to (default "127.0.0.1:9181") | ||
| --version-id string Collection version ID | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
The new page fails MD040 at several fences (Line 15, Line 21, Line 28, Line 35). Please annotate each block with a language (e.g., bash/text).
🛠 Suggested markdown fix
-```
+```bash
defradb client collection delete --name <name> [flags]- +bash
delete a collection by name:
defradb client collection delete --name Users
-```
+```text
-h, --help help for delete
-n, --name string Collection name to delete
- +text
--collection-id string Collection ID
--get-inactive Get inactive collections as well as active
-i, --identity string Hex formatted private key used to authenticate with ACP
...
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/website/references/cli/defradb_client_collection_delete.md` around lines
15 - 56, The fenced code blocks on this page (the command usage block "defradb
client collection delete --name <name> [flags]", the Examples block showing
"defradb client collection delete --name Users", the short Options block listing
"-h, --help" and "-n, --name", and the larger "Options inherited from parent
commands" block) lack language identifiers and trigger MD040; add appropriate
languages to each fence (e.g., use ```bash for the command and example blocks
and ```text for the options blocks) so every triple-backtick block specifies a
language.
| "responses": { | ||
| "200": { | ||
| "$ref": "#/components/responses/success" | ||
| }, | ||
| "400": { | ||
| "$ref": "#/components/responses/error" | ||
| }, | ||
| "default": { | ||
| "description": "" | ||
| } |
There was a problem hiding this comment.
Document 404 for DELETE /collections responses.
DeleteCollection can fail when the target collection does not exist, but the operation currently omits 404 from documented responses.
📝 Suggested OpenAPI update
"responses": {
"200": {
"$ref": "#/components/responses/success"
},
"400": {
"$ref": "#/components/responses/error"
+ },
+ "404": {
+ "$ref": "#/components/responses/error"
},
"default": {
"description": ""
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "responses": { | |
| "200": { | |
| "$ref": "#/components/responses/success" | |
| }, | |
| "400": { | |
| "$ref": "#/components/responses/error" | |
| }, | |
| "default": { | |
| "description": "" | |
| } | |
| "responses": { | |
| "200": { | |
| "$ref": "#/components/responses/success" | |
| }, | |
| "400": { | |
| "$ref": "#/components/responses/error" | |
| }, | |
| "404": { | |
| "$ref": "#/components/responses/error" | |
| }, | |
| "default": { | |
| "description": "" | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/website/references/http/openapi.json` around lines 1108 - 1117, The
OpenAPI spec for the DeleteCollection operation (DELETE /collections) is missing
a 404 response; update the operation's "responses" block for DeleteCollection to
include a "404" entry (e.g., "404": { "$ref": "#/components/responses/not_found"
} or inline {"description":"Collection not found","content":{...}}) so that
attempts to delete a non-existent collection are properly documented; locate the
DeleteCollection operation in openapi.json and add the 404 response reference or
define a new components/responses/not_found and reference it.
| deleteCollection.Responses = openapi3.NewResponses() | ||
| deleteCollection.Responses.Set("200", successResponse) | ||
| deleteCollection.Responses.Set("400", errorResponse) | ||
|
|
There was a problem hiding this comment.
OpenAPI operation should declare 401 (and 404) for delete.
This operation currently documents only 200/400, but the handler returns mapped domain errors via httpStatusFromError(err), which includes unauthenticated/missing-identity and not-found paths.
📚 Suggested response map update in operation builder
deleteCollection.Responses = openapi3.NewResponses()
deleteCollection.Responses.Set("200", successResponse)
deleteCollection.Responses.Set("400", errorResponse)
+ deleteCollection.Responses.Set("401", errorResponse)
+ deleteCollection.Responses.Set("404", errorResponse)Based on learnings: When handling client.ErrNotAuthorizedToPerformOperation in the defradb repository, treat it as an unauthenticated/missing-identity condition (not insufficient permissions) and map it to HTTP 401 Unauthorized.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deleteCollection.Responses = openapi3.NewResponses() | |
| deleteCollection.Responses.Set("200", successResponse) | |
| deleteCollection.Responses.Set("400", errorResponse) | |
| deleteCollection.Responses = openapi3.NewResponses() | |
| deleteCollection.Responses.Set("200", successResponse) | |
| deleteCollection.Responses.Set("400", errorResponse) | |
| deleteCollection.Responses.Set("401", errorResponse) | |
| deleteCollection.Responses.Set("404", errorResponse) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@http/handler_store.go` around lines 840 - 843, The OpenAPI spec for the
deleteCollection operation is missing 401 and 404 responses; update the
responses map on the deleteCollection operation (where
deleteCollection.Responses is set) to include entries for "401" and "404" (using
the same errorResponse schema or distinct responses as appropriate) so the
documented responses match the handler's httpStatusFromError mappings; also
ensure any logic that converts client.ErrNotAuthorizedToPerformOperation to a
401 (not 403) is reflected in the spec.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #4688 +/- ##
=========================================
Coverage 76.29% 76.29%
=========================================
Files 535 537 +2
Lines 43548 43696 +148
=========================================
+ Hits 33222 33335 +113
- Misses 7773 7797 +24
- Partials 2553 2564 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
shahzadlone
force-pushed
the
lone/feat/syntax-sugar-collection-delete
branch
from
8b0c800 to
4835faa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/website/references/cli/defradb_client_collection_delete.md (1)
15-56:⚠️ Potential issue | 🟡 MinorThe missing language identifiers have already been flagged.
This issue was already identified in a previous review comment with a comprehensive fix. The fenced code blocks at lines 15, 21, 28, and 35 still lack language identifiers as required by MD040.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/website/references/cli/defradb_client_collection_delete.md` around lines 15 - 56, The Markdown file contains fenced code blocks without language identifiers (MD040) — update each fenced block shown under the top command snippet, the "Examples" section, and the two "Options" sections so they include a language tag (e.g., ```bash or ```text) to satisfy the linter; specifically modify the three or four triple-backtick blocks that contain the command usage, example "delete a collection by name" and the option lists so each opening ``` becomes ```bash or ```text consistently.
🧹 Nitpick comments (1)
http/handler_store.go (1)
833-842: OpenAPI spec should document 401 and 404 responses.The handler uses
httpStatusFromError(err)which can return 401 (forErrNotAuthorizedToPerformOperation) and 404 (forErrCollectionNotFound). These should be documented in the OpenAPI spec for accurate API documentation.📚 Suggested response map update
deleteCollection.Responses = openapi3.NewResponses() deleteCollection.Responses.Set("200", successResponse) deleteCollection.Responses.Set("400", errorResponse) + deleteCollection.Responses.Set("401", errorResponse) + deleteCollection.Responses.Set("404", errorResponse)Based on learnings: When handling
client.ErrNotAuthorizedToPerformOperationin the defradb repository, treat it as an unauthenticated/missing-identity condition and map it to HTTP 401 Unauthorized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@http/handler_store.go` around lines 833 - 842, The OpenAPI operation "delete_collection" (deleteCollection) is missing documented 401 and 404 responses that can be returned by httpStatusFromError (e.g., ErrNotAuthorizedToPerformOperation -> 401 and ErrCollectionNotFound -> 404). Update deleteCollection.Responses to Set("401", errorResponse) and Set("404", errorResponse) (or create distinct unauthorizedResponse/notFoundResponse if you prefer clearer schemas) so the spec reflects those error states; keep the existing 200 and 400 entries (successResponse and errorResponse) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/acp/nac/relation_admin/delete_collection_test.go`:
- Around line 53-57: The CLI-only test's expected permission is wrong: locate
the DeleteCollection test case (action.DeleteCollection) in
delete_collection_test.go and change its ExpectedError from expecting
NodeGetCollectionPerm to expect NodePatchCollectionPerm to match the store
implementation (which checks NodePatchCollectionPerm for delete).
---
Duplicate comments:
In `@docs/website/references/cli/defradb_client_collection_delete.md`:
- Around line 15-56: The Markdown file contains fenced code blocks without
language identifiers (MD040) — update each fenced block shown under the top
command snippet, the "Examples" section, and the two "Options" sections so they
include a language tag (e.g., ```bash or ```text) to satisfy the linter;
specifically modify the three or four triple-backtick blocks that contain the
command usage, example "delete a collection by name" and the option lists so
each opening ``` becomes ```bash or ```text consistently.
---
Nitpick comments:
In `@http/handler_store.go`:
- Around line 833-842: The OpenAPI operation "delete_collection"
(deleteCollection) is missing documented 401 and 404 responses that can be
returned by httpStatusFromError (e.g., ErrNotAuthorizedToPerformOperation -> 401
and ErrCollectionNotFound -> 404). Update deleteCollection.Responses to
Set("401", errorResponse) and Set("404", errorResponse) (or create distinct
unauthorizedResponse/notFoundResponse if you prefer clearer schemas) so the spec
reflects those error states; keep the existing 200 and 400 entries
(successResponse and errorResponse) intact.
🪄 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
Run ID: 781e68cf-72d5-4720-b6c5-72fc21984e3b
📒 Files selected for processing (31)
cbindings/delete_collection.gocbindings/wrapper.gocbindings/wrapper_tx.gocli/cli.gocli/collection.gocli/collection_delete.goclient/db.goclient/errors.goclient/mocks/txn.goclient/options/store.godocs/website/references/cli/defradb_client_collection.mddocs/website/references/cli/defradb_client_collection_delete.mddocs/website/references/http/openapi.jsonhttp/client.gohttp/client_tx.gohttp/handler_store.gointernal/db/store.gointernal/db/txn.gojs/client.gojs/client_store.gojs/client_tx.gotests/action/delete_collection.gotests/clients/cli/wrapper.gotests/clients/cli/wrapper_tx.gotests/clients/http/wrapper.gotests/clients/http/wrapper_tx.gotests/clients/js/wrapper.gotests/clients/js/wrapper_tx.gotests/integration/acp/nac/delete_collection_test.gotests/integration/acp/nac/relation_admin/delete_collection_test.gotests/integration/collection/delete/delete_collection_test.go
✅ Files skipped from review due to trivial changes (3)
- cli/collection.go
- docs/website/references/http/openapi.json
- tests/action/delete_collection.go
🚧 Files skipped from review as they are similar to previous changes (19)
- docs/website/references/cli/defradb_client_collection.md
- cli/cli.go
- tests/clients/cli/wrapper_tx.go
- tests/clients/js/wrapper.go
- tests/clients/js/wrapper_tx.go
- js/client_store.go
- tests/clients/http/wrapper_tx.go
- http/client_tx.go
- internal/db/store.go
- http/client.go
- client/options/store.go
- cbindings/wrapper.go
- js/client_tx.go
- cli/collection_delete.go
- cbindings/delete_collection.go
- client/mocks/txn.go
- tests/integration/acp/nac/delete_collection_test.go
- js/client.go
- tests/integration/collection/delete/delete_collection_test.go
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 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:
client/errors.gocbindings/wrapper_tx.gointernal/db/txn.gotests/clients/cli/wrapper.goclient/db.gohttp/handler_store.gotests/clients/http/wrapper.gotests/integration/acp/nac/relation_admin/delete_collection_test.go
🪛 markdownlint-cli2 (0.22.0)
docs/website/references/cli/defradb_client_collection_delete.md
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (9)
docs/website/references/cli/defradb_client_collection_delete.md (1)
1-61: Well-structured CLI documentation.The documentation is clear and comprehensive. It properly explains the command's behavior (head-only deletion, empty collection requirement), provides usage examples, and documents all options. The structure follows standard CLI reference format with Synopsis, Examples, Options, and SEE ALSO sections.
client/errors.go (1)
101-101: LGTM!The new sentinel error follows the established pattern in this file and is appropriately placed alongside the similar
ErrIndexNameRequirederror.tests/clients/http/wrapper.go (1)
295-301: LGTM!The HTTP wrapper method follows the established delegation pattern used throughout this file.
cbindings/wrapper_tx.go (1)
136-143: LGTM!The transaction wrapper correctly sets the transaction context via
datastore.CtxSetFromClientTxnbefore delegating to the underlying wrapper, consistent with other methods in this file.internal/db/txn.go (1)
226-233: LGTM!The transaction method correctly initializes the context via
InitContextbefore delegating to the database, consistent with other store operations in this file.tests/clients/cli/wrapper.go (1)
426-438: LGTM!The CLI wrapper correctly builds the command arguments and handles identity options, following the established pattern used by other collection operations in this file.
client/db.go (1)
214-224: LGTM!The interface method is well-documented with clear semantics: only the head version is deleted per call, and the operation errors if documents exist. This provides good guidance for API consumers.
http/handler_store.go (1)
145-172: LGTM!The handler correctly validates the required
nameparameter, properly handles both explicit transactions and direct DB operations, and useshttpStatusFromErrorfor consistent error response mapping.tests/integration/acp/nac/relation_admin/delete_collection_test.go (1)
24-76: LGTM for test structure and coverage.The test correctly validates the NAC admin relationship flow: initial denial without relationship, granting admin access, then successful deletion. The restart-with-NAC pattern and subsequent actions are well-structured.
| &action.DeleteCollection{ | ||
| Identity: testUtils.ClientIdentity(2), | ||
| Name: "Users", | ||
| ExpectedError: testUtils.FormatExpectedErrorWithPermission(acpTypes.NodePatchCollectionPerm), | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for how DeleteCollection permission is checked across client types
# Expected: Find the permission check logic to understand if the discrepancy is intentional
echo "=== Searching for DeleteCollection permission checks ==="
rg -n -C5 'DeleteCollection' --type=go -g '!*_test.go' | head -100
echo ""
echo "=== Searching for NodePatchCollectionPerm usage ==="
rg -n 'NodePatchCollectionPerm' --type=go | head -50
echo ""
echo "=== Searching for NodeGetCollectionPerm usage ==="
rg -n 'NodeGetCollectionPerm' --type=go | head -50Repository: sourcenetwork/defradb
Length of output: 16579
🏁 Script executed:
#!/bin/bash
# Check which permission is actually checked in DeleteCollection implementation
echo "=== DeleteCollection implementation in store.go ==="
rg -n -B5 -A10 'func.*DeleteCollection' internal/db/store.go
echo ""
echo "=== Check lines around 184 in store.go for DeleteCollection ==="
sed -n '175,195p' internal/db/store.goRepository: sourcenetwork/defradb
Length of output: 1088
🏁 Script executed:
#!/bin/bash
# Read the delete_collection_test.go file to see the test expectations
echo "=== Test file content around lines 40-130 ==="
sed -n '40,130p' tests/integration/acp/nac/relation_admin/delete_collection_test.goRepository: sourcenetwork/defradb
Length of output: 2868
CLI test expects incorrect permission for DeleteCollection operation.
Line 107 in the CLI-only test expects NodeGetCollectionPerm, but the implementation in internal/db/store.go:258 checks NodePatchCollectionPerm for the DeleteCollection operation. This mismatches the non-CLI test at line 56, which correctly expects NodePatchCollectionPerm.
The TODO comment acknowledges this pending work: "Investigate and test this behavior across all client types when implementing granular NAC permissions" (issue #4383). Update the expected permission at line 107 to match the actual implementation: NodePatchCollectionPerm.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/acp/nac/relation_admin/delete_collection_test.go` around
lines 53 - 57, The CLI-only test's expected permission is wrong: locate the
DeleteCollection test case (action.DeleteCollection) in
delete_collection_test.go and change its ExpectedError from expecting
NodeGetCollectionPerm to expect NodePatchCollectionPerm to match the store
implementation (which checks NodePatchCollectionPerm for delete).
There was a problem hiding this comment.
I think this needs a few tweaks before it can be merged.
thought: The description is incorrect, Implement collection delete sugar syntax command - the current implementation in the PR is not syntax sugar, it happens to call the same internal function, but it is not syntax sugar - this is apparent in the skipping of the validation, and any other logic current and future, that PatchCollection performs. If we want it to be syntax sugar, then it should call PatchCollection.
|
|
||
| defer txn.Discard() | ||
|
|
||
| col, err := db.getCollectionByName(ctx, name) |
There was a problem hiding this comment.
todo: This seems out of place, you are including business logic in the boilerplate layer. This hides it, and makes it harder to call internally.
Please pull lines 269-282 out to a deleteCollection function
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
todo: You do not appear to be performing any validation on the changes, please make sure validateCollectionChanges is called.
Please also make sure you have tests covering stuff that the validation might flag, such as deleting collections that are referenced by other collections.
| `, | ||
| }, | ||
| &action.DeleteCollection{ | ||
| Name: "Users", |
There was a problem hiding this comment.
todo: Related to the validation todo - this action looks like it will result in an invalid GQL schema, you should include a test that verifies that the resultant GQL schema is actually valid, GetCollections is not sufficient for this.
2 participants
Relevant issue(s)
Resolves #4678
Description
collection deletesugar syntax command