Skip to content

chore: Convert non-reactive methods consuming @FeatureFlagged to reactive#39594

Merged
abhvsn merged 2 commits intoreleasefrom
chore/make-feature-flagged-method-reactive
Mar 7, 2025
Merged

chore: Convert non-reactive methods consuming @FeatureFlagged to reactive#39594
abhvsn merged 2 commits intoreleasefrom
chore/make-feature-flagged-method-reactive

Conversation

@abhvsn
Copy link
Contributor

@abhvsn abhvsn commented Mar 6, 2025

Description

Tip

Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).

Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.

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.

/test All

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13698576435
Commit: d8ade4d
Cypress dashboard.
Tags: @tag.All
Spec:


Thu, 06 Mar 2025 13:34:00 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • Refactor
    • Migrated key permission checks and resource operations to a reactive, asynchronous model that boosts system responsiveness and scalability.
  • Tests
    • Updated the testing suite to align with reactive patterns, ensuring enhanced reliability and robustness.
    • Adjusted assertions to handle reactive types appropriately, improving clarity and specificity in test outcomes.
  • Chores
    • Streamlined internal workflows and error handling, leading to improved performance and maintainability.

These internal improvements help achieve smoother and more efficient operations without altering the user-facing experience.

@abhvsn abhvsn requested a review from a team as a code owner March 6, 2025 11:11
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 6, 2025

Walkthrough

This pull request refactors numerous service and test classes to adopt reactive programming for permission handling. Synchronous permission checks have been replaced with asynchronous retrieval using Mono and Flux. Multiple methods—covering action collections, datasource operations, email templates, application management, and permission interfaces—now leverage flatMap and related operators for non-blocking execution. Meanwhile, test cases have been updated with StepVerifier and .block() invocations. No breaking changes to public APIs were introduced aside from updated return types.

Changes

File(s) Change Summary
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/...
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/...
Updated deletion, archiving, and import methods to retrieve permissions reactively using Mono/Flux and flatMap instead of direct synchronous calls.
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/...
app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/...
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/...
app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java
Refactored lambda expressions and permission checks in creation, deletion, and policy update flows to use reactive operations and asynchronous permission resolution.
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/...
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceStructureSolutionCEImpl.java
Modified create/archive and schema preview methods to obtain permissions reactively, using Mono wrappers and flatMap for error handling and asynchronous control flow.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/EmailServiceHelperCE*.java
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/EmailServiceCEImpl.java
Changed email template and subject retrieval methods to return Mono via Mono.just(), replacing direct String returns.
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/...
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/...
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ContextPermissionCE.java
Updated permission interfaces and implementations to return Mono instead of raw AclPermission, standardizing reactive permission handling.
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/...
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/...
Updated import and artifact-related services to process resources reactively, with modified method signatures returning Mono and error propagation through reactive streams.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCE*.java Refactored user workspace and forking services to retrieve permissions asynchronously via reactive patterns for enhanced permission validation.
src/test/java/com/appsmith/server/... Refactored tests to use StepVerifier for reactive assertions and added .block() calls; cleanup methods now use flatMapMany for permission iteration and resource deletion.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Service
    participant PermissionProvider
    participant Repository

    Client->>Service: Call operation (e.g. deleteResource)
    Service->>PermissionProvider: getDeletePermission() 
    PermissionProvider-->>Service: Mono<AclPermission>
    Service->>Service: flatMap(permission -> …)
    Service->>Repository: findById(id, permission)
    Repository-->>Service: Mono<Resource>
    Service-->>Client: Mono<Resource> (or error)
Loading

Possibly related PRs

Suggested labels

Task, Enhancement, ok-to-test, Query & JS Pod, Integrations Product, Integrations Pod General

Suggested reviewers

  • nidhi-nair
  • AnaghHegde

Poem

In the code where once was sync,
Now Mono flows in every link.
FlatMap by flatMap, permissions align,
Reactive streams make the flow divine.
Here’s to a refactor so bright—
CodeRabbit’s work shines through the night!
🚀✨


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 220d5d9 and d8ade4d.

📒 Files selected for processing (2)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/ActionCollectionImportableServiceCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/ActionCollectionImportableServiceCEImpl.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: perform-test / client-build / client-build
  • GitHub Check: perform-test / rts-build / build
  • GitHub Check: perform-test / server-build / server-unit-tests
  • GitHub Check: server-unit-tests / server-unit-tests
  • GitHub Check: server-spotless / spotless-check

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@abhvsn abhvsn added the ok-to-test Required label for CI label Mar 6, 2025
@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Mar 6, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (8)
app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/ImportArtifactPermissionProviderTest.java (3)

66-74: Consider using assertTrue instead of assertEquals(Boolean.TRUE, ...).
For readability and clarity, using assertTrue is more direct when expecting a boolean result.

