chore: moved out sendExecuteAnalyticsEvent from the action's execute flow and optimised verifyDatasourceAndMakeRequest#39828
Conversation
WalkthroughThe changes refactor the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "Requestor"
participant Handler as "ActionExecutionSolutionCEImpl"
participant Auth as "validateAuthentication()"
participant DS as "Datasource Context Retrieval"
participant Org as "Organization ID Retrieval"
Client->>Handler: Call verifyDatasourceAndMakeRequest()
Handler->>Auth: Execute validateAuthentication()
Auth-->>Handler: Return validated datasource (cached as validatedDatasourceMono)
Handler->>DS: Retrieve datasource context via flatMap
DS-->>Handler: Provide datasource context (datasourceContextMono)
Handler->>Org: Retrieve organization id
Org-->>Handler: Return organization id (organizationIdMono)
Handler->>Handler: Combine results using Mono.zip
Handler-->>Client: Return consolidated response
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 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
|
vsvamsi1
changed the title
github-actions
bot
added
the
skip-changelog
![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 (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (1)
791-794: Safer feature flag handling with OptionalUsing
Optional.ofNullableto handle potential null values from the feature flags is a robust approach that preventsNullPointerExceptions. The.orElse(Collections.emptyMap())provides a safe fallback.Consider extracting this feature flag retrieval to a private method for better readability:
- Map<String, Boolean> features = Optional.ofNullable( - featureFlagService.getCachedOrganizationFeatureFlags(organizationId)) - .map(CachedFeatures::getFeatures) - .orElse(Collections.emptyMap()); + Map<String, Boolean> features = getFeatureFlagsForOrganization(organizationId); // Add this method elsewhere in the class: +private Map<String, Boolean> getFeatureFlagsForOrganization(String organizationId) { + return Optional.ofNullable(featureFlagService.getCachedOrganizationFeatureFlags(organizationId)) + .map(CachedFeatures::getFeatures) + .orElse(Collections.emptyMap()); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (6)
37-37: New import for feature flag cachingThe addition of
CachedFeaturesimport supports the optimized feature flag handling implemented further in the code.
767-772: Good optimization: Caching the validated datasourceThe authentication validation result is now cached with
.cache(), which is an excellent optimization to avoid redundant authentication validation work when the datasource is used in subsequent operations.
773-778: Improved code structure with dedicated datasource context MonoExtracting the datasource context retrieval into its own dedicated Mono improves the code's modularity and readability. This change makes the flow of operations clearer and easier to understand.
780-780: Improved separation of concerns with organizational IDExtracting the organization ID retrieval into a separate Mono improves the code structure by properly isolating this operation.
782-788: Effective reactive programming with Mono.zipUsing
Mono.zipto combine the three separate Monos is a more elegant approach that clearly shows the dependencies between these operations. This improves readability and maintainability.
948-957: Performance improvement: Moving analytics to a separate threadMoving the analytics event sending to a separate thread with
subscribeOn(Schedulers.boundedElastic())is an excellent improvement that ensures the analytics processing doesn't block or slow down the main execution flow. This aligns perfectly with the PR objective of movingsendExecuteAnalyticsEventout of the critical path.
vsvamsi1
requested review from
ApekshaBhosale,
NilanshBansal,
nidhi-nair and
rajatagrawal
…flow and optimised verifyDatasourceAndMakeRequest (appsmithorg#39828) ## Description - Removed sendExecuteAnalyticsEvent from the execute critical path. - Flattened critical path of verifyDatasourceAndMakeRequest by 1 and caching mono. Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > If you modify the content in this section, you are likely to disrupt the CI result for your PR. <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined backend processes to improve the efficiency of data validation and processing. - Enhanced error management and stability by refining how authentication checks, feature flags, and data context are handled. - Maintained consistent public interfaces while boosting overall system reliability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit