Adds Get product handler - #46
Conversation
… and tests
- Implement `GetProductById` handler for fetching a product by ID.
- Implement `GetProductsByPlatform` handler to fetch products by platform ID.
- Add routes for `/api/products/{id}` and `/api/platforms/{platform_id}/products` to the router.
- Include unit tests for both handlers, covering success and error scenarios.
… requests with examples and documentation.
jlpdeveloper
added
enhancement
📝 WalkthroughWalkthroughImplements two HTTP GET handlers for retrieving products: one listing products by platform ID and another fetching a product by ID. Handlers include request path parsing, database calls with 10-second context timeouts, error mapping (404 for not found, 400 for invalid params, 500 for database failures), and JSON response serialization. Routes are registered and comprehensive tests are added. ChangesProduct GET Handlers
Sequence DiagramsequenceDiagram
actor Client
participant Handler as HTTP Handler
participant Parser as Request Parser
participant DB as Database Query
participant Encoder as JSON Encoder
rect rgba(100, 150, 200, 0.5)
note over Client,Encoder: GetProductsByPlatform Flow
Client->>Handler: GET /api/platforms/:platform_id/products
Handler->>Parser: Extract & parse platform_id
alt Invalid platform_id
Parser-->>Handler: 400 Error
Handler-->>Client: 400 Bad Request
else Valid platform_id
Handler->>DB: GetProductsByPlatform(platform_id) + 10s timeout
alt DB Success
DB-->>Handler: Product slice (or nil)
Handler->>Encoder: Convert nil→empty slice, set Content-Type
Encoder->>Encoder: JSON encode
Encoder-->>Client: 200 + JSON array
else DB Error
DB-->>Handler: Error
Handler-->>Client: 500 Internal Error
end
end
end
rect rgba(150, 100, 200, 0.5)
note over Client,Encoder: GetProductById Flow
Client->>Handler: GET /api/products/:id
Handler->>Parser: Extract & parse id
alt Invalid id
Parser-->>Handler: 400 Error
Handler-->>Client: 400 Bad Request
else Valid id
Handler->>DB: GetProductById(id) + 10s timeout
alt pgx.ErrNoRows
DB-->>Handler: Not Found Error
Handler-->>Client: 404 Not Found
else DB Success
DB-->>Handler: Product
Handler->>Encoder: Set Content-Type
Encoder->>Encoder: JSON encode
Encoder-->>Client: 200 + JSON product
else Other DB Error
DB-->>Handler: Error
Handler-->>Client: 500 Internal Error
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/router.go (1)
59-67:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd integration tests to verify routing behavior and refactor to consolidate platform-related endpoints.
The handler
GetProductsByPlatformexists and has unit tests, but the route at line 66 (r.Get("/api/platforms/{platform_id}/products", ...)) is not tested through the router. The unit test manually constructs an HTTP request and invokes the handler directly, bypassing chi's routing logic entirely. This means the actual routing behavior—specifically whether chi's radix tree correctly prioritizes the root-level route over the/api/platforms/*wildcard created by the Mount inregisterPlatformCallHandler—is unverified.Without integration tests, it is unclear whether a GET request to
/api/platforms/1/productscorrectly reachesGetProductsByPlatformor is intercepted by the platform sub-router.Additionally, the current architecture splits a logically grouped endpoint (
/api/platforms/{platform_id}/products) across two separate handler functions in different packages, each with separate database querier dependencies. This can be improved by consolidating platform-related endpoints.Recommended actions:
- Add an integration test that validates the full router correctly handles GET
/api/platforms/{platform_id}/products- Refactor to move
GetProductsByPlatforminto the platform handler or create a shared sub-router pattern to group platform-related concerns logically🤖 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 `@router/router.go` around lines 59 - 67, Add an integration test that boots the real chi router (call registerProductCallHandler and registerPlatformCallHandler as in production) and issues an HTTP GET to "/api/platforms/1/products" asserting the response comes from the GetProductsByPlatform handler (verify status/body) to ensure chi routing prefers the root-level route over the platform sub-router; then refactor routing by moving GetProductsByPlatform into the platform handler (or create a shared sub-router under "/api/platforms" that registers the products route) and update dependencies so the platform handler (or sub-router) uses a single querier instead of split product/platform queriers, ensuring all platform-related endpoints are consolidated in registerPlatformCallHandler and removing the duplicate r.Get("/api/platforms/{platform_id}/products", ...) from registerProductCallHandler.
🧹 Nitpick comments (1)
api/product/product_test.go (1)
234-398: ⚡ Quick winMissing zero/negative boundary tests for both new handlers.
TestDeleteProductexplicitly covers"0"and"-1"as invalid IDs (lines 164–174), which impliesinternal.GetIntFromRequestPathrejects them.TestGetProductByIdandTestGetProductsByPlatformonly test a non-numeric string ("abc") for the invalid-input case, leaving the zero and negative boundaries untested.As per the coding guideline to assess sufficient code coverage, add the missing cases for consistency:
🧪 Missing test cases to add
// Inside TestGetProductById tests slice: +{ + name: "Invalid ID (Zero)", + id: "0", + mockSetup: func(m *mockProductQuerier) {}, + expectedStatus: http.StatusBadRequest, +}, +{ + name: "Invalid ID (Negative)", + id: "-1", + mockSetup: func(m *mockProductQuerier) {}, + expectedStatus: http.StatusBadRequest, +}, // Inside TestGetProductsByPlatform tests slice: +{ + name: "Invalid Platform ID (Zero)", + platformID: "0", + mockSetup: func(m *mockProductQuerier) {}, + expectedStatus: http.StatusBadRequest, +}, +{ + name: "Invalid Platform ID (Negative)", + platformID: "-1", + mockSetup: func(m *mockProductQuerier) {}, + expectedStatus: http.StatusBadRequest, +},🤖 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 `@api/product/product_test.go` around lines 234 - 398, Add zero and negative-ID invalid-input test cases to both TestGetProductsByPlatform and TestGetProductById: insert table entries with platformID/id values "0" and "-1" (matching the pattern used in TestDeleteProduct) that call no mockSetup and expect http.StatusBadRequest; these should exercise the same internal.GetIntFromRequestPath validation and ensure handlers GetProductsByPlatform and GetProductById return 400 for zero/negative IDs.
🤖 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.
Outside diff comments:
In `@router/router.go`:
- Around line 59-67: Add an integration test that boots the real chi router
(call registerProductCallHandler and registerPlatformCallHandler as in
production) and issues an HTTP GET to "/api/platforms/1/products" asserting the
response comes from the GetProductsByPlatform handler (verify status/body) to
ensure chi routing prefers the root-level route over the platform sub-router;
then refactor routing by moving GetProductsByPlatform into the platform handler
(or create a shared sub-router under "/api/platforms" that registers the
products route) and update dependencies so the platform handler (or sub-router)
uses a single querier instead of split product/platform queriers, ensuring all
platform-related endpoints are consolidated in registerPlatformCallHandler and
removing the duplicate r.Get("/api/platforms/{platform_id}/products", ...) from
registerProductCallHandler.
---
Nitpick comments:
In `@api/product/product_test.go`:
- Around line 234-398: Add zero and negative-ID invalid-input test cases to both
TestGetProductsByPlatform and TestGetProductById: insert table entries with
platformID/id values "0" and "-1" (matching the pattern used in
TestDeleteProduct) that call no mockSetup and expect http.StatusBadRequest;
these should exercise the same internal.GetIntFromRequestPath validation and
ensure handlers GetProductsByPlatform and GetProductById return 400 for
zero/negative IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 742fec95-8f8b-41b4-ac52-dfe47c9c7616
📒 Files selected for processing (5)
_bruno/Product/Get Product By ID.yml_bruno/Product/Get Products By Platform.ymlapi/product/get.goapi/product/product_test.gorouter/router.go
1 participant
Description
Code Rabbit Summary
Summary by CodeRabbit
New Features
Tests
Fixes
Closes #24
Post Deployment Tasks?