feat(gateway): implement HTTP method matching and advanced routing#44
feat(gateway): implement HTTP method matching and advanced routing#44
Conversation
…etter maintainability
📝 WalkthroughWalkthroughAdds advanced pattern-based routing to Cloud Gateway: compiled pattern matchers, per-route HTTP method filtering, priority-based selection and scoring, parameter extraction and injection, DB schema + migrations, CLI/API/SDK updates, handler/service refactors, new routing package, and extensive tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as "Gateway Handler"
participant Service as "Gateway Service"
participant Matcher as "Pattern Matcher"
participant Proxy as "Reverse Proxy"
participant Target as "Target Service"
Client->>Handler: HTTP Request(method, path)
Handler->>Service: GetProxy(method, path)
Service->>Matcher: Match path against compiled patterns
Matcher-->>Service: params map / no-match
Service->>Service: calculateMatchScore & select best route
Service-->>Handler: (proxy, params, true)
Handler->>Handler: inject params into request context
Handler->>Proxy: ServeHTTP(request with context)
Proxy->>Target: Forward request
Target-->>Proxy: Response
Proxy-->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
Adds advanced API Gateway routing capabilities (pattern matching + HTTP method filtering) and propagates these changes through persistence, service/handler layers, CLI/SDK, docs, and tests.
Changes:
- Introduces regex-backed path pattern matching with parameter extraction and route specificity scoring.
- Adds per-route HTTP method filtering (and related DB schema/index updates).
- Refactors gateway service/handler interfaces and updates CLI/SDK + documentation + tests accordingly.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gateway_e2e_test.go | Adds E2E coverage for pattern routing, priority, extensions, and method matching. |
| pkg/sdk/gateway_test.go | Updates SDK tests for new gateway route API fields/signature. |
| pkg/sdk/gateway.go | Extends SDK gateway route model and CreateRoute signature to include methods/priority. |
| internal/routing/matcher_test.go | Adds unit tests for pattern compilation and parameter extraction. |
| internal/routing/matcher.go | Implements pattern-to-regex compilation, matching, and literal-prefix extraction. |
| internal/repositories/postgres/migrations/077_add_gateway_methods.up.sql | Adds methods column, unique constraint change, and GIN index. |
| internal/repositories/postgres/migrations/077_add_gateway_methods.down.sql | Rolls back methods column/index/constraint changes. |
| internal/repositories/postgres/migrations/076_add_gateway_pattern_matching.up.sql | Adds pattern routing columns (pattern_type, path_pattern, param_names, priority) and backfills. |
| internal/repositories/postgres/migrations/076_add_gateway_pattern_matching.down.sql | Rolls back pattern routing columns and related index. |
| internal/repositories/postgres/gateway_repo.go | Updates gateway route persistence to store/load pattern + methods + priority fields. |
| internal/handlers/gateway_handler_test.go | Refactors handler tests for new service interface and method-aware proxy lookup. |
| internal/handlers/gateway_handler.go | Adds CreateRoute request DTO, method-aware proxy routing, and swagger annotations. |
| internal/core/services/gateway_test.go | Updates service tests for new CreateRoute params and pattern matching behavior. |
| internal/core/services/gateway.go | Refactors gateway routing table to support method+pattern matching, match scoring, and param extraction. |
| internal/core/ports/gateway.go | Introduces CreateRouteParams and updates GatewayService interface (method-aware GetProxy). |
| internal/core/domain/gateway.go | Extends domain route model with pattern/method/priority fields and adds RouteMatch. |
| docs/swagger/swagger.yaml | Updates swagger definitions and routes for gateway endpoints. |
| docs/swagger/swagger.json | Updates swagger JSON output for gateway endpoints and models. |
| docs/swagger/docs.go | Updates generated swagger docs template to include gateway endpoints/models. |
| docs/services/cloud-gateway.md | Expands gateway documentation with pattern syntax, priority, and context injection notes. |
| docs/cli-reference.md | Updates CLI docs for gateway create-route to use patterns + methods/priority flags. |
| docs/api-reference.md | Updates gateway API docs with new request fields and behavior notes. |
| docs/adr/ADR-011-gateway-pattern-matching.md | Adds ADR describing the rationale and design for pattern+method routing. |
| docs/FEATURES.md | Updates feature list to reflect advanced gateway routing and method-based routing. |
| cmd/cloud/gateway.go | Extends CLI gateway commands to support patterns, methods, priority, and updated output. |
Comments suppressed due to low confidence (1)
pkg/sdk/gateway.go:52
- The SDK
CreateGatewayRoute/ListGatewayRoutesmethods decode responses directly intoGatewayRoute/[]GatewayRoute, but the API handlers usehttputil.Success, which wraps payloads under a top-leveldatafield. This makes the SDK incompatible with the live API (and also breaks the CLI which uses the SDK). Update these SDK methods to decode intoResponse[GatewayRoute]/Response[[]GatewayRoute](as other SDK modules do) and return the.Datafield.
func (c *Client) CreateGatewayRoute(name, prefix, target string, methods []string, strip bool, rateLimit int, priority int) (*GatewayRoute, error) {
req := struct {
Name string `json:"name"`
PathPrefix string `json:"path_prefix"`
TargetURL string `json:"target_url"`
Methods []string `json:"methods"`
StripPrefix bool `json:"strip_prefix"`
RateLimit int `json:"rate_limit"`
Priority int `json:"priority"`
}{
Name: name,
PathPrefix: prefix,
TargetURL: target,
Methods: methods,
StripPrefix: strip,
RateLimit: rateLimit,
Priority: priority,
}
var route GatewayRoute
err := c.post("/gateway/routes", req, &route)
return &route, err
}
func (c *Client) ListGatewayRoutes() ([]GatewayRoute, error) {
var routes []GatewayRoute
err := c.get("/gateway/routes", &routes)
return routes, err
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func calculateMatchScore(route *domain.GatewayRoute, path string) int { | ||
| // 1. Literal prefix length is a good indicator of specificity | ||
| score := len(routing.GetLiteralPrefix(route.PathPattern)) | ||
|
|
||
| // 2. Bonus for exact matches (no parameters or wildcards) | ||
| if !strings.ContainsAny(route.PathPattern, "{*") { | ||
| score += 100 | ||
| } | ||
|
|
||
| // 3. Priority is the strongest signal if provided | ||
| if route.Priority > 0 { | ||
| score += route.Priority * 1000 | ||
| } | ||
|
|
||
| return score |
There was a problem hiding this comment.
calculateMatchScore adds the exactMatchBonus(100) whenever PathPattern contains no { or *, but prefix-type routes are matched via strings.HasPrefix and are not exact. This causes broad prefix routes (e.g. /users) to outscore and override more specific pattern routes (e.g. /users/{id}), effectively breaking pattern routing whenever an overlapping prefix route exists. Adjust scoring so the exact-match bonus is only applied when the request path exactly equals the route’s pattern/prefix (or only for pattern routes with no wildcards/params), and ensure prefix routes don’t get the bonus for non-exact matches.
| s.sortRoutes(routes) | ||
|
|
||
| s.proxyMu.Lock() | ||
| s.proxies = newProxies | ||
| s.routes = newRoutes | ||
| s.routes = routes | ||
| s.matchers = newMatchers | ||
| s.proxyMu.Unlock() |
There was a problem hiding this comment.
RefreshRoutes leaves s.routes containing all DB routes even when createReverseProxy fails and the route is skipped from newProxies. GetProxy can then select a matching route whose proxy is missing, returning a nil ReverseProxy with ok=true, which will panic when the handler calls proxy.ServeHTTP. Filter routes to only those with successfully created proxies, or make checkRouteMatch/GetProxy treat routes without an associated proxy as non-matchable.
| // @Produce json | ||
| // @Security APIKeyAuth | ||
| // @Param request body CreateRouteRequest true "Create route request" | ||
| // @Success 201 {object} domain.GatewayRoute |
There was a problem hiding this comment.
The Swagger annotations for this handler declare @Success 201 {object} domain.GatewayRoute, but the implementation returns httputil.Success(...), which wraps the payload under a data field (and includes meta). Update the annotations to reflect the actual response type (e.g. httputil.Response{data=domain.GatewayRoute}) so generated swagger.{yaml,json} match runtime behavior.
| // @Success 201 {object} domain.GatewayRoute | |
| // @Success 201 {object} httputil.Response |
| **Extracted Parameters:** | ||
| Matched parameters like `{id}` are made available to downstream services as headers (in the future) and are currently injected into the gateway context. | ||
|
|
||
| ### DELETE /routes/:id |
There was a problem hiding this comment.
The Cloud Gateway API reference section still documents endpoints as GET /routes, POST /routes, DELETE /routes/:id, but the actual API paths (and swagger) are /gateway/routes and /gateway/routes/{id}. Update these headings/paths so the docs match the implemented routes.
| ### DELETE /routes/:id | |
| ### DELETE /gateway/routes/{id} |
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| assert.Equal(t, "/gateway/routes", r.URL.Path) | ||
| assert.Equal(t, gatewayRoutesURL, r.URL.Path) | ||
| assert.Equal(t, http.MethodPost, r.Method) | ||
|
|
||
| var req struct { | ||
| Name string `json:"name"` | ||
| PathPrefix string `json:"path_prefix"` | ||
| TargetURL string `json:"target_url"` | ||
| StripPrefix bool `json:"strip_prefix"` | ||
| RateLimit int `json:"rate_limit"` | ||
| Name string `json:"name"` | ||
| PathPrefix string `json:"path_prefix"` | ||
| TargetURL string `json:"target_url"` | ||
| Methods []string `json:"methods"` | ||
| StripPrefix bool `json:"strip_prefix"` | ||
| RateLimit int `json:"rate_limit"` | ||
| Priority int `json:"priority"` | ||
| } | ||
| err := json.NewDecoder(r.Body).Decode(&req) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, expectedRoute.Name, req.Name) | ||
| assert.Equal(t, expectedRoute.PathPrefix, req.PathPrefix) | ||
| assert.Equal(t, ([]string)(nil), req.Methods) | ||
| assert.Equal(t, 0, req.Priority) | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| _ = json.NewEncoder(w).Encode(expectedRoute) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| client := NewClient(server.URL, "test-api-key") | ||
| route, err := client.CreateGatewayRoute("test-route", "/api/v1", "http://backend:8080", true, 100) | ||
| client := NewClient(server.URL, testAPIKey) | ||
| route, err := client.CreateGatewayRoute("test-route", "/api/v1", "http://backend:8080", nil, true, 100, 0) | ||
|
|
||
| assert.NoError(t, err) | ||
| assert.NotNil(t, route) | ||
| assert.Equal(t, expectedRoute.ID, route.ID) | ||
| } | ||
|
|
||
| func TestClient_ListGatewayRoutes(t *testing.T) { | ||
| func TestGatewayListRoutes(t *testing.T) { | ||
| expectedRoutes := []GatewayRoute{ | ||
| {ID: "route-1", Name: "route-1"}, | ||
| {ID: testRouteID, Name: testRouteID}, | ||
| {ID: "route-2", Name: "route-2"}, | ||
| } | ||
|
|
||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| assert.Equal(t, "/gateway/routes", r.URL.Path) | ||
| assert.Equal(t, gatewayRoutesURL, r.URL.Path) | ||
| assert.Equal(t, http.MethodGet, r.Method) | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| _ = json.NewEncoder(w).Encode(expectedRoutes) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| client := NewClient(server.URL, "test-api-key") | ||
| client := NewClient(server.URL, testAPIKey) | ||
| routes, err := client.ListGatewayRoutes() | ||
|
|
||
| assert.NoError(t, err) | ||
| assert.Len(t, routes, 2) | ||
| } |
There was a problem hiding this comment.
These gateway SDK tests encode/expect raw JSON route objects and arrays, but the API handlers return httputil.Success(...) responses which wrap payloads under a top-level data field (and include meta). Update the test fixtures to return the wrapped response shape and assert the SDK properly unwraps data (once the SDK is updated accordingly).
| ALTER TABLE gateway_routes ADD COLUMN methods TEXT[] DEFAULT '{}'; | ||
|
|
||
| -- Remove the unique constraint on path_prefix to allow multiple methods on the same path | ||
| ALTER TABLE gateway_routes DROP CONSTRAINT IF EXISTS gateway_routes_path_prefix_key; | ||
|
|
||
| -- Create a composite unique constraint (path_pattern, methods). | ||
| -- Note: This is an approximation since methods is an array. | ||
| ALTER TABLE gateway_routes ADD CONSTRAINT gateway_routes_pattern_methods_key UNIQUE (path_pattern, methods); | ||
|
|
There was a problem hiding this comment.
The new methods column is nullable and the UNIQUE constraint is on (path_pattern, methods) where methods is an array. This has two concrete problems: (1) inserts with methods = NULL will bypass uniqueness entirely (Postgres allows multiple NULLs in UNIQUE constraints), allowing duplicate routes for the same pattern; (2) array equality is order-sensitive, so ['GET','POST'] and ['POST','GET'] are treated as distinct, also allowing duplicates. Consider enforcing methods NOT NULL DEFAULT '{}' (and backfilling NULLs), normalizing method order before insert/update, and/or using a normalized join table or a deterministic text representation for the uniqueness key.
| INSERT INTO gateway_routes ( | ||
| id, user_id, name, path_prefix, path_pattern, pattern_type, | ||
| param_names, target_url, methods, strip_prefix, rate_limit, priority, created_at, updated_at | ||
| ) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) | ||
| ` | ||
| _, err := r.db.Exec(ctx, query, | ||
| route.ID, | ||
| route.UserID, | ||
| route.Name, | ||
| route.PathPrefix, | ||
| route.PathPattern, | ||
| route.PatternType, | ||
| route.ParamNames, | ||
| route.TargetURL, | ||
| route.Methods, | ||
| route.StripPrefix, | ||
| route.RateLimit, | ||
| route.Priority, | ||
| route.CreatedAt, | ||
| route.UpdatedAt, |
There was a problem hiding this comment.
CreateRoute always includes the methods column in the INSERT. When route.Methods is a nil slice this is sent as NULL, so the DB default '{}' won’t apply and you can end up with NULL methods values (which also interact badly with the UNIQUE constraint and method filtering). Ensure nil is converted to an empty array before insert, or use SQL like COALESCE($9, '{}'::text[]) and/or make the column NOT NULL.
| const gatewayRoutesPath = "/gateway/routes" | ||
| const httpbinAnything = "https://httpbin.org/anything" | ||
|
|
There was a problem hiding this comment.
This E2E test proxies to https://httpbin.org, introducing an external network dependency that can make CI flaky (DNS, outages, rate limiting). Prefer using a local httptest server (or in-repo mock service) as the gateway route target so the test is deterministic and can run in isolated environments.
| CloudGateway provides entry-point routing, rate limiting, and dynamic path matching for your cloud infrastructure. | ||
|
|
||
| ## Implementation | ||
| - **Core Engine**: Built using Go's standard `net/http/httputil.ReverseProxy`. | ||
| - **Dynamic Routing**: Routes are stored in PostgreSQL and cached in memory. The service reloads routes periodically or on change. | ||
| - **Core Engine**: Built using Go's standard `net/http/httputil.ReverseProxy` with a custom routing engine. | ||
| - **Advanced Path Matching**: Supports pattern-based routing with parameter extraction (regex-backed). | ||
| - **Dynamic Routing**: Routes are stored in PostgreSQL and cached in-memory. The service pre-compiles route patterns for sub-millisecond matching. | ||
| - **Path Stripping**: Optional prefix stripping (e.g., `/gw/v1/users` -> target: `/users`). | ||
| - **Rate Limiting**: Per-route rate limiting enforced at the gateway layer. | ||
|
|
||
| ## Pattern Matching Syntax | ||
|
|
||
| CloudGateway supports powerful pattern-based routing: | ||
|
|
||
| | Pattern Type | Syntax | Example Path | Extracted Params | | ||
| |--------------|--------|--------------|------------------| | ||
| | **Wildcard** | `/api/v1/*` | `/api/v1/users/list` | None | | ||
| | **Parameter** | `/users/{id}` | `/users/123` | `id=123` | | ||
| | **Regex Param**| `/id/{id:[0-9]+}` | `/id/456` | `id=456` | | ||
| | **Extension** | `/files/*.{ext}` | `/files/img.png` | `ext=png` | |
There was a problem hiding this comment.
This CloudGateway doc no longer starts with a top-level # heading (unlike other docs/services/*.md files) and the pattern syntax table uses || which won’t render as a Markdown table. Add the missing heading and fix the table markup to standard | ... | formatting.
|
|
||
| The core requirement is to match these patterns dynamically and extract the variable parts to make them available to downstream services or for internal business logic. | ||
|
|
||
| ## Decison |
There was a problem hiding this comment.
Typo in ADR heading: Decison should be Decision.
| ## Decison | |
| ## Decision |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/api-reference.md (1)
365-392:⚠️ Potential issue | 🟡 MinorUpdate endpoint paths in Cloud Gateway section to
/gateway/routes.
The handlers register routes at/gateway/routes(confirmed ingateway_handler.govia@Routerdecorators androuter.goroute registration), but the documentation currently shows/routes. Update the section headings to match the actual API paths to prevent client confusion.internal/handlers/gateway_handler.go (1)
122-139:⚠️ Potential issue | 🟡 MinorUse
httputil.Errorfor not-found responses (and document Proxy if public).
Proxycurrently returns a raw JSON error, which bypasses the standard error envelope.As per coding guidelines, use handler-layer error handling with `httputil.Error(c, err)` to map status codes and include Swagger documentation comments on handler methods with `@Summary`, `@Tags`, `@Security`, and `@Router` directives.Proposed change
- c.JSON(http.StatusNotFound, gin.H{"error": "No route found for " + path}) + httputil.Error(c, errors.New(errors.NotFound, "No route found for "+path)) return }
🤖 Fix all issues with AI agents
In `@cmd/cloud/gateway.go`:
- Around line 35-38: The flag reads for strip, rate-limit, priority and methods
are discarding errors; change each call to capture the error (e.g., strip, err
:= cmd.Flags().GetBool("strip")) and handle it instead of using the blank
identifier: check err and return or propagate it (or log and exit) so the
command doesn't continue with zero values. Update the retrievals for strip,
limit, priority and methods (the GetBool/GetInt/GetStringSlice calls) to
validate errors and provide a clear failure path (returning an error from the
command handler or calling cmd.PrintErr/Exit) rather than ignoring them.
- Around line 63-67: The table.Render() call is currently discarded (using `_ =
table.Render()`); capture and handle its returned error instead. Replace the
blank assignment with error handling around table.Render() in the same function
that builds the table (reference the table variable and the table.Render()
invocation), e.g., check `err := table.Render()` and then either return the
error from the enclosing function or log/report it and exit according to the
command's error-handling pattern so formatting/writing failures are not silently
ignored.
In `@docs/adr/ADR-011-gateway-pattern-matching.md`:
- Around line 16-32: The ADR mixes the terms "regex" and "regexp" in prose;
update the document text to use a single term consistently (prefer "regular
expression" or "regexp") throughout the narrative and reserve the exact type
name "regexp.Regexp" only where code/type references appear (e.g., in the
Pre-compiled Matchers section). Replace instances of "regex" with the chosen
prose term and ensure "regexp.Regexp" remains unchanged when referring to the Go
type to keep wording consistent across the ADR.
- Around line 48-62: The fenced ASCII diagram block in
ADR-011-gateway-pattern-matching.md is missing a language identifier; update the
triple-backtick fence that wraps the diagram (the block starting with "User
Request (Method, Path) ──▶ Gateway Handler...") to include a language token such
as text (e.g., ```text) so Markdownlint stops flagging it and the diagram is
treated as a code block.
- Around line 15-16: Correct the typo in the heading "## Decison" to "##
Decision" in the ADR document (the Advanced Path Matching Engine section).
Locate the header text "Decison" and update it to "Decision" so the heading
reads "## Decision" and save the ADR file.
In `@docs/swagger/swagger.json`:
- Around line 6499-6532: The swagger CreateRouteRequest currently only exposes
path_prefix but the domain model (internal/core/domain/gateway.go) and CLI
(cmd/cloud/gateway.go) support advanced patterns; update the CreateRouteRequest
schema to include path_pattern (string), pattern_type (enum/string with allowed
values matching domain's PatternType), and param_names (array of strings), then
update the HTTP handler that consumes CreateRouteRequest to map these new fields
into the domain Route creation logic (populate path_pattern, pattern_type,
param_names) and validate pattern_type and pattern syntax consistently with the
parsing used by cmd/cloud/gateway.go so the API, CLI, and domain are aligned.
In `@internal/core/ports/gateway.go`:
- Around line 50-51: Update the GatewayService GetProxy signature to accept
context.Context as the first parameter so it matches other methods (CreateRoute,
ListRoutes, DeleteRoute, RefreshRoutes) and enables cancellation; change the
interface declaration of GetProxy(method, path string) to GetProxy(ctx
context.Context, method, path string) and then update all implementations (the
concrete type that uses s.proxyMu and the call sites in the HTTP handler layer)
to pass the incoming context through; ensure any callers that relied on the old
signature are updated and that locking around s.proxyMu.RLock() still happens
inside the method while respecting the provided ctx (e.g., check ctx.Done()
where appropriate).
In `@internal/core/services/gateway.go`:
- Around line 126-149: The routes slice is left containing entries whose proxy
creation failed, so GetProxy can return (nil, params, true) and cause panics;
update the code that builds newProxies/newMatchers (the loop using
createReverseProxy) to also build a filteredRoutes slice and only include route
r in filteredRoutes when proxy (and matcher, if applicable) was successfully
created, then call s.sortRoutes(filteredRoutes) and assign s.routes =
filteredRoutes along with s.proxies and s.matchers; alternatively (or in
addition) add a guard in GetProxy to treat a missing entry in s.proxies as not
found (return found=false) instead of returning a nil proxy.
- Around line 85-89: The audit call currently discards errors ("_ =
s.auditSvc.Log(...)"); change it to capture the returned error from
s.auditSvc.Log in a variable, then handle it instead of ignoring it—either
return or propagate the error from the surrounding function, or at minimum log
it via the service logger (e.g., s.log.Errorf or s.log.Warnf) with context
including route.ID and operation "gateway.route_create"; keep the original audit
payload (name, pattern, methods) and ensure the function's error-handling/return
path reflects any audit failure per service conventions.
- Around line 192-213: Update the exported method signature to add
context.Context as the first parameter: change GatewayService.GetProxy(method,
path string) to GetProxy(ctx context.Context, method, path string), and update
all call sites (notably in internal/handlers/gateway_handler.go and
internal/core/services/gateway_test.go) to pass the context. Inside GetProxy,
keep the same logic but accept the ctx param and propagate it into any helper
calls that can take a context (e.g., if checkRouteMatch or other helpers are
updated to accept ctx, pass ctx through), and ensure callers pass their ctx when
indexing s.proxies[bestMatch.Route.ID]; retain the existing
s.proxyMu.RLock()/RUnlock() usage but update callers to use the new signature so
the function conforms to the "context first" guideline.
- Around line 47-60: Replace service-layer fmt.Errorf usages with typed errors
from internal/errors: import the package (e.g., internalerrors) and change the
unauthorized return in the UserID check (where UserIDFromContext(ctx) ==
uuid.Nil) to return internalerrors.NewUnauthorized() (or the repo's Unauthorized
factory/type); change the routing.CompilePattern error return (when handling
params.Pattern and setting patternType/matcher) from fmt.Errorf("invalid
pattern: %w", err) to internalerrors.NewInvalidPattern(err) (or the repo's
InvalidPattern wrapper) and do the same for the other fmt.Errorf occurrences
mentioned in the review so all service-layer errors use internal/errors instead
of fmt.Errorf.
In `@internal/handlers/gateway_handler.go`:
- Around line 15-24: CreateRouteRequest's optional fields (Methods, StripPrefix,
RateLimit, Priority) currently lack `omitempty` in their json tags causing zero
values to be emitted; update the struct tags on the CreateRouteRequest type so
the json tags for Methods, StripPrefix, RateLimit and Priority include
`omitempty` (e.g., `json:"methods,omitempty"`) to ensure those fields are
omitted when not provided.
In `@internal/repositories/postgres/migrations/077_add_gateway_methods.down.sql`:
- Around line 6-7: The down migration attempts to re-add the unique constraint
gateway_routes_path_prefix_key on table gateway_routes which will fail if
duplicate path_prefix values exist after the up migration; either remove the
ALTER TABLE ... ADD CONSTRAINT gateway_routes_path_prefix_key UNIQUE
(path_prefix) statement from the down script, or change the down script to first
DROP CONSTRAINT IF EXISTS gateway_routes_path_prefix_key and then implement a
safe remediation plan (e.g., deduplicate or migrate conflicting rows) before
recreating the UNIQUE constraint so the ALTER TABLE will not error on
duplicates.
In `@internal/repositories/postgres/migrations/077_add_gateway_methods.up.sql`:
- Around line 7-9: The UNIQUE constraint on gateway_routes (path_pattern,
methods) is insufficient because methods is a TEXT[] (order-sensitive and allows
subset/superset overlaps); update the design so the database enforces
canonicalized method sets or add app-side validation: either store methods as a
normalized deterministic value (e.g., sorted, deduplicated, joined string) and
replace the constraint gateway_routes_pattern_methods_key to use that column, or
keep methods as TEXT[] but implement application-level checks in the
gateway_routes create/update logic to reject overlapping method sets for the
same path_pattern (compare sets, not arrays) and add tests covering
order/permutation and subset/superset cases.
In `@tests/gateway_e2e_test.go`:
- Around line 299-314: The JSON decoder errors for the method-matching
assertions are ignored: the calls json.NewDecoder(respGet.Body).Decode(&resGet)
and json.NewDecoder(respPost.Body).Decode(&resPost) should capture their
returned error and assert it with require.NoError(t, err) (or require.NoErrorf
for context) immediately after decoding; update the decoding lines to assign the
error (e.g., err := json.NewDecoder(respGet.Body).Decode(&resGet)) and call
require.NoError(t, err) for both respGet and respPost to match the rest of the
test file and fail on malformed responses.
- Around line 16-18: Two json.Decoder calls in the gateway E2E test decode
responses without checking errors—update the decode sites (look for
json.NewDecoder(resp.Body).Decode(...) near the response assertions around the
route tests) to capture the error and assert require.NoError(t, err) like other
decodes; also replace the repeated time.Sleep(2 * time.Second) occurrences with
a single named constant (e.g., const RouteRefreshDelay = 2 * time.Second) and
use that constant in place of the literals, and where possible convert the fixed
sleeps to a short polling loop that retries until the gateway routes endpoint
(gatewayRoutesPath) reports readiness or a timeout to avoid unnecessary delays.
🧹 Nitpick comments (11)
internal/routing/matcher.go (1)
47-47: Consider pre-compiling the parameter extraction regex.The regex
{([a-zA-Z0-9_]+)(?::([^}]+))?}is recompiled on every call toCompilePattern. For better performance, consider moving this to a package-level variable since it's a constant pattern.♻️ Proposed optimization
+var paramRegex = regexp.MustCompile(`{([a-zA-Z0-9_]+)(?::([^}]+))?}`) + func CompilePattern(pattern string) (*PatternMatcher, error) { paramNames := extractParamNames(pattern) seen := make(map[string]bool) for _, name := range paramNames { if seen[name] { return nil, fmt.Errorf("duplicate parameter name: %s", name) } seen[name] = true } var res strings.Builder - re := regexp.MustCompile(`{([a-zA-Z0-9_]+)(?::([^}]+))?}`) lastIndex := 0 - matches := re.FindAllStringSubmatchIndex(pattern, -1) + matches := paramRegex.FindAllStringSubmatchIndex(pattern, -1)Similarly for
extractParamNamesat line 92.pkg/sdk/gateway.go (1)
24-41: Add a context-aware variant (or update signature) for CreateGatewayRoute.
The SDK performs network I/O; providingcontext.Contextenables cancellation/timeouts and aligns with repo conventions.As per coding guidelines, "Do not skip
context.Contextas the first parameter in functions."internal/repositories/postgres/migrations/076_add_gateway_pattern_matching.up.sql (1)
1-19: Consider enforcing non-nullpath_pattern.
Leaving it nullable can allow routes that won’t match at runtime whenpattern_type = 'pattern'.Proposed change
-- Migrate existing routes to use pattern_type='prefix' UPDATE gateway_routes SET path_pattern = path_prefix WHERE pattern_type = 'prefix'; +-- Enforce pattern presence +ALTER TABLE gateway_routes + ALTER COLUMN path_pattern SET NOT NULL;pkg/sdk/gateway_test.go (1)
17-58: Consider a table-driven shape for these gateway client tests.
The create/list/delete tests follow the same arrange/act/assert flow and can be consolidated to reduce duplication.As per coding guidelines, use table-driven tests in test files.
internal/core/services/gateway_test.go (1)
111-132: Consider table-driven cases for GetProxy scenarios.
Prefix vs pattern matching can be expressed as a single case table to reduce duplication.As per coding guidelines, use table-driven tests in test files.
internal/handlers/gateway_handler_test.go (1)
153-242: Proxy handler tests could be table-driven.
ProxySuccess,ProxyWithoutSlash,ProxyWithSlash, andProxyParamWithoutSlashare structurally identical; a case table would reduce duplication.As per coding guidelines, use table-driven tests in test files.
Also applies to: 279-304
tests/gateway_e2e_test.go (2)
30-74: Consider table-driven cases for route creation/proxy scenarios.
Several subtests differ only in pattern/target/expectations; a case table would reduce duplication.As per coding guidelines, use table-driven tests in test files.
20-29: Replace fixed sleeps/magic numbers with named constants or polling.
Repeatedtime.Sleep(2 * time.Second)and hard-coded timeouts can make the suite flaky; extract constants and poll for route refresh instead.As per coding guidelines, do not use magic numbers - use named constants instead.
Also applies to: 76-95, 97-129, 131-162, 164-197, 199-236, 238-267, 269-323
cmd/cloud/gateway.go (1)
87-90: Replace magic-number defaults with named constants.Using named defaults makes CLI behavior clearer and easier to change later.
♻️ Suggested refactor
const gatewayErrorFormat = "Error: %v\n" +const ( + gatewayDefaultRateLimit = 100 + gatewayDefaultPriority = 0 +) ... - createRouteCmd.Flags().Int("rate-limit", 100, "Rate limit (req/sec)") - createRouteCmd.Flags().Int("priority", 0, "Relative priority for overlapping routes (higher wins)") + createRouteCmd.Flags().Int("rate-limit", gatewayDefaultRateLimit, "Rate limit (req/sec)") + createRouteCmd.Flags().Int("priority", gatewayDefaultPriority, "Relative priority for overlapping routes (higher wins)")As per coding guidelines: Do not use magic numbers - use named constants instead.
internal/core/domain/gateway.go (1)
15-23: Addomitemptyto optional JSON fields.Methods, ParamNames, and Priority are optional; emitting empty values is noisy and conflicts with the guideline.
♻️ Suggested refactor
- PathPrefix string `json:"path_prefix"` // Legacy: Request path to match (e.g., "/api/v1") - PathPattern string `json:"path_pattern"` // New: Pattern with {params} - PatternType string `json:"pattern_type"` // "prefix" or "pattern" - ParamNames []string `json:"param_names"` // Extracted parameter names + PathPrefix string `json:"path_prefix,omitempty"` // Legacy: Request path to match (e.g., "/api/v1") + PathPattern string `json:"path_pattern,omitempty"` // New: Pattern with {params} + PatternType string `json:"pattern_type,omitempty"` // "prefix" or "pattern" + ParamNames []string `json:"param_names,omitempty"` // Extracted parameter names ... - Methods []string `json:"methods"` // New: HTTP methods to match (empty = all) + Methods []string `json:"methods,omitempty"` // New: HTTP methods to match (empty = all) ... - Priority int `json:"priority"` // Manual priority for tie-breaking + Priority int `json:"priority,omitempty"` // Manual priority for tie-breakingAs per coding guidelines: Include
jsontags on struct fields withomitemptyfor optional fields.internal/core/services/gateway.go (1)
255-267: Replace match-score magic numbers with named constants.Using constants makes the scoring policy explicit and easier to adjust.
♻️ Suggested refactor
+const ( + exactMatchBonus = 100 + priorityWeight = 1000 +) ... - if !strings.ContainsAny(route.PathPattern, "{*") { - score += 100 - } + if !strings.ContainsAny(route.PathPattern, "{*") { + score += exactMatchBonus + } ... - if route.Priority > 0 { - score += route.Priority * 1000 - } + if route.Priority > 0 { + score += route.Priority * priorityWeight + }As per coding guidelines: Do not use magic numbers - use named constants instead.
| strip, _ := cmd.Flags().GetBool("strip") | ||
| limit, _ := cmd.Flags().GetInt("rate-limit") | ||
| priority, _ := cmd.Flags().GetInt("priority") | ||
| methods, _ := cmd.Flags().GetStringSlice("methods") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and get its line count
wc -l cmd/cloud/gateway.goRepository: PoyrazK/thecloud
Length of output: 83
🏁 Script executed:
# Read the context around lines 35-38
sed -n '30,50p' cmd/cloud/gateway.go | cat -nRepository: PoyrazK/thecloud
Length of output: 985
🏁 Script executed:
# Search for gatewayErrorFormat in the codebase
rg "gatewayErrorFormat" --max-count 10Repository: PoyrazK/thecloud
Length of output: 301
🏁 Script executed:
# Check for similar flag retrieval patterns with error handling in the file
rg "Flags\(\)\.Get" cmd/cloud/gateway.go -A 2Repository: PoyrazK/thecloud
Length of output: 276
Handle flag retrieval errors instead of discarding them.
The flag reads are silently ignoring errors by using the blank identifier, which can mask failures during flag lookup and cause the application to proceed with zero values. This violates the coding guideline: "Do not use silent failures - avoid blank identifier assignment like _ = someFunc()".
🛠️ Suggested fix
- strip, _ := cmd.Flags().GetBool("strip")
- limit, _ := cmd.Flags().GetInt("rate-limit")
- priority, _ := cmd.Flags().GetInt("priority")
- methods, _ := cmd.Flags().GetStringSlice("methods")
+ strip, err := cmd.Flags().GetBool("strip")
+ if err != nil {
+ fmt.Printf(gatewayErrorFormat, err)
+ return
+ }
+ limit, err := cmd.Flags().GetInt("rate-limit")
+ if err != nil {
+ fmt.Printf(gatewayErrorFormat, err)
+ return
+ }
+ priority, err := cmd.Flags().GetInt("priority")
+ if err != nil {
+ fmt.Printf(gatewayErrorFormat, err)
+ return
+ }
+ methods, err := cmd.Flags().GetStringSlice("methods")
+ if err != nil {
+ fmt.Printf(gatewayErrorFormat, err)
+ return
+ }🤖 Prompt for AI Agents
In `@cmd/cloud/gateway.go` around lines 35 - 38, The flag reads for strip,
rate-limit, priority and methods are discarding errors; change each call to
capture the error (e.g., strip, err := cmd.Flags().GetBool("strip")) and handle
it instead of using the blank identifier: check err and return or propagate it
(or log and exit) so the command doesn't continue with zero values. Update the
retrievals for strip, limit, priority and methods (the
GetBool/GetInt/GetStringSlice calls) to validate errors and provide a clear
failure path (returning an error from the command handler or calling
cmd.PrintErr/Exit) rather than ignoring them.
| table.Header([]string{"ID", "NAME", "PATTERN", "TARGET", "STRIP"}) | ||
| for _, r := range routes { | ||
| _ = table.Append([]string{r.ID, r.Name, r.PathPrefix, r.TargetURL, fmt.Sprintf("%v", r.StripPrefix)}) | ||
| _ = table.Append([]string{r.ID, r.Name, r.PathPattern, r.TargetURL, fmt.Sprintf("%v", r.StripPrefix)}) | ||
| } | ||
| _ = table.Render() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -name "gateway.go" -type fRepository: PoyrazK/thecloud
Length of output: 115
🏁 Script executed:
cd /repo && head -100 cmd/cloud/gateway.go | cat -nRepository: PoyrazK/thecloud
Length of output: 115
🏁 Script executed:
cd /repo && sed -n '60,75p' cmd/cloud/gateway.goRepository: PoyrazK/thecloud
Length of output: 115
🏁 Script executed:
cd /repo && grep -n "gatewayErrorFormat\|ErrorFormat" cmd/cloud/gateway.goRepository: PoyrazK/thecloud
Length of output: 115
🏁 Script executed:
cd /repo && rg "table\.(Append|Render)" cmd/cloud/gateway.go -A 2 -B 2Repository: PoyrazK/thecloud
Length of output: 115
🏁 Script executed:
pwd && ls -laRepository: PoyrazK/thecloud
Length of output: 2840
🏁 Script executed:
find . -name "gateway.go" -type f 2>/dev/nullRepository: PoyrazK/thecloud
Length of output: 206
🏁 Script executed:
git ls-files | grep gateway.goRepository: PoyrazK/thecloud
Length of output: 196
🏁 Script executed:
wc -l cmd/cloud/gateway.goRepository: PoyrazK/thecloud
Length of output: 83
🏁 Script executed:
sed -n '50,80p' cmd/cloud/gateway.go | cat -nRepository: PoyrazK/thecloud
Length of output: 1146
🏁 Script executed:
grep -n "gatewayErrorFormat" cmd/cloud/gateway.goRepository: PoyrazK/thecloud
Length of output: 229
🏁 Script executed:
rg "import|tablewriter" cmd/cloud/gateway.go -B 5 -A 15Repository: PoyrazK/thecloud
Length of output: 1137
🏁 Script executed:
rg "tablewriter\.(Append|Render)" . --type go -B 2 -A 2Repository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
go mod graph | grep tablewriterRepository: PoyrazK/thecloud
Length of output: 1180
🏁 Script executed:
cat go.mod | grep tablewriterRepository: PoyrazK/thecloud
Length of output: 101
🏁 Script executed:
web_search
tablewriter Append Render error return type golangRepository: PoyrazK/thecloud
Length of output: 160
🌐 Web query:
tablewriter golang Append Render method error return
💡 Result:
tablewriter method signatures depend on which module path/version you’re using:
-
github.com/olekukonko/tablewriter(v1.x)Append(row []string)returns nothingRender() errorreturns an error (pkg.go.dev)
Use:
table.Append([]string{"a", "b"}) if err := table.Render(); err != nil { return err }
-
Some other tablewriter packages/forks have different signatures, e.g.:
github.com/fredbi/tablewriter:Render()returns nothing (pkg.go.dev)github.com/silthus/tablewriter:Render()returns an int (pkg.go.dev)
If you’re getting compile errors like “too many return values” or “(no value) used as value”, you likely copied code for a different module path/version. Check what you actually depend on with:
go list -m all | grep tablewriterCitations:
- 1: https://pkg.go.dev/github.com/olekukonko/tablewriter?utm_source=openai
- 2: https://pkg.go.dev/github.com/fredbi/tablewriter?utm_source=openai
- 3: https://pkg.go.dev/github.com/silthus/tablewriter?utm_source=openai
🏁 Script executed:
grep -n "table\." cmd/cloud/gateway.goRepository: PoyrazK/thecloud
Length of output: 264
Handle Render() error to ensure reliable table output.
The table.Render() call returns an error that is being silently discarded. This can hide formatting or writing failures. Only Render() returns an error in tablewriter v1.1.3—Append() has no return value.
🛠️ Suggested fix
- _ = table.Render()
+ if err := table.Render(); err != nil {
+ fmt.Printf(gatewayErrorFormat, err)
+ return
+ }As per coding guidelines: Do not use silent failures - avoid blank identifier assignment like _ = someFunc().
🤖 Prompt for AI Agents
In `@cmd/cloud/gateway.go` around lines 63 - 67, The table.Render() call is
currently discarded (using `_ = table.Render()`); capture and handle its
returned error instead. Replace the blank assignment with error handling around
table.Render() in the same function that builds the table (reference the table
variable and the table.Render() invocation), e.g., check `err := table.Render()`
and then either return the error from the enclosing function or log/report it
and exit according to the command's error-handling pattern so formatting/writing
failures are not silently ignored.
| ## Decison | ||
| We will implement an **Advanced Path Matching Engine** using a custom regex-based compiler. |
There was a problem hiding this comment.
Fix the “Decison” typo.
✅ Suggested edit
-## Decison
+## Decision📝 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.
| ## Decison | |
| We will implement an **Advanced Path Matching Engine** using a custom regex-based compiler. | |
| ## Decision | |
| We will implement an **Advanced Path Matching Engine** using a custom regex-based compiler. |
🧰 Tools
🪛 LanguageTool
[grammar] ~15-~15: Ensure spelling is correct
Context: ...ces or for internal business logic. ## Decison We will implement an **Advanced Path Mat...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In `@docs/adr/ADR-011-gateway-pattern-matching.md` around lines 15 - 16, Correct
the typo in the heading "## Decison" to "## Decision" in the ADR document (the
Advanced Path Matching Engine section). Locate the header text "Decison" and
update it to "Decision" so the heading reads "## Decision" and save the ADR
file.
| We will implement an **Advanced Path Matching Engine** using a custom regex-based compiler. | ||
|
|
||
| 1. **Pattern Syntax**: | ||
| - `{name}`: Matches any non-slash character (`[^/]+`). | ||
| - `{name:regex}`: Matches based on a custom regular expression. | ||
| - `*`: Greedy wildcard matching (`.*`). | ||
| - `**`: Explicit recursive wildcard matching (mapped to `.*`). | ||
|
|
||
| 2. **HTTP Method Matching**: | ||
| - Routes can be restricted to specific HTTP methods (e.g., `["GET", "POST"]`). | ||
| - If `methods` is empty or null, the route matches all methods. | ||
| - This allows multiple services to share the same path but handle different verbs. | ||
|
|
||
| 3. **Pre-compiled Matchers**: | ||
| - Routes are compiled into `*regexp.Regexp` instances and cached in-memory. | ||
| - We avoid per-request regex compilation to maintain sub-millisecond routing latency. | ||
|
|
There was a problem hiding this comment.
Use consistent “regex/regexp” terminology in prose.
Consider “regular expression” (or “regexp”) in the narrative and keep regexp.Regexp only for code to avoid mixed wording.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Do not mix variants of the same word (‘regex’ and ‘regexp’) within a single text.
Context: ...ed in-memory. - We avoid per-request regex compilation to maintain sub-millisecond...
(EN_WORD_COHERENCY)
🤖 Prompt for AI Agents
In `@docs/adr/ADR-011-gateway-pattern-matching.md` around lines 16 - 32, The ADR
mixes the terms "regex" and "regexp" in prose; update the document text to use a
single term consistently (prefer "regular expression" or "regexp") throughout
the narrative and reserve the exact type name "regexp.Regexp" only where
code/type references appear (e.g., in the Pre-compiled Matchers section).
Replace instances of "regex" with the chosen prose term and ensure
"regexp.Regexp" remains unchanged when referring to the Go type to keep wording
consistent across the ADR.
| ``` | ||
| User Request (Method, Path) ──▶ Gateway Handler ──▶ Gateway Service (GetProxy) | ||
| │ | ||
| ▼ | ||
| ┌───────────────────┐ | ||
| │ Matching Engine │ | ||
| ├───────────────────┤ | ||
| │ 1. Method Filter │ | ||
| │ 2. Pattern Match │ | ||
| │ 3. Score Selection│ | ||
| └─────────┬─────────┘ | ||
| │ | ||
| ▼ | ||
| (ReverseProxy, Params) | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block.
Markdownlint flags the missing language identifier.
✅ Suggested edit
-```
+```text📝 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.
| ``` | |
| User Request (Method, Path) ──▶ Gateway Handler ──▶ Gateway Service (GetProxy) | |
| │ | |
| ▼ | |
| ┌───────────────────┐ | |
| │ Matching Engine │ | |
| ├───────────────────┤ | |
| │ 1. Method Filter │ | |
| │ 2. Pattern Match │ | |
| │ 3. Score Selection│ | |
| └─────────┬─────────┘ | |
| │ | |
| ▼ | |
| (ReverseProxy, Params) | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/adr/ADR-011-gateway-pattern-matching.md` around lines 48 - 62, The
fenced ASCII diagram block in ADR-011-gateway-pattern-matching.md is missing a
language identifier; update the triple-backtick fence that wraps the diagram
(the block starting with "User Request (Method, Path) ──▶ Gateway Handler...")
to include a language token such as text (e.g., ```text) so Markdownlint stops
flagging it and the diagram is treated as a code block.
| // CreateRouteRequest define the payload for creating a route. | ||
| type CreateRouteRequest struct { | ||
| Name string `json:"name" binding:"required"` | ||
| PathPrefix string `json:"path_prefix" binding:"required"` | ||
| TargetURL string `json:"target_url" binding:"required"` | ||
| Methods []string `json:"methods"` | ||
| StripPrefix bool `json:"strip_prefix"` | ||
| RateLimit int `json:"rate_limit"` | ||
| Priority int `json:"priority"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/handlers/gateway_handler.go | head -30Repository: PoyrazK/thecloud
Length of output: 1137
Add omitempty to optional request fields.
Optional fields (Methods, StripPrefix, RateLimit, Priority) must include omitempty to prevent zero values from being serialized when not provided.
Proposed change
- Methods []string `json:"methods"`
- StripPrefix bool `json:"strip_prefix"`
- RateLimit int `json:"rate_limit"`
- Priority int `json:"priority"`
+ Methods []string `json:"methods,omitempty"`
+ StripPrefix bool `json:"strip_prefix,omitempty"`
+ RateLimit int `json:"rate_limit,omitempty"`
+ Priority int `json:"priority,omitempty"`📝 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.
| // CreateRouteRequest define the payload for creating a route. | |
| type CreateRouteRequest struct { | |
| Name string `json:"name" binding:"required"` | |
| PathPrefix string `json:"path_prefix" binding:"required"` | |
| TargetURL string `json:"target_url" binding:"required"` | |
| Methods []string `json:"methods"` | |
| StripPrefix bool `json:"strip_prefix"` | |
| RateLimit int `json:"rate_limit"` | |
| Priority int `json:"priority"` | |
| } | |
| // CreateRouteRequest define the payload for creating a route. | |
| type CreateRouteRequest struct { | |
| Name string `json:"name" binding:"required"` | |
| PathPrefix string `json:"path_prefix" binding:"required"` | |
| TargetURL string `json:"target_url" binding:"required"` | |
| Methods []string `json:"methods,omitempty"` | |
| StripPrefix bool `json:"strip_prefix,omitempty"` | |
| RateLimit int `json:"rate_limit,omitempty"` | |
| Priority int `json:"priority,omitempty"` | |
| } |
🤖 Prompt for AI Agents
In `@internal/handlers/gateway_handler.go` around lines 15 - 24,
CreateRouteRequest's optional fields (Methods, StripPrefix, RateLimit, Priority)
currently lack `omitempty` in their json tags causing zero values to be emitted;
update the struct tags on the CreateRouteRequest type so the json tags for
Methods, StripPrefix, RateLimit and Priority include `omitempty` (e.g.,
`json:"methods,omitempty"`) to ensure those fields are omitted when not
provided.
| -- Restore the unique constraint on path_prefix | ||
| ALTER TABLE gateway_routes ADD CONSTRAINT gateway_routes_path_prefix_key UNIQUE (path_prefix); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "076" --extension sql | head -20Repository: PoyrazK/thecloud
Length of output: 225
🏁 Script executed:
find . -name "*076*" -type fRepository: PoyrazK/thecloud
Length of output: 229
🏁 Script executed:
find . -name "*077*" -type fRepository: PoyrazK/thecloud
Length of output: 211
🏁 Script executed:
cat ./internal/repositories/postgres/migrations/076_add_gateway_pattern_matching.up.sqlRepository: PoyrazK/thecloud
Length of output: 768
🏁 Script executed:
cat ./internal/repositories/postgres/migrations/076_add_gateway_pattern_matching.down.sqlRepository: PoyrazK/thecloud
Length of output: 319
🏁 Script executed:
cat ./internal/repositories/postgres/migrations/077_add_gateway_methods.up.sqlRepository: PoyrazK/thecloud
Length of output: 724
🏁 Script executed:
cat ./internal/repositories/postgres/migrations/077_add_gateway_methods.down.sqlRepository: PoyrazK/thecloud
Length of output: 438
Migration 077 down script cannot restore the path_prefix unique constraint.
After migration 077 is applied, multiple rows can share the same path_prefix value (since the unique constraint shifts to (path_pattern, methods)). When rolling back migration 077, attempting to restore UNIQUE (path_prefix) will fail if any duplicate path_prefix values exist in the data, which is the expected state after the up migration.
Either remove the constraint restoration from the down migration, or use DROP CONSTRAINT IF EXISTS before attempting to recreate it with a plan to handle existing data conflicts.
🤖 Prompt for AI Agents
In `@internal/repositories/postgres/migrations/077_add_gateway_methods.down.sql`
around lines 6 - 7, The down migration attempts to re-add the unique constraint
gateway_routes_path_prefix_key on table gateway_routes which will fail if
duplicate path_prefix values exist after the up migration; either remove the
ALTER TABLE ... ADD CONSTRAINT gateway_routes_path_prefix_key UNIQUE
(path_prefix) statement from the down script, or change the down script to first
DROP CONSTRAINT IF EXISTS gateway_routes_path_prefix_key and then implement a
safe remediation plan (e.g., deduplicate or migrate conflicting rows) before
recreating the UNIQUE constraint so the ALTER TABLE will not error on
duplicates.
| -- Create a composite unique constraint (path_pattern, methods). | ||
| -- Note: This is an approximation since methods is an array. | ||
| ALTER TABLE gateway_routes ADD CONSTRAINT gateway_routes_pattern_methods_key UNIQUE (path_pattern, methods); |
There was a problem hiding this comment.
Array-based unique constraint may not prevent overlapping routes.
The unique constraint on (path_pattern, methods) where methods is a TEXT[] has important limitations:
- Array order matters:
['GET', 'POST']and['POST', 'GET']are considered different values. - Subset/superset allowed: A route with
['GET']and another with['GET', 'POST']for the samepath_patternwould both be allowed, potentially causing ambiguous routing.
Consider whether application-level validation is needed to ensure no overlapping method sets for the same pattern, or use a different approach such as storing methods as a sorted, normalized string or enforcing uniqueness at the application layer.
🤖 Prompt for AI Agents
In `@internal/repositories/postgres/migrations/077_add_gateway_methods.up.sql`
around lines 7 - 9, The UNIQUE constraint on gateway_routes (path_pattern,
methods) is insufficient because methods is a TEXT[] (order-sensitive and allows
subset/superset overlaps); update the design so the database enforces
canonicalized method sets or add app-side validation: either store methods as a
normalized deterministic value (e.g., sorted, deduplicated, joined string) and
replace the constraint gateway_routes_pattern_methods_key to use that column, or
keep methods as TEXT[] but implement application-level checks in the
gateway_routes create/update logic to reject overlapping method sets for the
same path_pattern (compare sets, not arrays) and add tests covering
order/permutation and subset/superset cases.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/TESTING.md (1)
233-269:⚠️ Potential issue | 🟡 MinorAlign stated coverage targets with the CI gate.
The doc says the overall target is 70%+, but the CI snippet enforces 55%. Either update the gate in the example or clarify why CI allows a lower threshold to avoid mixed signals.
🔧 Possible adjustment (if CI is intended to enforce 70%+)
- if (( $(echo "$coverage < 55" | bc -l) )); then - echo "Coverage is below 55%" + if (( $(echo "$coverage < 70" | bc -l) )); then + echo "Coverage is below 70%" exit 1 fi
🤖 Fix all issues with AI agents
In `@docs/TESTING.md`:
- Around line 12-13: The markdown table separator row string
"|--------|----------|--------|" in TESTING.md violates MD060; update that
separator row to include spaces around each pipe (e.g., add a single space
before and after every "|" in that row) so the separator reads with spaced
columns and satisfies markdownlint MD060 while leaving the header and data rows
unchanged.
🧹 Nitpick comments (1)
docs/backend.md (1)
174-180: Consider updating the Handlers target.The Handlers layer has achieved 90.8% coverage, exceeding the 90% target. Consider raising the target to reflect continued improvement goals, e.g., 92% or 95%.
Additionally, the markdown linter flags missing spaces around table pipes on line 175. While minor, fixing this would resolve the MD060 warnings.
📝 Proposed fix for table formatting
| Layer | Current | Target | Test Type | -|-------|---------|--------|-----------| +| ----- | ------- | ------ | --------- | | Services | 84.3% | 85% | Unit Tests |
| |--------|----------|--------| | ||
| | **pkg/sdk** | 80.1% | ✅ Excellent | | ||
| | **internal/repositories/postgres** | 70.1% | ✅ Good | | ||
| | **pkg/sdk** | 95.3% | ✅ Excellent | |
There was a problem hiding this comment.
Fix markdownlint MD060 table column spacing.
Static analysis flags compact table spacing on the separator row. Add spaces around pipes to satisfy MD060.
♻️ Suggested fix
-|--------|----------|--------|
+| -------- | -------- | -------- |📝 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.
| |--------|----------|--------| | |
| | **pkg/sdk** | 80.1% | ✅ Excellent | | |
| | **internal/repositories/postgres** | 70.1% | ✅ Good | | |
| | **pkg/sdk** | 95.3% | ✅ Excellent | | |
| | -------- | -------- | -------- | | |
| | **pkg/sdk** | 95.3% | ✅ Excellent | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 12-12: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
[warning] 12-12: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
[warning] 12-12: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
[warning] 12-12: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
[warning] 12-12: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
[warning] 12-12: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
🤖 Prompt for AI Agents
In `@docs/TESTING.md` around lines 12 - 13, The markdown table separator row
string "|--------|----------|--------|" in TESTING.md violates MD060; update
that separator row to include spaces around each pipe (e.g., add a single space
before and after every "|" in that row) so the separator reads with spaced
columns and satisfies markdownlint MD060 while leaving the header and data rows
unchanged.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| &route.PathPrefix, | ||
| &route.PathPattern, | ||
| &route.PatternType, | ||
| &route.ParamNames, | ||
| &route.TargetURL, | ||
| &route.Methods, |
There was a problem hiding this comment.
param_names is selected as a JSONB column (per migration 076), but row.Scan(&route.ParamNames) expects pgx to decode directly into []string. This won’t work for JSONB without explicit unmarshalling (or using pgtype.JSONB). Either switch the column to TEXT[] or adjust scan/exec to marshal/unmarshal JSON.
| PathPrefix: "/api", | ||
| TargetURL: "http://localhost", | ||
| PathPattern: testRoutePattern, | ||
| PatternType: "wildcard", |
There was a problem hiding this comment.
PatternType is set to "wildcard" here, but the DB constraint added in migration 076 only allows pattern_type IN ('prefix','pattern'), and the service logic also checks for PatternType == "pattern". Using "wildcard" will violate the constraint and/or prevent matching. Use "pattern" for wildcard/param/regex patterns and keep "prefix" for plain prefixes.
| PatternType: "wildcard", | |
| PatternType: "pattern", |
| Name: "test-route", | ||
| PathPrefix: "/v1-test", | ||
| PathPattern: "/v1-test/*", | ||
| PatternType: "wildcard", | ||
| ParamNames: []string{}, | ||
| TargetURL: "http://test:80", | ||
| Methods: []string{"GET", "POST"}, | ||
| StripPrefix: true, |
There was a problem hiding this comment.
PatternType is set to "wildcard" but migration 076 enforces pattern_type to be only prefix or pattern (and the service matches only pattern). This value should be changed to pattern (or the constraint/service updated consistently).
| -- Create a composite unique constraint (path_pattern, methods). | ||
| -- Note: This is an approximation since methods is an array. | ||
| ALTER TABLE gateway_routes ADD CONSTRAINT gateway_routes_pattern_methods_key UNIQUE (path_pattern, methods); | ||
|
|
||
| -- Create an index to speed up route lookups by method if needed | ||
| CREATE INDEX idx_gateway_routes_methods ON gateway_routes USING GIN (methods); |
There was a problem hiding this comment.
A unique constraint on (path_pattern, methods) where methods is a TEXT[] is order-sensitive in Postgres, so ['GET','POST'] and ['POST','GET'] will be treated as different and allow duplicates. If uniqueness is intended regardless of ordering, normalize methods (e.g., store sorted/uppercased), or add a generated column (e.g., methods_key as sorted array/string) and unique-index on (path_pattern, methods_key) (or model methods in a join table).
| newProxies[r.ID] = proxy | ||
| if r.PatternType == "pattern" { | ||
| matcher, err := routing.CompilePattern(r.PathPattern) | ||
| if err == nil { | ||
| newMatchers[r.ID] = matcher | ||
| } | ||
| req.Host = target.Host | ||
| } |
There was a problem hiding this comment.
If a route has PatternType == "pattern" but pattern compilation fails, it is still kept in s.routes while no matcher is added. That makes the route permanently unmatchable (since checkRouteMatch requires a matcher entry). Consider skipping such routes entirely (and ideally logging the compile error) so routing behavior is consistent.
| // Sort routes by specificity (longer literal prefixes and higher priority first) | ||
| sort.Slice(routes, func(i, j int) bool { | ||
| scoreI := calculateMatchScore(routes[i], "") | ||
| scoreJ := calculateMatchScore(routes[j], "") | ||
| return scoreI > scoreJ // Descending order |
There was a problem hiding this comment.
sort.Slice is not stable; when two routes have the same calculateMatchScore, their relative order becomes non-deterministic. Since GetProxy picks the first max-score match, this can cause inconsistent routing. Use sort.SliceStable and add explicit tie-breakers (e.g., higher Priority, longer literal prefix, then CreatedAt/ID).
| // Sort routes by specificity (longer literal prefixes and higher priority first) | |
| sort.Slice(routes, func(i, j int) bool { | |
| scoreI := calculateMatchScore(routes[i], "") | |
| scoreJ := calculateMatchScore(routes[j], "") | |
| return scoreI > scoreJ // Descending order | |
| // Sort routes by specificity in a stable and deterministic way. | |
| // Primary order: higher match score first. | |
| // Tie-breakers: higher Priority, longer literal prefix, then PathPattern, then ID. | |
| sort.SliceStable(routes, func(i, j int) bool { | |
| ri := routes[i] | |
| rj := routes[j] | |
| scoreI := calculateMatchScore(ri, "") | |
| scoreJ := calculateMatchScore(rj, "") | |
| if scoreI != scoreJ { | |
| // Higher score first (descending). | |
| return scoreI > scoreJ | |
| } | |
| // Tie-breaker 1: higher Priority first. | |
| if ri.Priority != rj.Priority { | |
| return ri.Priority > rj.Priority | |
| } | |
| // Tie-breaker 2: longer literal prefix first. | |
| litI := len(routing.GetLiteralPrefix(ri.PathPattern)) | |
| litJ := len(routing.GetLiteralPrefix(rj.PathPattern)) | |
| if litI != litJ { | |
| return litI > litJ | |
| } | |
| // Tie-breaker 3: lexicographic order of PathPattern for determinism. | |
| if ri.PathPattern != rj.PathPattern { | |
| return ri.PathPattern < rj.PathPattern | |
| } | |
| // Final tie-breaker: ID to ensure total ordering. | |
| return ri.ID.String() < rj.ID.String() |
| ALTER TABLE gateway_routes | ||
| ADD COLUMN pattern_type VARCHAR(20) DEFAULT 'prefix' NOT NULL, | ||
| ADD COLUMN path_pattern TEXT, | ||
| ADD COLUMN param_names JSONB DEFAULT '[]'::jsonb, |
There was a problem hiding this comment.
param_names is created as JSONB here, but the Go repository scans/inserts it as a []string directly (see internal/repositories/postgres/gateway_repo.go scanning into &route.ParamNames). With pgx this will not decode JSONB into []string automatically, so reads/inserts are likely to fail at runtime. Align the DB type and Go type (e.g., change param_names to TEXT[], or keep JSONB and use pgtype.JSONB/custom marshal+unmarshal in the repository).
| ADD COLUMN param_names JSONB DEFAULT '[]'::jsonb, | |
| ADD COLUMN param_names TEXT[] DEFAULT '{}'::text[], |
| userID := appcontext.UserIDFromContext(ctx) | ||
| if userID == uuid.Nil { | ||
| return nil, fmt.Errorf("unauthorized") | ||
| } |
There was a problem hiding this comment.
Returning fmt.Errorf("unauthorized") will be treated as an unknown error by httputil.Error and surfaced as a 500. This should return a typed API error (e.g., errors.New(errors.Unauthorized, ...)) so callers get a 401.
| // @Security APIKeyAuth | ||
| // @Success 200 {array} domain.GatewayRoute | ||
| // @Failure 500 {object} httputil.Response |
There was a problem hiding this comment.
Swagger annotation says this returns a raw []domain.GatewayRoute, but the handler returns httputil.Response with the routes inside the data field. Update the @Success schema to match the actual envelope so clients generated from OpenAPI don’t break.
| | Pattern Type | Syntax | Example Path | Extracted Params | | ||
| |--------------|--------|--------------|------------------| | ||
| | **Wildcard** | `/api/v1/*` | `/api/v1/users/list` | None | | ||
| | **Parameter** | `/users/{id}` | `/users/123` | `id=123` | | ||
| | **Regex Param**| `/id/{id:[0-9]+}` | `/id/456` | `id=456` | | ||
| | **Extension** | `/files/*.{ext}` | `/files/img.png` | `ext=png` | |
There was a problem hiding this comment.
The markdown table uses || separators, which won’t render as a table in standard Markdown. Use single | delimiters (and keep header/separator rows) so the pattern syntax table renders correctly.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/repositories/postgres/gateway_repo_unit_test.go (1)
20-49:⚠️ Potential issue | 🟡 MinorAdd
mock.ExpectationsWereMet()assertion to verify mocked expectations are called.Without this, the test can pass even if the repository doesn't execute the expected database operation. This should be added after the
err = repo.CreateRoute()call.Assertion to add
err = repo.CreateRoute(context.Background(), route) assert.NoError(t, err) +assert.NoError(t, mock.ExpectationsWereMet())
🧹 Nitpick comments (1)
internal/repositories/postgres/gateway_repo_unit_test.go (1)
14-18: Add constants for numeric test values to avoid magic numbers.RateLimit and Priority literals are used later; define them here (e.g.,
testRateLimit,testPriority) and reuse across tests.💡 Suggested constants
const ( testRouteName = "test-route" testRoutePattern = "/api/*" testTargetURL = "http://localhost" + testRateLimit = 100 + testPriority = 1 )As per coding guidelines:
**/*.go: Do not use magic numbers - use named constants instead.
This PR introduces HTTP method matching for the API Gateway, allowing granular routing rules based on verbs. It also refactors the GatewayService for better maintainability and updates the documentation, SDK, and CLI.
Key changes:
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.