Refactors router - #96
Conversation
…up, and add unit tests for route registration
… and add unit tests for flow route registration.
…n to dedicated file, and update unit tests accordingly.
…ng, extract route registration to a dedicated file, and add unit tests for route registration.
…t, flow, and capability modules into dedicated files. Simplify `SetupRouter` and update unit tests accordingly.
📝 WalkthroughWalkthroughHandlers and services in capability, flow, platform, and product are rewired to unexported constructors and concrete handler types. Each package now exposes ChangesPackage-level RegisterRoutes refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
router/router_test.go (1)
25-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest only validates static route registration, not runtime dispatch.
TestSetupRouterwalks the tree withchi.Walkand asserts each expectedmethod routestring is present, which is good coverage for confirming each package'sRegisterRouteswired something into the mux. However, several expected entries are nested paths registered directly on the top-level router while a sibling packageMounts an overlapping prefix (see comment onrouter.golines 36-39) —chi.Walkwill list both entries regardless of which one actually wins at request time.To give confidence that route behavior is truly unchanged (a stated PR acceptance criterion), consider adding a lightweight
httptest-based check for at least the routes that sit under another package's mounted prefix (e.g.,GET /platforms/{id}/products,GET /products/{id}/flows,GET /flows/{id}/capabilities) to confirm they reach the intended handler rather than a 404 from the mounted subrouter.As per path instructions, this file should be assessed for "sufficient code coverage for the changes associated in the pull request."
🧪 Example addition to strengthen coverage
func TestSetupRouter_NestedRoutesReachHandlers(t *testing.T) { r := SetupRouter(&mockDBTX{}) cases := []struct{ method, path string }{ {http.MethodGet, "/platforms/1/products"}, {http.MethodGet, "/products/1/flows"}, {http.MethodGet, "/flows/1/capabilities"}, } for _, c := range cases { req := httptest.NewRequest(c.method, c.path, nil) rec := httptest.NewRecorder() r.ServeHTTP(rec, req) if rec.Code == http.StatusNotFound { t.Errorf("%s %s: expected route to be reachable, got 404", c.method, c.path) } } }🤖 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_test.go` around lines 25 - 70, `TestSetupRouter` only checks that routes are registered via `chi.Walk`, but it does not verify that overlapping mounted prefixes still dispatch to the intended handlers at runtime. Keep the existing `SetupRouter` route coverage, and add a small `httptest`-based test that exercises the nested paths under the mounted prefix, using `SetupRouter`, `ServeHTTP`, and the affected routes like `/platforms/{id}/products`, `/products/{id}/flows`, and `/flows/{id}/capabilities`, to confirm they do not return 404.Source: Path instructions
internal/flow/router_test.go (1)
57-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise route dispatch, not just route presence.
chi.Walkconfirms patterns exist, but it won’t catch wiringGET /flows/{id}/pathto the wrong handler. The existingmockService.calledmap is ideal for asserting each route invokes the expected service method.As per path instructions,
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request".🧪 Suggested coverage extension
import ( "context" "net/http" + "net/http/httptest" "products/internal/flow/db" + "strings" "testing" @@ for _, route := range want { if !got[route] { t.Errorf("expected route %q to be registered", route) } } + + dispatchTests := []struct { + method string + path string + body string + wantCall string + }{ + {http.MethodPost, "/flows/1/steps", `{"current":"550e8400-e29b-41d4-a716-446655440000","next":"550e8400-e29b-41d4-a716-446655440000"}`, "CreateFlowStep"}, + {http.MethodGet, "/flows/1/steps", "", "GetFlowSteps"}, + {http.MethodGet, "/flows/1/path", "", "GetFlowPath"}, + {http.MethodGet, "/flows/1/", "", "GetFlowById"}, + {http.MethodPut, "/flows/1/", `{"name":"Updated Flow"}`, "UpdateFlow"}, + {http.MethodDelete, "/flows/1/", "", "DeleteFlow"}, + {http.MethodDelete, "/flow-steps/1", "", "DeleteFlowStep"}, + {http.MethodPost, "/products/1/flows", `{"name":"Test Flow"}`, "CreateFlow"}, + {http.MethodGet, "/products/1/flows", "", "GetFlowsByProduct"}, + } + + for _, tt := range dispatchTests { + t.Run(tt.method+" "+tt.path, func(t *testing.T) { + svc := newMockService() + r := chi.NewRouter() + registerRoutesWithHandler(r, newHandler(svc)) + + req := httptest.NewRequest(tt.method, tt.path, strings.NewReader(tt.body)) + rr := httptest.NewRecorder() + r.ServeHTTP(rr, req) + + if !svc.called[tt.wantCall] { + t.Fatalf("expected %s to be called", tt.wantCall) + } + }) + } }🤖 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/flow/router_test.go` around lines 57 - 90, The current TestRegisterRoutesWithHandler only checks that routes are registered, not that each route dispatches to the correct handler. Update the test to exercise actual requests against the router from registerRoutesWithHandler and assert the mockService.called map records the expected service method for each route, using newHandler and newMockService to verify wiring such as GET /flows/{id}/path and the other flow/product endpoints.Source: Path instructions
internal/product/router_test.go (1)
16-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSame route-set completeness gap as the platform router test.
Only presence of
wantroutes is checked; unexpected extra registrations ingotwould not be caught.♻️ Suggested addition
for _, route := range want { if !got[route] { t.Errorf("expected route %q to be registered", route) } } + + if len(got) != len(want) { + t.Errorf("expected %d routes, got %d: %v", len(want), len(got), got) + }As per path instructions, "Assess 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/router_test.go` around lines 16 - 38, The route coverage in router_test.go only verifies that the expected entries in chi.Walk appear, so extra unexpected registrations in got would still pass. Update the test around chi.Walk to assert the route set is exact by checking that every route in got is present in want and that the counts match, using the existing got and want collections in the test.Source: Path instructions
internal/platform/router_test.go (1)
16-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't catch unexpected extra route registrations.
The assertions only check that each wanted route is present in
got; they don't verifygothas no additional/unexpected routes, so an accidental duplicate or mis-registered route pattern would silently pass.♻️ Suggested addition
for _, route := range want { if !got[route] { t.Errorf("expected route %q to be registered", route) } } + + if len(got) != len(want) { + t.Errorf("expected %d routes, got %d: %v", len(want), len(got), got) + }As per path instructions, "Assess 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/router_test.go` around lines 16 - 38, The route registration test in router_test.go only checks that expected entries from chi.Walk are present, so extra or duplicate routes can still pass. Update the assertions around the got map in TestRouter (the chi.Walk callback and want list) to also verify the total registered routes match exactly, ensuring no unexpected route patterns are registered alongside the expected /platforms and /platforms/{id} routes.Source: Path instructions
🤖 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/flow/service_flow_steps.go`:
- Around line 99-107: The traversal in the path-building loop can re-enqueue
nodes forever when the flow graph contains cycles, so update the logic in the
path construction routine to track visited nodes before appending `nexts` to
`queue`. Use the existing `queue`, `pathMap`, and `PathItem` processing in
`service_flow_steps.go` to skip already-seen nodes or edges so cyclic paths like
`A -> B -> C -> B` terminate cleanly.
---
Nitpick comments:
In `@internal/flow/router_test.go`:
- Around line 57-90: The current TestRegisterRoutesWithHandler only checks that
routes are registered, not that each route dispatches to the correct handler.
Update the test to exercise actual requests against the router from
registerRoutesWithHandler and assert the mockService.called map records the
expected service method for each route, using newHandler and newMockService to
verify wiring such as GET /flows/{id}/path and the other flow/product endpoints.
In `@internal/platform/router_test.go`:
- Around line 16-38: The route registration test in router_test.go only checks
that expected entries from chi.Walk are present, so extra or duplicate routes
can still pass. Update the assertions around the got map in TestRouter (the
chi.Walk callback and want list) to also verify the total registered routes
match exactly, ensuring no unexpected route patterns are registered alongside
the expected /platforms and /platforms/{id} routes.
In `@internal/product/router_test.go`:
- Around line 16-38: The route coverage in router_test.go only verifies that the
expected entries in chi.Walk appear, so extra unexpected registrations in got
would still pass. Update the test around chi.Walk to assert the route set is
exact by checking that every route in got is present in want and that the counts
match, using the existing got and want collections in the test.
In `@router/router_test.go`:
- Around line 25-70: `TestSetupRouter` only checks that routes are registered
via `chi.Walk`, but it does not verify that overlapping mounted prefixes still
dispatch to the intended handlers at runtime. Keep the existing `SetupRouter`
route coverage, and add a small `httptest`-based test that exercises the nested
paths under the mounted prefix, using `SetupRouter`, `ServeHTTP`, and the
affected routes like `/platforms/{id}/products`, `/products/{id}/flows`, and
`/flows/{id}/capabilities`, to confirm they do not return 404.
🪄 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: 1252d53b-011f-44a6-ba8e-c0b55d7931fc
📒 Files selected for processing (25)
internal/capability/handler.gointernal/capability/handler_test.gointernal/capability/router.gointernal/capability/router_test.gointernal/capability/service.gointernal/capability/service_capability.gointernal/flow/handler.gointernal/flow/handler_test.gointernal/flow/router.gointernal/flow/router_test.gointernal/flow/serivice_flow.gointernal/flow/service.gointernal/flow/service_flow_steps.gointernal/platform/handler.gointernal/platform/handler_test.gointernal/platform/router.gointernal/platform/router_test.gointernal/platform/service.gointernal/product/handler.gointernal/product/handler_test.gointernal/product/router.gointernal/product/router_test.gointernal/product/service.gorouter/router.gorouter/router_test.go
…o v0.21.0, and `text` to v0.38.0 in `go.mod` and `go.sum`.
…ed route accessibility
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_test.go (1)
14-24: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn a non-nil
pgx.Rowsstub frommockDBTX.Query. The nested route handlers callGetProductsByPlatform,GetFlowsByProduct,GetCapabilitiesByFlow, andGetCapabilitiesByProduct, and each generated query path doesdefer rows.Close()afterQuery; returningnilhere will panic as soon as one of those handlers runs.QueryRowisn’t exercised by these routes.🤖 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_test.go` around lines 14 - 24, The mockDBTX.Query stub currently returns a nil pgx.Rows, which will panic when the route handlers invoke Query and then defer rows.Close() in GetProductsByPlatform, GetFlowsByProduct, GetCapabilitiesByFlow, or GetCapabilitiesByProduct. Update mockDBTX.Query to return a non-nil pgx.Rows test double that safely supports Close and any iteration the handlers expect, and leave QueryRow unchanged since it is not used by these routes.Source: Path instructions
🤖 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_test.go`:
- Around line 14-24: The mockDBTX.Query stub currently returns a nil pgx.Rows,
which will panic when the route handlers invoke Query and then defer
rows.Close() in GetProductsByPlatform, GetFlowsByProduct, GetCapabilitiesByFlow,
or GetCapabilitiesByProduct. Update mockDBTX.Query to return a non-nil pgx.Rows
test double that safely supports Close and any iteration the handlers expect,
and leave QueryRow unchanged since it is not used by these routes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6b0ac44-4b3b-4869-ba69-6f4759b34258
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.modmain.gorouter/router.gorouter/router_test.go
1 participant
Description
Code Rabbit Summary
Summary by CodeRabbit
Fixes
Closes #95
Post Deployment Tasks?