Here is a proposed diff:

-assertEquals(
-    Boolean.TRUE,
-    importArtifactPermissionProvider.canCreateDatasource(new Workspace()).block());
+assertTrue(
+    importArtifactPermissionProvider.canCreateDatasource(new Workspace()).block());

111-119: Avoid mixing .block() inside list creation.
Test code typically blocks or uses StepVerifier, but calling .block() in the test setup can cause subtle concurrency issues if repeated widely. Consider deferring .block() until final assertions or use StepVerifier.


128-128: Maintain consistent testing style.
You're mixing .subscribe(Assertions::assertFalse) with .block(). For better maintainability, stick to a single testing approach (e.g., all .block() or all .subscribe) across these checks.

Also applies to: 130-130, 132-132

app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (1)

93-107: No major concerns, but consider adding error-handling.
Currently, if getApplicationCreatePermission() yields an error or empty result, the downstream flow won't handle it. You may want to propagate or explicitly handle such scenarios to strengthen reliability in case of permission resolution issues.

app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/importable/NewPageImportableServiceCEImpl.java (1)

466-500: Logic for branching could be consolidated for readability.
The permission check and subsequent branching logic (comparing pagesFromOtherBranches vs. creating a new page) works well but could be refactored into smaller methods. This might improve readability and maintainability while supporting future enhancements.

app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (1)

321-405: Watch out for potential concurrency issues when using shared lists in Flux.
Appending to newNewActionList and existingNewActionList within the reactive stream is fine sequentially, but if the code is ever parallelized, these shared mutable lists could cause concurrency issues. Consider a concurrency-safe collection or collecting items within each reactive pipeline before combining them.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java (1)

169-177: Well-structured use of zipWhen and explicit TRUE comparison

The refactoring from direct flatMap to zipWhen creates a cleaner separation between retrieving the permission group and checking if it's the last admin role. The explicit comparison with TRUE is better than relying on implicit boolean evaluation.

You could further improve this by using the following pattern to eliminate the need for a separate variable:

