[Test PR] Implemented API Key Support for Federated APIs#13819
Open
dpiyumal2319 wants to merge 8 commits intowso2:masterfrom
Open
[Test PR] Implemented API Key Support for Federated APIs#13819dpiyumal2319 wants to merge 8 commits intowso2:masterfrom
dpiyumal2319 wants to merge 8 commits intowso2:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds federated API key management: pluggable connector interface, validation hook for gateway environments, persistence for connector-owned reference artifacts, event enrichment and an async notifier to synchronize create/replace/revoke/apply-rate-limit operations across external gateways with rollback on partial failures. Changes
Sequence Diagram(s)sequenceDiagram
participant APIConsumer as APIConsumerImpl
participant Notifier as FederatedApiKeyNotifier
participant GatewayHolder as GatewayHolder
participant Connector as FederatedApiKeyConnector
participant Gateway as ExternalGateway
participant DAO as ApiKeyMgtDAO
APIConsumer->>Notifier: publishEvent(APIKeyEvent with apiKey, uuids, context)
activate Notifier
Notifier->>DAO: getApiExternalApiMappings(apiUuid)
Notifier->>GatewayHolder: getTenantApiKeyConnectorInstance(org, envUuid)
activate GatewayHolder
GatewayHolder->>Connector: init(environment)
deactivate GatewayHolder
Connector->>Gateway: createApiKey(keyUuid, keyValue, props)
Gateway-->>Connector: FederatedApiKeyCreationResult(referenceArtifact, metadata)
Connector-->>Notifier: result
Notifier->>DAO: addOrUpdateApiKeyExternalApiKeyMapping(keyUuid, envId, artifact)
alt partial failure during loop
Notifier->>Connector: revokeApiKey(referenceArtifact) for succeeded envs
Notifier->>DAO: deleteApiKeyExternalApiKeyMappings(keyUuid)
end
Notifier-->>APIConsumer: publishEvent completion
deactivate Notifier
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
dpiyumal2319
requested review from
AnuGayan,
Arshardh,
chamilaadhi,
dushaniw,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
|
|
dpiyumal2319
requested review from
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
ashera96,
hisanhunais,
npamudika and
pubudu538
as code owners
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
| * | ||
| * @param environment gateway environment configuration | ||
| * @throws APIManagementException if initialization fails | ||
| */ |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| */ | |
| void init(Environment environment) throws APIManagementException; | |
| log.info("Initializing FederatedApiKeyConnector for environment: " + environment.getName()); |
| * @param context API key operation context | ||
| * @return credential creation result containing connector-owned reference artifact and metadata | ||
| * @throws APIManagementException if operation fails | ||
| */ |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| */ | |
| FederatedApiKeyCreationResult createApiKey(FederatedApiKeyContext context) throws APIManagementException; | |
| log.info("Creating API key for API: " + context.getApiId()); |
Comment on lines
+77
to
+82
|
|
||
| public FederatedApiKeyCreationResult build() { | ||
| if (referenceArtifact == null || referenceArtifact.trim().isEmpty()) { | ||
| throw new IllegalStateException("referenceArtifact must not be blank"); | ||
| } | ||
| return new FederatedApiKeyCreationResult(this); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| public FederatedApiKeyCreationResult build() { | |
| if (referenceArtifact == null || referenceArtifact.trim().isEmpty()) { | |
| throw new IllegalStateException("referenceArtifact must not be blank"); | |
| } | |
| return new FederatedApiKeyCreationResult(this); | |
| public FederatedApiKeyCreationResult build() { | |
| if (referenceArtifact == null || referenceArtifact.trim().isEmpty()) { | |
| throw new IllegalStateException("referenceArtifact must not be blank"); | |
| } | |
| log.debug("Building FederatedApiKeyCreationResult with metadata size: " + (metadata != null ? metadata.size() : 0)); | |
| return new FederatedApiKeyCreationResult(this); | |
| } |
| * @param environment external gateway environment with plain text connector configurations | ||
| * @throws APIManagementException if the environment cannot be validated | ||
| */ | ||
| default void validateEnvironment(Environment environment) throws APIManagementException { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| default void validateEnvironment(Environment environment) throws APIManagementException { | |
| default void validateEnvironment(Environment environment) throws APIManagementException { | |
| log.debug("Validating external gateway environment: {}", environment.getName()); | |
| } |
Comment on lines
+33
to
+35
| public GatewayConfigurationContext(List<SubscriptionPolicy> subscriptionPolicies) { | ||
| this.subscriptionPolicies = subscriptionPolicies != null ? subscriptionPolicies : Collections.emptyList(); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| public GatewayConfigurationContext(List<SubscriptionPolicy> subscriptionPolicies) { | |
| this.subscriptionPolicies = subscriptionPolicies != null ? subscriptionPolicies : Collections.emptyList(); | |
| } | |
| public GatewayConfigurationContext(List<SubscriptionPolicy> subscriptionPolicies) { | |
| this.subscriptionPolicies = subscriptionPolicies != null ? subscriptionPolicies : Collections.emptyList(); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Initialized GatewayConfigurationContext with " + this.subscriptionPolicies.size() + " subscription policies"); | |
| } | |
| } |
Comment on lines
235
to
+237
| validateForUniqueVhostNames(environment); | ||
| Environment environmentToStore = new Environment(environment); | ||
| validateGatewayConfigurationValues(null, environmentToStore); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| validateForUniqueVhostNames(environment); | |
| Environment environmentToStore = new Environment(environment); | |
| validateGatewayConfigurationValues(null, environmentToStore); | |
| validateForUniqueVhostNames(environment); | |
| Environment environmentToStore = new Environment(environment); | |
| log.info("Adding new environment for tenant: " + tenantDomain + ", environment name: " + environment.getName()); | |
| validateGatewayConfigurationValues(null, environmentToStore); |
Comment on lines
+1021
to
+1022
| } catch (APIManagementException e) { | ||
| throw new APIManagementException(e.getMessage(), e, |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| } catch (APIManagementException e) { | |
| throw new APIManagementException(e.getMessage(), e, | |
| } catch (APIManagementException e) { | |
| log.error("Failed to validate gateway configuration for gateway type: " + updatedGatewayConfigurationDto.getGatewayType() + ", error: " + e.getMessage()); | |
| throw new APIManagementException(e.getMessage(), e, |
Comment on lines
610
to
+612
| apiKeyInfoDTO.getCreatedTime(), apiKeyInfoDTO.getValidityPeriod(), | ||
| apiKeyInfoDTO.getPermittedIP(), apiKeyInfoDTO.getPermittedReferer(), "ACTIVE", "APPLICATION"); | ||
| apiKeyEvent.setApiKey(apiKey); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
Suggested change
| apiKeyInfoDTO.getCreatedTime(), apiKeyInfoDTO.getValidityPeriod(), | |
| apiKeyInfoDTO.getPermittedIP(), apiKeyInfoDTO.getPermittedReferer(), "ACTIVE", "APPLICATION"); | |
| apiKeyEvent.setApiKey(apiKey); | |
| apiKeyInfoDTO.getAuthUser(), apiKeyInfoDTO.getApiKeyProperties(), | |
| apiKeyInfoDTO.getCreatedTime(), apiKeyInfoDTO.getValidityPeriod(), | |
| apiKeyInfoDTO.getPermittedIP(), apiKeyInfoDTO.getPermittedReferer(), "ACTIVE", "APPLICATION"); | |
| log.info("Generated API key for application: " + application.getName() + " with UUID: " + application.getUUID()); | |
| apiKeyEvent.setApiKey(apiKey); |
Comment on lines
+24121
to
+24123
| */ | ||
| public void addOrUpdateApiKeyExternalApiKeyMapping(String apiKeyUuid, String environmentId, | ||
| String referenceArtifact) throws APIManagementException { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 10
Suggested change
| */ | |
| public void addOrUpdateApiKeyExternalApiKeyMapping(String apiKeyUuid, String environmentId, | |
| String referenceArtifact) throws APIManagementException { | |
| public void addOrUpdateApiKeyExternalApiKeyMapping(String apiKeyUuid, String environmentId, | |
| String referenceArtifact) throws APIManagementException { | |
| log.debug("Adding or updating external API key mapping for API key: {} and environment: {}", apiKeyUuid, environmentId); | |
| try (Connection connection = APIMgtDBUtil.getConnection()) { |
Comment on lines
+24141
to
+24142
| } | ||
| connection.commit(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 11
Suggested change
| } | |
| connection.commit(); | |
| connection.commit(); | |
| log.info("Successfully added or updated external API key mapping for API key: {} and environment: {}", apiKeyUuid, environmentId); | |
| } catch (SQLException e) { |
Comment on lines
+75
to
+77
| public static FederatedApiKeyConnector getTenantApiKeyConnectorInstance(Environment environment) | ||
| throws APIManagementException { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 12
Suggested change
| public static FederatedApiKeyConnector getTenantApiKeyConnectorInstance(Environment environment) | |
| throws APIManagementException { | |
| public static FederatedApiKeyConnector getTenantApiKeyConnectorInstance(Environment environment) | |
| throws APIManagementException { | |
| log.info("Initializing Federated API Key Connector for environment: " + environment.getName()); |
Comment on lines
+103
to
+104
| .newInstance(); | ||
| connector.init(resolvedEnvironment); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 13
Suggested change
| .newInstance(); | |
| connector.init(resolvedEnvironment); | |
| connector.init(resolvedEnvironment); | |
| log.info("Successfully initialized Federated API Key Connector: " + implementationClassName); | |
| return connector; |
Comment on lines
208
to
+209
| bundleContext.registerService(Notifier.class.getName(), new APIKeyNotifier(), null); | ||
| bundleContext.registerService(Notifier.class.getName(), new FederatedApiKeyNotifier(), null); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 14
Suggested change
| bundleContext.registerService(Notifier.class.getName(), new APIKeyNotifier(), null); | |
| bundleContext.registerService(Notifier.class.getName(), new FederatedApiKeyNotifier(), null); | |
| bundleContext.registerService(Notifier.class.getName(), new APIKeyNotifier(), null); | |
| bundleContext.registerService(Notifier.class.getName(), new FederatedApiKeyNotifier(), null); | |
| log.debug("Registered FederatedApiKeyNotifier service"); |
Comment on lines
+75
to
+85
| throw new NotifierException("Failed to process federated API key event", e); | ||
| } | ||
| } | ||
|
|
||
| private void handleAPIKeyEvent(APIKeyEvent apiKeyEvent) throws APIManagementException { | ||
|
|
||
| APIConstants.EventType eventType = resolveEventType(apiKeyEvent.getType()); | ||
| if (eventType == null) { | ||
| log.warn("Unknown federated API key event type: " + apiKeyEvent.getType()); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 15
Suggested change
| throw new NotifierException("Failed to process federated API key event", e); | |
| } | |
| } | |
| private void handleAPIKeyEvent(APIKeyEvent apiKeyEvent) throws APIManagementException { | |
| APIConstants.EventType eventType = resolveEventType(apiKeyEvent.getType()); | |
| if (eventType == null) { | |
| log.warn("Unknown federated API key event type: " + apiKeyEvent.getType()); | |
| return; | |
| } | |
| private void handleAPIKeyEvent(APIKeyEvent apiKeyEvent) throws APIManagementException { | |
| APIConstants.EventType eventType = resolveEventType(apiKeyEvent.getType()); | |
| if (eventType == null) { | |
| log.warn("Unknown federated API key event type: " + apiKeyEvent.getType()); | |
| return; | |
| } | |
| log.info("Processing federated API key event: " + eventType.name() + " for key UUID: " + apiKeyEvent.getUuid()); | |
| switch (eventType) { |
Comment on lines
+159
to
+163
| + "for environment: " + gatewayEnvironment.getEnvironmentId()); | ||
| } | ||
| newlyCreatedReferenceArtifacts.put(gatewayEnvironment.getEnvironmentId(), result.getReferenceArtifact()); | ||
| } | ||
| } catch (APIManagementException e) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 16
Suggested change
| + "for environment: " + gatewayEnvironment.getEnvironmentId()); | |
| } | |
| newlyCreatedReferenceArtifacts.put(gatewayEnvironment.getEnvironmentId(), result.getReferenceArtifact()); | |
| } | |
| } catch (APIManagementException e) { | |
| } | |
| } catch (APIManagementException e) { | |
| log.error("Failed to create federated API keys. Rolling back created keys.", e); | |
| rollbackCreatedApiKeys(apiUuid, organization, event, gatewayEnvironments, newlyCreatedReferenceArtifacts); | |
| throw e; |
|
|
||
| } | ||
|
|
||
| public static boolean isFederatedGatewayApi(String apiUuid) throws APIManagementException { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 17
Suggested change
| public static boolean isFederatedGatewayApi(String apiUuid) throws APIManagementException { | |
| public static boolean isFederatedGatewayApi(String apiUuid) throws APIManagementException { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Checking if API with UUID: " + apiUuid + " is a federated gateway API"); | |
| } |
Comment on lines
+687
to
+688
| String normalizedGatewayVendor = handleGatewayVendorRetrieval(gatewayVendor); | ||
| return APIConstants.EXTERNAL_GATEWAY_VENDOR.equalsIgnoreCase(normalizedGatewayVendor); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 18
Suggested change
| String normalizedGatewayVendor = handleGatewayVendorRetrieval(gatewayVendor); | |
| return APIConstants.EXTERNAL_GATEWAY_VENDOR.equalsIgnoreCase(normalizedGatewayVendor); | |
| String normalizedGatewayVendor = handleGatewayVendorRetrieval(gatewayVendor); | |
| boolean isFederated = APIConstants.EXTERNAL_GATEWAY_VENDOR.equalsIgnoreCase(normalizedGatewayVendor); | |
| if (log.isDebugEnabled()) { | |
| log.debug("API with UUID: " + apiUuid + " has gateway vendor: " + normalizedGatewayVendor + | |
| ", isFederated: " + isFederated); | |
| } | |
| return isFederated; |
Comment on lines
230
to
231
| public static Environment fromEnvDtoToEnv(EnvironmentDTO envDTO) throws APIManagementException { | ||
| Environment env = new Environment(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 19
Suggested change
| public static Environment fromEnvDtoToEnv(EnvironmentDTO envDTO) throws APIManagementException { | |
| Environment env = new Environment(); | |
| public static Environment fromEnvDtoToEnv(EnvironmentDTO envDTO) throws APIManagementException { | |
| log.info("Converting EnvironmentDTO to Environment for environment: " + envDTO.getName()); |
Comment on lines
+302
to
+304
| if (additionalPropertiesDTOs == null) { | ||
| return additionalProperties; | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 20
Suggested change
| if (additionalPropertiesDTOs == null) { | |
| return additionalProperties; | |
| } | |
| if (additionalPropertiesDTOs == null) { | |
| log.debug("No additional properties provided for environment configuration"); | |
| return additionalProperties; | |
| } |
Comment on lines
+259
to
+261
| private static GatewayConfigurationContext buildGatewayConfigurationContext() { | ||
| String tenantDomain = CarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | ||
| if (tenantDomain == null) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 21
Suggested change
| private static GatewayConfigurationContext buildGatewayConfigurationContext() { | |
| String tenantDomain = CarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | |
| if (tenantDomain == null) { | |
| private static GatewayConfigurationContext buildGatewayConfigurationContext() { | |
| log.debug("Building gateway configuration context"); | |
| String tenantDomain = CarbonContext.getThreadLocalCarbonContext().getTenantDomain(); |
Comment on lines
+271
to
+273
| return new GatewayConfigurationContext(policyList); | ||
| } catch (APIManagementException e) { | ||
| log.warn("Failed to load subscription policies for gateway configuration context", e); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 22
Suggested change
| return new GatewayConfigurationContext(policyList); | |
| } catch (APIManagementException e) { | |
| log.warn("Failed to load subscription policies for gateway configuration context", e); | |
| } catch (APIManagementException e) { | |
| log.warn("Failed to load subscription policies for gateway configuration context", e); | |
| log.error("Failed to retrieve subscription policies for tenant domain: " + tenantDomain); | |
| return new GatewayConfigurationContext(new ArrayList<>()); |
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds control-plane support for federated gateway API key lifecycle operations (create/revoke/replace + plan association), plus a connector-owned validation hook for external gateway environment onboarding/updates.
Changes:
- Added per-environment external API-key reference mapping persistence (new DB table + DAO methods + tests).
- Introduced federated API key connector SPI and a new notifier to drive external gateway key operations from existing events.
- Extended gateway connector configuration schema support (context-aware configs + multi-label UI support) and added onboarding/update validation before persistence/encryption.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql | Adds AM_API_KEY_EXTERNAL_API_KEY_MAPPING table for PostgreSQL. |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sql | Adds mapping table for Oracle RAC. |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sql | Adds mapping table for Oracle 23c. |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql | Adds mapping table for Oracle. |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sql | Adds mapping table for MySQL Cluster. |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql | Adds mapping table for MySQL. |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql | Adds mapping table for MSSQL. |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql | Adds mapping table for H2 (tests/dev). |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sql | Adds mapping table for DB2. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml | Adds labels to gateway configuration schema; fixes YAML indentation. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml | Mirrors schema update in admin v1 definition. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/.../SettingsMappingUtil.java | Builds gateway config context (subscription policies) and maps labels; uses context-aware connector config generation. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/.../EnvironmentMappingUtil.java | Adjusts environment DTO→model mapping and null-handling for additional properties. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/.../EnvironmentsApiServiceImpl.java | Minor import cleanup aligned with mapping changes. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/.../GatewayConfigurationDTO.java | Adds labels field to generated DTO + equality/hash/toString updates. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/test/.../APIMgtDAOTest.java | Adds lifecycle test coverage for the new mapping DAO APIs. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/.../utils/APIUtil.java | Adds isFederatedGatewayApi helper; clarifies mapping reference javadoc. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/.../events/APIKeyRegenerationEvent.java | Adds UUID/context fields + transient raw key for federated regeneration flow. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/.../events/APIKeyEvent.java | Adds transient raw key field for federated create flow. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/.../events/APIKeyAssociationEvent.java | Adds API key UUID field to support per-key external mapping lookups. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/.../notifier/FederatedApiKeyNotifier.java | New notifier implementing federated API key operations against external gateways. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/.../internal/APIManagerComponent.java | Registers FederatedApiKeyNotifier. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/.../factory/GatewayHolder.java | Adds API key connector instantiation + environment decryption wiring. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/.../dao/constants/SQLConstants.java | Adds SQL constants for mapping CRUD. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/.../dao/ApiMgtDAO.java | Implements mapping CRUD APIs storing opaque reference artifacts. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/.../APIConsumerImpl.java | Populates new event fields (raw key, uuids) needed by federated notifier. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/.../APIAdminImpl.java | Adds connector-owned environment validation hook + masked-secret restoration for updates. |
| components/apimgt/org.wso2.carbon.apimgt.api/src/main/.../model/GatewayConfigurationContext.java | New context model for enriching connector configuration schemas. |
| components/apimgt/org.wso2.carbon.apimgt.api/src/main/.../model/GatewayAgentConfiguration.java | Adds API key connector implementation accessor, context-aware configs, and environment validation hook. |
| components/apimgt/org.wso2.carbon.apimgt.api/src/main/.../model/FederatedApiKeyCreationResult.java | New model for connector create/replace results with reference artifact + metadata. |
| components/apimgt/org.wso2.carbon.apimgt.api/src/main/.../model/FederatedApiKeyContext.java | New operation context model (includes sensitive key value excluded from serialization). |
| components/apimgt/org.wso2.carbon.apimgt.api/src/main/.../model/ConfigurationDto.java | Adds labels map for UI/connector schema metadata. |
| components/apimgt/org.wso2.carbon.apimgt.api/src/main/.../FederatedApiKeyConnector.java | New SPI for external gateway API key lifecycle + plan association operations. |
| components/apimgt/org.wso2.carbon.apimgt.api/src/main/.../ExceptionCodes.java | Adds managed exception code for federated gateway onboarding validation failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dpiyumal2319
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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml (1)
1593-1599:⚠️ Potential issue | 🟠 MajorAdd missing
apim:app_token_type_upgradescope toGET /applications.Line 1593-Line 1599 is missing the scope required for the admin token-type-upgrade flow to list applications.
🔧 Proposed fix
/applications: get: @@ security: - OAuth2Security: - apim:admin - apim:app_settings_change - apim:app_owner_change + - apim:app_token_type_upgrade - apim:app_import_export - apim:admin_application_view @@ securitySchemes: OAuth2Security: type: oauth2 flows: password: tokenUrl: https://localhost:9443/oauth2/token scopes: @@ apim:app_owner_change: Retrieve and manage applications + apim:app_token_type_upgrade: Upgrade application token type apim:app_settings_change: Change Application SettingsBased on learnings: Ensure that in admin-api.yaml files the apim:app_token_type_upgrade scope is included in the GET /applications endpoint's security block to support the admin UI token-type-upgrade flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml` around lines 1593 - 1599, The GET /applications operation's security block is missing the apim:app_token_type_upgrade scope; update the OAuth2Security entry for the GET /applications endpoint (the OAuth2Security array under that operation) to include "apim:app_token_type_upgrade" alongside apim:admin, apim:app_settings_change, apim:app_owner_change, apim:app_import_export, and apim:admin_application_view so the admin token-type-upgrade flow can list applications.components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml (1)
1593-1599:⚠️ Potential issue | 🟠 MajorAdd missing
apim:app_token_type_upgradescope forGET /applications.Line 1593-Line 1599 is missing
apim:app_token_type_upgrade, which can break admin UI flows that list applications before token-type upgrades.🔧 Proposed fix
/applications: get: ... security: - OAuth2Security: - apim:admin - apim:app_settings_change - apim:app_owner_change + - apim:app_token_type_upgrade - apim:app_import_export - apim:admin_application_viewsecuritySchemes: OAuth2Security: type: oauth2 flows: password: tokenUrl: https://localhost:9443/oauth2/token scopes: ... apim:app_owner_change: Retrieve and manage applications + apim:app_token_type_upgrade: Upgrade application token types apim:app_settings_change: Change Application Settings ...Based on learnings: Ensure that in admin-api.yaml files the
apim:app_token_type_upgradescope is included in theGET /applicationsendpoint's security block to support admin UI token-type upgrade flows.Also applies to: 8110-8154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml` around lines 1593 - 1599, The GET /applications endpoint's security block (the OAuth2Security list) is missing the apim:app_token_type_upgrade scope; update the OAuth2Security entry for the GET /applications operation to include "apim:app_token_type_upgrade" alongside the existing scopes (e.g., apim:admin, apim:app_settings_change, apim:app_owner_change, apim:app_import_export, apim:admin_application_view) so the admin UI can perform token-type upgrade flows; ensure you update the same security block occurrences referenced (also around the 8110-8154 region) so all admin-api.yaml definitions are consistent.
🧹 Nitpick comments (8)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/FederatedApiKeyCreationResult.java (1)
73-75: Optionally snapshotmetadataat setter time for predictability.Today, if callers mutate the input map after calling
metadata(...)but beforebuild(), those changes are picked up. Capturing a snapshot in the setter avoids that surprise.♻️ Proposed tweak
public Builder metadata(Map<String, Object> metadata) { - this.metadata = metadata; + this.metadata = metadata == null ? null : new HashMap<>(metadata); return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/FederatedApiKeyCreationResult.java` around lines 73 - 75, The Builder.metadata(Map<String, Object> metadata) setter currently stores the caller's map by reference, so mutations after calling metadata(...) are observed at build time; change the setter in FederatedApiKeyCreationResult.Builder to defensively copy the incoming map (e.g., new HashMap<>(metadata) or Collections.unmodifiableMap(new HashMap<>(...))) and handle nulls appropriately so the Builder holds an isolated snapshot of metadata for predictable build() behavior.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql (1)
2916-2923: IndexGATEWAY_ENV_IDto avoid full scans for env-based access patterns.Line 2922 adds an FK on
GATEWAY_ENV_ID, but there is no standalone index for that column; the PK only optimizesAPI_KEY_UUID-first access.Proposed DDL addition
+CREATE INDEX IF NOT EXISTS IDX_AM_API_KEY_EXT_MAP_GW_ENV + ON AM_API_KEY_EXTERNAL_API_KEY_MAPPING (GATEWAY_ENV_ID);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql` around lines 2916 - 2923, Add a standalone index on GATEWAY_ENV_ID in the AM_API_KEY_EXTERNAL_API_KEY_MAPPING table to speed up environment-based lookups; update the DDL by creating an index (e.g., CREATE INDEX or ALTER TABLE ADD INDEX) for column GATEWAY_ENV_ID so queries that filter by GATEWAY_ENV_ID don’t require a full table scan despite the composite primary key (refer to AM_API_KEY_EXTERNAL_API_KEY_MAPPING and the GATEWAY_ENV_ID column and its FK to AM_GATEWAY_ENVIRONMENT(UUID)).features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sql (1)
4107-4114: Add a dedicated index forGATEWAY_ENV_IDin the new mapping table.Line 4113 introduces the FK on
GATEWAY_ENV_ID, but the current PK index starts withAPI_KEY_UUID; gateway-environment lookups and parent-row maintenance can degrade without a separate index.Proposed DDL addition
+CREATE INDEX IDX_AM_API_KEY_EXT_MAP_GW_ENV + ON AM_API_KEY_EXTERNAL_API_KEY_MAPPING (GATEWAY_ENV_ID);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sql` around lines 4107 - 4114, The new table AM_API_KEY_EXTERNAL_API_KEY_MAPPING defines a FK on GATEWAY_ENV_ID but only has a composite PK starting with API_KEY_UUID, which can make gateway-environment lookups and FK maintenance slow; add a dedicated index on GATEWAY_ENV_ID for this table (e.g., create an index like IDX_AM_API_KEY_EXT_GATEWAY_ENV_ID on AM_API_KEY_EXTERNAL_API_KEY_MAPPING(GATEWAY_ENV_ID)) so the foreign key and queries filtering by GATEWAY_ENV_ID use an efficient path; ensure the index name follows existing naming conventions and commit the DDL in the same SQL file near the table definition.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql (1)
4417-4424: Align Oracle mapping column length with gateway UUID definition.Line 4419 uses
GATEWAY_ENV_ID VARCHAR(255), whileAM_GATEWAY_ENVIRONMENT.UUIDisVARCHAR(45)(Line 3667). Keeping identical sizing avoids cross-schema drift and FK-definition ambiguity.Proposed fix
CREATE TABLE AM_API_KEY_EXTERNAL_API_KEY_MAPPING ( API_KEY_UUID VARCHAR(64) NOT NULL, - GATEWAY_ENV_ID VARCHAR(255) NOT NULL, + GATEWAY_ENV_ID VARCHAR(45) NOT NULL, REFERENCE_ARTIFACT BLOB NOT NULL, PRIMARY KEY (API_KEY_UUID, GATEWAY_ENV_ID), FOREIGN KEY (API_KEY_UUID) REFERENCES AM_API_KEY(API_KEY_UUID), FOREIGN KEY (GATEWAY_ENV_ID) REFERENCES AM_GATEWAY_ENVIRONMENT(UUID) ) /🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql` around lines 4417 - 4424, The GATEWAY_ENV_ID column in AM_API_KEY_EXTERNAL_API_KEY_MAPPING is defined as VARCHAR(255) but must match the referenced AM_GATEWAY_ENVIRONMENT.UUID size (VARCHAR(45)); update the column definition in the CREATE TABLE for AM_API_KEY_EXTERNAL_API_KEY_MAPPING to use VARCHAR(45) so the foreign key FK to AM_GATEWAY_ENVIRONMENT(UUID) aligns exactly, keeping the same primary key and FK constraints (API_KEY_UUID, GATEWAY_ENV_ID) and not changing other types.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql (1)
3330-3337: Add a secondary index forGATEWAY_ENV_IDin PostgreSQL.Because the primary key starts with
API_KEY_UUID, operations filtered byGATEWAY_ENV_ID(including FK-driven checks) are not efficiently indexed. Add a dedicated index to avoid table scans as this mapping grows.♻️ Suggested SQL
CREATE TABLE IF NOT EXISTS AM_API_KEY_EXTERNAL_API_KEY_MAPPING ( API_KEY_UUID VARCHAR(64) NOT NULL, GATEWAY_ENV_ID VARCHAR(255) NOT NULL, REFERENCE_ARTIFACT BYTEA NOT NULL, PRIMARY KEY (API_KEY_UUID, GATEWAY_ENV_ID), FOREIGN KEY (API_KEY_UUID) REFERENCES AM_API_KEY(API_KEY_UUID), FOREIGN KEY (GATEWAY_ENV_ID) REFERENCES AM_GATEWAY_ENVIRONMENT(UUID) ); +CREATE INDEX IF NOT EXISTS IDX_AM_API_KEY_EXT_MAP_GW_ENV + ON AM_API_KEY_EXTERNAL_API_KEY_MAPPING (GATEWAY_ENV_ID);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql` around lines 3330 - 3337, Add a dedicated index on the GATEWAY_ENV_ID column of the AM_API_KEY_EXTERNAL_API_KEY_MAPPING table to avoid full table scans when queries filter by GATEWAY_ENV_ID; update the PostgreSQL migration to create an index (e.g., CREATE INDEX IF NOT EXISTS ...) using the table name AM_API_KEY_EXTERNAL_API_KEY_MAPPING and column GATEWAY_ENV_ID so FK-driven lookups and queries that only filter by GATEWAY_ENV_ID are served by the new index.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sql (1)
4415-4422: Add a dedicated index forGATEWAY_ENV_IDFK lookups.The composite PK starts with
API_KEY_UUID, so the FK onGATEWAY_ENV_ID(Line 4417, Line 4421) is not efficiently indexed for parent delete/update checks onAM_GATEWAY_ENVIRONMENT(UUID).♻️ Proposed DDL addition
CREATE TABLE AM_API_KEY_EXTERNAL_API_KEY_MAPPING ( API_KEY_UUID VARCHAR(64) NOT NULL, GATEWAY_ENV_ID VARCHAR(255) NOT NULL, REFERENCE_ARTIFACT BLOB NOT NULL, PRIMARY KEY (API_KEY_UUID, GATEWAY_ENV_ID), FOREIGN KEY (API_KEY_UUID) REFERENCES AM_API_KEY(API_KEY_UUID), FOREIGN KEY (GATEWAY_ENV_ID) REFERENCES AM_GATEWAY_ENVIRONMENT(UUID) ) / +CREATE INDEX IDX_AM_AKEAKM_GATEWAY_ENV_ID + ON AM_API_KEY_EXTERNAL_API_KEY_MAPPING (GATEWAY_ENV_ID) +/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sql` around lines 4415 - 4422, Add a dedicated non-unique index on the GATEWAY_ENV_ID column in the AM_API_KEY_EXTERNAL_API_KEY_MAPPING table to support efficient FK lookups to AM_GATEWAY_ENVIRONMENT(UUID); locate the table definition for AM_API_KEY_EXTERNAL_API_KEY_MAPPING and add a CREATE INDEX (e.g., IDX_AM_API_KEY_EXT_GW_ENV on GATEWAY_ENV_ID) after the table DDL so delete/update checks on the AM_GATEWAY_ENVIRONMENT(UUID) FK use the index.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (1)
681-683: Remove redundant debug guard for a static message.This
isDebugEnabled()check is unnecessary for a literallog.debug(...)call and adds avoidable branching.♻️ Proposed refactor
- if (log.isDebugEnabled()) { - log.debug("Skipping federated gateway API check because API UUID is blank. Assuming non-federated."); - } + log.debug("Skipping federated gateway API check because API UUID is blank. Assuming non-federated.");Based on learnings: In Java codebases, do not guard simple
log.debug("message")calls withisDebugEnabled()when the message is a static literal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java` around lines 681 - 683, Remove the redundant debug-level guard around the static log message in APIUtil: replace the if (log.isDebugEnabled()) { log.debug("Skipping federated gateway API check because API UUID is blank. Assuming non-federated."); } pattern with a direct log.debug(...) call; change code referencing log.isDebugEnabled() in APIUtil to simply invoke log.debug("Skipping federated gateway API check because API UUID is blank. Assuming non-federated."); so only the unnecessary branching using log.isDebugEnabled() is removed while preserving the log.debug invocation.components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/dao/test/APIMgtDAOTest.java (1)
205-254: Keep seeded DB rows isolated to this testLine 210 seeds
AM_API_KEY/AM_GATEWAY_ENVIRONMENT, but Line 252 only removes mapping rows. Add explicit cleanup for seeded records in afinallyblock to keep the test DB deterministic across repeated runs.♻️ Suggested cleanup pattern
@@ - String initialReferenceArtifact = "{\"credential\":\"initial\"}"; - String updatedReferenceArtifact = "{\"credential\":\"updated\"}"; - apiMgtDAO.addOrUpdateApiKeyExternalApiKeyMapping(oldApiKeyUuid, gatewayEnvUuid, initialReferenceArtifact); - Map<String, String> initialMappings = apiMgtDAO.getApiKeyExternalApiKeyMappings(oldApiKeyUuid); - assertEquals(1, initialMappings.size()); - assertEquals(initialReferenceArtifact, initialMappings.get(gatewayEnvUuid)); - - apiMgtDAO.addOrUpdateApiKeyExternalApiKeyMapping(oldApiKeyUuid, gatewayEnvUuid, updatedReferenceArtifact); - Map<String, String> mappings = apiMgtDAO.getApiKeyExternalApiKeyMappings(oldApiKeyUuid); - assertEquals(1, mappings.size()); - assertEquals(updatedReferenceArtifact, mappings.get(gatewayEnvUuid)); - - apiMgtDAO.deleteApiKeyExternalApiKeyMappings(oldApiKeyUuid); - assertTrue(apiMgtDAO.getApiKeyExternalApiKeyMappings(oldApiKeyUuid).isEmpty()); + try { + String initialReferenceArtifact = "{\"credential\":\"initial\"}"; + String updatedReferenceArtifact = "{\"credential\":\"updated\"}"; + apiMgtDAO.addOrUpdateApiKeyExternalApiKeyMapping(oldApiKeyUuid, gatewayEnvUuid, initialReferenceArtifact); + Map<String, String> initialMappings = apiMgtDAO.getApiKeyExternalApiKeyMappings(oldApiKeyUuid); + assertEquals(1, initialMappings.size()); + assertEquals(initialReferenceArtifact, initialMappings.get(gatewayEnvUuid)); + + apiMgtDAO.addOrUpdateApiKeyExternalApiKeyMapping(oldApiKeyUuid, gatewayEnvUuid, updatedReferenceArtifact); + Map<String, String> mappings = apiMgtDAO.getApiKeyExternalApiKeyMappings(oldApiKeyUuid); + assertEquals(1, mappings.size()); + assertEquals(updatedReferenceArtifact, mappings.get(gatewayEnvUuid)); + + apiMgtDAO.deleteApiKeyExternalApiKeyMappings(oldApiKeyUuid); + assertTrue(apiMgtDAO.getApiKeyExternalApiKeyMappings(oldApiKeyUuid).isEmpty()); + } finally { + try (Connection connection = APIMgtDBUtil.getConnection(); + PreparedStatement deleteMappings = connection.prepareStatement( + "DELETE FROM AM_API_KEY_EXTERNAL_API_KEY_MAPPING WHERE API_KEY_UUID IN (?, ?)"); + PreparedStatement deleteKeys = connection.prepareStatement( + "DELETE FROM AM_API_KEY WHERE API_KEY_UUID IN (?, ?)"); + PreparedStatement deleteGateway = connection.prepareStatement( + "DELETE FROM AM_GATEWAY_ENVIRONMENT WHERE UUID = ?")) { + deleteMappings.setString(1, oldApiKeyUuid); + deleteMappings.setString(2, newApiKeyUuid); + deleteMappings.executeUpdate(); + deleteKeys.setString(1, oldApiKeyUuid); + deleteKeys.setString(2, newApiKeyUuid); + deleteKeys.executeUpdate(); + deleteGateway.setString(1, gatewayEnvUuid); + deleteGateway.executeUpdate(); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/dao/test/APIMgtDAOTest.java` around lines 205 - 254, The test method testApiKeyExternalApiKeyMappingLifecycle seeds rows in AM_API_KEY and AM_GATEWAY_ENVIRONMENT but only deletes mappings; add a finally block that removes the seeded rows to keep DB isolated: after exercising addOrUpdateApiKeyExternalApiKeyMapping/getApiKeyExternalApiKeyMappings/deleteApiKeyExternalApiKeyMappings, open a connection and execute DELETE FROM AM_API_KEY WHERE API_KEY_UUID IN (oldApiKeyUuid, newApiKeyUuid) and DELETE FROM AM_GATEWAY_ENVIRONMENT WHERE UUID = gatewayEnvUuid (or equivalent), ensuring the cleanup runs even if assertions fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/GatewayConfigurationContext.java`:
- Around line 33-38: The GatewayConfigurationContext exposes its internal
mutable subscriptionPolicies list (constructor
GatewayConfigurationContext(List<SubscriptionPolicy>) and getter
getSubscriptionPolicies()), causing shared-state leaks when a single instance is
reused by SettingsMappingUtil.getSettingsGatewayConfigurationDTOList(); fix by
defensively copying and wrapping the list as unmodifiable in the constructor
(e.g., this.subscriptionPolicies = Collections.unmodifiableList(new
ArrayList<>(...))) and ensure getSubscriptionPolicies() returns the unmodifiable
reference (or a defensive copy) so callers cannot mutate the internal list.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIAdminImpl.java`:
- Around line 1021-1024: The catch block in APIAdminImpl currently forwards
e.getMessage() into the thrown APIManagementException and the
ExceptionCodes.from payload, which can leak vendor/HTTP details; instead, log
the original exception (including e.getMessage()) server-side (e.g., using the
class logger) and throw a new APIManagementException with a fixed, APIM-managed
error message (do not include e.getMessage()) while still passing the original
exception as the cause; when calling ExceptionCodes.from(...) use a sanitized
message (e.g., a generic "Gateway onboarding validation failed") and keep
updatedGatewayConfigurationDto.getGatewayType() as the contextual parameter.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.java`:
- Around line 4753-4754: Replace the use of the method input apiUUId when
populating the event with the persisted API UUID from the stored association
record: instead of apiKeyAssociationEvent.setApiUUId(apiUUId), fetch the
persisted associated API UUID from the association/DB object you loaded earlier
(e.g., persistedAssociation.getApiUUId() or associatedApi.getApiUUId()) and pass
that value into apiKeyAssociationEvent.setApiUUId(...), keeping the
apiKeyAssociationEvent.setApiKeyUUId(keyUUId) call unchanged; this ensures
APIConsumerImpl emits the delete event with the persisted UUID rather than a
potentially stale input value.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java`:
- Around line 24136-24144: The current upsert in ApiMgtDAO uses
updateStatement.executeUpdate() followed by an insertStatement
(SQLConstants.ADD_API_KEY_EXTERNAL_API_KEY_MAPPING_SQL) which races under
concurrent writers; replace this with a concurrency-safe upsert strategy: either
switch to a DB-native upsert/merge SQL (preferred) or wrap the insertStatement
in a catch for duplicate-key/SQLIntegrityConstraintViolationException and, on
that exception, re-run the updateStatement (i.e., perform a fallback update) so
the code converges instead of failing; locate the logic around
updateStatement.executeUpdate and the insert using
SQLConstants.ADD_API_KEY_EXTERNAL_API_KEY_MAPPING_SQL to implement the change.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/factory/GatewayHolder.java`:
- Around line 86-101: The code in GatewayHolder is passing a potentially null
value from GatewayAgentConfiguration.getApiKeyConnectorImplementation() into
Class.forName() and is also swallowing environment-resolution/API errors by
catching APIManagementException; add a null guard: check
gatewayAgentConfiguration.getApiKeyConnectorImplementation() for null (and
log/return null or skip instantiation) before calling Class.forName(), and
change the catch clause to only catch reflection-related exceptions
(ClassNotFoundException, NoSuchMethodException, InstantiationException,
IllegalAccessException, InvocationTargetException) so APIManagementException
thrown during environment resolution propagates up unchanged; keep the existing
log+rethrow for the reflection exceptions referencing the same message and
exception variable.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/events/APIKeyEvent.java`:
- Around line 157-163: Remove the misleading transient modifier from the apiKey
field in the APIKeyEvent class: edit class APIKeyEvent to declare the apiKey
field without the transient keyword so it is retained in memory and available to
notifiers (ensure getApiKey and setApiKey remain unchanged); no Serializable
implementation or other code changes are needed—just drop the transient modifier
from the apiKey field declaration.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java`:
- Around line 367-398: The loop currently calls connector.replaceApiKey(...) for
each gateway and defers DB writes, which can orphan successful replacements if a
later step fails; change the flow so that after each successful replaceApiKey
(check result and result.getReferenceArtifact()), immediately persist that
mapping by calling
getApiMgtDAO().addOrUpdateApiKeyExternalApiKeyMapping(event.getNewApiKeyUuid(),
gatewayEnvironment.getEnvironmentId(), result.getReferenceArtifact()) and also
record that environment in replacementReferenceArtifacts, then continue to the
next environment; only after the entire loop succeeds call
getApiMgtDAO().deleteApiKeyExternalApiKeyMappings(event.getOldApiKeyUuid()); if
any exception occurs during the per-environment DB write or subsequent
iterations, roll back by deleting any new mappings for event.getNewApiKeyUuid()
(using
getApiMgtDAO().deleteApiKeyExternalApiKeyMappings(event.getNewApiKeyUuid())) and
rethrow the exception.
- Around line 486-488: In resolveConnector (class FederatedApiKeyNotifier)
handle the case where
GatewayHolder.getTenantApiKeyConnectorInstance(organization,
environment.getEnvironmentId()) returns null by throwing an
APIManagementException immediately with a clear message containing the
organization and environment id; this ensures callers don’t receive a null and
avoids downstream NPEs and preserves the notifier's error handling path.
- Around line 119-124: The resolveEventType(String type) method should guard
against a null input before calling Enum.valueOf to avoid a
NullPointerException; update resolveEventType to first if (type == null) return
null, then call APIConstants.EventType.valueOf(type) inside the existing
try/catch (catching IllegalArgumentException and returning null) so callers like
handleAPIKeyEvent() and handleAPIKeyAssociationEvent() receive null for
null/invalid types instead of crashing.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/EnvironmentMappingUtil.java`:
- Around line 296-305: The loop in
fromAdditionalPropertiesDTOToAdditionalProperties(List<AdditionalPropertyDTO>
additionalPropertiesDTOs) can NPE if the list contains null entries; update the
for loop to skip null entries (e.g., continue when entry is null) before calling
entry.getKey()/entry.getValue(), and keep using
additionalProperties.putIfAbsent(...) for non-null entries so the method safely
handles payloads like [null].
In
`@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sql`:
- Around line 3754-3758: The GATEWAY_ENV_ID child column is declared as
VARCHAR(255) but references AM_GATEWAY_ENVIRONMENT.UUID which is VARCHAR(45),
causing a DB2 referential integrity mismatch; update the child column definition
(GATEWAY_ENV_ID) in the affected table(s) to VARCHAR(45) to exactly match
AM_GATEWAY_ENVIRONMENT.UUID, and apply the same fix for the other occurrence
noted (the similar mismatch around line 3258) so all FK declarations referencing
AM_GATEWAY_ENVIRONMENT.UUID use VARCHAR(45).
In
`@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql`:
- Around line 3227-3231: The GATEWAY_ENV_ID column is declared as VARCHAR(255)
but must match the referenced AM_GATEWAY_ENVIRONMENT.UUID length (VARCHAR(45));
update the column definition in the table that contains GATEWAY_ENV_ID (the
statement defining PRIMARY KEY (API_KEY_UUID, GATEWAY_ENV_ID) and FOREIGN KEY
(GATEWAY_ENV_ID) REFERENCES AM_GATEWAY_ENVIRONMENT(UUID)) to use VARCHAR(45) so
it matches AM_GATEWAY_ENVIRONMENT.UUID and other GATEWAY_ENV_ID occurrences.
In
`@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sql`:
- Around line 3105-3112: The GATEWAY_ENV_ID column in table
AM_API_KEY_EXTERNAL_API_KEY_MAPPING is declared as VARCHAR(255) but references
AM_GATEWAY_ENVIRONMENT.UUID which is VARCHAR(45); update the column definition
for GATEWAY_ENV_ID in the CREATE TABLE for AM_API_KEY_EXTERNAL_API_KEY_MAPPING
to VARCHAR(45) so the foreign key types match (mirror the pattern used in
AM_API_EXTERNAL_API_MAPPING) and keep the FOREIGN KEY (GATEWAY_ENV_ID)
REFERENCES AM_GATEWAY_ENVIRONMENT(UUID) intact.
---
Outside diff comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml`:
- Around line 1593-1599: The GET /applications operation's security block is
missing the apim:app_token_type_upgrade scope; update the OAuth2Security entry
for the GET /applications endpoint (the OAuth2Security array under that
operation) to include "apim:app_token_type_upgrade" alongside apim:admin,
apim:app_settings_change, apim:app_owner_change, apim:app_import_export, and
apim:admin_application_view so the admin token-type-upgrade flow can list
applications.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml`:
- Around line 1593-1599: The GET /applications endpoint's security block (the
OAuth2Security list) is missing the apim:app_token_type_upgrade scope; update
the OAuth2Security entry for the GET /applications operation to include
"apim:app_token_type_upgrade" alongside the existing scopes (e.g., apim:admin,
apim:app_settings_change, apim:app_owner_change, apim:app_import_export,
apim:admin_application_view) so the admin UI can perform token-type upgrade
flows; ensure you update the same security block occurrences referenced (also
around the 8110-8154 region) so all admin-api.yaml definitions are consistent.
---
Nitpick comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/FederatedApiKeyCreationResult.java`:
- Around line 73-75: The Builder.metadata(Map<String, Object> metadata) setter
currently stores the caller's map by reference, so mutations after calling
metadata(...) are observed at build time; change the setter in
FederatedApiKeyCreationResult.Builder to defensively copy the incoming map
(e.g., new HashMap<>(metadata) or Collections.unmodifiableMap(new
HashMap<>(...))) and handle nulls appropriately so the Builder holds an isolated
snapshot of metadata for predictable build() behavior.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java`:
- Around line 681-683: Remove the redundant debug-level guard around the static
log message in APIUtil: replace the if (log.isDebugEnabled()) {
log.debug("Skipping federated gateway API check because API UUID is blank.
Assuming non-federated."); } pattern with a direct log.debug(...) call; change
code referencing log.isDebugEnabled() in APIUtil to simply invoke
log.debug("Skipping federated gateway API check because API UUID is blank.
Assuming non-federated."); so only the unnecessary branching using
log.isDebugEnabled() is removed while preserving the log.debug invocation.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/dao/test/APIMgtDAOTest.java`:
- Around line 205-254: The test method testApiKeyExternalApiKeyMappingLifecycle
seeds rows in AM_API_KEY and AM_GATEWAY_ENVIRONMENT but only deletes mappings;
add a finally block that removes the seeded rows to keep DB isolated: after
exercising
addOrUpdateApiKeyExternalApiKeyMapping/getApiKeyExternalApiKeyMappings/deleteApiKeyExternalApiKeyMappings,
open a connection and execute DELETE FROM AM_API_KEY WHERE API_KEY_UUID IN
(oldApiKeyUuid, newApiKeyUuid) and DELETE FROM AM_GATEWAY_ENVIRONMENT WHERE UUID
= gatewayEnvUuid (or equivalent), ensuring the cleanup runs even if assertions
fail.
In
`@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql`:
- Around line 2916-2923: Add a standalone index on GATEWAY_ENV_ID in the
AM_API_KEY_EXTERNAL_API_KEY_MAPPING table to speed up environment-based lookups;
update the DDL by creating an index (e.g., CREATE INDEX or ALTER TABLE ADD
INDEX) for column GATEWAY_ENV_ID so queries that filter by GATEWAY_ENV_ID don’t
require a full table scan despite the composite primary key (refer to
AM_API_KEY_EXTERNAL_API_KEY_MAPPING and the GATEWAY_ENV_ID column and its FK to
AM_GATEWAY_ENVIRONMENT(UUID)).
In
`@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sql`:
- Around line 4415-4422: Add a dedicated non-unique index on the GATEWAY_ENV_ID
column in the AM_API_KEY_EXTERNAL_API_KEY_MAPPING table to support efficient FK
lookups to AM_GATEWAY_ENVIRONMENT(UUID); locate the table definition for
AM_API_KEY_EXTERNAL_API_KEY_MAPPING and add a CREATE INDEX (e.g.,
IDX_AM_API_KEY_EXT_GW_ENV on GATEWAY_ENV_ID) after the table DDL so
delete/update checks on the AM_GATEWAY_ENVIRONMENT(UUID) FK use the index.
In
`@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sql`:
- Around line 4107-4114: The new table AM_API_KEY_EXTERNAL_API_KEY_MAPPING
defines a FK on GATEWAY_ENV_ID but only has a composite PK starting with
API_KEY_UUID, which can make gateway-environment lookups and FK maintenance
slow; add a dedicated index on GATEWAY_ENV_ID for this table (e.g., create an
index like IDX_AM_API_KEY_EXT_GATEWAY_ENV_ID on
AM_API_KEY_EXTERNAL_API_KEY_MAPPING(GATEWAY_ENV_ID)) so the foreign key and
queries filtering by GATEWAY_ENV_ID use an efficient path; ensure the index name
follows existing naming conventions and commit the DDL in the same SQL file near
the table definition.
In
`@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql`:
- Around line 4417-4424: The GATEWAY_ENV_ID column in
AM_API_KEY_EXTERNAL_API_KEY_MAPPING is defined as VARCHAR(255) but must match
the referenced AM_GATEWAY_ENVIRONMENT.UUID size (VARCHAR(45)); update the column
definition in the CREATE TABLE for AM_API_KEY_EXTERNAL_API_KEY_MAPPING to use
VARCHAR(45) so the foreign key FK to AM_GATEWAY_ENVIRONMENT(UUID) aligns
exactly, keeping the same primary key and FK constraints (API_KEY_UUID,
GATEWAY_ENV_ID) and not changing other types.
In
`@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql`:
- Around line 3330-3337: Add a dedicated index on the GATEWAY_ENV_ID column of
the AM_API_KEY_EXTERNAL_API_KEY_MAPPING table to avoid full table scans when
queries filter by GATEWAY_ENV_ID; update the PostgreSQL migration to create an
index (e.g., CREATE INDEX IF NOT EXISTS ...) using the table name
AM_API_KEY_EXTERNAL_API_KEY_MAPPING and column GATEWAY_ENV_ID so FK-driven
lookups and queries that only filter by GATEWAY_ENV_ID are served by the new
index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a514bd45-21b7-4225-878c-650d36ea8d85
⛔ Files ignored due to path filters (1)
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/dto/GatewayConfigurationDTO.javais excluded by!**/gen/**
📒 Files selected for processing (33)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/ExceptionCodes.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/FederatedApiKeyConnector.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/ConfigurationDto.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/FederatedApiKeyContext.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/FederatedApiKeyCreationResult.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/GatewayAgentConfiguration.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/GatewayConfigurationContext.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIAdminImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/constants/SQLConstants.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/factory/GatewayHolder.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/internal/APIManagerComponent.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/events/APIKeyAssociationEvent.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/events/APIKeyEvent.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/events/APIKeyRegenerationEvent.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/dao/test/APIMgtDAOTest.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/EnvironmentsApiServiceImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/EnvironmentMappingUtil.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/SettingsMappingUtil.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yamlcomponents/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yamlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiKeyMgtDAO.java`:
- Around line 912-922: The deleteApiKeyExternalApiKeyMappings method lacks
explicit transaction handling and should mirror other write methods: obtain the
Connection from APIMgtDBUtil.getConnection(), call
connection.setAutoCommit(false) before preparing
SQLConstants.DELETE_API_KEY_EXTERNAL_API_KEY_MAPPINGS_SQL, execute the update,
call connection.commit() on success, and call connection.rollback() in the catch
block before handling the exception; finally ensure the connection is closed in
a try-with-resources or finally-like construct so commit/rollback occurs on the
same Connection instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dccb3277-fa10-407e-afb4-78c769112f8a
📒 Files selected for processing (3)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/FederatedApiKeyConnector.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiKeyMgtDAO.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java
🚧 Files skipped from review as they are similar to previous changes (1)
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/FederatedApiKeyConnector.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java (1)
391-401: Remove unused methodresolveEnvIdToApiRefMap.This private method at lines 391-401 is not invoked anywhere in the codebase. It should be removed to reduce code maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java` around lines 391 - 401, The private method resolveEnvIdToApiRefMap in class FederatedApiKeyNotifier is unused and should be removed: delete the entire method declaration (private Map<String, String> resolveEnvIdToApiRefMap(...)) from FederatedApiKeyNotifier, ensure there are no remaining calls or references to resolveEnvIdToApiRefMap elsewhere, and run a build/test to confirm no compile or reference errors remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java`:
- Around line 391-401: The private method resolveEnvIdToApiRefMap in class
FederatedApiKeyNotifier is unused and should be removed: delete the entire
method declaration (private Map<String, String> resolveEnvIdToApiRefMap(...))
from FederatedApiKeyNotifier, ensure there are no remaining calls or references
to resolveEnvIdToApiRefMap elsewhere, and run a build/test to confirm no compile
or reference errors remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2483dab8-1635-43a8-85ab-dea0b1c31c93
📒 Files selected for processing (2)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/FederatedApiKeyConnector.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml (1)
1593-1599:⚠️ Potential issue | 🟠 MajorAdd missing
apim:app_token_type_upgradescope toGET /applications.The security block for Line 1529 (
GET /applications) still missesapim:app_token_type_upgrade, which blocks the admin UI token-type-upgrade pre-listing flow.🔧 Suggested patch
security: - OAuth2Security: - apim:admin - apim:app_settings_change - apim:app_owner_change + - apim:app_token_type_upgrade - apim:app_import_export - apim:admin_application_viewBased on learnings: Ensure
apim:app_token_type_upgradeis included in theGET /applicationssecurity block inadmin-api.yamlresources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml` around lines 1593 - 1599, The GET /applications operation's OAuth2Security block is missing the apim:app_token_type_upgrade scope; update the security list under the GET /applications definition (OAuth2Security) to include apim:app_token_type_upgrade alongside apim:admin, apim:app_settings_change, apim:app_owner_change, apim:app_import_export, and apim:admin_application_view so the admin UI token-type-upgrade flow can authorize correctly.
♻️ Duplicate comments (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java (1)
344-372:⚠️ Potential issue | 🔴 CriticalPersist each successful replacement before moving to the next environment.
The remote replace happens at Line 355, but the new reference artifact is only written at Lines 364-369. If one environment replaces successfully and a later replace or DAO write fails, Carbon is left with only the stale old reference for the environment that already switched.
At minimum, write
addOrUpdateApiKeyExternalApiKeyMapping(event.getNewApiKeyUuid(), environmentId, ...)immediately after each successfulreplaceApiKey(...), and defer only the old-key cleanup until the whole loop succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java` around lines 344 - 372, The loop performs remote replaceApiKey(...) for each environment but defers persisting the new reference artifacts until after all replacements, risking partial state if a later replace/DAO write fails; change the logic in the for-loop over apiKeyReferenceArtifacts so that after a successful call to FederatedApiKeyConnector.replaceApiKey(...) and validation of FederatedApiKeyCreationResult.getReferenceArtifact(), you immediately call getApiKeyMgtDAO().addOrUpdateApiKeyExternalApiKeyMapping(event.getNewApiKeyUuid(), environmentId, result.getReferenceArtifact()) and only store the value in replacementReferenceArtifacts for bookkeeping, then after the entire loop succeeds call getApiKeyMgtDAO().deleteApiKeyExternalApiKeyMappings(event.getOldApiKeyUuid()) as before (and ensure rollback on exceptions still deletes any new mappings for event.getNewApiKeyUuid()).
🧹 Nitpick comments (2)
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/SettingsMappingUtil.java (1)
195-197: Lazy-load subscription policies only when federated connectors exist.
buildSubscriptionPolicies()now runs on every settings request before we even know whether any external gateway connector is registered. In deployments that only use regular/APK gateways, this adds an avoidable DAO round trip to the admin bootstrap path.♻️ Suggested change
- List<SubscriptionPolicy> subscriptionPolicies = buildSubscriptionPolicies(); Map<String, GatewayAgentConfiguration> gatewayConfigurations = ServiceReferenceHolder.getInstance().getExternalGatewayConnectorConfigurations(); + List<SubscriptionPolicy> subscriptionPolicies = + gatewayConfigurations.isEmpty() ? new ArrayList<>() : buildSubscriptionPolicies();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/SettingsMappingUtil.java` around lines 195 - 197, The code eagerly calls buildSubscriptionPolicies() for every settings request even when no external gateway connectors exist; change it to first read gatewayConfigurations via ServiceReferenceHolder.getInstance().getExternalGatewayConnectorConfigurations() and only call buildSubscriptionPolicies() (and populate subscriptionPolicies) if that map is non-null and not empty; keep the same variable names (subscriptionPolicies, gatewayConfigurations) and ensure subscriptionPolicies is initialized lazily to avoid the unnecessary DAO call when no federated connectors are registered.components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/GatewayEnvironmentValidationResult.java (1)
24-40: Encapsulate fields and makeerrorsnull-safe by default.
isValidanderrorsshould beprivate, anderrorsshould default to an empty list to avoid null handling leaks to callers.♻️ Suggested refactor
import java.util.List; +import java.util.Collections; public class GatewayEnvironmentValidationResult { - boolean isValid; - List<String> errors; + private boolean isValid; + private List<String> errors = Collections.emptyList(); @@ public void setErrors(List<String> errors) { - this.errors = errors; + this.errors = (errors == null) ? Collections.emptyList() : errors; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/GatewayEnvironmentValidationResult.java` around lines 24 - 40, Make the fields private and ensure errors is null-safe by default: change the package-visible fields isValid and errors in GatewayEnvironmentValidationResult to private, initialize errors to an empty List (e.g., Collections.emptyList() or new ArrayList<>()) so callers never get null, and update the existing accessors isValid(), setValid(boolean), getErrors(), and setErrors(List<String>) to operate on the private fields (setErrors should defensively copy or replace null with an empty list). Ensure the class name GatewayEnvironmentValidationResult and the methods isValid, setValid, getErrors, and setErrors are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/GatewayAgentConfiguration.java`:
- Line 91: The new abstract method validateEnvironment(Environment environment)
in GatewayAgentConfiguration is a binary-breaking change; make it a default
method on the interface that returns a successful
GatewayEnvironmentValidationResult so existing external implementations don't
get AbstractMethodError. Locate GatewayAgentConfiguration and change
validateEnvironment to a default implementation that constructs/returns a
success GatewayEnvironmentValidationResult (using the same result type) and
leave APIAdminImpl and existing callers unchanged so third-party connectors can
still override when custom validation is required.
In
`@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sql`:
- Line 3258: Update the column length for GATEWAY_ENV_ID in the
AM_API_EXTERNAL_API_MAPPING and AM_API_KEY_EXTERNAL_API_KEY_MAPPING table
definitions to use VARCHAR(45) (to match AM_GATEWAY_ENVIRONMENT.UUID) in all SQL
dialect variants that still use VARCHAR(255) (PostgreSQL, Oracle, Oracle RAC,
Oracle 23c, H2, MySQL); locate the CREATE TABLE statements for
AM_API_EXTERNAL_API_MAPPING and AM_API_KEY_EXTERNAL_API_KEY_MAPPING in each
dialect file and change the GATEWAY_ENV_ID type from VARCHAR(255) to
VARCHAR(45), keeping any NOT NULL or other constraints intact so the schema is
consistent across databases.
---
Outside diff comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml`:
- Around line 1593-1599: The GET /applications operation's OAuth2Security block
is missing the apim:app_token_type_upgrade scope; update the security list under
the GET /applications definition (OAuth2Security) to include
apim:app_token_type_upgrade alongside apim:admin, apim:app_settings_change,
apim:app_owner_change, apim:app_import_export, and apim:admin_application_view
so the admin UI token-type-upgrade flow can authorize correctly.
---
Duplicate comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java`:
- Around line 344-372: The loop performs remote replaceApiKey(...) for each
environment but defers persisting the new reference artifacts until after all
replacements, risking partial state if a later replace/DAO write fails; change
the logic in the for-loop over apiKeyReferenceArtifacts so that after a
successful call to FederatedApiKeyConnector.replaceApiKey(...) and validation of
FederatedApiKeyCreationResult.getReferenceArtifact(), you immediately call
getApiKeyMgtDAO().addOrUpdateApiKeyExternalApiKeyMapping(event.getNewApiKeyUuid(),
environmentId, result.getReferenceArtifact()) and only store the value in
replacementReferenceArtifacts for bookkeeping, then after the entire loop
succeeds call
getApiKeyMgtDAO().deleteApiKeyExternalApiKeyMappings(event.getOldApiKeyUuid())
as before (and ensure rollback on exceptions still deletes any new mappings for
event.getNewApiKeyUuid()).
---
Nitpick comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/GatewayEnvironmentValidationResult.java`:
- Around line 24-40: Make the fields private and ensure errors is null-safe by
default: change the package-visible fields isValid and errors in
GatewayEnvironmentValidationResult to private, initialize errors to an empty
List (e.g., Collections.emptyList() or new ArrayList<>()) so callers never get
null, and update the existing accessors isValid(), setValid(boolean),
getErrors(), and setErrors(List<String>) to operate on the private fields
(setErrors should defensively copy or replace null with an empty list). Ensure
the class name GatewayEnvironmentValidationResult and the methods isValid,
setValid, getErrors, and setErrors are updated accordingly.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/SettingsMappingUtil.java`:
- Around line 195-197: The code eagerly calls buildSubscriptionPolicies() for
every settings request even when no external gateway connectors exist; change it
to first read gatewayConfigurations via
ServiceReferenceHolder.getInstance().getExternalGatewayConnectorConfigurations()
and only call buildSubscriptionPolicies() (and populate subscriptionPolicies) if
that map is non-null and not empty; keep the same variable names
(subscriptionPolicies, gatewayConfigurations) and ensure subscriptionPolicies is
initialized lazily to avoid the unnecessary DAO call when no federated
connectors are registered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa524201-de2e-4e93-aff0-dcec0f01e8cb
📒 Files selected for processing (13)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/GatewayAgentConfiguration.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/GatewayEnvironmentValidationResult.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIAdminImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiKeyMgtDAO.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/factory/GatewayHolder.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/SettingsMappingUtil.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yamlcomponents/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yamlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/factory/GatewayHolder.java
- components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiKeyMgtDAO.java
- Made connector resolution tenant aware
- Made the api environment validation follow the pattern of API Validation
- Made the GatewayAgentConfiguration backward compatible
dpiyumal2319
force-pushed
the
dasunw-api-key-federated
branch
from
a1813b9 to
97a0da5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java (1)
354-369:⚠️ Potential issue | 🔴 CriticalPersist each successful replacement before continuing.
replaceApiKey()mutates the remote credential at Line 355, but the new UUID→reference mapping is deferred until after the whole loop. If a later replace or DAO write fails, earlier environments are already on the new key while the DB still points at the old UUID/reference, so subsequent revoke/apply flows target stale artifacts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java` around lines 354 - 369, The loop currently collects replacementReferenceArtifacts and defers DB writes, risking remote credentials being changed while the DB still points to old references; move the persistence step inline so each successful replaceApiKey call is immediately committed. Specifically, in the loop that calls FederatedApiKeyConnector.replaceApiKey(...) and receives a FederatedApiKeyCreationResult, call getApiKeyMgtDAO().addOrUpdateApiKeyExternalApiKeyMapping(event.getNewApiKeyUuid(), environmentId, result.getReferenceArtifact()) right after verifying result (and before proceeding to the next environment), remove the later deferred bulk addOrUpdate that uses replacementReferenceArtifacts, and let any thrown exceptions propagate to abort further replacements so state stays consistent (keep deleteApiKeyExternalApiKeyMappings(event.getOldApiKeyUuid()) after successful completion of all replacements).
🧹 Nitpick comments (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (1)
681-683: RemoveisDebugEnabled()guard for literal debug messages.Static string literals do not require the guard; it adds unnecessary overhead.
♻️ Suggested change
- if (log.isDebugEnabled()) { - log.debug("Skipping federated gateway API check because API UUID is blank. Assuming non-federated."); - } + log.debug("Skipping federated gateway API check because API UUID is blank. Assuming non-federated.");Note: This pattern appears in multiple places in the file (lines 3275–3277, 3295–3297, 3316–3318, 3343–3345).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java` around lines 681 - 683, In APIUtil remove the unnecessary log.isDebugEnabled() guards around literal debug messages and call log.debug(...) directly; specifically replace the if-block that wraps the literal "Skipping federated gateway API check because API UUID is blank. Assuming non-federated." with a direct log.debug(...) call, and do the same for the other occurrences in this class (the other literal debug messages around the reported ranges), referencing the log variable and the exact message strings to locate each spot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/factory/GatewayHolder.java`:
- Around line 89-99: The reflective loading of the API key connector must
validate and guard against misconfiguration: trim and verify
gatewayAgentConfiguration.getApiKeyConnectorImplementation() is not null/blank
(reject empty or whitespace and throw an APIManagementException with a clear
message referencing the configured gateway type), then attempt
Class.forName(...).getDeclaredConstructor().newInstance(); and wrap any
ClassCastException thrown when casting to FederatedApiKeyConnector into an
APIManagementException (with context including the configured class name and
expected interface), and ensure connector.init(resolvedEnvironment) is only
called after a successful, type-checked instantiation; update the existing catch
block that handles reflection exceptions to also catch ClassCastException and
produce a managed APIManagementException.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java`:
- Around line 210-218: The code in FederatedApiKeyNotifier calls
getApiKeyMgtDAO().deleteApiKeyExternalApiKeyMappings(event.getUuid())
unconditionally even when some environments were skipped
(StringUtils.isBlank(apiKeyReferenceArtifact) leads to continue), which drops
mappings you still need; modify the logic to only delete mappings for
environments whose remote revoke/regenerate succeeded (or skip deletion entirely
when any environment was skipped) by tracking successful environment keys (e.g.,
collect entry.getKey() for which connector.revokeApiKey(...) returned/ran
without skipping) and call the DAO to delete only those specific
external-api-key mappings instead of deleting all mappings for event.getUuid();
apply the same change to the other similar blocks around the regenerate flow
(lines referenced in the review).
In
`@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql`:
- Around line 4417-4424: The AM_API_KEY_EXTERNAL_API_KEY_MAPPING table has no ON
DELETE CASCADE, so add an explicit delete of its rows by GATEWAY_ENV_ID before
removing the environment; update the code paths that delete environments
(ApiMgtDAO.deleteEnvironment() and the PlatformGatewayDAO method that executes
DELETE_ENVIRONMENT_SQL) to execute a DELETE FROM
AM_API_KEY_EXTERNAL_API_KEY_MAPPING WHERE GATEWAY_ENV_ID = ? (binding the
environment UUID) immediately prior to executing DELETE_ENVIRONMENT_SQL,
mirroring how deleteApiKeyExternalApiKeyMappings() is used when deleting API
keys.
---
Duplicate comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java`:
- Around line 354-369: The loop currently collects replacementReferenceArtifacts
and defers DB writes, risking remote credentials being changed while the DB
still points to old references; move the persistence step inline so each
successful replaceApiKey call is immediately committed. Specifically, in the
loop that calls FederatedApiKeyConnector.replaceApiKey(...) and receives a
FederatedApiKeyCreationResult, call
getApiKeyMgtDAO().addOrUpdateApiKeyExternalApiKeyMapping(event.getNewApiKeyUuid(),
environmentId, result.getReferenceArtifact()) right after verifying result (and
before proceeding to the next environment), remove the later deferred bulk
addOrUpdate that uses replacementReferenceArtifacts, and let any thrown
exceptions propagate to abort further replacements so state stays consistent
(keep deleteApiKeyExternalApiKeyMappings(event.getOldApiKeyUuid()) after
successful completion of all replacements).
---
Nitpick comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java`:
- Around line 681-683: In APIUtil remove the unnecessary log.isDebugEnabled()
guards around literal debug messages and call log.debug(...) directly;
specifically replace the if-block that wraps the literal "Skipping federated
gateway API check because API UUID is blank. Assuming non-federated." with a
direct log.debug(...) call, and do the same for the other occurrences in this
class (the other literal debug messages around the reported ranges), referencing
the log variable and the exact message strings to locate each spot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0482a60-6115-4ccb-9df3-fc66a2dd542a
⛔ Files ignored due to path filters (1)
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/dto/GatewayConfigurationDTO.javais excluded by!**/gen/**
📒 Files selected for processing (30)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/ExceptionCodes.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/FederatedApiKeyConnector.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/ConfigurationDto.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/FederatedApiKeyCreationResult.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/GatewayAgentConfiguration.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/GatewayEnvironmentValidationResult.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIAdminImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConsumerImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiKeyMgtDAO.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/constants/SQLConstants.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/factory/GatewayHolder.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/internal/APIManagerComponent.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/events/APIKeyAssociationEvent.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/events/APIKeyEvent.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/events/APIKeyRegenerationEvent.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/EnvironmentsApiServiceImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/SettingsMappingUtil.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yamlcomponents/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yamlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sqlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql
✅ Files skipped from review due to trivial changes (8)
- components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml
- components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/EnvironmentsApiServiceImpl.java
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/ConfigurationDto.java
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/GatewayEnvironmentValidationResult.java
- components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sql
- components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/constants/SQLConstants.java
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql
🚧 Files skipped from review as they are similar to previous changes (9)
- components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/internal/APIManagerComponent.java
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sql
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/FederatedApiKeyCreationResult.java
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql
- components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/events/APIKeyRegenerationEvent.java
- components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIAdminImpl.java
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sql
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql
Comment on lines
+89
to
+99
| String connectorImplementation = | ||
| gatewayAgentConfiguration.getApiKeyConnectorImplementation(); | ||
| if (connectorImplementation == null) { | ||
| throw new APIManagementException("No federated API key connector implementation is " | ||
| + "configured for gateway type: " + resolvedEnvironment.getGatewayType()); | ||
| } | ||
| FederatedApiKeyConnector connector = (FederatedApiKeyConnector) Class.forName( | ||
| connectorImplementation) | ||
| .getDeclaredConstructor().newInstance(); | ||
| connector.init(resolvedEnvironment); | ||
| return connector; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify reflective loading path currently does null-only check and unchecked cast
rg -n -C3 'getApiKeyConnectorImplementation|Class\.forName|FederatedApiKeyConnector connector' \
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/factory/GatewayHolder.javaRepository: wso2/carbon-apimgt
Length of output: 1658
🏁 Script executed:
# Get the full method containing the getTenantApiKeyConnectorInstance code
fd -n "GatewayHolder.java" components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java | head -1 | xargs -I {} sh -c 'wc -l {} && echo "---" && cat -n {}'Repository: wso2/carbon-apimgt
Length of output: 293
🏁 Script executed:
# Get full method context around lines 89-99 to see exception handling
sed -n '75,110p' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/factory/GatewayHolder.java | cat -nRepository: wso2/carbon-apimgt
Length of output: 2711
🏁 Script executed:
# Check if there's a try-catch wrapping this reflective call
sed -n '70,120p' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/factory/GatewayHolder.java | cat -nRepository: wso2/carbon-apimgt
Length of output: 2837
🏁 Script executed:
# Look for FederatedApiKeyConnector definition to understand if it's interface/class
fd -name "FederatedApiKeyConnector.java" components/Repository: wso2/carbon-apimgt
Length of output: 293
🏁 Script executed:
fd "FederatedApiKeyConnector.java" components/Repository: wso2/carbon-apimgt
Length of output: 178
Harden reflective connector loading against misconfiguration.
The catch block (lines 36–40) does not catch ClassCastException. If the configured class does not implement FederatedApiKeyConnector, the unchecked cast at line 95 throws ClassCastException and bypasses the managed APIManagementException path. Additionally, line 91 only checks for null; blank or whitespace configuration values are not rejected and can produce confusing errors during class loading.
🔧 Proposed fix
String connectorImplementation =
gatewayAgentConfiguration.getApiKeyConnectorImplementation();
- if (connectorImplementation == null) {
+ if (connectorImplementation == null || connectorImplementation.trim().isEmpty()) {
throw new APIManagementException("No federated API key connector implementation is "
+ "configured for gateway type: " + resolvedEnvironment.getGatewayType());
}
- FederatedApiKeyConnector connector = (FederatedApiKeyConnector) Class.forName(
- connectorImplementation)
- .getDeclaredConstructor().newInstance();
+ Object connectorInstance = Class.forName(connectorImplementation)
+ .getDeclaredConstructor().newInstance();
+ if (!(connectorInstance instanceof FederatedApiKeyConnector)) {
+ throw new APIManagementException("Configured class does not implement "
+ + FederatedApiKeyConnector.class.getName() + ": " + connectorImplementation);
+ }
+ FederatedApiKeyConnector connector = (FederatedApiKeyConnector) connectorInstance;
connector.init(resolvedEnvironment);
return connector;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/factory/GatewayHolder.java`
around lines 89 - 99, The reflective loading of the API key connector must
validate and guard against misconfiguration: trim and verify
gatewayAgentConfiguration.getApiKeyConnectorImplementation() is not null/blank
(reject empty or whitespace and throw an APIManagementException with a clear
message referencing the configured gateway type), then attempt
Class.forName(...).getDeclaredConstructor().newInstance(); and wrap any
ClassCastException thrown when casting to FederatedApiKeyConnector into an
APIManagementException (with context including the configured class name and
expected interface), and ensure connector.init(resolvedEnvironment) is only
called after a successful, type-checked instantiation; update the existing catch
block that handles reflection exceptions to also catch ClassCastException and
produce a managed APIManagementException.
Comment on lines
+210
to
+218
| if (StringUtils.isBlank(apiKeyReferenceArtifact)) { | ||
| log.warn("Remote API key reference artifact is missing for federated API key UUID: " + event.getUuid() | ||
| + " in environment: " + entry.getKey() + ". Skipping remote revocation."); | ||
| continue; | ||
| } | ||
| FederatedApiKeyConnector connector = resolveConnector(organization, entry.getKey()); | ||
| connector.revokeApiKey(apiKeyReferenceArtifact); | ||
| } | ||
| getApiKeyMgtDAO().deleteApiKeyExternalApiKeyMappings(event.getUuid()); |
There was a problem hiding this comment.
Don't delete mapping rows for environments you skipped.
Both revoke and regenerate continue when the stored reference artifact is blank, but then they still delete the whole mapping set. Since ApiKeyMgtDAO.getApiKeyExternalApiKeyMappings() normalizes a null REFERENCE_ARTIFACT to "", these branches are reachable; deleting the rows here permanently drops the only handle you have to reconcile that remote key later. Either fail the operation or keep those mappings until their revoke/replace succeeds.
Also applies to: 347-351, 369-371
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/notifier/FederatedApiKeyNotifier.java`
around lines 210 - 218, The code in FederatedApiKeyNotifier calls
getApiKeyMgtDAO().deleteApiKeyExternalApiKeyMappings(event.getUuid())
unconditionally even when some environments were skipped
(StringUtils.isBlank(apiKeyReferenceArtifact) leads to continue), which drops
mappings you still need; modify the logic to only delete mappings for
environments whose remote revoke/regenerate succeeded (or skip deletion entirely
when any environment was skipped) by tracking successful environment keys (e.g.,
collect entry.getKey() for which connector.revokeApiKey(...) returned/ran
without skipping) and call the DAO to delete only those specific
external-api-key mappings instead of deleting all mappings for event.getUuid();
apply the same change to the other similar blocks around the regenerate flow
(lines referenced in the review).
Comment on lines
+4417
to
+4424
| CREATE TABLE AM_API_KEY_EXTERNAL_API_KEY_MAPPING ( | ||
| API_KEY_UUID VARCHAR(64) NOT NULL, | ||
| GATEWAY_ENV_ID VARCHAR(45) NOT NULL, | ||
| REFERENCE_ARTIFACT BLOB NOT NULL, | ||
| PRIMARY KEY (API_KEY_UUID, GATEWAY_ENV_ID), | ||
| FOREIGN KEY (API_KEY_UUID) REFERENCES AM_API_KEY(API_KEY_UUID), | ||
| FOREIGN KEY (GATEWAY_ENV_ID) REFERENCES AM_GATEWAY_ENVIRONMENT(UUID) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate relevant DAO/constants files =="
fd -i 'ApiKeyMgtDAO.java|SQLConstants.java|APIAdminImpl.java' components
echo
echo "== Find SQL usage of the new mapping table =="
rg -n --type=java -C3 'AM_API_KEY_EXTERNAL_API_KEY_MAPPING|API_KEY_EXTERNAL_API_KEY_MAPPING'
echo
echo "== Check delete statements/order for parent tables and mapping table =="
rg -n --type=java -C3 'DELETE FROM AM_API_KEY_EXTERNAL_API_KEY_MAPPING|DELETE FROM AM_API_KEY|DELETE FROM AM_GATEWAY_ENVIRONMENT'Repository: wso2/carbon-apimgt
Length of output: 15840
🏁 Script executed:
cd components/apimgt && head -c 20000 org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiKeyMgtDAO.java | tail -c 10000Repository: wso2/carbon-apimgt
Length of output: 10062
🏁 Script executed:
# Get the full delete method context
sed -n '920,950p' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiKeyMgtDAO.javaRepository: wso2/carbon-apimgt
Length of output: 1264
🏁 Script executed:
# Find the method that calls the delete and any related cleanup
rg -A 20 'def.*delete.*ApiKey|void.*delete.*[Aa]pi[Kk]ey' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiKeyMgtDAO.java --type=javaRepository: wso2/carbon-apimgt
Length of output: 1079
🏁 Script executed:
# Check if there's any delete of AM_API_KEY that also cleans up the mapping
rg -B5 -A10 'DELETE_API_KEY\b|DELETE FROM AM_API_KEY' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ --type=javaRepository: wso2/carbon-apimgt
Length of output: 11588
🏁 Script executed:
# Check for GATEWAY_ENVIRONMENT delete and any mapping cleanup
rg -B5 -A10 'DELETE_ENVIRONMENT_SQL|deleteEnvironment|DELETE FROM AM_GATEWAY_ENVIRONMENT' components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ --type=javaRepository: wso2/carbon-apimgt
Length of output: 9321
Add explicit cleanup for AM_API_KEY_EXTERNAL_API_KEY_MAPPING rows before deleting AM_GATEWAY_ENVIRONMENT.
The FK on GATEWAY_ENV_ID lacks ON DELETE CASCADE. While mapping rows are explicitly deleted before deleting an API key (via deleteApiKeyExternalApiKeyMappings() in ApiKeyMgtDAO), the same cleanup is missing before deleting an AM_GATEWAY_ENVIRONMENT row. In ApiMgtDAO.deleteEnvironment() and PlatformGatewayDAO, the environment is deleted without first removing dependent mapping rows, which will cause FK constraint violation.
Add a delete statement for AM_API_KEY_EXTERNAL_API_KEY_MAPPING WHERE GATEWAY_ENV_ID = ? before executing the DELETE_ENVIRONMENT_SQL statement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql`
around lines 4417 - 4424, The AM_API_KEY_EXTERNAL_API_KEY_MAPPING table has no
ON DELETE CASCADE, so add an explicit delete of its rows by GATEWAY_ENV_ID
before removing the environment; update the code paths that delete environments
(ApiMgtDAO.deleteEnvironment() and the PlatformGatewayDAO method that executes
DELETE_ENVIRONMENT_SQL) to execute a DELETE FROM
AM_API_KEY_EXTERNAL_API_KEY_MAPPING WHERE GATEWAY_ENV_ID = ? (binding the
environment UUID) immediately prior to executing DELETE_ENVIRONMENT_SQL,
mirroring how deleteApiKeyExternalApiKeyMappings() is used when deleting API
keys.
- Return connector-owned reference artifact strings directly from federated API key operations and rename subscription policy association methods for clearer semantics. - Simplify API key regeneration event payload/notifier handling and tighten subscription policy UUID resolution by tenant context. - Update external API mapping schema/env-id width and switch API key external mapping persistence to explicit insert flow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Summary
This PR adds the control-plane backend changes needed for federated gateway API key support and external gateway onboarding validation.
The changes are intentionally kept narrow on the carbon side:
What changed
1. Federated gateway API key backend flow
Added the backend support required for federated API key lifecycle operations against external gateways.
This includes:
2. Mapping-driven external key handling
Adjusted the federated API key flow to rely on mapping/state that already exists in the control plane instead of introducing unnecessary API-level checks where
they are not required.
This keeps the behavior closer to the actual external mapping model and avoids extra coupling in notifier logic.
3. Gateway environment validation hook
Added connector-owned validation support for gateway onboarding/update.
On the carbon side:
This keeps validation logic inside the connector while preserving a small carbon diff.
4. Managed validation error handling
Validation failures now return APIM-managed messages instead of exposing raw AWS/Azure/Kong error text.
Carbon only wraps and propagates managed validation failures; it does not introduce a separate validation flow.
Design notes
Why
The goal of this PR is to support federated gateway API key behavior and gateway onboarding validation with: