[Discuss - DO NOT MERGE] Make API Key Update Idempotent - Create if Not Found #1083
Conversation
WalkthroughTwo modifications to the gateway controller: a formatting adjustment in the policy builder and a logic enhancement in API key management that adds fallback key creation during updates when keys are missing. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
Thushani-Jayasekera
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/utils/api_key.go`:
- Around line 507-536: When falling through to creation inside the
storage.IsNotFoundError(err) block, ensure the created key uses the URL path
name by setting the creation request's Name to params.APIKeyName before calling
s.CreateAPIKey; update the APIKeyCreationParams (or params.Request) so
Request.Name = params.APIKeyName in the block where you build creationParams,
then call s.CreateAPIKey and return the APIKeyUpdateResult as before.
🧹 Nitpick comments (3)
gateway/gateway-controller/pkg/policy/builder.go (1)
122-124: Good fix; the WebSubApi branch has the same indentation issue.The indentation correction for the RestApi branch looks good. However, note that the equivalent block in the WebSubApi branch (lines 78–80) still has the same mis-indentation — the
if v, okblock is indented one level less than it should be. Consider fixing both in this PR for consistency.gateway/gateway-controller/pkg/utils/api_key.go (2)
510-513: Redundant structured log fields.The
handle,api_key_name, andcorrelation_idfields are already attached to theloggerat lines 487–490. Repeating them in theslog.Infocall at lines 510–513 produces duplicate keys in the log output.Proposed fix
- logger.Info("API key not found for update, creating new API key", - slog.String("handle", params.Handle), - slog.String("api_key_name", params.APIKeyName), - slog.String("correlation_id", params.CorrelationID)) + logger.Info("API key not found for update, creating new API key")Similarly, lines 527–529 and 541–544 repeat fields already on the logger.
524-536: Response message will say "generated/registered" instead of "updated" on the create-fallback path.When
CreateAPIKeysucceeds on the fallback path, the response message is either "API key generated successfully" or "API key registered successfully" (frombuildAPIKeyResponseat line 1151–1155). The caller ofUpdateAPIKeymay expect update-related messaging. If this is consumed by downstream clients or the event handler, it could be confusing.Consider whether you want to override
creationResult.Response.Messagebefore returning, or if this is acceptable for the upsert use case.
| // Only create a new API key if it's a "not found" error | ||
| // For other errors (DB connection, etc.), return the error | ||
| if storage.IsNotFoundError(err) { | ||
| logger.Info("API key not found for update, creating new API key", | ||
| slog.String("handle", params.Handle), | ||
| slog.String("api_key_name", params.APIKeyName), | ||
| slog.String("correlation_id", params.CorrelationID)) | ||
|
|
||
| // Create the new API key using the provided request | ||
| creationParams := APIKeyCreationParams{ | ||
| Handle: params.Handle, | ||
| Request: params.Request, | ||
| User: user, | ||
| CorrelationID: params.CorrelationID, | ||
| Logger: logger, | ||
| } | ||
|
|
||
| creationResult, err := s.CreateAPIKey(creationParams) | ||
| if err != nil { | ||
| logger.Error("Failed to create new API key during update", | ||
| slog.Any("error", err), | ||
| slog.String("handle", params.Handle), | ||
| slog.String("correlation_id", params.CorrelationID)) | ||
| return nil, fmt.Errorf("failed to create new API key: %w", err) | ||
| } | ||
|
|
||
| // Convert creation result to update result | ||
| return &APIKeyUpdateResult{ | ||
| Response: creationResult.Response, | ||
| }, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check APIKeyCreationRequest struct definition and Name field
echo "=== APIKeyCreationRequest struct definition ==="
rg -n --type=go 'type APIKeyCreationRequest struct' -A 20
echo ""
echo "=== UpdateAPIKey callers ==="
rg -n --type=go 'UpdateAPIKey\(' -B 2 -A 5 -g '!*_test.go'
echo ""
echo "=== UpdateExternalAPIKeyFromEvent callers ==="
rg -n --type=go 'UpdateExternalAPIKeyFromEvent\(' -B 2 -A 10 -g '!*_test.go'
echo ""
echo "=== UpdateExternalAPIKeyFromEvent function signature and implementation ==="
rg -n --type=go -B 2 -A 30 'func.*UpdateExternalAPIKeyFromEvent' -g '!*_test.go'Repository: wso2/api-platform
Length of output: 21499
🏁 Script executed:
#!/bin/bash
# Check CreateAPIKey implementation to see how it determines the key name
echo "=== CreateAPIKey function implementation ==="
rg -n --type=go -B 5 -A 50 'func \(s \*APIKeyService\) CreateAPIKey\(params APIKeyCreationParams\)' | head -100
echo ""
echo "=== createAPIKeyFromRequest function to see name generation ==="
rg -n --type=go -B 5 -A 40 'func \(s \*APIKeyService\) createAPIKeyFromRequest' | head -100
echo ""
echo "=== Check what fields are in APIKeyUpdateParams ==="
rg -n --type=go 'type APIKeyUpdateParams struct' -A 15
echo ""
echo "=== Check the REST API spec for Name field handling in update ==="
rg -n --type=go 'UpdateAPIKeyRequest struct' -A 20Repository: wso2/api-platform
Length of output: 12524
🏁 Script executed:
#!/bin/bash
# Check the complete APIKeyCreationRequest struct definition to see all fields
echo "=== Complete APIKeyCreationRequest struct ==="
rg -n --type=go 'type APIKeyCreationRequest struct' -A 30 gateway/gateway-controller/pkg/api/generated/generated.go
echo ""
echo "=== Check how the HTTP handler builds the request for UpdateAPIKey ==="
rg -n --type=go -B 5 -A 20 'func \(s \*APIServer\) UpdateAPIKey' gateway/gateway-controller/pkg/api/handlers/handlers.go
echo ""
echo "=== Check line 215 where createAPIKeyFromRequest is called to see parameters ==="
sed -n '940,1050p' gateway/gateway-controller/pkg/utils/api_key.goRepository: wso2/api-platform
Length of output: 6935
🏁 Script executed:
#!/bin/bash
# Check how the event handler constructs the APIKeyCreationRequest for UpdateExternalAPIKeyFromEvent
echo "=== Event handler: how apiKeyCreationRequest is built from event ==="
rg -n --type=go -B 10 -A 20 'UpdateExternalAPIKeyFromEvent' gateway/gateway-controller/pkg/controlplane/client.go | head -100
echo ""
echo "=== Check the event structure (apikey.updated) ==="
rg -n --type=go 'type.*APIKeyEvent\|apikey.updated' -B 5 -A 15
echo ""
echo "=== Check how the event payload is parsed and used ==="
rg -n --type=go -B 15 -A 5 'payload.KeyName' gateway/gateway-controller/pkg/controlplane/client.goRepository: wso2/api-platform
Length of output: 4977
🏁 Script executed:
#!/bin/bash
# Check if there's any validation in the HTTP handler to ensure Name is set
echo "=== Full HTTP handler UpdateAPIKey implementation ==="
sed -n '2394,2480p' gateway/gateway-controller/pkg/api/handlers/handlers.go
echo ""
echo "=== Check bindRequestBody to see if it validates anything ==="
rg -n --type=go -B 5 -A 15 'func.*bindRequestBody' gateway/gateway-controller/pkg/api/handlers/handlers.goRepository: wso2/api-platform
Length of output: 3738
Ensure the created API key's name matches params.APIKeyName when falling through to create-on-not-found in the HTTP handler path.
When an HTTP client calls PUT /apis/{id}/api-keys/{apiKeyName} and the key doesn't exist, the code falls through to CreateAPIKey. However, the params.APIKeyName from the URL path is never passed to the creation request. CreateAPIKey will use request.Name if provided in the request body, or auto-generate a name—potentially creating a key with a different name than the URL path specified.
The event handler path (apikey.updated) avoids this by explicitly setting request.Name = payload.KeyName before calling UpdateExternalAPIKeyFromEvent, but the HTTP handler path lacks this alignment. Clients making PUT requests expect the created resource to use the identifier from the URL path, consistent with REST semantics.
Consider explicitly setting the name in the creation request for the HTTP path to ensure consistency:
Proposed fix
// Create the new API key using the provided request
creationParams := APIKeyCreationParams{
Handle: params.Handle,
Request: params.Request,
User: user,
CorrelationID: params.CorrelationID,
Logger: logger,
}
+
+ // Ensure the created key uses the name from the URL path if not explicitly provided in the request
+ if creationParams.Request.Name == nil || *creationParams.Request.Name == "" {
+ creationParams.Request.Name = ¶ms.APIKeyName
+ }🤖 Prompt for AI Agents
In `@gateway/gateway-controller/pkg/utils/api_key.go` around lines 507 - 536, When
falling through to creation inside the storage.IsNotFoundError(err) block,
ensure the created key uses the URL path name by setting the creation request's
Name to params.APIKeyName before calling s.CreateAPIKey; update the
APIKeyCreationParams (or params.Request) so Request.Name = params.APIKeyName in
the block where you build creationParams, then call s.CreateAPIKey and return
the APIKeyUpdateResult as before.
1 participant
Summary
This PR enhances the API key update functionality to be more resilient by creating a new API key when the
specified key doesn't exist, instead of failing with an error. This change makes the PUT /api-keys/{id}
endpoint and apikey.updated event handler idempotent.
Changes
returned
Behavior:
✅ API key exists → Updates it normally
✅ API key doesn't exist (NotFound) → Creates new API key
✅ Database/connection errors → Returns error (no creation attempt)
Benefits
Testing
Summary by CodeRabbit