-                .zipWhen(permissionGroup -> isLastAdminRoleEntity(permissionGroup))
-                .flatMap(tuple2 -> {
-                    PermissionGroup permissionGroup = tuple2.getT1();
-                    Boolean isLastAdminRoleEntity = tuple2.getT2();
-                    if (TRUE.equals(isLastAdminRoleEntity)) {
+                .zipWhen(this::isLastAdminRoleEntity)
+                .flatMap(tuple2 -> {
+                    PermissionGroup permissionGroup = tuple2.getT1();
+                    if (TRUE.equals(tuple2.getT2())) {
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ImportArtifactPermissionProviderCE.java (1)

99-118: Consider converting remaining synchronous methods

The hasEditPermission methods still return boolean while other methods now return Mono<Boolean>. For consistency and to fully embrace the reactive paradigm, consider converting these methods as well.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 943e7f2 and 220d5d9.

📒 Files selected for processing (73)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/ActionCollectionImportableServiceCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/applications/ActionCollectionApplicationImportableServiceCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/ArtifactPermissionCE.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/EmailServiceHelperCE.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/EmailServiceHelperCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ImportArtifactPermissionProviderCE.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/imports/importable/artifactbased/ArtifactBasedImportableServiceCE.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/artifactbased/ArtifactBasedImportServiceCE.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/applications/NewActionApplicationImportableServiceCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/importable/NewPageImportableServiceCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (7 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/EmailServiceCEImpl.java (4 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (4 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (4 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCE.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java (3 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionPermissionCE.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionPermissionCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ApplicationPermissionCE.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ApplicationPermissionCEImpl.java (3 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ContextPermissionCE.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/CreateDBTablePageSolutionCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourcePermissionCE.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourcePermissionCEImpl.java (3 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceStructureSolutionCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PagePermissionCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/WorkspacePermissionCE.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/WorkspacePermissionCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspectTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/TestComponentImpl.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/ce/TestComponentCE.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/ce/TestComponentCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/fork/ApplicationForkingServiceTests.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/EmailServiceHelperCETest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/ImportArtifactPermissionProviderTest.java (5 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (2 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCETest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationSnapshotServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/CurlImporterServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceContextServiceTest.java (2 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceStorageServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java (2 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutServiceTest.java (2 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/MockDataServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/ThemeServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserWorkspaceServiceUnitTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java (2 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ThemeImportableServiceCETest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/AuthenticationServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/CreateDBTablePageSolutionTests.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/DatasourceStructureSolutionTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/DatasourceTriggerSolutionTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCETest.java (1 hunks)
  • app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ArtifactBuilderExtension.java (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: perform-test / server-build / server-unit-tests
  • GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (132)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutServiceTest.java (2)

156-158: Reactive refactoring of permission handling looks good

The changes adopt reactive programming by properly chaining the async permission retrieval with flatMapMany. This aligns with the PR's objective of converting non-reactive methods to reactive implementations.


528-537: String formatting change approved

The multi-line string formatting has been adjusted without changing the functional content, resulting in consistent formatting.

app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java (3)

436-439: Good implementation of reactive permission retrieval

The change properly follows reactive programming principles by first obtaining the permission as a Mono and then using flatMap to apply it to the findById operation.


640-647: Correctly implements reactive permission validation

The code now properly retrieves the application create permission as a Mono and applies it in the validation process using flatMap, maintaining the reactive flow.


648-655: Consistent reactive pattern for datasource permissions

This change follows the same reactive pattern implemented elsewhere, ensuring consistency in the codebase. The permission is retrieved as a Mono and then applied to the validation through flatMap.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java (2)

418-421: Reactive conversion implementation looks good.

The change from direct permission access to reactive pattern using flatMapMany aligns well with the PR objective of making permission methods reactive.


4175-4178: Good reactive implementation with appropriate blocking for test context.

The conversion of getActionCreatePermission() to return a reactive type with proper handling in the test environment is well done. Using .block() is appropriate in test scenarios.

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ApplicationPermissionCE.java (2)

5-5: Added Mono import for reactive programming.

The import of reactor.core.publisher.Mono is needed to support the reactive return type used in the interface method.


13-13: Method signature updated to return reactive type.

The getPageCreatePermission() method now returns Mono<AclPermission> instead of AclPermission, following the reactive programming paradigm. This change ensures consistent use of reactive types throughout the permission handling system.

app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/artifactbased/ArtifactBasedImportServiceCE.java (1)

22-22: Method signature updated to return reactive type.

The getImportArtifactPermissionProviderForImportingArtifact method now returns Mono<ImportArtifactPermissionProvider> instead of a synchronous ImportArtifactPermissionProvider. This change is consistent with the conversion to reactive programming approach.

app/server/appsmith-server/src/test/java/com/appsmith/server/fork/ApplicationForkingServiceTests.java (1)

613-613: Added .block() to handle reactive return type.

The test is now calling .block() on the result of workspacePermission.getDatasourceCreatePermission() to extract the value from the Mono. This change is necessary to accommodate the method's new reactive return type.

app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/ArtifactPermissionCE.java (2)

4-4: Added Mono import for reactive programming.

The import of reactor.core.publisher.Mono is needed to support the reactive return type used in the interface method.


10-10: Method signature updated to return reactive type.

The getDeletePermission() method now returns Mono<AclPermission> instead of AclPermission, aligning with the reactive programming approach. This change ensures that permission checks won't block threads unnecessarily.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceServiceTest.java (1)

146-151: Good use of reactive pattern for permission handling.

The cleanup method now properly retrieves the delete permission asynchronously and uses flatMapMany to pass it to the findByWorkspaceId method, which is consistent with the reactive approach throughout the codebase.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCE.java (1)

25-25: Correct conversion to reactive return type.

Method signature updated to return Mono<Boolean> instead of Boolean, maintaining consistency with the reactive programming model used across the application.

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionPermissionCEImpl.java (2)

5-5: Added missing Mono import.

The import for reactor.core.publisher.Mono has been correctly added to support the method signature change.


32-34: Properly implemented reactive permission method.

Method signature has been updated to return a Mono wrapped permission value, consistent with the reactive programming pattern being adopted across the codebase.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/MockDataServiceTest.java (1)

134-136: Good reactive pattern implementation.

Cleanup method now properly retrieves permissions as a Mono and uses flatMapMany to chain operations, maintaining consistency with the reactive approach.

app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (2)

4943-4944: Good addition of .block() for synchronous permission retrieval.

This ensures the Mono returned by getPageCreatePermission() is resolved before accessing its value, aligning with reactive programming principles in test methods.


4988-4989: LGTM! Consistent with the reactive approach.

Similar to the earlier change, this properly waits for the permission value to be resolved before filtering policies.

app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java (1)

268-274: Good refactoring to fully reactive pattern

The change from directly passing applicationPermission.getDeletePermission() to using flatMapMany maintains the reactive chain properly, aligning with the PR's objective to convert non-reactive methods to reactive implementations.

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionPermissionCE.java (1)

3-7:

Details

❓ Verification inconclusive

Conversion from sync to reactive return type.

The change of getDeletePermission() to return Mono<AclPermission> instead of AclPermission aligns with the reactive programming model. Ensure all implementations and calling code have been updated to handle this reactive type.


🏁 Script executed:

#!/bin/bash
# Check implementations to ensure they've been updated to return Mono<AclPermission>
rg "getDeletePermission\(\)\s*\{\s*return\s+(?!Mono\.)" -A 5

Length of output: 356


Please verify that all implementations correctly return a reactive type.

The change to have getDeletePermission() return a Mono<AclPermission> is correct for aligning with our reactive programming model. However, the initial regex check encountered an issue because the look-ahead syntax wasn’t supported without enabling PCRE2. To ensure complete verification, please run the following updated shell script which enables PCRE2:

This will help verify that there are no implementations inadvertently retaining synchronous returns. If the script shows no problematic instances, it confirms that all updates to support the reactive return type have been applied.


🏁 Script executed:

#!/bin/bash
# Re-run check to ensure all implementations of getDeletePermission return Mono<AclPermission>
rg --pcre2 "getDeletePermission\(\)\s*\{\s*return\s+(?!Mono\.)" -A 5

Length of output: 192


Please manually verify that all implementations and their consuming code have been updated to handle the reactive return type.

The conversion of getDeletePermission() to return Mono<AclPermission> is appropriate and aligns with our reactive programming model. Although the automated check (with PCRE2 enabled) did not reveal any synchronous returns, there was an execution error due to the `` marker. Please double-check that no implementations inadvertently still return a synchronous AclPermission and that all downstream code is adapted accordingly.

  • Confirm that every override of getDeletePermission() now returns Mono<AclPermission>.
  • Ensure that all callers correctly subscribe to or handle the reactive type.
  • Re-run verification without the `` prefix if necessary.

Please ensure that both implementations of getDeletePermission() and all consuming code have been manually verified to handle the reactive type correctly.

The conversion to return Mono<AclPermission> is appropriate for our reactive model. While automated checks did not flag any synchronous returns, manual verification is recommended for:

  • Confirming that every override of getDeletePermission() returns Mono<AclPermission>.
  • Ensuring that all caller routines subscribe to or otherwise manage the reactive type appropriately.
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/DatasourceTriggerSolutionTest.java (1)

124-126: Updated to handle reactive permission.

The test cleanup method has been properly updated to use flatMapMany with the reactive permission result. This pattern correctly maintains the reactive chain.

app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCETest.java (1)

251-253: Updated to handle reactive permission.

The test cleanup method has been properly updated to use flatMapMany with the reactive permission result, maintaining the reactive chain.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java (1)

574-576: Updated to handle reactive permission.

The implementation correctly uses flatMap to process the reactive permission result before finding the workspace by ID. The change preserves the error handling flow.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/ThemeServiceTest.java (1)

110-115: Improved reactive flow with flatMapMany.

The change converts synchronous permission retrieval to reactive flow by using flatMapMany to process each permission. This aligns with reactive programming principles and avoids blocking on permission retrieval.

app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/DatasourceStructureSolutionTest.java (1)

129-134: Reactive pattern applied consistently.

The cleanup method now uses reactive pattern with flatMapMany to process permissions, which is consistent with the changes across the codebase. Good job maintaining consistency.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java (1)

88-93: Reactive permission handling implemented.

The change from synchronous to reactive permission handling improves the code flow and aligns with the reactive programming paradigm being adopted across the codebase.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceStorageServiceTest.java (1)

81-86: Consistent reactive pattern applied.

This change follows the same pattern as other test classes, using flatMapMany to maintain the reactive flow when working with permissions. The consistency across the codebase is excellent.

app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCETest.java (1)

295-297: Improved reactive flow in cleanup method.

The refactoring from direct method invocation to a reactive chain using .flatMapMany() properly maintains the reactive nature of the permission-based application retrieval. This change is in line with the overall conversion to reactive methods throughout the codebase.

app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/AuthenticationServiceTest.java (1)

104-106: Good reactive pattern implementation.

The cleanup method now correctly retrieves the delete permission as a reactive type and chains operations using flatMapMany, consistent with the reactive programming model being adopted throughout the application.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ThemeImportableServiceCETest.java (1)

91-93: Consistent reactive pattern application.

The cleanup implementation now uses proper reactive flow with flatMapMany(), following the same pattern as in other test classes. This consistency helps maintain a uniform approach to permission-based operations throughout the codebase.

app/server/appsmith-server/src/main/java/com/appsmith/server/imports/importable/artifactbased/ArtifactBasedImportableServiceCE.java (2)

11-11: Import added for Mono reactive type.

Added import supports the method signature change to return reactive type.


44-44: Interface method signature now returns Mono.

The method now returns Mono<T> instead of void, properly supporting reactive implementation. This change is essential for the non-blocking execution chain and ensures implementing classes follow the reactive pattern.

app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceTest.java (1)

232-236: Refactored permission retrieval to reactive pattern.

The code now properly follows reactive programming principles by first obtaining the delete permission as a Mono and then using flatMapMany to process it, rather than directly passing the permission retrieval call to the findByWorkspaceId method.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (1)

219-224: Consistent conversion to reactive pattern.

This change aligns with the project's shift toward reactive programming. The permission retrieval now returns a Mono that's properly processed through the reactive chain rather than being directly consumed.

app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/CreateDBTablePageSolutionTests.java (1)

244-249: Standardized permission handling to reactive flow.

The cleanup method now correctly uses reactive patterns for permission handling, consistent with similar changes throughout the codebase. This maintains a unified approach to permission-based operations.

app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ArtifactBuilderExtension.java (1)

97-99: Aligned with reactive programming model.

The afterEach method now properly handles permission retrieval reactively, consistent with the pattern applied to other test cleanup methods. This ensures a uniform approach to permission-based application deletion across the test suite.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java (1)

287-289: Improved reactive flow for permission handling

The change refactors permission handling to follow a more reactive approach by using flatMapMany operator on the permission retrieval. This is consistent with modern reactive programming patterns.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserWorkspaceServiceUnitTest.java (1)

121-123: Aligned permission handling with reactive paradigm

Updated to use flatMapMany for processing permissions, improving consistency with the reactive programming model used throughout the codebase.

app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/ce/TestComponentCE.java (3)

16-16: Converted synchronous method to reactive

Method signature changed from returning String to Mono<String> to support asynchronous execution.


18-18: Converted void method to reactive

Method signature changed from void to Mono<Void> for exception handling in reactive context.


20-20: Converted void method to reactive

Method signature changed from void to Mono<Void> for exception handling in reactive context.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationSnapshotServiceTest.java (1)

80-82: Enhanced permission handling with reactive approach

Changed to use flatMapMany on permission retrieval, creating a more consistent reactive flow.

app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java (1)

480-480: Proper reactive pattern implementation

The change from directly passing the permission to using flatMap for the reactive permission flow is well-implemented. This change correctly handles the asynchronous nature of permission retrieval.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java (1)

89-92: Correctly adapts test to reactive permission retrieval

The modification properly handles the now-reactive permission by first retrieving it with getDeletePermission() and then using flatMapMany to apply it to the workspace lookup. This maintains the same test functionality while adapting to the reactive API changes.

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ContextPermissionCE.java (3)

4-4: Required import for reactive types

Added import for Mono which is now needed for the reactive return types.


8-8: Appropriate return type change to reactive pattern

Changing the return type from AclPermission to Mono<AclPermission> aligns with the project's move toward a fully reactive approach for permission handling.


12-14: Improved reactive implementation for optional method

The default implementation now properly returns Mono.empty() instead of null, which is the correct reactive pattern for representing absence of a value. This avoids potential NullPointerExceptions and follows reactive best practices.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/CurlImporterServiceTest.java (1)

210-212: Correctly updates test cleanup to handle reactive permissions

This change properly adapts the test cleanup to use the now-reactive permission retrieval pattern, maintaining the same functionality while working with the updated API.

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PagePermissionCEImpl.java (3)

5-5: Added import for reactive programming.

Added Mono import to support the reactive implementation of permission methods.


27-28: Converted permission method to reactive.

Changed return type from AclPermission to Mono<AclPermission> to align with reactive programming model. This change improves consistency with other reactive methods in the application.


32-33: Converted permission method to reactive.

Changed return type from AclPermission to Mono<AclPermission> to align with reactive programming model. This change enables non-blocking permission handling.

app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/applications/ActionCollectionApplicationImportableServiceCEImpl.java (2)

21-21: Added Mono import for reactive implementation.

Added import for reactor.core.publisher.Mono to support the reactive pattern.


92-118: Refactored method to use reactive pattern.

The method has been properly converted from a void return type to Mono<ActionCollection>. The implementation now:

  1. Gets permission as a reactive Mono
  2. Uses flatMap to handle the permission check asynchronously
  3. Returns appropriate Mono responses based on permission result
  4. Maintains the same business logic flow in a reactive way

This change aligns with the broader refactoring effort to adopt reactive programming for permission handling.

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourcePermissionCEImpl.java (3)

5-5: Added import for reactive programming.

Added Mono import to support the reactive implementation of permission methods.


22-23: Converted permission method to reactive.

Changed return type from AclPermission to Mono<AclPermission> to align with reactive programming model. This makes permission checks consistent with other reactive methods.


37-38: Converted permission method to reactive.

Changed return type from AclPermission to Mono<AclPermission> to align with reactive programming model. This enables non-blocking permission handling.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (4)

121-126: Updated permission retrieval to reactive pattern.

Refactored permission checking to use reactive programming properly:

  1. First retrieves permission as a Mono
  2. Then uses flatMap to pass the resolved permission to the findById method
  3. Maintains the same error handling with switchIfEmpty

This improves consistency with the application's reactive programming model.


160-167: Converted permission handling to reactive pattern.

Similar to other methods, this code now properly retrieves the permission as a Mono and uses flatMap to chain the operation with newPageService.findById. This change enhances the reactive flow of the application.


185-191: Updated permission check to use reactive pattern.

Refactored to first retrieve the permission as a Mono, then use flatMap to pass it to the findById method. The error handling with switchIfEmpty remains unchanged. This change aligns with the overall reactive programming adoption in the codebase.


249-252: Converted permission handling to reactive pattern.

Permission checking has been properly updated to use the reactive approach with getActionCreatePermission() and flatMap. This change ensures consistency with other reactive methods in the application.

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourcePermissionCE.java (3)

4-4: Imported Mono for reactive return types

The addition of the Mono import supports the conversion of synchronous methods to reactive ones.


7-7: Method converted to reactive pattern

Changed return type from AclPermission to Mono<AclPermission> to support reactive programming flow.


11-11: Method converted to reactive pattern

Changed return type from AclPermission to Mono<AclPermission> to support reactive programming flow, consistent with the other changes.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.java (1)

158-163: Converted to reactive pattern in cleanup method

The cleanup method now properly handles the asynchronous nature of permission retrieval by using flatMapMany to process the result of getDeletePermission() before passing it to findByWorkspaceId.

app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1)

798-798: Converted to reactive pattern in deleteUnpublishedAction

Method now uses flatMap to handle the asynchronous permission retrieval before proceeding with deletion.

app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java (3)

283-285: Converted to reactive pattern in deleteWithoutPermissionUnpublishedActionCollection

Method now uses flatMap to handle the asynchronous permission retrieval before proceeding with deletion.


290-292: Converted to reactive pattern in deleteUnpublishedActionCollection

Method now uses flatMap to handle the asynchronous permission retrieval before proceeding with deletion.


424-429: Enhanced reactive handling in archiveGivenActionCollection

Good implementation with several improvements:

  1. Added caching of the permission Mono with .cache() to avoid multiple retrievals
  2. Used flatMapMany to properly handle the asynchronous flow
  3. Separated unpublished and published action retrieval flows clearly
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/applications/NewActionApplicationImportableServiceCEImpl.java (2)

21-21: No issues with this import.


93-119: Validate baseContext before casting.
Currently, the method casts baseContext to NewPage without any null or type checks. Ensure that baseContext is always non-null and of the correct type to avoid potential ClassCastException or NPE.

Would you like a script to search for all usage of createNewResource to confirm that baseContext is always a NewPage?

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/WorkspacePermissionCEImpl.java (1)

25-27: Good switch to reactive permissions.
Returning Mono<AclPermission> is consistent with the reactive approach used across the codebase.

Also applies to: 30-32, 35-37

app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspectTest.java (4)

108-115: Test updated correctly to handle reactive method

The test method ceEeSyncMethod_eeImplTest has been properly updated to use StepVerifier for testing reactive code. This aligns with the PR objective of converting methods to reactive implementations.


118-122: LGTM: Correct update to reactive testing

Properly updates the test to use StepVerifier for verifying the result of the reactive method.


125-134: Correctly implements error verification with StepVerifier

The error verification logic is now properly implemented using StepVerifier, checking both the exception type and message.


137-144: Error handling correctly implemented in reactive style

Good implementation of reactive error testing using StepVerifier for non-Appsmith exceptions.

app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (1)

304-344: Improved permission handling with reactive pattern

The method has been refactored to use reactive patterns correctly. The permission check now uses flatMap to conditionally process the next steps based on the permission result, which improves the flow and readability of the code.

The error handling is also properly implemented, with appropriate error messages logged when permission checks fail.

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/CreateDBTablePageSolutionCEImpl.java (2)

231-234: Correctly refactored to use reactive permission handling

The code now properly retrieves the permission first and then uses it in the subsequent operation using flatMap, which aligns with reactive programming patterns.


504-506: Good reactive pattern implementation

Similar to the previous change, permission retrieval has been properly converted to a reactive implementation using flatMap, enhancing consistency with the codebase's reactive approach.

app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/EmailServiceHelperCETest.java (4)

49-50: Test correctly updated for reactive method

Added .block() to properly retrieve the value from the Mono before assertion, maintaining test functionality after the method was converted to return a reactive type.


54-59: Properly updated assertion for reactive return type

The test correctly handles reactive return types by using .block() to retrieve the actual values before performing assertions.


64-66: LGTM: Correctly handles reactive return value

Properly updated to call .block() on the Mono to retrieve the actual template string before assertion.


70-71: Test correctly updated for reactive method

Correctly modified to use .block() to retrieve the template value from the Mono before assertion.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java (2)

258-260: Teardown logic approved
Using a reactive chain in the cleanup method aligns with the rest of the permission-based approach. Blocking here is acceptable for test teardown.


480-490: Clear test verification of executeOnLoad behavior
Verifying the actions' executeOnLoad values provides concise coverage and meets the intended test objective.

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceStructureSolutionCEImpl.java (1)

185-187: Reactive permission fetching
This chained approach to retrieve permissions before fetching the datasource is consistent with other reactive refactors. Looks good.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java (2)

44-44: Good addition of static import for Boolean.TRUE

Using static imports for Boolean constants improves readability in conditional statements.


389-393: Good conversion to reactive method signature

Converting isLastAdminRoleEntity to return Mono<Boolean> aligns with the project's shift to reactive programming. This change maintains consistency with other permission-related methods in the codebase.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/EmailServiceCEImpl.java (4)

37-46: Improved reactive flow with zipWith for template handling

Refactoring to use zipWith for combining parameter enrichment with template retrieval creates a cleaner reactive flow. This change makes the code more maintainable by explicitly combining related asynchronous operations.


63-76: Well-structured reactive combination using Mono.zip

Good refactoring to use Mono.zip for emailSubject and template. The explicit tuple destructuring in the flatMap makes the code more readable by clearly showing what each part of the tuple represents.


85-94: Consistent reactive pattern implementation

The refactoring follows the same pattern established in other methods, maintaining consistency throughout the service class. The zipWith operation provides a clean way to combine the parameter map with the email template.


109-130: Well-organized reactive chain using flatMap and zipWhen

The refactoring creates a cleaner separation of concerns with each step in the process: first getting the primary link text, then enriching parameters, then combining with subject and template. This makes the code easier to understand and maintain.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (4)

148-151: Good reactive permission handling

Converting the permission check to use flatMap aligns with reactive programming principles and maintains consistency with other permission-checking code in the application.


190-193: Consistent reactive flow for permission checks

The permission check now follows the same reactive pattern as elsewhere in the code, which improves consistency and readability.


343-347: Well-structured permission handling with Mono

Extracting the permission check into a separate Mono and using flatMap improves code clarity and maintains the reactive flow.


431-437: Consistent implementation of permission check

This refactoring maintains consistency with other permission checks in the class, ensuring that all code follows the same reactive pattern.

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ApplicationPermissionCEImpl.java (3)

5-5: Appropriate import addition for reactive return types

Adding the Mono import is necessary for the signature changes in the class.


36-38: Good conversion to reactive return type

Converting getDeletePermission to return Mono enables non-blocking flows in calling code. This change is part of a larger effort to make permission handling consistently reactive throughout the application.


51-53: Consistent reactive return type implementation

The change follows the same pattern as getDeletePermission, maintaining consistency across the permission interface. This allows callers to use a uniform approach when dealing with permissions.

app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceContextServiceTest.java (1)

141-143: Updated cleanup method to use reactive permission handling.

The method has been refactored to leverage reactive programming by calling getDeletePermission() and then using flatMapMany to pass the permission to applicationService.findByWorkspaceId. This aligns with the PR objective of converting non-reactive methods to reactive.

app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (2)

155-157: Refactored create method to use reactive permission handling.

The method now correctly leverages reactive programming by calling getDatasourceCreatePermission() and then using flatMap to pass the permission to createEx. This aligns with the PR objective of converting non-reactive methods to reactive.


885-887: Improved archiveById to use reactive permission handling.

The method has been refactored to use reactive programming with getDeletePermission() and flatMap, maintaining a consistent reactive flow. This change improves code consistency and aligns with modern reactive patterns.

app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/TestComponentImpl.java (3)

43-44: Converted ceEeSyncMethod to return Mono.

Method signature and implementation changed from synchronous to reactive, now returning Mono<String> instead of String. This aligns with the PR objective of converting methods to be reactive.


49-50: Converted ceEeThrowAppsmithException to return Mono with error.

Method now uses Mono.error() instead of directly throwing an exception, making it compatible with reactive error handling. This change improves exception handling in reactive streams.


55-56: Converted ceEeThrowNonAppsmithException to return Mono with error.

Method now uses Mono.error() instead of directly throwing an exception, aligning with reactive programming principles. This allows exceptions to be handled properly within reactive streams.

app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/EmailServiceHelperCEImpl.java (7)

39-41: Converted getForgotPasswordTemplate to return Mono.

Method signature and implementation changed to return reactive type Mono<String> instead of String, consistent with the PR's goal of making methods reactive.


44-48: Converted getWorkspaceInviteTemplate to return Mono.

Method now returns reactive type and wraps both return paths in Mono.just(). The conditional logic is maintained while adopting reactive patterns.


51-53: Converted getEmailVerificationTemplate to return Mono.

Method now returns Mono<String> instead of String, consistent with the reactive pattern changes throughout the codebase.


56-58: Converted getAdminInstanceInviteTemplate to return Mono.

Method now returns Mono<String> instead of String, consistent with the reactive pattern changes.


61-63: Converted getJoinInstanceCtaPrimaryText to return Mono.

Method now returns Mono<String> instead of String, following the reactive pattern.


66-68: Converted getSubjectJoinInstanceAsAdmin to return Mono.

Method now returns Mono<String> instead of String, following the reactive pattern.


71-73: Converted getSubjectJoinWorkspace to return Mono.

Method now returns Mono<String> instead of String, consistent with the reactive pattern changes.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (7)

161-163: Refactor to reactive pattern looks good

This change moves permission retrieval to use the reactive pattern, correctly using flatMap to chain the permission retrieval with the subsequent application lookup.


479-481: Clean reactive chain for permission handling

Good conversion to reactive permission handling using flatMap. This properly ensures the permission is retrieved before finding the workspace.


511-513: Well-structured reactive permission check

The conversion properly retrieves the delete permission in a reactive manner before using flatMap to chain to the next operation.


527-530: Consistent reactive permission pattern

The pattern for permission handling is consistently applied here as well. Good job on the refactoring.


562-571: Good use of cached permissions

Nice work caching the permission Monos and using them in multiple operations. This prevents unnecessary permission lookups.


916-928: Effective use of zipWith for multiple permissions

Good use of zipWith to combine multiple permission retrievals before proceeding with the operation. This maintains a clean reactive flow.


1497-1504: Appropriate refactoring to reactive permission retrieval

Good conversion to use reactive permission pattern consistently with the rest of the application.

app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/ce/TestComponentCEImpl.java (3)

30-32: Converted sync method to reactive properly

Successfully updated the return type to Mono<String> and implemented the method using Mono.just().


35-37: Good conversion of void method to Mono

Appropriate conversion of the void method to properly return an empty Mono.


40-42: Consistent reactive pattern implementation

Maintains consistency with the other exception method by properly returning Mono.empty().

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/WorkspacePermissionCE.java (4)

4-4: Required import for reactive types

Properly added the Mono import needed for the reactive return types.


7-7: Converted permission method to reactive

Successfully updated to return Mono as part of the reactive conversion.


9-9: Consistent reactive permission pattern

Good consistency in converting all permission-related methods to return Mono.


11-11: Completed reactive conversion for all methods

All permission methods now properly return Mono, maintaining a consistent API.

app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/EmailServiceHelperCE.java (1)

10-22: Successfully converted template methods to reactive

All email template-related methods have been properly updated to return Mono<String> instead of String, maintaining a consistent reactive approach throughout the service.

app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ImportArtifactPermissionProviderCE.java (4)

19-19: Good addition of the Mono import

Correctly added the import for Reactor's Mono class to support the reactive return types in the updated methods.


120-127: Appropriate conversion to reactive method

The method now correctly returns Mono<Boolean> instead of boolean, aligning with reactive programming principles. Good implementation using Mono.just(true) for the simple case and map for the permission check transformation.


129-134: Well-implemented reactive transformation

Clean conversion from synchronous to reactive implementation. The pattern using Mono.just(true) for the non-permission case and mapping the permission check result is consistent with best practices.


136-143: Consistent reactive implementation

This method follows the same pattern as the other converted methods, maintaining consistency in the reactive approach throughout the class.

@abhvsn abhvsn merged commit 6e25f51 into release Mar 7, 2025
85 checks passed
@abhvsn abhvsn deleted the chore/make-feature-flagged-method-reactive branch March 7, 2025 05:40
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this pull request Apr 12, 2025
…tive (appsmithorg#39594)

## Description
PR to convert non-reactive methods consuming @FeatureFlagged to reactive.


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._

/test All

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/13698576435>
> Commit: d8ade4d
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13698576435&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.All`
> Spec:
> <hr>Thu, 06 Mar 2025 13:34:00 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **Refactor**
- Migrated key permission checks and resource operations to a reactive,
asynchronous model that boosts system responsiveness and scalability.
- **Tests**
- Updated the testing suite to align with reactive patterns, ensuring
enhanced reliability and robustness.
- Adjusted assertions to handle reactive types appropriately, improving
clarity and specificity in test outcomes.
- **Chores**
- Streamlined internal workflows and error handling, leading to improved
performance and maintainability.

These internal improvements help achieve smoother and more efficient
operations without altering the user-facing experience.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants