Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: NPE while creating a policies copy #36172

Merged
merged 3 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,8 @@ public Mono<T> setUserPermissionsInObject(T obj, Set<String> permissionGroups) {
Set<String> permissions = new HashSet<>();
obj.setUserPermissions(permissions);

Set<Policy> policies = new HashSet<>(obj.getPolicies());
Set<Policy> existingPolicies = obj.getPolicies();
final Set<Policy> policies = new HashSet<>(existingPolicies == null ? Set.of() : existingPolicies);
if (CollectionUtils.isEmpty(policies) || permissionGroups.isEmpty()) {
return Mono.just(obj);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ public <T extends BaseDomain> T addPoliciesToExistingObject(@NonNull Map<String,
policyMap1.put(entry.getKey(), policy);
}

final Set<Policy> policies = new HashSet<>(obj.getPolicies());
Set<Policy> existingPolicies = obj.getPolicies();
final Set<Policy> policies = new HashSet<>(existingPolicies == null ? Set.of() : existingPolicies);

// Append the user to the existing permission policy if it already exists.
for (Policy policy : policies) {
Expand Down Expand Up @@ -102,7 +103,8 @@ public <T extends BaseDomain> T removePoliciesFromExistingObject(Map<String, Pol
policyMap1.put(entry.getKey(), entry.getValue());
}

Set<Policy> policies = new HashSet<>(obj.getPolicies());
Set<Policy> existingPolicies = obj.getPolicies();
final Set<Policy> policies = new HashSet<>(existingPolicies == null ? Set.of() : existingPolicies);
// Remove the user from the existing permission policy if it exists.
for (Policy policy : policies) {
String permission = policy.getPermission();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package com.appsmith.server.repositories.ce;

import com.appsmith.external.models.BaseDomain;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.util.HashSet;
import java.util.Set;
import java.util.UUID;

import static org.junit.jupiter.api.Assertions.assertNotNull;

class BaseAppsmithRepositoryImplTest {

BaseAppsmithRepositoryCEImpl<TestClass> baseAppsmithRepositoryImpl;

@BeforeEach
public void setup() {
baseAppsmithRepositoryImpl = new BaseAppsmithRepositoryCEImpl<>() {};
}
Comment on lines +16 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


class TestClass extends BaseDomain {}

@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());
}
Comment on lines +25 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

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());
}
Comment on lines +37 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
Loading