refactor(gateway-controller): split handlers.go into per-resource files #1657
Merged
renuka-fernando merged 4 commits intowso2:mainfrom Apr 8, 2026
Merged
refactor(gateway-controller): split handlers.go into per-resource files #1657renuka-fernando merged 4 commits intowso2:mainfrom
renuka-fernando merged 4 commits intowso2:mainfrom
Conversation
Extract handler methods from the monolithic handlers.go into separate files per resource type, following the existing rest_api_handler.go pattern: - api_key_handler.go — REST API key CRUD + resolveAPIIDByHandle - llm_provider_handler.go — LLM provider CRUD + provider API keys - llm_provider_template_handler.go — LLM provider template CRUD - llm_proxy_handler.go — LLM proxy CRUD + proxy API keys - mcp_proxy_handler.go — MCP proxy CRUD + convertAPIPolicy - secret_handler.go — secret CRUD - subscription_handler.go — subscription CRUD + response helpers - subscription_plan_handler.go — subscription plan CRUD - websub_api_handler.go — WebSub API CRUD handlers.go now contains only the APIServer struct, NewAPIServer, core infrastructure methods, and shared helpers.
Contributor
WalkthroughAdds many new Gin HTTP handlers for API keys, LLM providers/templates/proxies, MCP proxies, secrets, subscriptions and subscription plans; updates Go caching inputs in multiple GitHub Actions workflows; and introduces/standardizes sentinel errors and wrapped error returns in LLM and API-key utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIServer
participant LLMService as LLM Deployment Service
participant DB
participant ControlPlane
Client->>APIServer: POST /llm-proxies (raw body)
APIServer->>APIServer: extract correlation, logger
APIServer->>LLMService: CreateLLMProxy(content, content-type, origin, correlation)
LLMService->>DB: Save proxy config
DB-->>LLMService: saved metadata (handle, createdAt)
LLMService-->>APIServer: success (handle, createdAt, staleFlag)
alt not stale AND ControlPlane connected AND push enabled
APIServer->>ControlPlane: waitForDeploymentAndPush(handle)
ControlPlane-->>APIServer: push result (async)
end
APIServer-->>Client: 201 Created (id/createdAt)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
gateway/gateway-controller/pkg/api/handlers/websub_api_handler.go (1)
30-71: Consider consolidating repeated 501 response code into a shared helper.Lines 30-71 duplicate the same payload in every handler. A small helper + constant will reduce drift risk when message/status changes.
♻️ Suggested cleanup
+const webSubNotImplementedMessage = "WebSub API management is not implemented" + +func (s *APIServer) respondWebSubNotImplemented(c *gin.Context) { + c.JSON(http.StatusNotImplemented, api.ErrorResponse{ + Status: "error", + Message: webSubNotImplementedMessage, + }) +} + func (s *APIServer) CreateWebSubAPI(c *gin.Context) { - c.JSON(http.StatusNotImplemented, api.ErrorResponse{ - Status: "error", - Message: "WebSub API management is not implemented", - }) + s.respondWebSubNotImplemented(c) } func (s *APIServer) ListWebSubAPIs(c *gin.Context, params api.ListWebSubAPIsParams) { - c.JSON(http.StatusNotImplemented, api.ErrorResponse{ - Status: "error", - Message: "WebSub API management is not implemented", - }) + s.respondWebSubNotImplemented(c) } func (s *APIServer) GetWebSubAPIById(c *gin.Context, id string) { - c.JSON(http.StatusNotImplemented, api.ErrorResponse{ - Status: "error", - Message: "WebSub API management is not implemented", - }) + s.respondWebSubNotImplemented(c) } func (s *APIServer) UpdateWebSubAPI(c *gin.Context, id string) { - c.JSON(http.StatusNotImplemented, api.ErrorResponse{ - Status: "error", - Message: "WebSub API management is not implemented", - }) + s.respondWebSubNotImplemented(c) } func (s *APIServer) DeleteWebSubAPI(c *gin.Context, id string) { - c.JSON(http.StatusNotImplemented, api.ErrorResponse{ - Status: "error", - Message: "WebSub API management is not implemented", - }) + s.respondWebSubNotImplemented(c) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/websub_api_handler.go` around lines 30 - 71, Multiple handlers (CreateWebSubAPI, ListWebSubAPIs, GetWebSubAPIById, UpdateWebSubAPI, DeleteWebSubAPI) return the same 501 JSON payload repeatedly; extract a shared constant for the status/message and a small helper method (e.g., writeNotImplemented(c *gin.Context) or s.writeNotImplemented) that sends c.JSON(http.StatusNotImplemented, api.ErrorResponse{...}) and replace each handler body to call that helper to avoid duplication and reduce drift risk..github/workflows/sample-service-pr-check.yml (1)
23-23: Include workspace files in cache key to avoid stale cache hits.The
cache-dependency-pathon line 23 only hashes**/go.sum, but this repo uses a Go workspace withgo.workandgo.work.sumat the root. Sincesamples/sample-service(the workspace member) has no localgo.sumfile, workspace dependency changes tracked ingo.work.sumwon't invalidate the cache. Addgo.workandgo.work.sumto the cache key.💡 Proposed update
- cache-dependency-path: '**/go.sum' + cache-dependency-path: | + **/go.sum + go.work + go.work.sum🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/sample-service-pr-check.yml at line 23, Update the cache key configuration so workspace files are included: modify the cache-dependency-path entry (symbol: cache-dependency-path) to include go.work and go.work.sum in addition to **/go.sum, ensuring changes in the repository-level workspace files will invalidate the cache and avoid stale cache hits for workspace members like samples/sample-service.gateway/gateway-controller/pkg/api/handlers/secret_handler.go (3)
91-93: Inconsistent logger usage: some handlers uses.loggerdirectly.
ListSecrets,GetSecret, andDeleteSecretuses.loggerdirectly (lines 92, 148, 310), whileCreateSecretandUpdateSecretusemiddleware.GetLogger(c, s.logger). The middleware version includes request-scoped context. Consider usingmiddleware.GetLoggerconsistently.♻️ Suggested fix for ListSecrets
func (s *APIServer) ListSecrets(c *gin.Context) { - log := s.logger + log := middleware.GetLogger(c, s.logger) correlationID := middleware.GetCorrelationID(c)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/secret_handler.go` around lines 91 - 93, ListSecrets, GetSecret, and DeleteSecret use s.logger directly while CreateSecret and UpdateSecret use middleware.GetLogger(c, s.logger); update the former three to obtain a request-scoped logger by calling middleware.GetLogger(c, s.logger) at the start of each handler (e.g., in ListSecrets, GetSecret, DeleteSecret) and replace uses of s.logger in those functions with the returned log variable so all handlers consistently include request context.
368-369: Consider returning a response body for DELETE success.
DeleteSecretreturnsc.Status(http.StatusOK)with no body (line 369), while other handlers return JSON responses. For API consistency and client debugging, consider returning a JSON body similar to other delete handlers.🔧 Suggested fix
- // Return 200 OK on successful deletion - c.Status(http.StatusOK) + // Return 200 OK on successful deletion + c.JSON(http.StatusOK, gin.H{ + "status": "success", + "message": "Secret deleted successfully", + "id": id, + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/secret_handler.go` around lines 368 - 369, The DELETE handler DeleteSecret currently responds with c.Status(http.StatusOK) and no body; change it to return a JSON response consistent with other handlers (use c.JSON(http.StatusOK, ...) ) containing a clear status and identifying info (e.g., {"status":"ok","message":"secret deleted","id": <secretID>} or similar), retrieving the secret ID from the request/context the same way DeleteSecret currently uses; ensure the response shape matches existing delete handlers for consistency.
67-75: Misleading error log message.The log message "Failed to encrypt Secret" (line 68) is misleading since
CreateSecretcan fail for parsing, validation, conflict, or persistence reasons—not just encryption. Consider a more accurate message like "Failed to create secret".🔧 Suggested fix
if err != nil { - log.Error("Failed to encrypt Secret", slog.Any("error", err)) + log.Error("Failed to create secret", slog.Any("error", err)) if storage.IsConflictError(err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/secret_handler.go` around lines 67 - 75, The log message in the CreateSecret handler is misleading—replace the specific "Failed to encrypt Secret" message with a more accurate, generic message such as "Failed to create secret" (while still logging the error with slog.Any) in the error handling block that uses storage.IsConflictError and c.JSON to return the response; update the slog.Error call so it reads a generic "Failed to create secret" and retains slog.Any("error", err) for context.gateway/gateway-controller/pkg/api/handlers/llm_provider_handler.go (3)
238-254: Log level mismatch:Warnused before error classification.Line 240 logs at
Warnlevel regardless of error type, but line 249 returns HTTP 500 for non-"not found" errors. Consider logging at appropriate level after classifying the error.🔧 Suggested fix
cfg, err := s.llmDeploymentService.DeleteLLMProvider(id, correlationID, log) if err != nil { - log.Warn("Failed to delete LLM provider configuration", slog.String("handle", id)) // Check if it's a not found error if strings.Contains(err.Error(), "not found") { + log.Warn("LLM provider configuration not found for deletion", slog.String("handle", id)) c.JSON(http.StatusNotFound, api.ErrorResponse{ Status: "error", Message: err.Error(), }) return } + log.Error("Failed to delete LLM provider configuration", slog.String("handle", id), slog.Any("error", err)) c.JSON(http.StatusInternalServerError, api.ErrorResponse{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/llm_provider_handler.go` around lines 238 - 254, The handler currently logs a warning before classifying the error from llmDeploymentService.DeleteLLMProvider; update the flow to classify the error first (check strings.Contains(err.Error(), "not found")) and then log at the appropriate level: use log.Warn (or Info) including context for "not found" cases and log.Error (including the error details and correlationID/handle) for other errors before returning the corresponding HTTP response in the DeleteLLMProvider handler.
50-57: Silently ignoredjson.Marshalerror could mask data corruption.The error from
json.Marshalon line 50 is discarded. While unlikely to fail for in-memory structs, silently ignoring it could mask issues with unexpected types incfg.SourceConfiguration.🔧 Suggested fix
- j, _ := json.Marshal(cfg.SourceConfiguration) - if err := json.Unmarshal(j, &prov); err != nil { + j, marshalErr := json.Marshal(cfg.SourceConfiguration) + if marshalErr != nil { + log.Error("Failed to marshal stored LLM provider configuration", + slog.String("uuid", cfg.UUID), slog.Any("error", marshalErr)) + c.JSON(http.StatusInternalServerError, api.ErrorResponse{Status: "error", + Message: "Failed to get stored LLM provider configuration"}) + return + } + if err := json.Unmarshal(j, &prov); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/llm_provider_handler.go` around lines 50 - 57, The json.Marshal error is being ignored for cfg.SourceConfiguration; change the code to capture and handle that error before proceeding to json.Unmarshal: replace the ignored marshal with j, err := json.Marshal(cfg.SourceConfiguration) and if err != nil log the failure (use the same logger/slog context as the existing unmarshalling error, include cfg.UUID and the err), return an HTTP 500 via c.JSON with an appropriate ErrorResponse, and only call json.Unmarshal(&prov) when the marshal succeeds.
141-158: Inconsistent "not found" error detection across handlers.
GetLLMProviderByIduses bothstorage.IsNotFoundError(err)andstrings.Contains(line 142), whileDeleteLLMProvider(line 242) only usesstrings.Contains. Consider standardizing onstorage.IsNotFoundErrorfor type-safe error checking.♻️ Suggested consistency fix
if err != nil { - if !storage.IsNotFoundError(err) && !strings.Contains(strings.ToLower(err.Error()), "not found") { + if storage.IsNotFoundError(err) { + log.Warn("LLM provider configuration not found", + slog.String("handle", id), + slog.Any("error", err)) + c.JSON(http.StatusNotFound, api.ErrorResponse{ + Status: "error", + Message: fmt.Sprintf("LLM provider configuration with handle '%s' not found", id), + }) + return + } + if !strings.Contains(strings.ToLower(err.Error()), "not found") { log.Error("Failed to look up LLM provider", slog.Any("error", err))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/llm_provider_handler.go` around lines 141 - 158, The two handlers (GetLLMProviderById and DeleteLLMProvider) use different "not found" detection—one mixes storage.IsNotFoundError(err) with strings.Contains(err.Error(), "not found") while the other only uses string matching; standardize to the type-safe check by replacing any strings.Contains(... "not found") usage with storage.IsNotFoundError(err) in both handlers (look for the error handling blocks in GetLLMProviderById and DeleteLLMProvider), remove the redundant string-based check, and adjust the conditional branches/logging to rely solely on storage.IsNotFoundError(err) for deciding between 404 and 500 responses.gateway/gateway-controller/pkg/api/handlers/subscription_handler.go (1)
66-77: Conflated error and nil-plan check may mask actual errors.Line 68 combines
err != nil || plan == nilin one condition with identical handling. IfGetSubscriptionPlanByIDreturns an actual error (e.g., database connection issue), it's logged as "plan not found" rather than surfacing the underlying error.♻️ Suggested fix
if req.SubscriptionPlanId != nil && *req.SubscriptionPlanId != "" { plan, err := s.db.GetSubscriptionPlanByID(*req.SubscriptionPlanId, "") - if err != nil || plan == nil { - log.Warn("Subscription plan not found for subscription creation", - slog.String("subscription_plan_id", *req.SubscriptionPlanId), - slog.String("api_id", apiID)) + if err != nil { + if storage.IsNotFoundError(err) { + log.Warn("Subscription plan not found", slog.String("subscription_plan_id", *req.SubscriptionPlanId)) + } else { + log.Error("Failed to get subscription plan", slog.String("subscription_plan_id", *req.SubscriptionPlanId), slog.Any("error", err)) + } + c.JSON(http.StatusBadRequest, api.ErrorResponse{ + Status: "error", + Message: "Subscription plan not found or not enabled", + }) + return + } + if plan == nil { + log.Warn("Subscription plan not found", slog.String("subscription_plan_id", *req.SubscriptionPlanId)) c.JSON(http.StatusBadRequest, api.ErrorResponse{ Status: "error", Message: "Subscription plan not found or not enabled",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/subscription_handler.go` around lines 66 - 77, Separate the error and nil-plan checks when calling s.db.GetSubscriptionPlanByID in the subscription creation flow: if err != nil, log the actual error (use log.Error or similar and include err, subscription_plan_id and apiID) and respond with an internal server error (500); else if plan == nil, keep the current warning (log.Warn with subscription_plan_id and apiID) and return the existing bad request response (400) indicating "Subscription plan not found or not enabled". Update the branch around s.db.GetSubscriptionPlanByID, req.SubscriptionPlanId, log.Warn/log.Error and the c.JSON responses accordingly so real DB/errors are not masked as "plan not found."gateway/gateway-controller/pkg/api/handlers/mcp_proxy_handler.go (1)
151-163: Silently ignoredjson.Marshalerror (same pattern as LLM provider handler).Similar to
llm_provider_handler.go, thejson.Marshalerror is discarded on line 152. Consider handling it consistently across handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/mcp_proxy_handler.go` around lines 151 - 163, The json.Marshal error for cfg.SourceConfiguration is being ignored; update the block around json.Marshal/json.Unmarshal so you check the error returned by json.Marshal(cfg.SourceConfiguration) and handle it the same way as the Unmarshal error: log the error via s.logger.Error (include the error and identifying fields like cfg.UUID and cfg.DisplayName), return an HTTP 500 JSON error response via c.JSON with an appropriate message, and return early; keep the existing json.Unmarshal handling for mcp unchanged (refer to variables/functions json.Marshal, json.Unmarshal, mcp, cfg.SourceConfiguration, s.logger.Error, and c.JSON).gateway/gateway-controller/pkg/api/handlers/subscription_plan_handler.go (1)
77-81: Variable shadowing:sshadows the receiver.Line 79 declares
s := string(*req.ThrottleLimitUnit), which shadows the*APIServerreceivers. This is a code smell that could cause confusion or bugs if the receiver is accidentally used within this scope.♻️ Suggested fix
var unitStr *string if req.ThrottleLimitUnit != nil { - s := string(*req.ThrottleLimitUnit) - unitStr = &s + u := string(*req.ThrottleLimitUnit) + unitStr = &u }The same shadowing occurs at lines 112, 203, and 226—consider renaming all instances.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/subscription_plan_handler.go` around lines 77 - 81, The code is shadowing the method receiver named s by declaring local variables like s := string(*req.ThrottleLimitUnit); rename those locals (e.g., unitVal or throttleUnitStr) and assign using val := string(...) then unitStr = &val (or use var unitVal = string(...); unitStr = &unitVal) to avoid shadowing the receiver; apply the same renaming pattern to the other occurrences (the locals at the positions corresponding to lines ~112, ~203, and ~226) in subscription_plan_handler.go so no local variable reuses the receiver name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/gateway-controller/pkg/api/handlers/api_key_handler.go`:
- Around line 92-96: Several handlers in api_key_handler.go are returning raw
backend error strings (err.Error()) in 500 responses; instead, log the detailed
error and return a stable generic 500 message. For each place that does
c.JSON(http.StatusInternalServerError, api.ErrorResponse{Status: "error",
Message: err.Error()}) (occurring around the blocks at 92-96, 148-151, 239-242,
309-312, 362-365), replace the response to use a fixed message like "internal
server error" (or "failed to process request") while calling the existing logger
(or fmt/log) to record the full err with context; keep api.ErrorResponse for
structure but do not include err.Error() in Message. Ensure this change is
applied consistently across the handler functions in this file so internal
errors are logged but not leaked to clients.
In
`@gateway/gateway-controller/pkg/api/handlers/llm_provider_template_handler.go`:
- Around line 57-63: The handler currently maps all errors (e.g., in the
template parsing and CRUD branches that call log.Error and c.JSON with 400/404)
to client errors; instead, change error handling in the template handler
functions (where you call log.Error, c.JSON and return after parsing/CRUD
failures) to distinguish validation and not-found errors from internal failures:
use errors.Is/As against your domain errors (e.g., api.ErrNotFound, validation
error types) or explicit sentinel errors returned by the template service,
return 400 for validation failures and 404 for not-found, and for any other
error return a 500 with a generic message while logging the full error (keep the
existing slog.Any/ log.Error usage for internal errors); apply this change to
all similar branches (the parse branch and the create/get/update/delete branches
referenced).
In `@gateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.go`:
- Around line 101-110: The current error handling in the LLM proxy creation
paths (e.g., the block using utils.IsPolicyDefinitionMissingError and returning
err.Error()) exposes internal error details and maps many backend failures to
400; change these handlers to explicitly detect and return client-safe 4xx
responses only for known validation/not-found/conflict cases (e.g.,
utils.IsPolicyDefinitionMissingError -> return the existing
PolicyDefinitionMissingUserMessage with 4xx), and for all other errors log the
full error with log.Error (keep slog.Any("error", err)) but return a stable
generic 500 response message (do not include err.Error()) to the client via
c.JSON. Apply the same pattern to the other similar handlers noted (the other
blocks around the listed ranges) so only explicit client-error helpers produce
4xx and all unexpected/internal failures produce a logged 500 with a generic
user message.
In `@gateway/gateway-controller/pkg/api/handlers/mcp_proxy_handler.go`:
- Around line 287-295: In UpdateMCPProxy, don't map every error to 404: inspect
the returned error from the lookup (use errors.Is/err == store.ErrNotFound or
your repository's sentinel error) and only return http.StatusNotFound for that
case; for validation errors return http.StatusBadRequest, and for unexpected
DB/internal errors return http.StatusInternalServerError. Update the log call
(log.Warn/log.Error) to include the actual error (err) and context (handle) and
use c.JSON with api.ErrorResponse using the appropriate HTTP status depending on
the error kind.
In `@gateway/gateway-controller/pkg/api/handlers/rest_api_handler.go`:
- Around line 76-85: The current error branch after h.parser.Parse(...)
incorrectly increments metrics.ValidationErrorsTotal with the "read_body_failed"
label (and the same issue around lines 183-192); change those increments to a
parse-specific label (e.g., "parse_body_failed" or "invalid_payload") so the
metric reflects parser/malformed payload errors rather than transport/read
failures—update the metrics.ValidationErrorsTotal.WithLabelValues(operation,
"<new_parse_label>").Inc() calls in the handler where h.parser.Parse is used
(and the analogous second branch) to use the new parse-specific label.
---
Nitpick comments:
In @.github/workflows/sample-service-pr-check.yml:
- Line 23: Update the cache key configuration so workspace files are included:
modify the cache-dependency-path entry (symbol: cache-dependency-path) to
include go.work and go.work.sum in addition to **/go.sum, ensuring changes in
the repository-level workspace files will invalidate the cache and avoid stale
cache hits for workspace members like samples/sample-service.
In `@gateway/gateway-controller/pkg/api/handlers/llm_provider_handler.go`:
- Around line 238-254: The handler currently logs a warning before classifying
the error from llmDeploymentService.DeleteLLMProvider; update the flow to
classify the error first (check strings.Contains(err.Error(), "not found")) and
then log at the appropriate level: use log.Warn (or Info) including context for
"not found" cases and log.Error (including the error details and
correlationID/handle) for other errors before returning the corresponding HTTP
response in the DeleteLLMProvider handler.
- Around line 50-57: The json.Marshal error is being ignored for
cfg.SourceConfiguration; change the code to capture and handle that error before
proceeding to json.Unmarshal: replace the ignored marshal with j, err :=
json.Marshal(cfg.SourceConfiguration) and if err != nil log the failure (use the
same logger/slog context as the existing unmarshalling error, include cfg.UUID
and the err), return an HTTP 500 via c.JSON with an appropriate ErrorResponse,
and only call json.Unmarshal(&prov) when the marshal succeeds.
- Around line 141-158: The two handlers (GetLLMProviderById and
DeleteLLMProvider) use different "not found" detection—one mixes
storage.IsNotFoundError(err) with strings.Contains(err.Error(), "not found")
while the other only uses string matching; standardize to the type-safe check by
replacing any strings.Contains(... "not found") usage with
storage.IsNotFoundError(err) in both handlers (look for the error handling
blocks in GetLLMProviderById and DeleteLLMProvider), remove the redundant
string-based check, and adjust the conditional branches/logging to rely solely
on storage.IsNotFoundError(err) for deciding between 404 and 500 responses.
In `@gateway/gateway-controller/pkg/api/handlers/mcp_proxy_handler.go`:
- Around line 151-163: The json.Marshal error for cfg.SourceConfiguration is
being ignored; update the block around json.Marshal/json.Unmarshal so you check
the error returned by json.Marshal(cfg.SourceConfiguration) and handle it the
same way as the Unmarshal error: log the error via s.logger.Error (include the
error and identifying fields like cfg.UUID and cfg.DisplayName), return an HTTP
500 JSON error response via c.JSON with an appropriate message, and return
early; keep the existing json.Unmarshal handling for mcp unchanged (refer to
variables/functions json.Marshal, json.Unmarshal, mcp, cfg.SourceConfiguration,
s.logger.Error, and c.JSON).
In `@gateway/gateway-controller/pkg/api/handlers/secret_handler.go`:
- Around line 91-93: ListSecrets, GetSecret, and DeleteSecret use s.logger
directly while CreateSecret and UpdateSecret use middleware.GetLogger(c,
s.logger); update the former three to obtain a request-scoped logger by calling
middleware.GetLogger(c, s.logger) at the start of each handler (e.g., in
ListSecrets, GetSecret, DeleteSecret) and replace uses of s.logger in those
functions with the returned log variable so all handlers consistently include
request context.
- Around line 368-369: The DELETE handler DeleteSecret currently responds with
c.Status(http.StatusOK) and no body; change it to return a JSON response
consistent with other handlers (use c.JSON(http.StatusOK, ...) ) containing a
clear status and identifying info (e.g., {"status":"ok","message":"secret
deleted","id": <secretID>} or similar), retrieving the secret ID from the
request/context the same way DeleteSecret currently uses; ensure the response
shape matches existing delete handlers for consistency.
- Around line 67-75: The log message in the CreateSecret handler is
misleading—replace the specific "Failed to encrypt Secret" message with a more
accurate, generic message such as "Failed to create secret" (while still logging
the error with slog.Any) in the error handling block that uses
storage.IsConflictError and c.JSON to return the response; update the slog.Error
call so it reads a generic "Failed to create secret" and retains
slog.Any("error", err) for context.
In `@gateway/gateway-controller/pkg/api/handlers/subscription_handler.go`:
- Around line 66-77: Separate the error and nil-plan checks when calling
s.db.GetSubscriptionPlanByID in the subscription creation flow: if err != nil,
log the actual error (use log.Error or similar and include err,
subscription_plan_id and apiID) and respond with an internal server error (500);
else if plan == nil, keep the current warning (log.Warn with
subscription_plan_id and apiID) and return the existing bad request response
(400) indicating "Subscription plan not found or not enabled". Update the branch
around s.db.GetSubscriptionPlanByID, req.SubscriptionPlanId, log.Warn/log.Error
and the c.JSON responses accordingly so real DB/errors are not masked as "plan
not found."
In `@gateway/gateway-controller/pkg/api/handlers/subscription_plan_handler.go`:
- Around line 77-81: The code is shadowing the method receiver named s by
declaring local variables like s := string(*req.ThrottleLimitUnit); rename those
locals (e.g., unitVal or throttleUnitStr) and assign using val := string(...)
then unitStr = &val (or use var unitVal = string(...); unitStr = &unitVal) to
avoid shadowing the receiver; apply the same renaming pattern to the other
occurrences (the locals at the positions corresponding to lines ~112, ~203, and
~226) in subscription_plan_handler.go so no local variable reuses the receiver
name.
In `@gateway/gateway-controller/pkg/api/handlers/websub_api_handler.go`:
- Around line 30-71: Multiple handlers (CreateWebSubAPI, ListWebSubAPIs,
GetWebSubAPIById, UpdateWebSubAPI, DeleteWebSubAPI) return the same 501 JSON
payload repeatedly; extract a shared constant for the status/message and a small
helper method (e.g., writeNotImplemented(c *gin.Context) or
s.writeNotImplemented) that sends c.JSON(http.StatusNotImplemented,
api.ErrorResponse{...}) and replace each handler body to call that helper to
avoid duplication and reduce drift risk.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97b4deab-ba8a-4ecb-962c-bed1ac8fa4cf
📒 Files selected for processing (28)
.github/workflows/cli-gw-build-periodic.yml.github/workflows/cli-release.yml.github/workflows/codecov.yml.github/workflows/gateway-integration-test-postgres.yml.github/workflows/gateway-integration-test.yml.github/workflows/gateway-release.yml.github/workflows/go-scan.yaml.github/workflows/operator-integration-test.yml.github/workflows/platform-api-pr-check.yml.github/workflows/platform-api-release.yml.github/workflows/sample-service-pr-check.yml.github/workflows/sample-service-release.yml.github/workflows/sdk-integration-test.ymlgateway/gateway-controller/pkg/api/handlers/api_key_handler.gogateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/api/handlers/handlers_test.gogateway/gateway-controller/pkg/api/handlers/llm_provider_handler.gogateway/gateway-controller/pkg/api/handlers/llm_provider_template_handler.gogateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.gogateway/gateway-controller/pkg/api/handlers/mcp_proxy_handler.gogateway/gateway-controller/pkg/api/handlers/rest_api_handler.gogateway/gateway-controller/pkg/api/handlers/secret_handler.gogateway/gateway-controller/pkg/api/handlers/subscription_handler.gogateway/gateway-controller/pkg/api/handlers/subscription_plan_handler.gogateway/gateway-controller/pkg/api/handlers/websub_api_handler.gogateway/gateway-controller/pkg/service/errors.gogateway/gateway-controller/pkg/service/restapi_service.gogateway/gateway-controller/pkg/service/restapi_service_impl.go
renuka-fernando
force-pushed
the
fix-1649
branch
from
f3b9027 to
4bb8692
Compare
Distinguish 4xx vs 5xx errors by sentinel type in the LLM proxy, template, and API key handlers. Prevents internal error details from leaking in 500 responses and fixes misclassified errors (e.g. not-found returned as 400, DB failures returned as 400/404). - Add ErrLLMProxyValidation, ErrLLMTemplateValidation, and ErrLLMTemplateNotFound sentinels; wrap service errors with %w so handlers can use errors.Is/storage.Is* helpers - Fix DeployLLMProxyConfiguration, UpdateLLMProxy, DeleteLLMProxy to propagate storage.ErrNotFound and ErrLLMProxyValidation - Fix api_key.go not-found returns to wrap storage.ErrNotFound with %w - Handlers now return 400 for validation, 404 for not-found, 409 for conflict, 500 (generic message + log.Error) for all other failures
Contributor
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)
gateway/gateway-controller/pkg/utils/llm_deployment.go (1)
1101-1125:⚠️ Potential issue | 🟠 MajorMalformed proxy updates still miss the new validation sentinel.
UpdateLLMProxyparses the body inisLLMProxyUndeployRequestbefore it reachesDeployLLMProxyConfiguration. That helper still returns a plain parse error, sogateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.gofalls through to 500 instead of the new 400 validation path on bad PUT payloads.🩹 Suggested fix
Apply the same sentinel in the pre-parse helper:
func (s *LLMDeploymentService) isLLMProxyUndeployRequest(params LLMDeploymentParams) (bool, error) { var proxyConfig api.LLMProxyConfiguration if err := s.parser.Parse(params.Data, params.ContentType, &proxyConfig); err != nil { - return false, fmt.Errorf("failed to parse proxy configuration: %w", err) + return false, fmt.Errorf("%w: failed to parse proxy configuration: %v", ErrLLMProxyValidation, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/utils/llm_deployment.go` around lines 1101 - 1125, The pre-parse helper isLLMProxyUndeployRequest is returning raw parse errors so UpdateLLMProxy surfaces a 500 instead of the intended 400 validation error; modify isLLMProxyUndeployRequest to apply the same validation sentinel used in DeployLLMProxyConfiguration (wrap or convert JSON/body parse errors into the validation sentinel type/error) so malformed PUT payloads produce the validation 400 path; ensure UpdateLLMProxy still calls isLLMProxyUndeployRequest and that DeployLLMProxyConfiguration continues to receive validated params.
♻️ Duplicate comments (2)
gateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.go (2)
214-221:⚠️ Potential issue | 🟠 MajorMap update conflicts to 409 as well.
UpdateLLMProxygoes through the same deployment/conflict checks as create. If the service returnsstorage.ErrConflict, this block currently falls through to 500.🩹 Suggested fix
if errors.Is(err, utils.ErrLLMProxyValidation) { log.Warn("LLM proxy configuration invalid", slog.Any("error", err)) c.JSON(http.StatusBadRequest, api.ErrorResponse{Status: "error", Message: err.Error()}) return } + if storage.IsConflictError(err) { + c.JSON(http.StatusConflict, api.ErrorResponse{Status: "error", Message: err.Error()}) + return + } log.Error("Failed to update LLM proxy configuration", slog.String("id", id), slog.Any("error", err))Also applies to: 229-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.go` around lines 214 - 221, The error handling in UpdateLLMProxy currently maps only storage.IsNotFoundError to 404 and otherwise returns 500; update the handler to also detect storage.ErrConflict (or via a helper like storage.IsConflictError) and return HTTP 409 with a clear conflict message. Locate the error handling branches around the existing storage.IsNotFoundError check in UpdateLLMProxy (and the similar block at the second occurrence) and add a conditional branch that checks for storage.ErrConflict and responds with c.JSON(http.StatusConflict, api.ErrorResponse{Status:"error", Message: fmt.Sprintf("LLM proxy configuration with handle '%s' conflict", id)}) before falling through to the generic 500 case.
102-109:⚠️ Potential issue | 🟠 MajorKeep unknown policy references on the 4xx path.
When
IsPolicyDefinitionMissingErroris true, the submitted proxy config is invalid from the client's perspective. Returning 500 here still reports a client-correctable payload problem as a server fault.🩹 Suggested fix
Use the same friendly message, but keep the status code in the client-error range in both branches:
- c.JSON(http.StatusInternalServerError, api.ErrorResponse{ + c.JSON(http.StatusBadRequest, api.ErrorResponse{ Status: "error", Message: utils.PolicyDefinitionMissingUserMessage, })Also applies to: 222-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.go` around lines 102 - 109, The handler currently treats utils.IsPolicyDefinitionMissingError as a server error and returns http.StatusInternalServerError; instead return a 4xx client error (e.g., http.StatusBadRequest or http.StatusUnprocessableEntity) while keeping the same friendly message (utils.PolicyDefinitionMissingUserMessage). Update the c.JSON call in the IsPolicyDefinitionMissingError branch (where log.Error is called) to use a 4xx status code, and apply the same change to the other occurrence around lines 222-228 so any invalid/unknown policy references return a client-error status rather than 500.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/gateway-controller/pkg/utils/llm_deployment.go`:
- Around line 746-749: The code currently converts any non-nil error returned by
s.GetLLMProviderTemplateByHandle into ErrLLMTemplateNotFound which hides real
DB/store errors; update the lookup handling in the function (and the similar
block used in DeleteLLMProviderTemplate) to inspect the returned error: if it is
a not-found sentinel from the store return ErrLLMTemplateNotFound (wrapping as
needed), otherwise return or wrap the underlying error (preserving the original
error) so backend failures surface as 5xx instead of 404.
---
Outside diff comments:
In `@gateway/gateway-controller/pkg/utils/llm_deployment.go`:
- Around line 1101-1125: The pre-parse helper isLLMProxyUndeployRequest is
returning raw parse errors so UpdateLLMProxy surfaces a 500 instead of the
intended 400 validation error; modify isLLMProxyUndeployRequest to apply the
same validation sentinel used in DeployLLMProxyConfiguration (wrap or convert
JSON/body parse errors into the validation sentinel type/error) so malformed PUT
payloads produce the validation 400 path; ensure UpdateLLMProxy still calls
isLLMProxyUndeployRequest and that DeployLLMProxyConfiguration continues to
receive validated params.
---
Duplicate comments:
In `@gateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.go`:
- Around line 214-221: The error handling in UpdateLLMProxy currently maps only
storage.IsNotFoundError to 404 and otherwise returns 500; update the handler to
also detect storage.ErrConflict (or via a helper like storage.IsConflictError)
and return HTTP 409 with a clear conflict message. Locate the error handling
branches around the existing storage.IsNotFoundError check in UpdateLLMProxy
(and the similar block at the second occurrence) and add a conditional branch
that checks for storage.ErrConflict and responds with
c.JSON(http.StatusConflict, api.ErrorResponse{Status:"error", Message:
fmt.Sprintf("LLM proxy configuration with handle '%s' conflict", id)}) before
falling through to the generic 500 case.
- Around line 102-109: The handler currently treats
utils.IsPolicyDefinitionMissingError as a server error and returns
http.StatusInternalServerError; instead return a 4xx client error (e.g.,
http.StatusBadRequest or http.StatusUnprocessableEntity) while keeping the same
friendly message (utils.PolicyDefinitionMissingUserMessage). Update the c.JSON
call in the IsPolicyDefinitionMissingError branch (where log.Error is called) to
use a 4xx status code, and apply the same change to the other occurrence around
lines 222-228 so any invalid/unknown policy references return a client-error
status rather than 500.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce722b7a-d494-4a3d-964c-41b0cc1a6bd1
📒 Files selected for processing (5)
gateway/gateway-controller/pkg/api/handlers/api_key_handler.gogateway/gateway-controller/pkg/api/handlers/llm_provider_template_handler.gogateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.gogateway/gateway-controller/pkg/utils/api_key.gogateway/gateway-controller/pkg/utils/llm_deployment.go
✅ Files skipped from review due to trivial changes (1)
- gateway/gateway-controller/pkg/api/handlers/api_key_handler.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/gateway-controller/pkg/api/handlers/llm_provider_template_handler.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/utils/llm_deployment.go (1)
379-379: Consider preserving the full error chain with%w.Using
%vfor the inner error loses its type information in the error chain. While current handlers only check for the sentinel viaerrors.Is, preserving the chain allows future callers to inspect the underlying parse error.♻️ Suggested improvement
- return nil, fmt.Errorf("%w: failed to parse proxy configuration: %v", ErrLLMProxyValidation, err) + return nil, fmt.Errorf("%w: failed to parse proxy configuration: %w", ErrLLMProxyValidation, err)Similarly at line 533:
- return nil, fmt.Errorf("%w: failed to parse template configuration: %v", ErrLLMTemplateValidation, err) + return nil, fmt.Errorf("%w: failed to parse template configuration: %w", ErrLLMTemplateValidation, err)Also applies to: 533-533
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/utils/llm_deployment.go` at line 379, The error construction is losing the wrapped error by using "%v"; update the fmt.Errorf calls that return ErrLLMProxyValidation (the instance referenced as ErrLLMProxyValidation in llm_deployment.go) to wrap the inner parse error with "%w" so the original error chain is preserved (apply this change at the occurrences noted around the current return at line 379 and the similar occurrence near line 533), ensuring callers can use errors.Unwrap / errors.Is to inspect the underlying parse error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@gateway/gateway-controller/pkg/utils/llm_deployment.go`:
- Line 379: The error construction is losing the wrapped error by using "%v";
update the fmt.Errorf calls that return ErrLLMProxyValidation (the instance
referenced as ErrLLMProxyValidation in llm_deployment.go) to wrap the inner
parse error with "%w" so the original error chain is preserved (apply this
change at the occurrences noted around the current return at line 379 and the
similar occurrence near line 533), ensuring callers can use errors.Unwrap /
errors.Is to inspect the underlying parse error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2638f006-903b-406b-a244-bb9aba296feb
📒 Files selected for processing (1)
gateway/gateway-controller/pkg/utils/llm_deployment.go
renuka-fernando
force-pushed
the
fix-1649
branch
from
ae436a7 to
7a642de
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
gateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.go (1)
103-109:⚠️ Potential issue | 🟠 MajorReturn a client error for missing policy definitions, not 500.
Line 103 and Line 229 classify
utils.IsPolicyDefinitionMissingError(err)as internal server error. This is a request-side semantic failure and should be returned as 4xx (typically 400) with the same safe message.Proposed patch
- c.JSON(http.StatusInternalServerError, api.ErrorResponse{ + c.JSON(http.StatusBadRequest, api.ErrorResponse{ Status: "error", Message: utils.PolicyDefinitionMissingUserMessage, })Apply in both
CreateLLMProxyandUpdateLLMProxy.Also applies to: 229-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.go` around lines 103 - 109, The handler treats utils.IsPolicyDefinitionMissingError as a 500 but it should be a client error; update both CreateLLMProxy and UpdateLLMProxy handlers to return HTTP 400 (Bad Request) instead of http.StatusInternalServerError when utils.IsPolicyDefinitionMissingError(err) is true, keeping the same api.ErrorResponse.Message (utils.PolicyDefinitionMissingUserMessage) and log call; ensure the change is applied in both the error branches that currently call c.JSON(http.StatusInternalServerError, ...) for that check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.go`:
- Around line 156-182: GetLLMProxyByHandle may return (nil, nil) so add a
nil-check on cfg immediately after the call in the handler (the block that
builds proxyDetail and accesses cfg.SourceConfiguration, cfg.DesiredState,
cfg.CreatedAt, cfg.UpdatedAt); if cfg == nil return a 404 JSON error (like the
storage.IsNotFoundError branch) with a clear message and log.Warn including the
handle, then return to avoid dereferencing nil.
- Around line 51-53: The code currently discards the error from json.Marshal
when converting cfg.SourceConfiguration (j, _ :=
json.Marshal(cfg.SourceConfiguration)) and always treats failures as
json.Unmarshal errors; change this to capture and check the marshal error before
attempting to unmarshal (e.g., j, err := json.Marshal(cfg.SourceConfiguration)),
and if marshal fails log a clear error referencing cfg.UUID and return/skip
processing rather than proceeding to json.Unmarshal into proxy; update the log
messages in the llm_proxy_handler.go handler where json.Marshal and
json.Unmarshal are used so marshal and unmarshal errors are reported separately
and accurately.
---
Duplicate comments:
In `@gateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.go`:
- Around line 103-109: The handler treats utils.IsPolicyDefinitionMissingError
as a 500 but it should be a client error; update both CreateLLMProxy and
UpdateLLMProxy handlers to return HTTP 400 (Bad Request) instead of
http.StatusInternalServerError when utils.IsPolicyDefinitionMissingError(err) is
true, keeping the same api.ErrorResponse.Message
(utils.PolicyDefinitionMissingUserMessage) and log call; ensure the change is
applied in both the error branches that currently call
c.JSON(http.StatusInternalServerError, ...) for that check.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 739dfb01-d6f1-4913-ab5f-231cfc84d26b
📒 Files selected for processing (5)
gateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.gogateway/gateway-controller/pkg/utils/llm_deployment.gogateway/gateway-controller/pkg/utils/llm_transformer.gogateway/it/features/llm-provider-templates.featuregateway/it/features/llm-proxies.feature
✅ Files skipped from review due to trivial changes (3)
- gateway/it/features/llm-provider-templates.feature
- gateway/it/features/llm-proxies.feature
- gateway/gateway-controller/pkg/utils/llm_transformer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/gateway-controller/pkg/utils/llm_deployment.go
CrowleyRajapakse
approved these changes
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Related Issue: #1649
Summary by CodeRabbit
New Features
Chores