feat: Introduce On-Demand Federated API Discovery and Import Support#13877
Draft
DinithEdirisinghe wants to merge 9 commits into
Draft
feat: Introduce On-Demand Federated API Discovery and Import Support#13877DinithEdirisinghe wants to merge 9 commits into
DinithEdirisinghe wants to merge 9 commits into
Conversation
…k management and de-duplication
…ton is there in the publisher UI
This reverts commit 265df5d.
…derscores in environment names
📝 WalkthroughWalkthroughThis PR adds end-to-end Federated API Discovery: two new default methods on the ChangesFederated API Discovery
Sequence Diagram(s)sequenceDiagram
actor Publisher
participant FederatedApisApi as FederatedApisApiServiceImpl
participant TaskStore as TASK_STORE / ACTIVE_TASK_BY_ENV
participant Executor as DiscoveryExecutor
participant Runner as FederatedAPIDiscoveryRunner
participant Connector as FederatedAPIDiscovery
Publisher->>FederatedApisApi: POST /federated-apis/discover?environment=X
FederatedApisApi->>TaskStore: evictStaleTask(tenant|env)
FederatedApisApi->>TaskStore: check ACTIVE_TASK_BY_ENV for existing pending
alt task already active
FederatedApisApi-->>Publisher: 202 existing taskId
else no active task
FederatedApisApi->>TaskStore: register new DiscoveryTask (PENDING)
FederatedApisApi->>Executor: submit runDiscovery(task, tenantCtx)
FederatedApisApi-->>Publisher: 202 new taskId
Executor->>Runner: discoverExternalAPIs(env, org)
Runner->>Connector: discoverMetadata()
Runner-->>Executor: Map NEW/UPDATE DiscoveredAPI
Executor->>TaskStore: mark task COMPLETED with result
Executor->>TaskStore: release ACTIVE_TASK_BY_ENV lock
end
Publisher->>FederatedApisApi: GET /federated-apis/status/{taskId}
FederatedApisApi->>TaskStore: lookup task
FederatedApisApi-->>Publisher: 200 status + result
Publisher->>FederatedApisApi: POST /federated-apis/import [apiIds]
FederatedApisApi->>Runner: importNewExternalAPIs(apiIds, env, org)
Runner->>Connector: discoverAPI(apiIds)
Runner->>Runner: importAPI + createExternalAPIMapping
FederatedApisApi-->>Publisher: 200 OK
Publisher->>FederatedApisApi: POST /federated-apis/update [apiIds]
FederatedApisApi->>Runner: updateExternalAPIs(apiIds, env, org)
Runner->>Connector: discoverAPI(apiIds)
Runner->>Runner: importAPI(update) + updateExternalAPIMapping
FederatedApisApi-->>Publisher: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.java[] 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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+53
to
+56
| default List<DiscoveredAPI> discoverMetadata() { | ||
| // Fallback to discoverAPI if not implemented by connector | ||
| return discoverAPI(); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| default List<DiscoveredAPI> discoverMetadata() { | |
| // Fallback to discoverAPI if not implemented by connector | |
| return discoverAPI(); | |
| } | |
| default List<DiscoveredAPI> discoverMetadata() { | |
| // Fallback to discoverAPI if not implemented by connector | |
| log.debug("discoverMetadata not implemented, falling back to discoverAPI()"); | |
| return discoverAPI(); | |
| } |
Comment on lines
+61
to
+64
| default List<DiscoveredAPI> discoverAPI(List<String> apiIds) { | ||
| // Fallback to discoverAPI if not implemented by connector | ||
| return discoverAPI(); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| default List<DiscoveredAPI> discoverAPI(List<String> apiIds) { | |
| // Fallback to discoverAPI if not implemented by connector | |
| return discoverAPI(); | |
| } | |
| default List<DiscoveredAPI> discoverAPI(List<String> apiIds) { | |
| // Fallback to discoverAPI if not implemented by connector | |
| if (log.isDebugEnabled()) { | |
| log.debug("discoverAPI(apiIds) not implemented, falling back to discoverAPI(). Requested API IDs count: " + (apiIds != null ? apiIds.size() : 0)); | |
| } | |
| return discoverAPI(); | |
| } |
Comment on lines
+591
to
+603
|
|
||
| String apiKey = discoveredAPI.getApi().getId().getApiName() + DELEM_COLON + discoveredAPI.getApi().getId().getVersion(); | ||
| String envScopedKey = discoveredAPI.getApi().getId().getApiName() + APIConstants.KEY_SEPARATOR | ||
| + environment.getName() + DELEM_COLON + discoveredAPI.getApi().getId().getVersion(); | ||
|
|
||
| // Track gateway APIs to identify deleted ones | ||
| discoveredAPIsFromFederatedGW.add(apiKey); | ||
| discoveredAPIsFromFederatedGW.add(envScopedKey); | ||
|
|
||
| boolean isExists = alreadyDiscoveredAPIsList.contains(apiKey) | ||
| || alreadyDiscoveredAPIsList.contains(envScopedKey) | ||
| || publishedAPIsList.contains(apiKey) | ||
| || publishedAPIsList.contains(envScopedKey); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| String apiKey = discoveredAPI.getApi().getId().getApiName() + DELEM_COLON + discoveredAPI.getApi().getId().getVersion(); | |
| String envScopedKey = discoveredAPI.getApi().getId().getApiName() + APIConstants.KEY_SEPARATOR | |
| + environment.getName() + DELEM_COLON + discoveredAPI.getApi().getId().getVersion(); | |
| // Track gateway APIs to identify deleted ones | |
| discoveredAPIsFromFederatedGW.add(apiKey); | |
| discoveredAPIsFromFederatedGW.add(envScopedKey); | |
| boolean isExists = alreadyDiscoveredAPIsList.contains(apiKey) | |
| || alreadyDiscoveredAPIsList.contains(envScopedKey) | |
| || publishedAPIsList.contains(apiKey) | |
| || publishedAPIsList.contains(envScopedKey); | |
| + environment.getName() + DELEM_COLON + discoveredAPI.getApi().getId().getVersion(); | |
| // Track gateway APIs to identify deleted ones | |
| discoveredAPIsFromFederatedGW.add(apiKey); | |
| discoveredAPIsFromFederatedGW.add(envScopedKey); | |
| boolean isExists = alreadyDiscoveredAPIsList.contains(apiKey) | |
| || alreadyDiscoveredAPIsList.contains(envScopedKey) | |
| || publishedAPIsList.contains(apiKey) | |
| || publishedAPIsList.contains(envScopedKey); | |
| if (!isExists) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Discovered new API: " + apiKey + " from environment: " + environment.getName()); | |
| } |
| return isMCPSupportEnabled; | ||
| } | ||
|
|
||
| private void setFederatedAPIDiscoveryConfigurations(OMElement omElement) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| private void setFederatedAPIDiscoveryConfigurations(OMElement omElement) { | |
| private void setFederatedAPIDiscoveryConfigurations(OMElement omElement) { | |
| log.debug("Setting FederatedAPIDiscovery configurations"); |
| } | ||
| OMElement schedulerEnabledElement = omElement.getFirstChildWithName(new QName("EnableSchedulerDiscovery")); | ||
| if (schedulerEnabledElement != null && StringUtils.isNotEmpty(schedulerEnabledElement.getText())) { | ||
| isFederatedAPIDiscoverySchedulerEnabled = Boolean.parseBoolean(schedulerEnabledElement.getText().trim()); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| isFederatedAPIDiscoverySchedulerEnabled = Boolean.parseBoolean(schedulerEnabledElement.getText().trim()); | |
| isFederatedAPIDiscoverySchedulerEnabled = Boolean.parseBoolean(schedulerEnabledElement.getText().trim()); | |
| log.info("FederatedAPIDiscovery scheduler enabled: " + isFederatedAPIDiscoverySchedulerEnabled); |
Comment on lines
112
to
+115
| settingsDTO.setAiAuthTokenProvided(config.getDesignAssistantConfigurationDto().isAuthTokenProvided() || | ||
| config.getDesignAssistantConfigurationDto().isKeyProvided()); | ||
| settingsDTO.setIsMCPSupportEnabled(config.isMCPSupportEnabled()); | ||
| settingsDTO.setIsFederatedAPIDiscoveryEnabled(!config.isFederatedAPIDiscoverySchedulerEnabled()); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| settingsDTO.setAiAuthTokenProvided(config.getDesignAssistantConfigurationDto().isAuthTokenProvided() || | |
| config.getDesignAssistantConfigurationDto().isKeyProvided()); | |
| settingsDTO.setIsMCPSupportEnabled(config.isMCPSupportEnabled()); | |
| settingsDTO.setIsFederatedAPIDiscoveryEnabled(!config.isFederatedAPIDiscoverySchedulerEnabled()); | |
| settingsDTO.setAiAuthTokenProvided(config.getDesignAssistantConfigurationDto().isAuthTokenProvided() || | |
| config.getDesignAssistantConfigurationDto().isKeyProvided()); | |
| settingsDTO.setIsMCPSupportEnabled(config.isMCPSupportEnabled()); | |
| settingsDTO.setIsFederatedAPIDiscoveryEnabled(!config.isFederatedAPIDiscoverySchedulerEnabled()); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Federated API Discovery enabled status: " + !config.isFederatedAPIDiscoverySchedulerEnabled()); | |
| } |
Comment on lines
+169
to
+170
| @Override | ||
| public Response discoverFederatedAPIs(String environment, MessageContext messageContext) |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
Suggested change
| @Override | |
| public Response discoverFederatedAPIs(String environment, MessageContext messageContext) | |
| // --- Evict stale completed/failed entries so a fresh run can start ---- | |
| evictStaleTask(envKey); | |
| log.info("Initiating federated API discovery for environment: " + environment + ", organization: " + organization); |
Comment on lines
+335
to
+339
| // Restore Carbon context on the worker thread | ||
| PrivilegedCarbonContext.startTenantFlow(); | ||
| PrivilegedCarbonContext.getThreadLocalCarbonContext().setTenantId(tenantId); | ||
| PrivilegedCarbonContext.getThreadLocalCarbonContext().setTenantDomain(tenantDomain); | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 9
Suggested change
| // Restore Carbon context on the worker thread | |
| PrivilegedCarbonContext.startTenantFlow(); | |
| PrivilegedCarbonContext.getThreadLocalCarbonContext().setTenantId(tenantId); | |
| PrivilegedCarbonContext.getThreadLocalCarbonContext().setTenantDomain(tenantDomain); | |
| FederatedAPIDiscoveryService service = getFederatedDiscoveryService(); | |
| if (service == null) { | |
| log.error("FederatedAPIDiscoveryService OSGi service is not available for discovery task: " + task.taskId); | |
| throw new IllegalStateException("FederatedAPIDiscoveryService OSGi service is not available."); | |
| } |
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.
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/FederatedAPIDiscoveryService.java (1)
57-86: ⚡ Quick winUse
UnsupportedOperationExceptionfor default unsupported interface operations.For these default methods intended to be overridden, prefer throwing
UnsupportedOperationExceptioninstead ofAPIManagementExceptionto align with established API-module behavior and clearer unsupported-operation semantics.Based on learnings, default interface methods meant for override should signal unsupported behavior via
UnsupportedOperationException.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/FederatedAPIDiscoveryService.java` around lines 57 - 86, The three default methods discoverExternalAPIs, importNewExternalAPIs, and updateExternalAPIs currently throw APIManagementException to indicate unsupported operations. Replace APIManagementException with UnsupportedOperationException in all three methods to align with established API-module behavior and provide clearer semantics for default interface methods that are meant to be overridden. This change should be made in the throw statements within each of these three default methods.Source: Learnings
components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml (3)
219-221: ⚡ Quick winDefine a response body for the import operation.
The 200 response has no content defined. For a bulk import operation, clients need feedback on which APIs succeeded or failed. Consider returning a summary (e.g.,
{ "imported": [...], "failed": [...] }) or at minimum an acknowledgment message.responses: description: OK + content: + application/json: + schema: + type: object + properties: + imported: + type: array + items: + type: string + failed: + type: array + items: + type: object + properties: + apiId: + type: string + reason: + type: string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml` around lines 219 - 221, The 200 response definition in the import operation (at line 219-221) is missing a response body specification. Add a content schema to the 200 response that defines the structure of the response payload. The schema should document the response body structure, such as an object containing arrays or counts for imported APIs and failed APIs to give clients visibility into which operations succeeded and which failed during the bulk import.
250-252: ⚡ Quick winDefine a response body for the update operation.
Same concern as the import endpoint—clients need to know which APIs were updated successfully and which failed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml` around lines 250 - 252, The 200 response for the update operation in publisher-api.yaml is missing a response body schema definition. Add a schema to the 200 response that clearly documents what information is returned to clients, including details about which APIs were updated successfully and which failed (such as success status, updated API identifiers, error details for failed updates, etc.). This should follow the same pattern used in the import endpoint to provide clients with comprehensive feedback about the operation results.
183-186: ⚡ Quick winConsider defining a schema for the result array items.
The
resultarray items are typed as baretype: object, which produces untyped/generic objects in generated client SDKs. If these items represent discovered APIs, consider referencing an existing schema (e.g.,APIInfo) or defining the expected properties to improve client type safety and documentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml` around lines 183 - 186, The `result` array items in the API response are defined as bare `type: object` without a schema reference, which causes client SDKs to generate generic untyped objects. Replace the bare object type definition with a proper schema reference using `$ref` pointing to an existing schema such as `APIInfo` (or an appropriate schema that represents the discovered API structure), or alternatively define the expected properties inline to provide type safety and improve client SDK generation.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml (1)
183-187: ⚡ Quick winUse a concrete schema for
resultitems instead of genericobject.
result.items: { type: object }makes the contract too loose for generated clients and increases drift risk between backend and UI. Please reference a named DTO schema for discovered API entries (and ideally reuse it across discover/status responses).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml` around lines 183 - 187, Replace the generic `type: object` in the `result.items` definition with a reference to a concrete named DTO schema for discovered API entries. Create a named schema component (in the components/schemas section) that properly defines the structure of a discovered API entry with all required properties, then use a `$ref` pointer to reference this schema in the result.items definition instead of the loose object type. Consider reusing this same schema definition across other discover and status responses to maintain consistency and reduce drift between backend and UI contracts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.federated.gateway/src/main/java/org/wso2/carbon/apimgt/federated/gateway/FederatedAPIDiscoveryRunner.java`:
- Around line 623-645: The cleanup block in the discoverExternalAPIs method that
iterates over alreadyDiscoveredAPIsList and calls
FederatedGatewayUtil.deleteDeployment is causing implicit deletion of
deployments when the method is called on-demand via the /federated-apis/discover
endpoint. Remove this entire cleanup block from discoverExternalAPIs to prevent
unintended side effects during discovery operations. Move this deletion logic to
either an explicit cleanup/sync method that is only called by the scheduler or
keep it in a separate scheduler-only path. This ensures that on-demand discovery
operations only list and provide metadata for available APIs without modifying
existing deployments, allowing users to make explicit import/update decisions
through the Publisher portal.
- Around line 583-618: The allTrackedAPIs map includes both DISCOVERED_API_LIST
and PUBLISHED_API_LIST, which causes publisher-owned APIs to be incorrectly
classified as federated updates when they have matching name:version but
different external mapping references. Add a guard condition before adding APIs
to updatedAPIs to exclude published-only APIs (check if the matched API exists
only in PUBLISHED_API_LIST and not in DISCOVERED_API_LIST using
alreadyAvailableAPIs.get(PUBLISHED_API_LIST) and
alreadyAvailableAPIs.get(DISCOVERED_API_LIST) comparisons). Apply the same
published-only rejection logic in the importNewExternalAPIs method before
building the import ZIP to keep both paths consistent with the existing
scheduler behavior that skips isPublishedAPIFromCP APIs.
- Around line 686-781: The try-catch block in the loop processing apiIds catches
all exceptions, logs them, and continues without communicating failures to the
caller. This causes the REST endpoints to report success even when all imports
fail. Modify the code to collect the results of each API import/update operation
(success or failure) in a data structure, then return this structured result to
the caller instead of silently continuing on error. This change must be applied
in two locations: the loop that processes apiIds in the discovery method (around
lines 686-781) and the similar update method referenced at lines 805-838. Ensure
that partial failures are properly communicated so the REST caller can inform
users about which APIs succeeded and which failed.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/FederatedApisApiServiceImpl.java`:
- Around line 477-486: In both the markCompleted and markFailed methods, reorder
the field assignments to set this.completedAt before setting this.status.
Currently this.status is being set before this.completedAt, which creates a race
condition where a polling/cleanup thread can observe the terminal status
(STATUS_COMPLETED or STATUS_FAILED) while completedAt is still 0, causing
isExpired() to incorrectly evict the task. Swap the order in both methods so
that System.currentTimeMillis() is assigned to this.completedAt first, then
this.status is set to the terminal status value.
- Around line 231-253: The status endpoint is incorrectly creating and executing
new discovery tasks when a taskId is not found in TASK_STORE, which violates the
read-only contract of a GET endpoint. Remove the entire block that handles the
null task case (where taskId is null after the TASK_STORE.get(taskId) lookup,
from the index parsing through the DISCOVERY_EXECUTOR.submit() call) and instead
return a not found HTTP response when the task does not exist in TASK_STORE.
This ensures the GET /status/{taskId} endpoint remains read-only and does not
trigger expensive discovery operations through fabricated task IDs.
- Around line 231-266: The code retrieves a DiscoveryTask from TASK_STORE and
returns its results without validating that the task belongs to the caller's
organization. Before the final return statement that calls task.toResponseMap(),
add a security check to compare the task's organization (accessible via
task.organization) with the validated organization from
RestApiUtil.getValidatedOrganization(messageContext). If the organizations do
not match, return a 404/403 response with an error message instead of exposing
the task details, ensuring that task IDs cannot be used to leak API information
across organizations.
- Around line 180-205: The current implementation has a race condition where
multiple threads can pass the initial check for existingTaskId and all proceed
to create and register new tasks, bypassing the de-duplication guarantee.
Refactor the logic to use atomic operations like putIfAbsent or compute on
ACTIVE_TASK_BY_ENV to claim the slot before calling resolveEnvironment().
Specifically, create the DiscoveryTask first, then atomically try to register it
in ACTIVE_TASK_BY_ENV using putIfAbsent. If putIfAbsent returns an existing
taskId pointing to a pending task, remove the newly created task from TASK_STORE
and return the existing task's status instead. This ensures only one task per
environment-organization pair can claim the active slot and proceed with work.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml`:
- Around line 18402-18405: The isFederatedAPIDiscoveryEnabled field in the
OpenAPI schema has default: true, which causes OpenAPI-generated clients to
assume Federated API Discovery is enabled when the field is missing, potentially
exposing unsupported UI flows. Remove the default: true line or change it to
default: false to implement fail-closed behavior that prevents accidental
enablement of unsupported features when the field is absent from API responses.
---
Nitpick comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/FederatedAPIDiscoveryService.java`:
- Around line 57-86: The three default methods discoverExternalAPIs,
importNewExternalAPIs, and updateExternalAPIs currently throw
APIManagementException to indicate unsupported operations. Replace
APIManagementException with UnsupportedOperationException in all three methods
to align with established API-module behavior and provide clearer semantics for
default interface methods that are meant to be overridden. This change should be
made in the throw statements within each of these three default methods.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml`:
- Around line 219-221: The 200 response definition in the import operation (at
line 219-221) is missing a response body specification. Add a content schema to
the 200 response that defines the structure of the response payload. The schema
should document the response body structure, such as an object containing arrays
or counts for imported APIs and failed APIs to give clients visibility into
which operations succeeded and which failed during the bulk import.
- Around line 250-252: The 200 response for the update operation in
publisher-api.yaml is missing a response body schema definition. Add a schema to
the 200 response that clearly documents what information is returned to clients,
including details about which APIs were updated successfully and which failed
(such as success status, updated API identifiers, error details for failed
updates, etc.). This should follow the same pattern used in the import endpoint
to provide clients with comprehensive feedback about the operation results.
- Around line 183-186: The `result` array items in the API response are defined
as bare `type: object` without a schema reference, which causes client SDKs to
generate generic untyped objects. Replace the bare object type definition with a
proper schema reference using `$ref` pointing to an existing schema such as
`APIInfo` (or an appropriate schema that represents the discovered API
structure), or alternatively define the expected properties inline to provide
type safety and improve client SDK generation.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml`:
- Around line 183-187: Replace the generic `type: object` in the `result.items`
definition with a reference to a concrete named DTO schema for discovered API
entries. Create a named schema component (in the components/schemas section)
that properly defines the structure of a discovered API entry with all required
properties, then use a `$ref` pointer to reference this schema in the
result.items definition instead of the loose object type. Consider reusing this
same schema definition across other discover and status responses to maintain
consistency and reduce drift between backend and UI contracts.
🪄 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: 5b3dde89-7f95-46ae-aa83-e7740747d6ed
⛔ Files ignored due to path filters (6)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/gen/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/dto/InlineResponse200DTO.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/gen/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/dto/InlineResponse202DTO.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/gen/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/dto/SettingsDTO.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/FederatedApisApi.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/FederatedApisApiService.javais excluded by!**/gen/**features/apimgt/org.wso2.carbon.apimgt.rest.api.service.catalog.feature/src/main/resources/runtimes/cxf3/snakeyaml-2.5.jaris excluded by!**/*.jar
📒 Files selected for processing (15)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/FederatedAPIDiscovery.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/FederatedAPIDiscoveryService.javacomponents/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/API.javacomponents/apimgt/org.wso2.carbon.apimgt.federated.gateway/src/main/java/org/wso2/carbon/apimgt/federated/gateway/FederatedAPIDiscoveryRunner.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yamlcomponents/apimgt/org.wso2.carbon.apimgt.rest.api.gateway/.swagger-codegen-ignorecomponents/apimgt/org.wso2.carbon.apimgt.rest.api.gateway/swagger.jsoncomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/SettingsMappingUtil.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/FederatedApisApiServiceImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yamlcomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/webapp/WEB-INF/beans.xmlcomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/webapp/WEB-INF/web.xmlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/org.wso2.carbon.apimgt.core.default.jsonfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/templates/repository/conf/api-manager.xml.j2
Comment on lines
+583
to
+618
| Map<String, ApiResult> allTrackedAPIs = new ConcurrentHashMap<>(); | ||
| allTrackedAPIs.putAll(alreadyAvailableAPIs.get(DISCOVERED_API_LIST)); | ||
| allTrackedAPIs.putAll(alreadyAvailableAPIs.get(PUBLISHED_API_LIST)); | ||
|
|
||
| List<String> discoveredAPIsFromFederatedGW = new ArrayList<>(); | ||
|
|
||
| for (DiscoveredAPI discoveredAPI : allAPIs) { | ||
| if (discoveredAPI == null || discoveredAPI.getApi() == null) continue; | ||
|
|
||
| String apiKey = discoveredAPI.getApi().getId().getApiName() + DELEM_COLON + discoveredAPI.getApi().getId().getVersion(); | ||
| String envScopedKey = discoveredAPI.getApi().getId().getApiName() + APIConstants.KEY_SEPARATOR | ||
| + environment.getName() + DELEM_COLON + discoveredAPI.getApi().getId().getVersion(); | ||
|
|
||
| // Track gateway APIs to identify deleted ones | ||
| discoveredAPIsFromFederatedGW.add(apiKey); | ||
| discoveredAPIsFromFederatedGW.add(envScopedKey); | ||
|
|
||
| boolean isExists = alreadyDiscoveredAPIsList.contains(apiKey) | ||
| || alreadyDiscoveredAPIsList.contains(envScopedKey) | ||
| || publishedAPIsList.contains(apiKey) | ||
| || publishedAPIsList.contains(envScopedKey); | ||
| if (!isExists) { | ||
| stripHeavyDefinition(discoveredAPI); | ||
| newAPIs.add(discoveredAPI); | ||
| continue; | ||
| } | ||
| String matchedKey = null; | ||
| if (allTrackedAPIs.containsKey(envScopedKey)) matchedKey = envScopedKey; | ||
| else if (allTrackedAPIs.containsKey(apiKey)) matchedKey = apiKey; | ||
| if (matchedKey != null) { | ||
| ApiResult wso2ApiResult = allTrackedAPIs.get(matchedKey); | ||
| String existingReferenceArtifact = getReferenceObjectForExistingAPIs(environment, wso2ApiResult); | ||
|
|
||
| if (discovery.isAPIUpdated(existingReferenceArtifact, discoveredAPI.getReferenceArtifact())) { | ||
| stripHeavyDefinition(discoveredAPI); | ||
| updatedAPIs.add(discoveredAPI); |
There was a problem hiding this comment.
Do not classify publisher-owned APIs as federated update/import candidates.
allTrackedAPIs includes PUBLISHED_API_LIST, so a CP-published API with the same name:version can be added to UPDATE when its external mapping reference is missing/different. The existing scheduler path skips isPublishedAPIFromCP before importing; keep the on-demand path consistent, and add the same published-only guard before direct import decisions.
Suggested direction
- Map<String, ApiResult> allTrackedAPIs = new ConcurrentHashMap<>();
- allTrackedAPIs.putAll(alreadyAvailableAPIs.get(DISCOVERED_API_LIST));
- allTrackedAPIs.putAll(alreadyAvailableAPIs.get(PUBLISHED_API_LIST));
+ Map<String, ApiResult> allTrackedAPIs = new HashMap<>();
+ allTrackedAPIs.putAll(alreadyAvailableAPIs.get(DISCOVERED_API_LIST));
...
- boolean isExists = alreadyDiscoveredAPIsList.contains(apiKey)
- || alreadyDiscoveredAPIsList.contains(envScopedKey)
- || publishedAPIsList.contains(apiKey)
- || publishedAPIsList.contains(envScopedKey);
- if (!isExists) {
+ boolean isTrackedFederatedAPI = alreadyDiscoveredAPIsList.contains(apiKey)
+ || alreadyDiscoveredAPIsList.contains(envScopedKey);
+ boolean isPublishedOnlyAPI = !isTrackedFederatedAPI
+ && (publishedAPIsList.contains(apiKey) || publishedAPIsList.contains(envScopedKey));
+ if (isPublishedOnlyAPI) {
+ continue;
+ }
+ if (!isTrackedFederatedAPI) {
stripHeavyDefinition(discoveredAPI);
newAPIs.add(discoveredAPI);
continue;
}Also use alreadyAvailableAPIs.get(PUBLISHED_API_LIST) in importNewExternalAPIs to reject published-only collisions before building the ZIP.
Also applies to: 680-708
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/apimgt/org.wso2.carbon.apimgt.federated.gateway/src/main/java/org/wso2/carbon/apimgt/federated/gateway/FederatedAPIDiscoveryRunner.java`
around lines 583 - 618, The allTrackedAPIs map includes both DISCOVERED_API_LIST
and PUBLISHED_API_LIST, which causes publisher-owned APIs to be incorrectly
classified as federated updates when they have matching name:version but
different external mapping references. Add a guard condition before adding APIs
to updatedAPIs to exclude published-only APIs (check if the matched API exists
only in PUBLISHED_API_LIST and not in DISCOVERED_API_LIST using
alreadyAvailableAPIs.get(PUBLISHED_API_LIST) and
alreadyAvailableAPIs.get(DISCOVERED_API_LIST) comparisons). Apply the same
published-only rejection logic in the importNewExternalAPIs method before
building the import ZIP to keep both paths consistent with the existing
scheduler behavior that skips isPublishedAPIFromCP APIs.
Comment on lines
+623
to
+645
| // Cleanup orphaned APIs (previously discovered but no longer on the gateway) | ||
| for (String apiName : alreadyDiscoveredAPIsList) { | ||
| if (!discoveredAPIsFromFederatedGW.contains(apiName)) { | ||
| try { | ||
| String apiUUID = FederatedGatewayUtil.getAPIUUID(apiName, adminUsername, organization); | ||
| if (apiUUID != null) { | ||
| FederatedGatewayUtil.deleteDeployment(apiUUID, organization, environment); | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("Automatically cleaned up orphaned API deployment: " + apiName | ||
| + " (UUID: " + apiUUID + ") from environment: " + environment.getName()); | ||
| } | ||
| } else { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("API UUID not found for: " + apiName | ||
| + ". Skipping removal from environment: " + environment.getName()); | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
| log.error("Failed to delete revision for API: " + apiName + " from environment: " | ||
| + environment.getName(), e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Keep discovery from deleting deployments implicitly.
discoverExternalAPIs is called by the async /federated-apis/discover worker, but this block deletes existing gateway deployments while producing the discovery result. A metadata/list operation can therefore remove WSO2 deployments before the user chooses import/update; move cleanup to an explicit sync/cleanup path or keep it in the scheduler-only flow. Based on the PR objective, on-demand discovery is meant to list/import APIs through the Publisher portal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/apimgt/org.wso2.carbon.apimgt.federated.gateway/src/main/java/org/wso2/carbon/apimgt/federated/gateway/FederatedAPIDiscoveryRunner.java`
around lines 623 - 645, The cleanup block in the discoverExternalAPIs method
that iterates over alreadyDiscoveredAPIsList and calls
FederatedGatewayUtil.deleteDeployment is causing implicit deletion of
deployments when the method is called on-demand via the /federated-apis/discover
endpoint. Remove this entire cleanup block from discoverExternalAPIs to prevent
unintended side effects during discovery operations. Move this deletion logic to
either an explicit cleanup/sync method that is only called by the scheduler or
keep it in a separate scheduler-only path. This ensures that on-demand discovery
operations only list and provide metadata for available APIs without modifying
existing deployments, allowing users to make explicit import/update decisions
through the Publisher portal.
Comment on lines
+686
to
+781
| for (String apiId : apiIds) { | ||
| try { | ||
| DiscoveredAPI discoveredAPI = apiLookup.get(apiId); | ||
| if (discoveredAPI == null) { | ||
| log.error("Could not find discovered API matching ID: " + apiId + ". Skipping."); | ||
| continue; | ||
| } | ||
| API api = discoveredAPI.getApi(); | ||
| APIDTO apidto = fromAPItoDTO(api); | ||
| if (apidto.getPolicies() == null || apidto.getPolicies().isEmpty()) { | ||
| apidto.setPolicies(Collections.singletonList(DEFAULT_SUB_POLICY_SUBSCRIPTIONLESS)); | ||
| } | ||
|
|
||
| String apiKey = apidto.getName() + DELEM_COLON + apidto.getVersion(); | ||
| String envScopedKey = apidto.getName() + APIConstants.KEY_SEPARATOR | ||
| + environment.getName() + DELEM_COLON + apidto.getVersion(); | ||
|
|
||
| boolean update = false; | ||
| boolean isNewVersion = false; | ||
| String existingAPI = null; | ||
| boolean alreadyExistsWithEnvScope = alreadyDiscoveredAPIsList.contains(envScopedKey); | ||
|
|
||
| if (!alreadyDiscoveredAPIsList.contains(apiKey) && !alreadyExistsWithEnvScope) { | ||
| String envPathName = apidto.getName() + APIConstants.KEY_SEPARATOR | ||
| + environment.getName(); | ||
| Optional<String> existingApiOpt = alreadyDiscoveredAPIsList.stream() | ||
| .map(String::trim) | ||
| .map(s -> { | ||
| int idx = s.lastIndexOf(DELEM_COLON); | ||
| if (idx <= 0 || idx >= s.length() - 1) return null; | ||
| String name = s.substring(0, idx); | ||
| String version = s.substring(idx + 1); | ||
| return new String[]{name, version}; | ||
| }) | ||
| .filter(Objects::nonNull) | ||
| .filter(parts -> (parts[0].equals(apidto.getName()) | ||
| || parts[0].equals(envPathName)) | ||
| && !parts[1].equals(apidto.getVersion())) | ||
| .map(parts -> parts[0] + DELEM_COLON + parts[1]) | ||
| .findFirst(); | ||
| isNewVersion = existingApiOpt.isPresent(); | ||
| existingAPI = existingApiOpt.orElse(null); | ||
| } | ||
|
|
||
| if (alreadyExistsWithEnvScope) { | ||
| if (api.getDisplayName() == null) { | ||
| apidto.displayName(apidto.getName()); | ||
| } | ||
| apidto.setName(apidto.getName() + APIConstants.KEY_SEPARATOR + environment.getName()); | ||
| } | ||
|
|
||
| API newAPI = null; | ||
| if (isNewVersion) { | ||
| String existingApiUUID = FederatedGatewayUtil.getAPIUUID(existingAPI, adminUsername, | ||
| organization); | ||
| if (existingApiUUID != null) { | ||
| newAPI = FederatedGatewayUtil.createNewAPIVersion(existingApiUUID, apidto.getVersion(), | ||
| organization); | ||
| update = true; | ||
| } | ||
| } | ||
|
|
||
| // Build deployment ZIP | ||
| JsonObject apiJson = (JsonObject) new Gson().toJsonTree(apidto); | ||
| apiJson = CommonUtil.addTypeAndVersionToFile(ImportExportConstants.TYPE_API, | ||
| ImportExportConstants.APIM_VERSION, apiJson); | ||
| InputStream apiZip = FederatedGatewayUtil.createZipAsInputStream( | ||
| apiJson.toString(), api.getSwaggerDefinition(), | ||
| FederatedGatewayUtil.createDeploymentYaml(environment), | ||
| apidto.getName()); | ||
|
|
||
| // Import API | ||
| ImportedAPIDTO importedApi = importExportAPI.importAPI(apiZip, false, | ||
| true, update, true, | ||
| new String[]{APIConstants.APIM_PUBLISHER_SCOPE, APIConstants.APIM_CREATOR_SCOPE}, | ||
| organization); | ||
|
|
||
| // Record the mapping using the CONNECTOR'S OWN reference artifact | ||
| if (update) { | ||
| if (newAPI != null) { | ||
| APIUtil.addApiExternalApiMapping(newAPI.getUuid(), | ||
| environment.getUuid(), discoveredAPI.getReferenceArtifact()); | ||
| } else { | ||
| APIUtil.updateApiExternalApiMapping(importedApi.getApi().getUuid(), | ||
| environment.getUuid(), discoveredAPI.getReferenceArtifact()); | ||
| } | ||
| } else { | ||
| APIUtil.addApiExternalApiMapping(importedApi.getApi().getUuid(), | ||
| environment.getUuid(), discoveredAPI.getReferenceArtifact()); | ||
| } | ||
| log.info("Successfully imported new API: " + api.getId().getApiName() | ||
| + " from environment: " + environment.getName()); | ||
| } catch (Exception e) { | ||
| log.error("Error importing API with ID: " + apiId | ||
| + " from environment: " + environment.getName(), e); | ||
| } |
There was a problem hiding this comment.
Propagate per-API import/update failures to the REST caller.
Both loops catch Exception, log it, and continue, so the service returns normally even when every requested API is missing or failed. The Publisher REST endpoints then return "APIs imported successfully" / "APIs updated successfully" after these void calls, which gives users a false success and can leave mappings/deployments partially applied.
Suggested direction
+ List<String> failedApiIds = new ArrayList<>();
for (String apiId : apiIds) {
try {
DiscoveredAPI discoveredAPI = apiLookup.get(apiId);
if (discoveredAPI == null) {
log.error("Could not find discovered API matching ID: " + apiId + ". Skipping.");
+ failedApiIds.add(apiId);
continue;
}
...
} catch (Exception e) {
log.error("Error importing API with ID: " + apiId
+ " from environment: " + environment.getName(), e);
+ failedApiIds.add(apiId);
}
}
+ if (!failedApiIds.isEmpty()) {
+ throw new APIManagementException("Failed to import federated APIs: " + failedApiIds);
+ }Apply the same pattern in updateExternalAPIs, or return a structured per-API result if partial success is intended.
Also applies to: 805-838
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/apimgt/org.wso2.carbon.apimgt.federated.gateway/src/main/java/org/wso2/carbon/apimgt/federated/gateway/FederatedAPIDiscoveryRunner.java`
around lines 686 - 781, The try-catch block in the loop processing apiIds
catches all exceptions, logs them, and continues without communicating failures
to the caller. This causes the REST endpoints to report success even when all
imports fail. Modify the code to collect the results of each API import/update
operation (success or failure) in a data structure, then return this structured
result to the caller instead of silently continuing on error. This change must
be applied in two locations: the loop that processes apiIds in the discovery
method (around lines 686-781) and the similar update method referenced at lines
805-838. Ensure that partial failures are properly communicated so the REST
caller can inform users about which APIs succeeded and which failed.
Comment on lines
+180
to
+205
| String existingTaskId = ACTIVE_TASK_BY_ENV.get(envKey); | ||
| if (existingTaskId != null) { | ||
| DiscoveryTask existing = TASK_STORE.get(existingTaskId); | ||
| if (existing != null && STATUS_PENDING.equals(existing.status)) { | ||
| log.debug("Discovery already in progress for env [" + environment | ||
| + "] org [" + organization + "], returning existing taskId: " + existingTaskId); | ||
| return Response.accepted(existing.toStatusMap()).build(); | ||
| } | ||
| } | ||
|
|
||
| // --- Resolve environment (needs credentials) --------------------------- | ||
| Environment env; | ||
| try { | ||
| env = resolveEnvironment(environment, organization); | ||
| } catch (APIManagementException e) { | ||
| RestApiUtil.handleInternalServerError( | ||
| "Failed to resolve environment: " + environment, e, log); | ||
| return null; | ||
| } | ||
|
|
||
| // --- Create and register new task ------------------------------------- | ||
| String taskId = environment + "_" + UUID.randomUUID().toString(); | ||
| DiscoveryTask task = new DiscoveryTask(taskId, environment, organization); | ||
|
|
||
| TASK_STORE.put(taskId, task); | ||
| ACTIVE_TASK_BY_ENV.put(envKey, taskId); |
There was a problem hiding this comment.
Make active-task registration atomic.
The current get → resolve → put sequence lets concurrent discover requests for the same organization|environment both create and submit workers, so the de-duplication guarantee can be bypassed. Use putIfAbsent/compute to claim ACTIVE_TASK_BY_ENV before submitting work, and remove any unused task from TASK_STORE when another pending task already owns the key.
Possible localized fix shape
- String existingTaskId = ACTIVE_TASK_BY_ENV.get(envKey);
- if (existingTaskId != null) {
- DiscoveryTask existing = TASK_STORE.get(existingTaskId);
- if (existing != null && STATUS_PENDING.equals(existing.status)) {
- log.debug("Discovery already in progress for env [" + environment
- + "] org [" + organization + "], returning existing taskId: " + existingTaskId);
- return Response.accepted(existing.toStatusMap()).build();
- }
- }
+ String existingTaskId = ACTIVE_TASK_BY_ENV.get(envKey);
+ if (existingTaskId != null) {
+ DiscoveryTask existing = TASK_STORE.get(existingTaskId);
+ if (existing != null && STATUS_PENDING.equals(existing.status)) {
+ log.debug("Discovery already in progress for env [" + environment
+ + "] org [" + organization + "], returning existing taskId: " + existingTaskId);
+ return Response.accepted(existing.toStatusMap()).build();
+ }
+ ACTIVE_TASK_BY_ENV.remove(envKey, existingTaskId);
+ }
// --- Resolve environment (needs credentials) ---------------------------
Environment env;
@@
String taskId = environment + "_" + UUID.randomUUID().toString();
DiscoveryTask task = new DiscoveryTask(taskId, environment, organization);
TASK_STORE.put(taskId, task);
- ACTIVE_TASK_BY_ENV.put(envKey, taskId);
+ String competingTaskId = ACTIVE_TASK_BY_ENV.putIfAbsent(envKey, taskId);
+ if (competingTaskId != null) {
+ TASK_STORE.remove(taskId);
+ DiscoveryTask competingTask = TASK_STORE.get(competingTaskId);
+ if (competingTask != null && STATUS_PENDING.equals(competingTask.status)) {
+ return Response.accepted(competingTask.toStatusMap()).build();
+ }
+ ACTIVE_TASK_BY_ENV.remove(envKey, competingTaskId);
+ return discoverFederatedAPIs(environment, messageContext);
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/FederatedApisApiServiceImpl.java`
around lines 180 - 205, The current implementation has a race condition where
multiple threads can pass the initial check for existingTaskId and all proceed
to create and register new tasks, bypassing the de-duplication guarantee.
Refactor the logic to use atomic operations like putIfAbsent or compute on
ACTIVE_TASK_BY_ENV to claim the slot before calling resolveEnvironment().
Specifically, create the DiscoveryTask first, then atomically try to register it
in ACTIVE_TASK_BY_ENV using putIfAbsent. If putIfAbsent returns an existing
taskId pointing to a pending task, remove the newly created task from TASK_STORE
and return the existing task's status instead. This ensures only one task per
environment-organization pair can claim the active slot and proceed with work.
Comment on lines
+231
to
+253
| DiscoveryTask task = TASK_STORE.get(taskId); | ||
| if (task == null) { | ||
| int index = taskId.lastIndexOf('_'); | ||
| if (index > 0) { | ||
| String environment = taskId.substring(0, index); | ||
| String organization = RestApiUtil.getValidatedOrganization(messageContext); | ||
| try { | ||
| Environment env = resolveEnvironment(environment, organization); | ||
| String envKey = organization + "|" + environment; | ||
|
|
||
| DiscoveryTask newTask = new DiscoveryTask(taskId, environment, organization); | ||
| TASK_STORE.put(taskId, newTask); | ||
| ACTIVE_TASK_BY_ENV.put(envKey, taskId); | ||
|
|
||
| int tenantId = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantId(); | ||
| String tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | ||
|
|
||
| DISCOVERY_EXECUTOR.submit(() -> | ||
| runDiscovery(newTask, env, organization, tenantId, tenantDomain, envKey)); | ||
|
|
||
| log.info("Federated API discovery task [" + taskId + "] lazily created on this node for env [" | ||
| + environment + "] org [" + organization + "]"); | ||
| return Response.ok(newTask.toResponseMap()).build(); |
There was a problem hiding this comment.
Do not start discovery from the status endpoint.
GET /status/{taskId} should be read-only, but an unknown task ID like <validEnv>_<anything> currently creates a new task and triggers external gateway discovery. This allows polling/fabricated IDs to bypass the POST de-dup flow and fan out expensive work; return not found for missing tasks instead.
Suggested simplification
DiscoveryTask task = TASK_STORE.get(taskId);
if (task == null) {
- int index = taskId.lastIndexOf('_');
- if (index > 0) {
- String environment = taskId.substring(0, index);
- String organization = RestApiUtil.getValidatedOrganization(messageContext);
- try {
- Environment env = resolveEnvironment(environment, organization);
- String envKey = organization + "|" + environment;
-
- DiscoveryTask newTask = new DiscoveryTask(taskId, environment, organization);
- TASK_STORE.put(taskId, newTask);
- ACTIVE_TASK_BY_ENV.put(envKey, taskId);
-
- int tenantId = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantId();
- String tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain();
-
- DISCOVERY_EXECUTOR.submit(() ->
- runDiscovery(newTask, env, organization, tenantId, tenantDomain, envKey));
-
- log.info("Federated API discovery task [" + taskId + "] lazily created on this node for env ["
- + environment + "] org [" + organization + "]");
- return Response.ok(newTask.toResponseMap()).build();
- } catch (Exception e) {
- log.error("Failed to lazily create discovery task for taskId: " + taskId, e);
- return Response.status(Response.Status.NOT_FOUND)
- .entity("{\"error\": \"Task not found or has expired: " + taskId + "\"}")
- .build();
- }
- } else {
- return Response.status(Response.Status.NOT_FOUND)
- .entity("{\"error\": \"Task not found or has expired: " + taskId + "\"}")
- .build();
- }
+ return Response.status(Response.Status.NOT_FOUND)
+ .entity(Map.of("error", "Task not found or has expired: " + taskId))
+ .build();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| DiscoveryTask task = TASK_STORE.get(taskId); | |
| if (task == null) { | |
| int index = taskId.lastIndexOf('_'); | |
| if (index > 0) { | |
| String environment = taskId.substring(0, index); | |
| String organization = RestApiUtil.getValidatedOrganization(messageContext); | |
| try { | |
| Environment env = resolveEnvironment(environment, organization); | |
| String envKey = organization + "|" + environment; | |
| DiscoveryTask newTask = new DiscoveryTask(taskId, environment, organization); | |
| TASK_STORE.put(taskId, newTask); | |
| ACTIVE_TASK_BY_ENV.put(envKey, taskId); | |
| int tenantId = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantId(); | |
| String tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | |
| DISCOVERY_EXECUTOR.submit(() -> | |
| runDiscovery(newTask, env, organization, tenantId, tenantDomain, envKey)); | |
| log.info("Federated API discovery task [" + taskId + "] lazily created on this node for env [" | |
| + environment + "] org [" + organization + "]"); | |
| return Response.ok(newTask.toResponseMap()).build(); | |
| DiscoveryTask task = TASK_STORE.get(taskId); | |
| if (task == null) { | |
| return Response.status(Response.Status.NOT_FOUND) | |
| .entity(Map.of("error", "Task not found or has expired: " + taskId)) | |
| .build(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/FederatedApisApiServiceImpl.java`
around lines 231 - 253, The status endpoint is incorrectly creating and
executing new discovery tasks when a taskId is not found in TASK_STORE, which
violates the read-only contract of a GET endpoint. Remove the entire block that
handles the null task case (where taskId is null after the
TASK_STORE.get(taskId) lookup, from the index parsing through the
DISCOVERY_EXECUTOR.submit() call) and instead return a not found HTTP response
when the task does not exist in TASK_STORE. This ensures the GET
/status/{taskId} endpoint remains read-only and does not trigger expensive
discovery operations through fabricated task IDs.
Comment on lines
+231
to
+266
| DiscoveryTask task = TASK_STORE.get(taskId); | ||
| if (task == null) { | ||
| int index = taskId.lastIndexOf('_'); | ||
| if (index > 0) { | ||
| String environment = taskId.substring(0, index); | ||
| String organization = RestApiUtil.getValidatedOrganization(messageContext); | ||
| try { | ||
| Environment env = resolveEnvironment(environment, organization); | ||
| String envKey = organization + "|" + environment; | ||
|
|
||
| DiscoveryTask newTask = new DiscoveryTask(taskId, environment, organization); | ||
| TASK_STORE.put(taskId, newTask); | ||
| ACTIVE_TASK_BY_ENV.put(envKey, taskId); | ||
|
|
||
| int tenantId = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantId(); | ||
| String tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | ||
|
|
||
| DISCOVERY_EXECUTOR.submit(() -> | ||
| runDiscovery(newTask, env, organization, tenantId, tenantDomain, envKey)); | ||
|
|
||
| log.info("Federated API discovery task [" + taskId + "] lazily created on this node for env [" | ||
| + environment + "] org [" + organization + "]"); | ||
| return Response.ok(newTask.toResponseMap()).build(); | ||
| } catch (Exception e) { | ||
| log.error("Failed to lazily create discovery task for taskId: " + taskId, e); | ||
| return Response.status(Response.Status.NOT_FOUND) | ||
| .entity("{\"error\": \"Task not found or has expired: " + taskId + "\"}") | ||
| .build(); | ||
| } | ||
| } else { | ||
| return Response.status(Response.Status.NOT_FOUND) | ||
| .entity("{\"error\": \"Task not found or has expired: " + taskId + "\"}") | ||
| .build(); | ||
| } | ||
| } | ||
| return Response.ok(task.toResponseMap()).build(); |
There was a problem hiding this comment.
Validate the polling caller’s organization before returning task results.
TASK_STORE is JVM-wide, but a found task is returned without checking that task.organization matches RestApiUtil.getValidatedOrganization(messageContext). A leaked or guessed task ID could expose discovered API names, versions, descriptions, and contexts across organizations; return 404/403 when the task belongs to a different organization.
Suggested check
- DiscoveryTask task = TASK_STORE.get(taskId);
+ String organization = RestApiUtil.getValidatedOrganization(messageContext);
+ DiscoveryTask task = TASK_STORE.get(taskId);
+ if (task != null && !organization.equals(task.organization)) {
+ return Response.status(Response.Status.NOT_FOUND)
+ .entity(Map.of("error", "Task not found or has expired: " + taskId))
+ .build();
+ }
if (task == null) {
int index = taskId.lastIndexOf('_');
if (index > 0) {
String environment = taskId.substring(0, index);
- String organization = RestApiUtil.getValidatedOrganization(messageContext);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| DiscoveryTask task = TASK_STORE.get(taskId); | |
| if (task == null) { | |
| int index = taskId.lastIndexOf('_'); | |
| if (index > 0) { | |
| String environment = taskId.substring(0, index); | |
| String organization = RestApiUtil.getValidatedOrganization(messageContext); | |
| try { | |
| Environment env = resolveEnvironment(environment, organization); | |
| String envKey = organization + "|" + environment; | |
| DiscoveryTask newTask = new DiscoveryTask(taskId, environment, organization); | |
| TASK_STORE.put(taskId, newTask); | |
| ACTIVE_TASK_BY_ENV.put(envKey, taskId); | |
| int tenantId = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantId(); | |
| String tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | |
| DISCOVERY_EXECUTOR.submit(() -> | |
| runDiscovery(newTask, env, organization, tenantId, tenantDomain, envKey)); | |
| log.info("Federated API discovery task [" + taskId + "] lazily created on this node for env [" | |
| + environment + "] org [" + organization + "]"); | |
| return Response.ok(newTask.toResponseMap()).build(); | |
| } catch (Exception e) { | |
| log.error("Failed to lazily create discovery task for taskId: " + taskId, e); | |
| return Response.status(Response.Status.NOT_FOUND) | |
| .entity("{\"error\": \"Task not found or has expired: " + taskId + "\"}") | |
| .build(); | |
| } | |
| } else { | |
| return Response.status(Response.Status.NOT_FOUND) | |
| .entity("{\"error\": \"Task not found or has expired: " + taskId + "\"}") | |
| .build(); | |
| } | |
| } | |
| return Response.ok(task.toResponseMap()).build(); | |
| String organization = RestApiUtil.getValidatedOrganization(messageContext); | |
| DiscoveryTask task = TASK_STORE.get(taskId); | |
| if (task != null && !organization.equals(task.organization)) { | |
| return Response.status(Response.Status.NOT_FOUND) | |
| .entity(Map.of("error", "Task not found or has expired: " + taskId)) | |
| .build(); | |
| } | |
| if (task == null) { | |
| int index = taskId.lastIndexOf('_'); | |
| if (index > 0) { | |
| String environment = taskId.substring(0, index); | |
| try { | |
| Environment env = resolveEnvironment(environment, organization); | |
| String envKey = organization + "|" + environment; | |
| DiscoveryTask newTask = new DiscoveryTask(taskId, environment, organization); | |
| TASK_STORE.put(taskId, newTask); | |
| ACTIVE_TASK_BY_ENV.put(envKey, taskId); | |
| int tenantId = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantId(); | |
| String tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | |
| DISCOVERY_EXECUTOR.submit(() -> | |
| runDiscovery(newTask, env, organization, tenantId, tenantDomain, envKey)); | |
| log.info("Federated API discovery task [" + taskId + "] lazily created on this node for env [" | |
| environment + "] org [" + organization + "]"); | |
| return Response.ok(newTask.toResponseMap()).build(); | |
| } catch (Exception e) { | |
| log.error("Failed to lazily create discovery task for taskId: " + taskId, e); | |
| return Response.status(Response.Status.NOT_FOUND) | |
| .entity("{\"error\": \"Task not found or has expired: " + taskId + "\"}") | |
| .build(); | |
| } | |
| } else { | |
| return Response.status(Response.Status.NOT_FOUND) | |
| .entity("{\"error\": \"Task not found or has expired: " + taskId + "\"}") | |
| .build(); | |
| } | |
| } | |
| return Response.ok(task.toResponseMap()).build(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/FederatedApisApiServiceImpl.java`
around lines 231 - 266, The code retrieves a DiscoveryTask from TASK_STORE and
returns its results without validating that the task belongs to the caller's
organization. Before the final return statement that calls task.toResponseMap(),
add a security check to compare the task's organization (accessible via
task.organization) with the validated organization from
RestApiUtil.getValidatedOrganization(messageContext). If the organizations do
not match, return a 404/403 response with an error message instead of exposing
the task details, ensuring that task IDs cannot be used to leak API information
across organizations.
Comment on lines
+477
to
+486
| void markCompleted(List<Map<String, Object>> apiList) { | ||
| this.result = apiList; | ||
| this.status = STATUS_COMPLETED; | ||
| this.completedAt = System.currentTimeMillis(); | ||
| } | ||
|
|
||
| void markFailed(String error) { | ||
| this.errorMessage = error; | ||
| this.status = STATUS_FAILED; | ||
| this.completedAt = System.currentTimeMillis(); |
There was a problem hiding this comment.
Publish completedAt before terminal status.
isExpired() treats non-pending tasks with completedAt == 0 as expired, and status is set before completedAt. A polling/cleanup thread can observe COMPLETED or FAILED in that tiny window and evict the task immediately; set completedAt before publishing the terminal status.
Suggested ordering fix
void markCompleted(List<Map<String, Object>> apiList) {
this.result = apiList;
- this.status = STATUS_COMPLETED;
this.completedAt = System.currentTimeMillis();
+ this.status = STATUS_COMPLETED;
}
void markFailed(String error) {
this.errorMessage = error;
- this.status = STATUS_FAILED;
this.completedAt = System.currentTimeMillis();
+ this.status = STATUS_FAILED;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void markCompleted(List<Map<String, Object>> apiList) { | |
| this.result = apiList; | |
| this.status = STATUS_COMPLETED; | |
| this.completedAt = System.currentTimeMillis(); | |
| } | |
| void markFailed(String error) { | |
| this.errorMessage = error; | |
| this.status = STATUS_FAILED; | |
| this.completedAt = System.currentTimeMillis(); | |
| void markCompleted(List<Map<String, Object>> apiList) { | |
| this.result = apiList; | |
| this.completedAt = System.currentTimeMillis(); | |
| this.status = STATUS_COMPLETED; | |
| } | |
| void markFailed(String error) { | |
| this.errorMessage = error; | |
| this.completedAt = System.currentTimeMillis(); | |
| this.status = STATUS_FAILED; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/FederatedApisApiServiceImpl.java`
around lines 477 - 486, In both the markCompleted and markFailed methods,
reorder the field assignments to set this.completedAt before setting
this.status. Currently this.status is being set before this.completedAt, which
creates a race condition where a polling/cleanup thread can observe the terminal
status (STATUS_COMPLETED or STATUS_FAILED) while completedAt is still 0, causing
isExpired() to incorrectly evict the task. Swap the order in both methods so
that System.currentTimeMillis() is assigned to this.completedAt first, then
this.status is set to the terminal status value.
Comment on lines
+18402
to
+18405
| isFederatedAPIDiscoveryEnabled: | ||
| type: boolean | ||
| description: This indicates whether the Federated API Discovery is enabled or not. | ||
| default: true |
There was a problem hiding this comment.
default: true for isFederatedAPIDiscoveryEnabled is risky for client behavior.
If this field is missing in any compatibility path, OpenAPI-generated clients may assume discovery is enabled and expose unsupported UI flows. Prefer removing the default or setting it to false (fail-closed).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml`
around lines 18402 - 18405, The isFederatedAPIDiscoveryEnabled field in the
OpenAPI schema has default: true, which causes OpenAPI-generated clients to
assume Federated API Discovery is enabled when the field is missing, potentially
exposing unsupported UI flows. Remove the default: true line or change it to
default: false to implement fail-closed behavior that prevents accidental
enablement of unsupported features when the field is absent from API responses.
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
1 participant
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 introduces the On-Demand Federated API Discovery feature, allowing users to discover and import APIs from federated gateways (such as Azure and Kong) on-demand. This moves away from relying solely on scheduler-based periodic polling, giving users real-time control over importing external gateway APIs from the Publisher portal.
Key Changes
/federated-apis/import, etc.) to trigger discovery and imports programmatically and on-demand.validateAzureAPIName) to support underscores, hyphens, and spaces, preventing validation failures on WSO2-scoped names during import.Testing & Verification