Conversation
Summary by CodeRabbit
WalkthroughAdds new Makefile targets for auditing, formatting, linting, tidying, testing, coverage, and benchmarking. Modifies repository delete methods to accept an optional force-delete flag enabling hard deletes. Updates tests to verify soft vs hard delete behavior and seeds data via batch creation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Caller
participant R as Repository[M]
participant DB as DB (ORM)
rect rgb(245,245,255)
note over C,R: Soft delete (default)
C->>R: DeleteByID(id)
R->>DB: Find (scoped)
DB-->>R: Record
R->>DB: Delete (scoped/soft)
DB-->>R: OK
R-->>C: nil/error
end
rect rgb(245,255,245)
note over C,R: Hard delete (force)
C->>R: DeleteByID(id, true)
R->>DB: Find WithDeleted
DB-->>R: Record
R->>DB: Unscoped Delete (hard)
DB-->>R: OK
R-->>C: nil/error
end
sequenceDiagram
autonumber
actor C as Caller
participant R as Repository[M]
participant DB as DB (ORM)
rect rgb(255,250,240)
note over C,R: Bulk delete
C->>R: DeleteMany(where) / DeleteMany(where, true)
alt soft (no force)
R->>DB: Delete matching (scoped/soft)
else hard (force)
R->>DB: Unscoped Delete matching
end
DB-->>R: OK
R-->>C: nil/error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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
🧹 Nitpick comments (7)
Makefile (3)
29-31: Use a clearer -run filter for benchmarks
-run=^Benchmark_$is a test filter and is a bit confusing. Prefer-run=^$to skip tests explicitly.benchmark: - go test ./... -benchmem -bench=. -run=^Benchmark_$ + go test ./... -benchmem -bench=. -run=^$
1-5: Pin tool versions for reproducibility
@latestand relying on host-installed linters make CI non-deterministic. Pin versions or expose variables.Minimal changes:
audit: go mod verify go vet ./... - go run golang.org/x/vuln/cmd/govulncheck@latest ./... + go run golang.org/x/vuln/cmd/govulncheck@v1.0.0 ./...Add near the top (outside this hunk) to make tools configurable:
# Tool versions (override in CI) GOVULNCHECK_VERSION ?= v1.0.0 GOLANGCI_LINT ?= golangci-lint # Then use: # go run golang.org/x/vuln/cmd/govulncheck@$(GOVULNCHECK_VERSION) ./... # $(GOLANGCI_LINT) run
29-31: Add conventional phony targets "all" and "clean"Also addresses checkmake warnings and improves DX.
+.PHONY: all clean +all: tidy format lint test + +clean: + go clean -testcache + rm -rf coveragemutation.go (2)
53-61: Simplify the force-delete flag handlingMinor readability tweak: compute the flag once and reuse.
-func (repo *Repository[M]) DeleteOne(where interface{}, isForceDelete ...bool) error { - withDeleted := false - if len(isForceDelete) > 0 && isForceDelete[0] { - withDeleted = true - } +func (repo *Repository[M]) DeleteOne(where interface{}, isForceDelete ...bool) error { + force := len(isForceDelete) > 0 && isForceDelete[0] - record, err := repo.FindOne(where, FindOneOptions{ - WithDeleted: withDeleted, + record, err := repo.FindOne(where, FindOneOptions{ + WithDeleted: force, }) @@ - tx := repo.DB - if withDeleted { + tx := repo.DB + if force { tx = tx.Unscoped() }Also applies to: 68-71
83-100: Global DeleteMany(nil, true) is powerful—consider guardrailsBypassing GORM’s global-delete protection with
Where("1 = 1")enables full-table soft/hard deletes. This is intentional per tests, but risky for consumers.Options:
- Require an explicit sentinel for “all rows” instead of
nil.- Gate “delete all” behind config/env (e.g.,
AllowGlobalDelete), or log a prominent warning.- Document prominently in README with examples.
mutation_test.go (2)
178-196: Minor var shadowing and consistency nitsInside the closure you re-declare
err(err := repo.DeleteMany(...)) after using it above. Not harmful but can be avoided; also preferanyoverinterface{}for consistency with the library.- err := repo.DeleteMany(map[string]interface{}{"name": "abc"}) + err = repo.DeleteMany(map[string]any{"name": "abc"})
196-217: Prefer NoError over Nil for error assertionsSlightly clearer intent.
- require.Nil(t, err) + require.NoError(t, err) @@ - require.Nil(t, err) + require.NoError(t, err) @@ - require.Nil(t, err) + require.NoError(t, err) @@ - require.Nil(t, err) + require.NoError(t, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefile(1 hunks)mutation.go(1 hunks)mutation_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
mutation_test.go (3)
query.go (2)
FindOneOptions(9-15)FindOptions(17-26)model.go (1)
Model(11-16)repository.go (1)
Repository(20-22)
mutation.go (2)
repository.go (1)
Repository(20-22)query.go (1)
FindOneOptions(9-15)
🪛 checkmake (0.2.2)
Makefile
[warning] 29-29: Missing required phony target "all"
(minphony)
[warning] 29-29: Missing required phony target "clean"
(minphony)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
Makefile (1)
11-13: Make linter invocation robustIf
golangci-lintisn’t on PATH, this will fail locally/CI. Either document the prerequisite or use a pinned container/binary via a configurable variable as suggested above.Would you like a
toolstarget to install/pin linters for CI?mutation.go (3)
53-61: Confirm policy for DeleteOne(nil, …)
where == nilwould delete “the first” matching record (sinceFindOne(nil, …)is allowed elsewhere). That’s a dangerous footgun.Do we want to disallow
DeleteOne(nil, …)(returning an error), or require an explicit sentinel (e.g.,sqlorm.All())?
68-73: Hard delete via Unscoped is correctThe Unscoped path is the right way to perform hard deletes in GORM.
79-81: Forwarding the variadic flag looks good
DeleteByIDcleanly delegates toDeleteOnewith the variadic flag.mutation_test.go (1)
158-171: Nice verification of soft vs hard delete semanticsAsserting visibility with
WithDeleted: truebefore and afterDeleteByID(..., true)is solid.
| .PHONY: coverage | ||
| coverage: | ||
| go clean -testcache | ||
| go test -v ./... -covermode=count -coverpkg=./... -coverprofile coverage/coverage.out | ||
| go tool cover -html coverage/coverage.out -o coverage/coverage.html No newline at end of file | ||
| go tool cover -html coverage/coverage.out -o coverage/coverage.html | ||
|
|
There was a problem hiding this comment.
Ensure coverage directory exists before writing outputs
Without creating the directory first, go test -coverprofile coverage/coverage.out and go tool cover ... -o coverage/coverage.html can fail with “no such file or directory”.
Apply:
.PHONY: coverage
coverage:
+ mkdir -p coverage
go clean -testcache
go test -v ./... -covermode=count -coverpkg=./... -coverprofile coverage/coverage.out
go tool cover -html coverage/coverage.out -o coverage/coverage.html📝 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.
| .PHONY: coverage | |
| coverage: | |
| go clean -testcache | |
| go test -v ./... -covermode=count -coverpkg=./... -coverprofile coverage/coverage.out | |
| go tool cover -html coverage/coverage.out -o coverage/coverage.html | |
| \ No newline at end of file | |
| go tool cover -html coverage/coverage.out -o coverage/coverage.html | |
| .PHONY: coverage | |
| coverage: | |
| mkdir -p coverage | |
| go clean -testcache | |
| go test -v ./... -covermode=count -coverpkg=./... -coverprofile coverage/coverage.out | |
| go tool cover -html coverage/coverage.out -o coverage/coverage.html |
🤖 Prompt for AI Agents
In Makefile around lines 23 to 28, the target writes coverage files into the
coverage/ directory without ensuring it exists; update the target to create the
directory first (e.g., run mkdir -p coverage) before running go test and go tool
cover so the coverage output and HTML file can be written successfully.
2 participants
No description provided.