chore: Segregate organization flags based on orgnization id#39608
chore: Segregate organization flags based on orgnization id#39608
Conversation
WalkthroughThe changes introduce organization-specific handling for feature flags across the codebase. Methods in core services and aspects now require an additional Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 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
|
github-actions
bot
added
the
skip-changelog
|
Blocked on #39594 |
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 (9)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ReactiveContextUtils.java (1)
1-22: New utility class for reactive user context retrieval.This new utility class properly extracts the current user from the reactive security context. The implementation handles the empty case appropriately with logging.
Consider adding a null check before casting to User to prevent potential ClassCastException if the principal is of a different type:
- .map(auth -> (User) auth.getPrincipal()) + .map(auth -> { + if (auth.getPrincipal() instanceof User) { + return (User) auth.getPrincipal(); + } + log.error("Principal is not of type User: {}", auth.getPrincipal().getClass()); + return null; + }) + .filter(user -> user != null)app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java (1)
89-97: Updated to retrieve organization-specific feature flags.The implementation now correctly retrieves feature flags using the organization ID from the trigger request, supporting the PR objective to segregate flags by organization.
The TODO comment indicates this is specifically needed for Google Sheets integration. Consider adding more detailed documentation about this requirement if it's intended to be a permanent solution rather than a temporary flag.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java (1)
292-292: Consider non-blocking alternatives in production code.While using
.block()is acceptable in test code, be mindful that this pattern shouldn't be carried over to production code as it can lead to thread blocking issues in a reactive application.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java (1)
123-128: Simplify feature flag retrieval logic to avoid redundant calls.The current implementation makes potentially redundant calls to
getCachedOrganizationFeatureFlags. Consider refactoring to reduce duplication:-Map<String, Boolean> featureFlagMap = - featureFlagService.getCachedOrganizationFeatureFlags(organizationId) - != null - ? featureFlagService - .getCachedOrganizationFeatureFlags(organizationId) - .getFeatures() - : Collections.emptyMap(); +Map<String, Boolean> featureFlagMap; +var cachedFlags = featureFlagService.getCachedOrganizationFeatureFlags(organizationId); +if (cachedFlags != null) { + featureFlagMap = cachedFlags.getFeatures(); +} else { + featureFlagMap = Collections.emptyMap(); +}app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java (2)
42-42: Consider using a thread-safe data structure.
If multiple threads update or read from this map concurrently, aConcurrentHashMapor proper synchronization may be necessary.- private Map<String, CachedFeatures> cachedOrganizationFeatureFlags = new HashMap<>(); + private Map<String, CachedFeatures> cachedOrganizationFeatureFlags = new ConcurrentHashMap<>();
200-203: Method reliably returns a non-null CachedFeatures.
SincegetOrDefaultalways returns a valid object, external null checks are unnecessary. This can simplify call sites checking for null.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (1)
766-771: Remove redundant null check for cached features.
getCachedOrganizationFeatureFlagswill never return null, so simplify the logic.- Map<String, Boolean> features = - featureFlagService.getCachedOrganizationFeatureFlags(organizationId) != null - ? featureFlagService.getCachedOrganizationFeatureFlags(organizationId).getFeatures() - : Collections.emptyMap(); + CachedFeatures orgFeatures = featureFlagService.getCachedOrganizationFeatureFlags(organizationId); + Map<String, Boolean> features = orgFeatures.getFeatures();app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspect.java (2)
76-79: Fix typo in error messageThere's a typo in the error message: "orgnization-specific" should be "organization-specific".
- "Add missing organizationId parameter and enforce non-null value for orgnization-specific feature flags retrieval in non-reactive methods"; + "Add missing organizationId parameter and enforce non-null value for organization-specific feature flags retrieval in non-reactive methods";
132-146: Organization ID extraction logic implementationThe implementation to extract organization ID from method arguments is sound, looking for parameters named either "organizationId" or "orgId". This facilitates feature flag checks for organization-specific contexts.
Consider enhancing this method to support a wider range of parameter naming patterns or potentially using annotations to explicitly mark which parameter contains the organization ID, as relying solely on exact parameter names can be fragile if naming conventions change.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspect.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ReactiveContextUtils.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/SessionUserServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspectTest.java(5 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/TestComponentImpl.java(2 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(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java (1)
Learnt from: abhvsn
PR: appsmithorg/appsmith#39171
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java:42-44
Timestamp: 2025-02-24T05:59:14.021Z
Learning: The reactive transition for helper methods consuming @FeatureFlagged annotations will be handled in a separate PR to maintain focused changes and proper separation of concerns.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (21)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCE.java (1)
45-45: Method signature updated to support organization-specific feature flags.The change adds an
organizationIdparameter to thegetCachedOrganizationFeatureFlagsmethod, aligning with the PR objective to segregate organization flags. This enables feature flag checks to be organization-specific, particularly useful for server-side internal calls.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/SessionUserServiceCEImpl.java (1)
37-39: Properly deprecated method with clear migration path.The method is correctly marked as deprecated with a TODO comment providing clear guidance to use
ReactiveContextUtils#getCurrentUser()instead. This supports the refactoring effort for organization-specific feature flags.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java (2)
10-10: Good addition of ReactiveContextUtils import.This import supports the new approach for retrieving the current user and organization context necessary for feature flag segregation.
294-294: Appropriate use of organization context for feature flags.Good update to pass the organization ID parameter when retrieving cached organization feature flags, which aligns with the PR objective of segregating flags by organization.
Also applies to: 310-310
app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/ce/TestComponentCEImpl.java (2)
30-32: Method signature updated correctly for synchronous operation.The method has been properly changed from returning
Mono<String>to directly returningString, and now includes the requiredorganizationIdparameter. This matches the interface declaration changes.
44-47: Good test coverage for organization-less scenario.Adding this method provides test coverage for situations where the organization context might not be available, which is important for comprehensive testing of the feature flag functionality.
app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/ce/TestComponentCE.java (2)
16-16: Interface method signature updated correctly.The method signature change aligns with the implementation in
TestComponentCEImpland supports the PR's goal of using organization context for feature flags.
22-22: Good addition of method for organization-less scenarios.Adding this method declaration supports testing edge cases where organization context might not be available.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java (2)
118-119: Good extraction of organization ID from request context.Extracting the organization ID from the updated trigger request DTO is the right approach for maintaining organization context throughout the feature flag evaluation process.
130-136: Proper integration of feature flags with plugin execution.The updated method call correctly passes the feature flag map to the plugin executor, ensuring that organization-specific flags are considered during execution.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java (1)
185-185: Duplicate concurrency concern.
This map mutation repeats the concurrency concern noted above. Ensure safe concurrent access or confirm that concurrency is not an issue in this context.app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/component/TestComponentImpl.java (2)
43-44: Verify null handling for organizationId.
Ensure that calls to this method handle a potentially missingorganizationIdgracefully, or confirm that it's always non-null in this usage.
58-63: Double-check necessity of the no-organization variant.
Since@FeatureFlaggedis tied to an organization context, confirm that skippingorganizationIdaligns with the intended design.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (2)
41-41: Import looks good.
No concerns regarding the new import statement.
758-762: Confirm safe handling of empty user contexts.
getCurrentUser()might sometimes be empty. Ensure the subsequent usage of.getOrganizationId()won’t trigger a null pointer when no user is present.app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspect.java (2)
70-82: Enforce organization context for non-reactive flagged methodsThis adds a necessary check for non-reactive methods using
@FeatureFlaggedannotation, requiring anorganizationIdparameter. The implementation properly validates the organization ID and throws an appropriate exception when missing.
125-130: Updated feature flag check method to require organization contextThe
isFeatureFlagEnabledmethod signature has been updated to require an organization ID parameter, aligning with the organization-specific feature flag requirements.app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspectTest.java (4)
46-47: Updated mock setup for organization-specific feature flagsThe test setup has been properly updated to use
any()matcher when mocking thegetCachedOrganizationFeatureFlagsmethod, reflecting the signature change.
113-119: Updated test to include organization ID parameterThe test has been correctly modified to set up the expected behavior with a specific organization ID and pass that ID to the method under test.
123-124: Updated test case to include organization IDThe test case properly passes the organization ID parameter to align with the new method signature requirements.
149-166:Details
❓ Verification inconclusive
Added test for missing organization ID scenario
This new test verifies that an exception is properly thrown when a non-reactive method annotated with
@FeatureFlaggedis called without an organization ID. The assertion correctly validates both the exception type and the error message.
🏁 Script executed:
#!/bin/bash # Check if TestComponent contains the ceEeSyncMethodWithoutOrganization method that's being tested fd -e java "TestComponent.java" -x cat {} | grep -A 5 "ceEeSyncMethodWithoutOrganization"Length of output: 89
Please verify that the
ceEeSyncMethodWithoutOrganizationmethod is present in the implementation of TestComponent.
- The search command for the method in
TestComponent.javadid not return any output. Please manually confirm whether the method is implemented under a different file name or has been relocated.- The new test in
FeatureFlaggedMethodInvokerAspectTest.javaappears correctly written—it checks that invoking a non-reactive@FeatureFlaggedmethod without an organization ID throws the expected exception with the proper error message.- Once you confirm that the
ceEeSyncMethodWithoutOrganizationmethod exists (or adjust its location accordingly), this test will effectively validate the missing organization ID scenario.
…ithorg/appsmith into chore/segregate-org-id-based-flags
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)
765-771: Consider adding logging for debuggingThe code correctly uses the organization-specific feature flag retrieval with proper null handling. However, consider adding debug logging when falling back to an empty map to aid in troubleshooting scenarios where feature flags aren't being applied as expected.
Map<String, Boolean> features = featureFlagService.getCachedOrganizationFeatureFlags(organizationId) != null ? featureFlagService .getCachedOrganizationFeatureFlags(organizationId) .getFeatures() : Collections.emptyMap(); +if (Collections.isEmpty(features)) { + log.debug("No feature flags found for organization ID: {}", organizationId); +}
📜 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/plugins/solutions/PluginTriggerSolutionCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/DatasourceTriggerSolutionCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/solutions/PluginTriggerSolutionCEImpl.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-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java (3)
41-41: Good data structure change for organization-specific feature flagsConverting the
cachedOrganizationFeatureFlagsfrom a single object to a map with organization IDs as keys aligns well with the PR objective of segregating flags by organization.
167-167: LGTM: Map usage is correctProperly storing organization-specific feature flags in the map using the organization ID as the key.
183-185: LGTM: Well-implemented accessor method with fallbackThe new method provides a clean interface for retrieving organization-specific feature flags with a sensible default for when no flags exist.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (2)
757-757: Good enhancement to include organization contextZipping with the organization ID retrieval is a clean approach to include the organization context needed for the feature flags.
759-762: LGTM: Proper extraction of zipped valuesCorrectly extracting the tuple values including the newly added organization ID.
…org#39608) ## Description PR to add organizationId param while fetching the organization level flags. This will be useful when one wants to use @FeatureFlagged for server internal calls e.g. Cron job, analytics event which is running on threads which does not have user context directly. 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/13891832232> > Commit: 95754e1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13891832232&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Mon, 17 Mar 2025 06:59:54 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 - **New Features** - Implemented organization-aware feature flag evaluation, ensuring that feature flags are dynamically applied based on the user's organization. - **Bug Fixes** - Improved error handling to enforce the presence of an organization identifier for operations, preventing misconfiguration. - **Tests** - Expanded test coverage to verify the correct behavior of organization-specific feature flag handling and proper error reporting when organization information is missing. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
PR to add organizationId param while fetching the organization level flags. This will be useful when one wants to use @FeatureFlagged for server internal calls e.g. Cron job, analytics event which is running on threads which does not have user context directly.
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.
/test All
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13891832232
Commit: 95754e1
Cypress dashboard.
Tags:
@tag.AllSpec:
Mon, 17 Mar 2025 06:59:54 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit