chore: Code split env manager for introducing blacklisted variables#39963
chore: Code split env manager for introducing blacklisted variables#39963
Conversation
WalkthroughThis pull request updates both client and server code. On the client side, the function for filtering general categories now accepts an optional feature flag parameter and the AdminSettings left pane integrates a feature flag check. On the server side, new interfaces and classes are introduced for handling blacklisted environment variables. Additionally, the environment management components have been updated to use reactive programming patterns, with method signatures and error handling modified accordingly. Test cases are adjusted to work with reactive streams. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant LP as LeftPane Component
participant GH as GeneralCategories Helper
U->>LP: Render Admin Settings
LP->>LP: Check feature flag (useFeatureFlag)
LP->>GH: Call getFilteredGeneralCategories(categories, {license_multi_org_enabled: isMultiOrgEnabled})
GH-->>LP: Return filtered categories
LP-->>U: Display filtered results
sequenceDiagram
participant C as Client
participant EM as EnvManagerCEImpl
participant BVH as BlacklistedEnvVariableHelper
C->>EM: Call transformEnvContent(envContent, changes)
EM->>BVH: Retrieve/filter blacklisted environment variables
BVH-->>EM: Provide filtered variables
EM->>EM: Process changes reactively (publishOn, error handling)
EM-->>C: Return Mono<List<String>> with transformed content
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
|
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
🔭 Outside diff range comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java (1)
179-239:⚠️ Potential issueReactive transformEnvContent with blacklisted variable removal.
The move toMono<List<String>>is consistent. Be cautious with the loop removing map entries during iteration (lines 199–203) asHashMapis not safe for removals while iterating overkeySet(); this can causeConcurrentModificationException. Consider iterating over entrySet in a safe manner (e.g., using an iterator).- for (String key : updatedChanges.keySet()) { - if (blacklistedEnvVariable.contains(key)) { - updatedChanges.remove(key); - } - } + for (Iterator<String> it = updatedChanges.keySet().iterator(); it.hasNext();) { + var key = it.next(); + if (blacklistedEnvVariable.contains(key)) { + it.remove(); + } + }
🧹 Nitpick comments (3)
app/client/src/ce/utils/adminSettingsHelpers.ts (1)
73-78: Consider adding filter logic for the multi-org feature flag.The function currently returns all categories without filtering based on the feature flags. If the purpose of adding the feature flag parameter is to conditionally show/hide categories, you should implement that logic.
return categories ?.map((category: Category) => { + // Example of potential implementation + if (featureFlags?.license_multi_org_enabled === false && category.isMultiOrgOnly) { + return null; + } return category; }) .filter(Boolean);app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/BlacklistedEnvVariableHelperCE.java (1)
5-7: Method name should be plural since it returns a collectionThe method returns a Set but uses singular "Variable" in its name.
- Set<String> getBlacklistedEnvVariableForAppsmithCloud(String organizationId); + Set<String> getBlacklistedEnvVariablesForAppsmithCloud(String organizationId);app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/BlacklistedEnvVariableHelperCEImpl.java (1)
7-8: Method name should be plural to match its return typeFor consistency with the return type (a collection), consider renaming this method to use the plural form.
- public Set<String> getBlacklistedEnvVariableForAppsmithCloud(String organizationId) { + public Set<String> getBlacklistedEnvVariablesForAppsmithCloud(String organizationId) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/client/src/ce/utils/adminSettingsHelpers.ts(1 hunks)app/client/src/pages/AdminSettings/LeftPane.tsx(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/BlacklistedEnvVariableHelper.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/BlacklistedEnvVariableHelperImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/BlacklistedEnvVariableHelperCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/BlacklistedEnvVariableHelperCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/EnvManagerImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java(12 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java(6 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
app/client/src/pages/AdminSettings/LeftPane.tsx (1)
app/client/src/ce/utils/adminSettingsHelpers.ts (1)
getFilteredGeneralCategories(68-78)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1)
FieldNameCE(3-212)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/BlacklistedEnvVariableHelperImpl.java (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/BlacklistedEnvVariableHelperCEImpl.java (1)
BlacklistedEnvVariableHelperCEImpl(5-10)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/EnvManagerImpl.java (1)
Component(24-66)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (29)
app/client/src/pages/AdminSettings/LeftPane.tsx (2)
205-207: Good addition of feature flag for multi-org support.This correctly adds a check for the multi-organization feature flag which will be needed for conditional rendering.
213-215: Implementation correctly passes the feature flag to helper function.The updated call now passes the feature flag to
getFilteredGeneralCategories, which aligns with the updated function signature in the adminSettingsHelpers file.app/client/src/ce/utils/adminSettingsHelpers.ts (1)
68-72: Function signature update is correct but implementation still pending.The function now accepts the featureFlags parameter but doesn't utilize it yet, as indicated by the ESLint comment. This is likely preparation for future filtering logic based on feature flags.
Is there a future PR planned to implement the actual filtering logic based on this feature flag? Once that's implemented, we should update the ESLint comment.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/BlacklistedEnvVariableHelper.java (1)
1-5: Interface looks clean and follows project patternsThis interface properly extends the CE version, following the established pattern in the codebase for separating core functionality from specific implementations.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/BlacklistedEnvVariableHelperImpl.java (1)
1-10: Implementation is correctly configuredThe implementation properly extends the CE implementation class and implements the interface. Spring component annotation and Lombok's RequiredArgsConstructor are appropriately used for dependency injection.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/BlacklistedEnvVariableHelperCEImpl.java (1)
6-9: Implementation correctly returns an empty setThe implementation is minimal but correct. Since this is likely a placeholder for the CE version, returning an empty set is appropriate.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCE.java (1)
14-14: Adopting reactive return type.
Switching the method signature fromList<String>toMono<List<String>>aligns with the reactive paradigm and looks appropriate. Make sure all calling sites handle the new reactive flow.app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java (12)
9-126: Introducing BlacklistedEnvVariableHelper and updated constructor spy.
The addition of theBlacklistedEnvVariableHelpermock and the updated constructor parameters maintain consistency with the new reactive method changes. Using a spy onEnvManagerImplis reasonable for partial mocking. Ensure each mock is verified in relevant test scenarios.
133-142: Verifying transformEnvContent with StepVerifier (first case).
The test verifies updates to a single variable. This looks fine. Keep an eye on edge cases (e.g., empty strings or null).
144-153: Verifying transformEnvContent with StepVerifier (second case).
Same pattern of verification for a different variable. Looks consistent and thorough.
155-166: Verifying transformEnvContent (third case).
Repeats the StepVerifier pattern for a third variable. Clear test coverage, no concerns here.
167-181: Multiple changes in transformEnvContent.
This verifies simultaneous changes to multiple variables. Good coverage. No issues spotted.
187-208: Testing empty vs. non-empty updates.
Coverage of edge scenarios for empty environment variable values is key. Good job adding these corner cases.
215-255: Quoted values transformations.
These tests confirm correct handling of quotes and special characters. The usage of StepVerifier is consistent, and the coverage of string escaping is thorough.
294-305: Disallowed variable scenario.
Validates that blacklisted changes result in errors. Nicely ensures the environment manager blocks forbidden variables.
312-327: Adding new environment variable.
Test ensures new variables can be introduced properly. Looks consistent.
334-349: Setting values containing quotes.
Verifies that special quoting logic works. Good test coverage for advanced quoting cases.
366-383: Filtering out empty values (first scenario).
ThegetAllNonEmpty()usage is tested to ensure no empty-value entries are returned. Straightforward and correct.
385-409: Filtering out empty values (multiple variables).
Extends the prior test using more variables. Very thorough. No issues here.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/EnvManagerImpl.java (2)
6-7: New import for BlacklistedEnvVariableHelper.
Importing the helper fosters blacklisted variable checks. Aligns with the recent changes.
44-65: Constructor injection for EmailService and BlacklistedEnvVariableHelper.
Passing these dependencies into the superclass is consistent with the new layering. Looks clean and well-structured.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java (8)
16-16: New import for BlacklistedEnvVariableHelper.
The import is correct and supports blacklisted variable logic in this class.
50-51: Using Schedulers and Context from Reactor.
Inclusion ofSchedulersandContextindicates a more robust reactive approach. Make sure to confirm proper concurrency usage.
88-88: Added reference to FieldNameCE.ORGANIZATION_ID.
Ensures correct context-based retrieval of the organization ID for blacklisted checks.
119-120: Introducing blacklistedEnvVariableHelper field.
This field ties into filtering disallowed environment variables. No immediate issues.
147-148: Added blacklistedEnvVariableHelper to constructor parameters.
The constructor change keeps the class consistent with the rest of the codebase.
166-166: Storing blacklistedEnvVariableHelper in the class field.
Straightforward assignment. Aligns with best practices for dependency injection.
373-392: applyChanges referencing applyChangesToEnvFileWithoutAclCheck.
The high-level logic for environment updates and ACL checks is coherent. No additional issues noted.
727-741: getAllNonEmpty now includes blacklisted filtering.
The approach to remove blacklisted entries with the organization ID is correct. Observed no other issues.
…ppsmithorg#39963) ## Description Code split PR for env manager for the corresponding PR at appsmithorg/appsmith-ee#6799 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 Settings ### 🔍 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/14125869636> > Commit: ec228ef > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=14125869636&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Settings` > Spec: > <hr>Fri, 28 Mar 2025 09:49:43 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** - Enhanced administrative settings with multi-organization support to improve category filtering based on feature flags. - **Refactor** - Upgraded environment variable management to employ asynchronous processing and stricter filtering of sensitive values. - **Tests** - Revised the test suite to better validate asynchronous behavior and the improved handling of environment variables. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…ppsmithorg#39963) ## Description Code split PR for env manager for the corresponding PR at appsmithorg/appsmith-ee#6799 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 Settings ### 🔍 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/14125869636> > Commit: ec228ef > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=14125869636&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Settings` > Spec: > <hr>Fri, 28 Mar 2025 09:49:43 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** - Enhanced administrative settings with multi-organization support to improve category filtering based on feature flags. - **Refactor** - Upgraded environment variable management to employ asynchronous processing and stricter filtering of sensitive values. - **Tests** - Revised the test suite to better validate asynchronous behavior and the improved handling of environment variables. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Code split PR for env manager for the corresponding PR at https://github.com/appsmithorg/appsmith-ee/pull/6799
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 Settings
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14125869636
Commit: ec228ef
Cypress dashboard.
Tags:
@tag.SettingsSpec:
Fri, 28 Mar 2025 09:49:43 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor
Tests