Fix MCP server import with overwrite for EXISTING_API subtype#13822
Open
Tharsanan1 wants to merge 2 commits intowso2:masterfrom
Open
Fix MCP server import with overwrite for EXISTING_API subtype#13822Tharsanan1 wants to merge 2 commits intowso2:masterfrom
Tharsanan1 wants to merge 2 commits intowso2:masterfrom
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Tharsanan1
requested review from
AnuGayan,
Arshardh,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
npamudika,
pubudu538,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
804
to
+806
| final API apiToUpdate = PublisherCommonUtils | ||
| .prepareForUpdateApi(targetApi, importedApiDTO, apiProvider, tokenScopes); | ||
| List<Backend> existingBackends = apiProvider.getMCPServerBackends(targetApi.getUuid(), organization); | ||
| List<Backend> importedBackends = getMCPServerBackends(extractedFolderPath); | ||
| if (existingBackends.isEmpty() || importedBackends.isEmpty()) { | ||
| throw new APIManagementException("No backends found to update for API: " + targetApi.getUuid()); | ||
| SubtypeConfigurationDTO subtypeConfigurationDTO = importedApiDTO.getSubtypeConfiguration(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| final API apiToUpdate = PublisherCommonUtils | |
| .prepareForUpdateApi(targetApi, importedApiDTO, apiProvider, tokenScopes); | |
| List<Backend> existingBackends = apiProvider.getMCPServerBackends(targetApi.getUuid(), organization); | |
| List<Backend> importedBackends = getMCPServerBackends(extractedFolderPath); | |
| if (existingBackends.isEmpty() || importedBackends.isEmpty()) { | |
| throw new APIManagementException("No backends found to update for API: " + targetApi.getUuid()); | |
| SubtypeConfigurationDTO subtypeConfigurationDTO = importedApiDTO.getSubtypeConfiguration(); | |
| final API apiToUpdate = PublisherCommonUtils | |
| .prepareForUpdateApi(targetApi, importedApiDTO, apiProvider, tokenScopes); | |
| log.info("Preparing to update API: " + targetApi.getUuid()); |
Comment on lines
+823
to
825
| PublisherCommonUtils.updateMCPServerBackend(targetApi.getUuid(), oldBackend, backend, | ||
| organization, apiProvider); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| PublisherCommonUtils.updateMCPServerBackend(targetApi.getUuid(), oldBackend, backend, | |
| organization, apiProvider); | |
| } | |
| PublisherCommonUtils.updateMCPServerBackend(targetApi.getUuid(), oldBackend, backend, | |
| organization, apiProvider); | |
| log.info("Successfully updated MCP Server backend for API: " + targetApi.getUuid()); |
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.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 |
📝 WalkthroughWalkthroughBackend endpoint configuration updates during MCP server import are now conditional. The code extracts the MCP API subtype and only performs backend updates if the subtype is Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ImportUtils
participant APIProvider
participant PublisherCommonUtils
participant Database
Client->>ImportUtils: importMCPServer(importedApiDTO, ...)
ImportUtils->>ImportUtils: extract subtype from importedApiDTO.subtypeConfiguration
alt subtype == "server_proxy" or "direct_backend"
ImportUtils->>APIProvider: getBackendsForAPI(existingApiId)
APIProvider->>Database: query existing backends
Database-->>APIProvider: return existing backends
APIProvider-->>ImportUtils: existing backends list
ImportUtils->>ImportUtils: validate non-empty existing & imported backends
ImportUtils->>ImportUtils: clone first existing backend and copy imported endpointConfig
ImportUtils->>PublisherCommonUtils: updateMCPServerBackend(updatedBackend)
PublisherCommonUtils->>Database: persist backend config
Database-->>PublisherCommonUtils: success
PublisherCommonUtils-->>ImportUtils: update result
else Other subtype or null/blank
ImportUtils->>ImportUtils: skip backend lookup/update
end
ImportUtils->>APIProvider: updateAPI(api)
APIProvider->>Database: persist API
Database-->>APIProvider: success
APIProvider-->>ImportUtils: result
ImportUtils-->>Client: return import result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes MCP server import-with-overwrite failing for subtype EXISTING_API (which legitimately has no backends) by skipping backend update logic unless the subtype requires backend processing.
Changes:
- Guarded backend fetch/update during overwrite to run only for
SERVER_PROXYandDIRECT_BACKENDsubtypes. - Added unit tests covering overwrite behavior across subtypes, including empty-backend edge cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
components/apimgt/.../mappings/ImportUtils.java |
Skips backend update during overwrite unless subtype is backend-driven (SERVER_PROXY / DIRECT_BACKEND). |
components/apimgt/.../mappings/ImportUtilsMCPServerTest.java |
Adds unit tests verifying overwrite behavior for EXISTING_API, backend-driven subtypes, and edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+806
to
+815
| SubtypeConfigurationDTO subtypeConfigurationDTO = importedApiDTO.getSubtypeConfiguration(); | ||
| String subtype = (subtypeConfigurationDTO != null) | ||
| ? subtypeConfigurationDTO.getSubtype() : null; | ||
| if (!StringUtils.isBlank(subtype) | ||
| && (APIConstants.API_SUBTYPE_SERVER_PROXY.equals(subtype) | ||
| || APIConstants.API_SUBTYPE_DIRECT_BACKEND.equals(subtype))) { | ||
| List<Backend> existingBackends = | ||
| apiProvider.getMCPServerBackends(targetApi.getUuid(), organization); | ||
| List<Backend> importedBackends = getMCPServerBackends(extractedFolderPath); | ||
| if (existingBackends.isEmpty() || importedBackends.isEmpty()) { |
There was a problem hiding this comment.
The overwrite-path subtype check duplicates the subtype list used elsewhere in this class (see backendAPIDefSupportedMCPSubtypes and its usage in retrievedMCPDto). Consider reusing that constant here (e.g., backendAPIDefSupportedMCPSubtypes.contains(subtype)) to avoid future drift if new MCP subtypes are introduced that require backend processing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtils.java (1)
809-811: Use the existing subtype allow-list constant to avoid drift.Line 809-Line 811 duplicates the same subtype allow-list already defined in
backendAPIDefSupportedMCPSubtypes. Reusing the constant keeps create/retrieve/update paths consistent when subtype support changes.♻️ Suggested change
- if (!StringUtils.isBlank(subtype) - && (APIConstants.API_SUBTYPE_SERVER_PROXY.equals(subtype) - || APIConstants.API_SUBTYPE_DIRECT_BACKEND.equals(subtype))) { + if (!StringUtils.isBlank(subtype) + && backendAPIDefSupportedMCPSubtypes.contains(subtype)) {🤖 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.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtils.java` around lines 809 - 811, The conditional that checks subtype against APIConstants.API_SUBTYPE_SERVER_PROXY and APIConstants.API_SUBTYPE_DIRECT_BACKEND duplicates the allow-list; replace that hard-coded check with a membership check against the existing backendAPIDefSupportedMCPSubtypes collection (preserving the StringUtils.isBlank(subtype) guard) so the code uses backendAPIDefSupportedMCPSubtypes.contains(subtype) (or equivalent) instead of the two APIConstants literals; update the conditional in ImportUtils (the block referencing subtype) so create/retrieve/update paths rely on the single source of truth.
🤖 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.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtils.java`:
- Around line 809-811: The conditional that checks subtype against
APIConstants.API_SUBTYPE_SERVER_PROXY and
APIConstants.API_SUBTYPE_DIRECT_BACKEND duplicates the allow-list; replace that
hard-coded check with a membership check against the existing
backendAPIDefSupportedMCPSubtypes collection (preserving the
StringUtils.isBlank(subtype) guard) so the code uses
backendAPIDefSupportedMCPSubtypes.contains(subtype) (or equivalent) instead of
the two APIConstants literals; update the conditional in ImportUtils (the block
referencing subtype) so create/retrieve/update paths rely on the single source
of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 905fde36-c418-46d0-bbb8-e572c154a4c3
📒 Files selected for processing (2)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtils.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/test/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtilsMCPServerTest.java
RakhithaRR
previously approved these changes
Apr 29, 2026
…ation The @SuppressStaticInitializationFor annotation on ImportUtilsMCPServerTest was preventing APIUtil static initialization, which contaminated the subsequent ImportUtilsTest run in the same JVM fork. The pom.xml already has the required --add-opens for java.util.regex, so the suppression is unnecessary. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/test/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtilsMCPServerTest.java (2)
316-355: Strengthen null/blank subtype tests with explicit “no backend update” verification.These tests already assert no backend fetch; add a static verify that
PublisherCommonUtils.updateMCPServerBackend(...)is never called, so the contract is locked at the backend-update boundary too.🔒 Suggested assertion hardening
Mockito.verify(apiProvider, Mockito.never()).getMCPServerBackends( ArgumentMatchers.anyString(), ArgumentMatchers.anyString()); + PowerMockito.verifyStatic(PublisherCommonUtils.class, Mockito.never()); + PublisherCommonUtils.updateMCPServerBackend( + ArgumentMatchers.anyString(), ArgumentMatchers.any(Backend.class), + ArgumentMatchers.any(Backend.class), ArgumentMatchers.anyString(), + ArgumentMatchers.any(APIProvider.class)); Mockito.verify(apiProvider).updateAPI( ArgumentMatchers.any(API.class), ArgumentMatchers.any(API.class));🤖 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.publisher.v1.common/src/test/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtilsMCPServerTest.java` around lines 316 - 355, Update the two test methods testImportMCPServerOverwriteWithNullSubtypeConfiguration and testImportMCPServerOverwriteWithBlankSubtype to also assert that PublisherCommonUtils.updateMCPServerBackend(...) is never invoked; locate the tests where ImportUtils.importMCPServer is called and after the existing Mockito.verify(apiProvider...) assertions add a Mockito.verify(PublisherCommonUtils.class, Mockito.never()).updateMCPServerBackend(ArgumentMatchers.any(), ArgumentMatchers.any(), ArgumentMatchers.any()); ensuring you reference the static method PublisherCommonUtils.updateMCPServerBackend to lock the contract that no backend update is attempted for null or blank subtype.
230-378: AddDIRECT_BACKENDempty-backend negative coverage to matchSERVER_PROXY.Lines 265-378 validate empty-backend failures only for
SERVER_PROXY. Since the production guard handles bothSERVER_PROXYandDIRECT_BACKEND, add at least one mirrored failing case forDIRECT_BACKENDto prevent subtype-specific regressions.🧪 Suggested test addition
+ `@Test` + public void testImportMCPServerOverwriteWithDirectBackendEmptyExistingBackends() throws Exception { + SubtypeConfigurationDTO subtypeConfig = new SubtypeConfigurationDTO(); + subtypeConfig.setSubtype(APIConstants.API_SUBTYPE_DIRECT_BACKEND); + mcpServerDTO.setSubtypeConfiguration(subtypeConfig); + setupMCPServerDTOMock(mcpServerDTO); + + Backend importedBackend = new Backend(); + importedBackend.setEndpointConfig("{\"url\":\"https://new.example.com\"}"); + + Mockito.when(apiProvider.getMCPServerBackends(API_UUID, ORGANIZATION)) + .thenReturn(new ArrayList<>()); + PowerMockito.doReturn(Collections.singletonList(importedBackend)).when(ImportUtils.class, + "getMCPServerBackends", EXTRACTED_PATH); + + try { + ImportUtils.importMCPServer(EXTRACTED_PATH, mcpServerDTO, true, false, true, + false, false, tokenScopes, null, ORGANIZATION); + Assert.fail("Should throw APIManagementException when existing backends are empty " + + "for DIRECT_BACKEND subtype"); + } catch (APIManagementException e) { + Assert.assertTrue(e.getMessage().contains("No backends found to update for API")); + } + }🤖 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.publisher.v1.common/src/test/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtilsMCPServerTest.java` around lines 230 - 378, Add a negative test that mirrors the SERVER_PROXY empty-backend case but for DIRECT_BACKEND: create a test (e.g. testImportMCPServerOverwriteWithDirectBackendEmptyExistingBackends) that sets SubtypeConfigurationDTO.subtype to APIConstants.API_SUBTYPE_DIRECT_BACKEND, calls setupMCPServerDTOMock(mcpServerDTO), stubs apiProvider.getMCPServerBackends(API_UUID, ORGANIZATION) to return an empty list and PowerMockito.doReturn(Collections.singletonList(importedBackend)) or vice versa using ImportUtils.getMCPServerBackends(EXTRACTED_PATH), then call ImportUtils.importMCPServer(...) expecting an APIManagementException whose message contains "No backends found to update for API"; reuse the same verification pattern (Mockito.when, PowerMockito.doReturn, exception assert) as in testImportMCPServerOverwriteWithServerProxyEmptyExistingBackends to ensure DIRECT_BACKEND empty-backend failure is covered.
🤖 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.rest.api.publisher.v1.common/src/test/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtilsMCPServerTest.java`:
- Around line 316-355: Update the two test methods
testImportMCPServerOverwriteWithNullSubtypeConfiguration and
testImportMCPServerOverwriteWithBlankSubtype to also assert that
PublisherCommonUtils.updateMCPServerBackend(...) is never invoked; locate the
tests where ImportUtils.importMCPServer is called and after the existing
Mockito.verify(apiProvider...) assertions add a
Mockito.verify(PublisherCommonUtils.class,
Mockito.never()).updateMCPServerBackend(ArgumentMatchers.any(),
ArgumentMatchers.any(), ArgumentMatchers.any()); ensuring you reference the
static method PublisherCommonUtils.updateMCPServerBackend to lock the contract
that no backend update is attempted for null or blank subtype.
- Around line 230-378: Add a negative test that mirrors the SERVER_PROXY
empty-backend case but for DIRECT_BACKEND: create a test (e.g.
testImportMCPServerOverwriteWithDirectBackendEmptyExistingBackends) that sets
SubtypeConfigurationDTO.subtype to APIConstants.API_SUBTYPE_DIRECT_BACKEND,
calls setupMCPServerDTOMock(mcpServerDTO), stubs
apiProvider.getMCPServerBackends(API_UUID, ORGANIZATION) to return an empty list
and PowerMockito.doReturn(Collections.singletonList(importedBackend)) or vice
versa using ImportUtils.getMCPServerBackends(EXTRACTED_PATH), then call
ImportUtils.importMCPServer(...) expecting an APIManagementException whose
message contains "No backends found to update for API"; reuse the same
verification pattern (Mockito.when, PowerMockito.doReturn, exception assert) as
in testImportMCPServerOverwriteWithServerProxyEmptyExistingBackends to ensure
DIRECT_BACKEND empty-backend failure is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 370afd86-9afc-4dea-8df7-52bb430ef682
📒 Files selected for processing (1)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/test/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtilsMCPServerTest.java
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
EXISTING_APIinCREATEDstate have no backends by design. The import-with-update path (ImportUtils.importMCPServer()) unconditionally required backends and threw"No backends found to update for API: <uuid>"(HTTP 500).SERVER_PROXYandDIRECT_BACKENDsubtypes — consistent with the create path which already has this guard.Test plan
ImportUtilsMCPServerTest.java— 8 tests, all passing)EXISTING_APIsubtype using overwrite — should succeed without errorSERVER_PROXYsubtype using overwrite — backend update should still work as before