feat(server): Enable upcoming integrations endpoint #40256
Conversation
WalkthroughThe changes introduce a new REST endpoint in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PluginControllerCE
participant PluginServiceCEImpl
participant ConfigService
participant CloudServicesConfig
participant ExternalAPI
Client->>PluginControllerCE: GET /upcoming-integrations
PluginControllerCE->>PluginServiceCEImpl: getUpcomingIntegrations()
PluginServiceCEImpl->>ConfigService: getInstanceId()
ConfigService-->>PluginServiceCEImpl: instanceId
PluginServiceCEImpl->>CloudServicesConfig: getBaseUrl()
CloudServicesConfig-->>PluginServiceCEImpl: baseUrl
PluginServiceCEImpl->>ExternalAPI: GET baseUrl + "?instanceId=..."
alt Success
ExternalAPI-->>PluginServiceCEImpl: integration data
PluginServiceCEImpl-->>PluginControllerCE: List<Map<String,String>>
PluginControllerCE-->>Client: 200 OK with data
else Error
PluginServiceCEImpl-->>PluginControllerCE: empty list
PluginControllerCE-->>Client: 200 OK with empty list and error message
end
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
…upcoming integrations fails
vivek-appsmith
force-pushed
the
feature/enable-upcoming-integrations-endpoint
branch
from
524547d to
6a7a11d
Compare
Failed server tests
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PluginControllerCE.java (2)
105-125: Consider enhancing error handling and type safety.The implementation correctly fetches and handles upcoming integrations, but has room for improvement:
- Using a raw Map type reduces type safety. Consider creating a dedicated DTO class.
- API path should be extracted to a constant or configuration property.
- Consider adding a timeout for the external API call.
- @GetMapping("/upcoming-integrations") - public Mono<ResponseDTO<Map>> getUpcomingIntegrations() { + private static final String UPCOMING_INTEGRATIONS_PATH = "/api/v1/external-saas/upcoming-integrations"; + + @GetMapping("/upcoming-integrations") + public Mono<ResponseDTO<Map<String, Object>>> getUpcomingIntegrations() { log.debug("Fetching upcoming integrations from external API"); - String apiUrl = cloudServicesConfig.getBaseUrl() + "/api/v1/external-saas/upcoming-integrations"; + String apiUrl = cloudServicesConfig.getBaseUrl() + UPCOMING_INTEGRATIONS_PATH; return WebClientUtils.create() .get() .uri(apiUrl) + .timeout(Duration.ofSeconds(5)) - .retrieve() - .bodyToMono(Map.class) - .map(response -> new ResponseDTO<>(HttpStatus.OK, response)) + .retrieve() + .bodyToMono(new ParameterizedTypeReference<Map<String, Object>>() {}) + .map(response -> new ResponseDTO<>(HttpStatus.OK, response)) .onErrorResume(error -> { log.warn("Error retrieving upcoming integrations from external service: {}", error.getMessage()); return Mono.just(new ResponseDTO<>( HttpStatus.OK.value(), Map.of("integrations", List.of()), "Unable to fetch upcoming integrations at this time")); }); }Don't forget to add the missing imports:
import java.time.Duration; import org.springframework.core.ParameterizedTypeReference;
119-123: Consider using appropriate HTTP status codes for error responses.Currently, the error response returns HTTP 200 with an empty list. While this simplifies client handling, following RESTful conventions would suggest using an appropriate error status code.
One option is to use a more specific status code like 503 SERVICE_UNAVAILABLE when external services fail:
.onErrorResume(error -> { log.warn("Error retrieving upcoming integrations from external service: {}", error.getMessage()); return Mono.just(new ResponseDTO<>( - HttpStatus.OK.value(), + HttpStatus.SERVICE_UNAVAILABLE.value(), Map.of("integrations", List.of()), "Unable to fetch upcoming integrations at this time")); });However, if the current approach is part of an established pattern in your application for handling errors, maintaining consistency is also important. Consider discussing with your team to establish consistent error handling policies.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/PluginController.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PluginControllerCE.java(4 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/PluginController.java (1)
3-3: Correctly updated constructor to support new endpoint.The constructor properly injects the CloudServicesConfigCE dependency and passes it to the parent class, enabling the upcoming integrations endpoint functionality.
Also applies to: 15-19
Failed server tests
|
1 similar comment
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/base/PluginServiceCEImpl.java (2)
674-706: Implementation could use some improvements for robustness.The method implementation works correctly but has a few areas that could be improved:
- Consider adding more comprehensive error logging
- The type casting from
ObjecttoMap<String, String>could be more robust- Missing Javadoc documentation
Consider enhancing error handling and adding documentation:
@Override public Mono<List<Map<String, String>>> getUpcomingIntegrations() { log.debug("Fetching upcoming integrations from external API"); return configService.getInstanceId().flatMap(instanceId -> { String apiUrl = cloudServicesConfig.getBaseUrl() + "/api/v1/config/external-saas/upcoming-integrations?instanceId=" + instanceId; return WebClientUtils.create() .get() .uri(apiUrl) .retrieve() .bodyToMono(Map.class) .map(response -> { // Extract the integrations list from the response if (response.containsKey("data")) { List<Map<String, String>> integrations = new ArrayList<>(); List<?> data = (List<?>) response.get("data"); for (Object item : data) { - if (item instanceof Map) { + if (item instanceof Map<?, ?> map) { + try { integrations.add((Map<String, String>) item); + } catch (ClassCastException e) { + log.warn("Invalid integration data format: {}", map); + } } } return integrations; } return List.<Map<String, String>>of(); }) .onErrorResume(error -> { - log.warn( - "Error retrieving upcoming integrations from external service: {}", error.getMessage()); + log.warn( + "Error retrieving upcoming integrations from external service", error); return Mono.just(List.<Map<String, String>>of()); }); }); }Also, consider adding Javadoc:
/** * Fetches upcoming integrations from the external cloud service. * * @return A Mono containing a list of integration details as Maps, or an empty list if the request fails */ @Override public Mono<List<Map<String, String>>> getUpcomingIntegrations() { // Existing implementation... }
679-680: Use URI builder for constructing URLs.Concatenating strings to create URLs is error-prone. Consider using UriComponentsBuilder for more robust URL construction.
- String apiUrl = cloudServicesConfig.getBaseUrl() - + "/api/v1/config/external-saas/upcoming-integrations?instanceId=" + instanceId; + String apiUrl = UriComponentsBuilder.fromHttpUrl(cloudServicesConfig.getBaseUrl()) + .path("/api/v1/config/external-saas/upcoming-integrations") + .queryParam("instanceId", instanceId) + .build() + .toString();Remember to add the import:
import org.springframework.web.util.UriComponentsBuilder;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PluginControllerCE.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/base/PluginServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/base/PluginServiceCEImpl.java(7 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/base/PluginServiceImpl.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/PluginServiceCEImplTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PluginControllerCE.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/base/PluginServiceCEImpl.java (1)
app/client/src/ce/selectors/organizationSelectors.tsx (1)
getInstanceId(54-55)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/base/PluginServiceCE.java (1)
56-57: Interface addition looks good.The new method signature follows the reactive programming pattern using Mono and returns a List of Maps, which is appropriate for a collection of integration details.
app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/base/PluginServiceImpl.java (1)
3-3: Constructor updates are correctly propagated.The changes properly pass the new dependencies to the parent class constructor.
Also applies to: 6-6, 28-30, 40-42
app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/base/PluginServiceCEImpl.java (1)
5-5: Dependencies and imports correctly added.The necessary imports and fields for the new functionality are properly included.
Also applies to: 19-19, 21-21, 52-52, 72-72, 94-94, 106-107, 114-115
...r/appsmith-server/src/test/java/com/appsmith/server/services/ce/PluginServiceCEImplTest.java
Show resolved
Hide resolved
...rver/appsmith-server/src/main/java/com/appsmith/server/plugins/base/PluginServiceCEImpl.java
Outdated
Show resolved
Hide resolved
Failed server tests
|
Failed server tests
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/14492016386. |
|
Deploy-Preview-URL: https://ce-40256.dp.appsmith.com |
This PR adds a new endpoint to the Plugin API that fetches upcoming integrations from Appsmith's cloud services. This feature enables users to discover and prepare for new integration options that will be available in future releases.
Changes
Technical Details
This enhancement will help users discover upcoming integration options and provide feedback on planned datasource connectors.
/ok-to-test tags="@tag.Sanity"
Solves #40048
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14490650726
Commit: a9ae874
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 16 Apr 2025 11:50:36 UTC
Summary by CodeRabbit