Migrate db logic to service layer - #69
Merged
Merged
Conversation
… subdirectories
…oved clarity and align interface usage in tests
…ct and adjust SQLC query for `RETURNING`
…latform` logic to service layer
…andlers and add comprehensive test coverage for service layer
…gic to service layer with extensive test coverage
jlpdeveloper
added
enhancement
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request refactors the platform, product, and flow packages by migrating sqlc-generated database code to dedicated ChangesDatabase Service Layer Refactoring and Package Reorganization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/platform/handler_test.go (1)
102-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCreate success test should assert the JSON payload, not only status.
The endpoint contract changed to return the created platform body on 201, but this test only checks status. Add response body assertions for the success case.
As per coding guidelines, "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request".
🤖 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/platform/handler_test.go` around lines 102 - 126, The CreatePlatform test only checks HTTP status but must also assert the JSON response body on success (201); update the table-driven test to include an expected response platform value and modify the mockPlatformService.createPlatform to return that db.Platform for the success case, then after h.CreatePlatform(rr, req) when tt.expectedStatus == http.StatusCreated unmarshal rr.Body into a db.Platform and compare its fields to the expected platform (or deep-equal the struct) to ensure the handler actually writes the created platform JSON; focus changes around platformHandler.CreatePlatform test setup and the mock createPlatform return values.internal/product/handler_test.go (1)
392-396:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert
GetProductByIdresponse payload fields in the success path.The success test currently validates only status and content type. Since the handler now returns
db.Product, add body assertions (e.g.,ID,Name) to lock the contract.Suggested assertion update
if tt.expectedStatus == http.StatusOK { if rr.Header().Get("Content-Type") != "application/json" { t.Errorf("expected Content-Type application/json, got %v", rr.Header().Get("Content-Type")) } + var got db.Product + if err := json.NewDecoder(rr.Body).Decode(&got); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + if got.ID != 1 || got.Name != "Test Product" { + t.Errorf("unexpected product payload: %+v", got) + } }As per coding guidelines,
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request".🤖 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/product/handler_test.go` around lines 392 - 396, The success branch in the test currently only checks status and Content-Type; update the success path (inside the if tt.expectedStatus == http.StatusOK block in handler_test.go) to decode rr.Body into a db.Product (or the concrete product struct used by the handler, e.g., db.Product) using json.Decoder and assert critical fields like ID and Name (and any other contract fields like Price or SKU) match the expected values from the test case (e.g., tt.expectedProduct or the fixture used to set up the mock). Ensure decoding errors are handled with t.Fatalf/t.Errorf and only run these assertions for the HTTP 200 case so the contract is locked for GetProductById.
🤖 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 `@internal/platform/service.go`:
- Around line 47-55: The UpdatePlatform (and similarly DeletePlatform) service
should detect pgx.ErrNoRows returned by p.db.UpdatePlatform (and
p.db.DeletePlatform) and map it to an internal.NotFound error instead of
propagating a 500: in postgresService.UpdatePlatform (and DeletePlatform) check
if errors.Is(err, pgx.ErrNoRows) and return internal.NewNotFoundError(id,
"Platform"); otherwise return the original err; keep the existing use of
req.ToParams(id) for UpdatePlatform and ensure you import/errors.Is and pgx
types as needed.
In `@internal/product/service_test.go`:
- Around line 68-186: The tests currently only cover success and NotFound
branches; add additional subtests for GetProductsByPlatform, GetProductById,
UpdateProduct, and DeleteProduct that exercise the non-not-found DB error path
by having the mockQuerier methods (getProductsByPlatform, getProductById,
updateProduct, deleteProduct) return a non-pgx.ErrNoRows error (e.g., a sentinel
errors.New("db error")) and assert the service methods (GetProductsByPlatform,
GetProductById, UpdateProduct, DeleteProduct) propagate that error (use
errors.Is or direct comparison) so the error-pass-through behavior is verified.
In `@justfile`:
- Around line 18-22: The justfile currently defines a Windows-only recipe for
test-cover under the [windows] attribute, removing the default Unix-like recipe
and breaking cross-platform use; add back a default test-cover recipe (name:
test-cover) that runs the equivalent commands for Unix shells (e.g., use a shell
pipeline to list packages excluding /db, run go test with -covermode=count
-coverprofile=coverage.out, then run go tool cover -func=coverage.out), and keep
the existing [windows] test-cover override for Windows-specific PowerShell
syntax so both platforms can run just test-cover.
---
Outside diff comments:
In `@internal/platform/handler_test.go`:
- Around line 102-126: The CreatePlatform test only checks HTTP status but must
also assert the JSON response body on success (201); update the table-driven
test to include an expected response platform value and modify the
mockPlatformService.createPlatform to return that db.Platform for the success
case, then after h.CreatePlatform(rr, req) when tt.expectedStatus ==
http.StatusCreated unmarshal rr.Body into a db.Platform and compare its fields
to the expected platform (or deep-equal the struct) to ensure the handler
actually writes the created platform JSON; focus changes around
platformHandler.CreatePlatform test setup and the mock createPlatform return
values.
In `@internal/product/handler_test.go`:
- Around line 392-396: The success branch in the test currently only checks
status and Content-Type; update the success path (inside the if
tt.expectedStatus == http.StatusOK block in handler_test.go) to decode rr.Body
into a db.Product (or the concrete product struct used by the handler, e.g.,
db.Product) using json.Decoder and assert critical fields like ID and Name (and
any other contract fields like Price or SKU) match the expected values from the
test case (e.g., tt.expectedProduct or the fixture used to set up the mock).
Ensure decoding errors are handled with t.Fatalf/t.Errorf and only run these
assertions for the HTTP 200 case so the contract is locked for GetProductById.
🪄 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: afa68d24-5165-4f5b-b971-9fd85024f460
📒 Files selected for processing (29)
.gitignoreinternal/flow/handler.gointernal/flow/handler_test.gointernal/flow/interface.gointernal/flow/service.gointernal/flow/service_test.gointernal/platform/db/db.gen.gointernal/platform/db/models.gen.gointernal/platform/db/platforms.sql.gen.gointernal/platform/db/querier.gen.gointernal/platform/handler.gointernal/platform/handler_test.gointernal/platform/interface.gointernal/platform/platforms.sqlinternal/platform/request.gointernal/platform/service.gointernal/platform/service_test.gointernal/product/db/db.gen.gointernal/product/db/models.gen.gointernal/product/db/products.sql.gen.gointernal/product/db/querier.gen.gointernal/product/handler.gointernal/product/handler_test.gointernal/product/interface.gointernal/product/request.gointernal/product/service.gointernal/product/service_test.gojustfilesqlc.yml
💤 Files with no reviewable changes (3)
- internal/product/interface.go
- internal/platform/interface.go
- internal/flow/interface.go
…Service` and `platformService`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Code Rabbit Summary
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Build & Testing
Fixes
Closes #66
Post Deployment Tasks?