fix: NPE while creating a policies copy #36172
Conversation
WalkthroughThe changes improve the handling of policies in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Repository
participant PolicySolution
User->>Repository: Request to set user permissions
Repository->>Repository: Retrieve existing policies
alt Existing policies are null
Repository->>Repository: Initialize policies to empty set
else Existing policies exist
Repository->>Repository: Initialize policies with existing set
end
Repository->>PolicySolution: Pass policies for processing
PolicySolution->>PolicySolution: Add or remove policies as needed
PolicySolution->>User: Respond with updated policies
Possibly related PRs
Tip New featuresWalkthrough comment now includes:
Notes:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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
|
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PolicySolutionCEImpl.java (2 hunks)
Additional context used
Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (1)
Learnt from: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.
Additional comments not posted (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PolicySolutionCEImpl.java (2)
72-73: Good handling of null values.The change to initialize
policieswith an empty set ifexistingPoliciesis null is a good practice to avoid NullPointerExceptions. This ensures that the method behaves predictably even whenexistingPoliciesis null.
106-107: Consistent null handling across methods.The approach to handle
existingPoliciesbeing null by initializingpolicieswith an empty set is consistent with the changes in theaddPoliciesToExistingObjectmethod. This consistency is crucial for maintaining predictable behavior across similar methods.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (1)
419-420: Proactive null handling insetUserPermissionsInObject.The modification to initialize
policieswith an empty set ifexistingPoliciesis null is a prudent measure to prevent NullPointerExceptions. This change ensures that the method can safely proceed without crashing due to null values.
abhvsn
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryImplTest.java (1 hunks)
| BaseAppsmithRepositoryCEImpl<TestClass> baseAppsmithRepositoryImpl; | ||
|
|
||
| @BeforeEach | ||
| public void setup() { | ||
| baseAppsmithRepositoryImpl = new BaseAppsmithRepositoryCEImpl<>() {}; | ||
| } |
There was a problem hiding this comment.
Ensure proper initialization in the setup method.
The setup method correctly initializes baseAppsmithRepositoryImpl. However, consider using @Mock for dependencies and @InjectMocks for the class under test to ensure isolation and control over the test environment.
| @Test | ||
| void testSetUserPermissionsInObject_whenPoliciesIsEmptySet_emptyCollectionValueIsSet() { | ||
| // Test the method setPoliciesInObject when the policies are null | ||
| // The method should set an empty collection value in the object | ||
| // The method should return the object | ||
| TestClass obj = baseAppsmithRepositoryImpl | ||
| .setUserPermissionsInObject(new TestClass(), null) | ||
| .block(); | ||
| assertNotNull(obj); | ||
| Assertions.assertEquals(0, obj.getPolicies().size()); | ||
| } |
There was a problem hiding this comment.
Review the logic and naming in the test method.
The test method testSetUserPermissionsInObject_whenPoliciesIsEmptySet_emptyCollectionValueIsSet seems to have a misleading name. The description and the test imply it handles null policies, not an empty set. Consider renaming the method to reflect that it tests the behavior when policies are null.
Additionally, the test asserts that the policies size is zero, which contradicts the setup where policies are set to null. This might indicate a misunderstanding in the implementation or the test itself. Clarify whether the method should indeed set an empty set or handle null values differently.
| @Test | ||
| void testSetUserPermissionsInObject_whenPoliciesIsNull_nullPoliciesAreSet() { | ||
| // Test the method setPoliciesInObject when the policies are empty | ||
| // The method should set an empty collection value in the object | ||
| // The method should return the object | ||
| TestClass obj = new TestClass(); | ||
| obj.setPolicies(null); | ||
| Set<String> permissionGroups = new HashSet<>(); | ||
| permissionGroups.add(UUID.randomUUID().toString()); | ||
| obj = baseAppsmithRepositoryImpl | ||
| .setUserPermissionsInObject(obj, permissionGroups) | ||
| .block(); | ||
| assertNotNull(obj); | ||
| Assertions.assertNull(obj.getPolicies()); | ||
| } |
There was a problem hiding this comment.
Clarify test behavior and improve assertions.
The test method testSetUserPermissionsInObject_whenPoliciesIsNull_nullPoliciesAreSet is named as if it tests null policies, but the setup and assertions suggest it tests an empty set scenario. The method name should be corrected to reflect the actual test scenario.
The use of Assertions.assertNull(obj.getPolicies()) is appropriate if the expected behavior is to maintain the null state of policies. However, ensure that this aligns with the intended functionality of the method being tested. If the method is supposed to convert null policies into an empty set, this assertion would be incorrect.
| @Test | ||
| void testAddNewPoliciesToNullPoliciesObject() { | ||
| TestClass obj = new TestClass(); | ||
| obj.setPolicies(null); | ||
| Map<String, Policy> policyMap = new HashMap<>(); | ||
| policyMap.put("read", new Policy("read", new HashSet<>(Set.of("group1")))); | ||
|
|
||
| BaseDomain result = policySolution.addPoliciesToExistingObject(policyMap, obj); | ||
|
|
||
| assertTrue(result.getPolicies().containsAll(policyMap.values())); | ||
| } |
There was a problem hiding this comment.
Well done on adding the test for null policies!
This test method testAddNewPoliciesToNullPoliciesObject is a crucial addition to ensure that your application gracefully handles cases where the policies object might be null. It's good to see that you're not only setting the policies to null but also verifying that the policies are added correctly afterwards.
However, let's consider a few enhancements:
- Additional Assertions: It might be beneficial to add more granular assertions to check individual elements of the policies to ensure that each policy is not only present but also correctly configured.
- Exception Handling: While the current test checks for the successful addition of policies, consider adding a test case to verify the system's behavior when an exception is expected, such as adding a null policy to the map.
Here's a suggestion to expand the test coverage:
@Test
void testAddNullPolicyToPoliciesObject() {
TestClass obj = new TestClass();
obj.setPolicies(null);
Map<String, Policy> policyMap = new HashMap<>();
policyMap.put("read", null); // Intentionally adding a null policy
Exception exception = assertThrows(NullPointerException.class, () -> {
policySolution.addPoliciesToExistingObject(policyMap, obj);
});
assertEquals("Policy cannot be null", exception.getMessage());
}This additional test will help ensure robust error handling in your policy management logic.
## Description PR to handle the empty collection and null values for policies. We expect web should never end up in this state but we have received enough evidences that because of broken functionality in GAC we may end up in this scenario. One such example is appsmithorg#36063 /test all ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > 🔴 🔴 🔴 Some tests have failed. > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10767073729> > Commit: e225d54 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10767073729&attempt=2&selectiontype=test&testsstatus=failed&specsstatus=fail" target="_blank">Cypress dashboard</a>. > Tags: @tag.All > Spec: > The following are new failures, please fix them before merging the PR: <ol> > <li>cypress/e2e/Regression/ClientSide/Debugger/Widget_property_navigation_spec.ts > <li>cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_spec.ts</ol> > <a href="https://internal.appsmith.com/app/cypress-dashboard/identified-flaky-tests-65890b3c81d7400d08fa9ee3?branch=master" target="_blank">List of identified flaky tests</a>. > <hr>Mon, 09 Sep 2024 07:01:41 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 - **Bug Fixes** - Improved handling of user permissions to prevent potential errors related to null values in policies. - Enhanced robustness in policy management methods to avoid null-related errors when retrieving existing policies. - **Tests** - Added unit tests for user permissions handling and policy management to ensure robust behavior under various conditions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
PR to handle the empty collection and null values for policies. We expect web should never end up in this state but we have received enough evidences that because of broken functionality in GAC we may end up in this scenario. One such example is #36063
/test all
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10767073729
Commit: e225d54
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Debugger/Widget_property_navigation_spec.ts
- cypress/e2e/Regression/ClientSide/OneClickBinding/PropertyControl_spec.ts
List of identified flaky tests.Mon, 09 Sep 2024 07:01:41 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit