Skip to content

fix: Add custom deserialiser for idPolicies projection #38127

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
abhvsn wants to merge 1 commit into from
Closed

fix: Add custom deserialiser for idPolicies projection #38127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 21 additions & 3 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/projections/IdPoliciesOnly.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,29 @@
package com.appsmith.server.projections;

import com.appsmith.external.models.Policy;
import com.fasterxml.jackson.databind.ObjectMapper;
import lombok.Getter;

import java.util.HashMap;
import java.util.Map;

public interface IdPoliciesOnly {
String getId();
@Getter
public class IdPoliciesOnly {
String id;
Map<String, Policy> policyMap = new HashMap<>();

Map<String, Policy> getPolicyMap();
// TODO Abhijeet: This is a temporary fix to convert the map of Object to map of Policy
public IdPoliciesOnly(String id, Map<String, Object> policyMap) {
this.id = id;
if (policyMap == null) {
return;
}
policyMap.forEach((key, value) -> {
if (value instanceof Policy) {
this.policyMap.put(key, (Policy) value);
} else if (value instanceof Map) {
this.policyMap.put(key, new ObjectMapper().convertValue(value, Policy.class));
}
});
}
Comment on lines +15 to +28

@coderabbitai coderabbitai Bot Dec 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

鈿狅笍 Potential issue

Handle ObjectMapper conversion exceptions and add parameter validation

The constructor contains potential issues:

  1. ObjectMapper conversion could throw exceptions
  2. Missing validation for id parameter
  3. TODO comment indicates temporary solution

Consider this implementation:

-    public IdPoliciesOnly(String id, Map<String, Object> policyMap) {
+    public IdPoliciesOnly(final String id, final Map<String, Object> policyMap) {
+        if (id == null || id.trim().isEmpty()) {
+            throw new IllegalArgumentException("Id cannot be null or empty");
+        }
         this.id = id;
         if (policyMap == null) {
             return;
         }
         policyMap.forEach((key, value) -> {
             if (value instanceof Policy) {
                 this.policyMap.put(key, (Policy) value);
             } else if (value instanceof Map) {
+                try {
                     this.policyMap.put(key, new ObjectMapper().convertValue(value, Policy.class));
+                } catch (IllegalArgumentException e) {
+                    throw new IllegalStateException("Failed to convert policy map value", e);
+                }
             }
         });
     }
馃摑 Committable suggestion

鈥硷笍 IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// TODO Abhijeet: This is a temporary fix to convert the map of Object to map of Policy
public IdPoliciesOnly(String id, Map<String, Object> policyMap) {
this.id = id;
if (policyMap == null) {
return;
}
policyMap.forEach((key, value) -> {
if (value instanceof Policy) {
this.policyMap.put(key, (Policy) value);
} else if (value instanceof Map) {
this.policyMap.put(key, new ObjectMapper().convertValue(value, Policy.class));
}
});
}
// TODO Abhijeet: This is a temporary fix to convert the map of Object to map of Policy
public IdPoliciesOnly(final String id, final Map<String, Object> policyMap) {
if (id == null || id.trim().isEmpty()) {
throw new IllegalArgumentException("Id cannot be null or empty");
}
this.id = id;
if (policyMap == null) {
return;
}
policyMap.forEach((key, value) -> {
if (value instanceof Policy) {
this.policyMap.put(key, (Policy) value);
} else if (value instanceof Map) {
try {
this.policyMap.put(key, new ObjectMapper().convertValue(value, Policy.class));
} catch (IllegalArgumentException e) {
throw new IllegalStateException("Failed to convert policy map value", e);
}
}
});
}

}
3 changes: 0 additions & 3 deletions ...erver/src/main/java/com/appsmith/server/repositories/ce/ActionCollectionRepositoryCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.appsmith.server.repositories.ce;

import com.appsmith.server.domains.ActionCollection;
import com.appsmith.server.projections.IdPoliciesOnly;
import com.appsmith.server.repositories.BaseRepository;
import com.appsmith.server.repositories.CustomActionCollectionRepository;

Expand All @@ -10,6 +9,4 @@
public interface ActionCollectionRepositoryCE
extends BaseRepository<ActionCollection, String>, CustomActionCollectionRepository {
List<ActionCollection> findByApplicationId(String applicationId);

List<IdPoliciesOnly> findIdsAndPolicyMapByApplicationIdIn(List<String> applicationIds);
}
3 changes: 3 additions & 0 deletions ...src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.appsmith.server.acl.AclPermission;
import com.appsmith.server.domains.ActionCollection;
import com.appsmith.server.domains.User;
import com.appsmith.server.projections.IdPoliciesOnly;
import com.appsmith.server.repositories.AppsmithRepository;
import org.springframework.data.domain.Sort;

Expand Down Expand Up @@ -43,4 +44,6 @@ List<ActionCollection> findByPageIdAndViewMode(

List<ActionCollection> findAllNonComposedByPageIdAndViewMode(
String pageId, boolean viewMode, AclPermission permission, User currentUser);

List<IdPoliciesOnly> findIdsAndPolicyMapByApplicationIdIn(List<String> applicationIds);
}
9 changes: 9 additions & 0 deletions ...main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
import com.appsmith.server.acl.AclPermission;
import com.appsmith.server.constants.FieldName;
import com.appsmith.server.domains.ActionCollection;
import com.appsmith.server.domains.NewAction;
import com.appsmith.server.domains.User;
import com.appsmith.server.helpers.ce.bridge.Bridge;
import com.appsmith.server.helpers.ce.bridge.BridgeQuery;
import com.appsmith.server.projections.IdPoliciesOnly;
import com.appsmith.server.repositories.BaseAppsmithRepositoryImpl;
import org.springframework.data.domain.Sort;

Expand Down Expand Up @@ -181,4 +183,11 @@ public List<ActionCollection> findAllNonComposedByPageIdAndViewMode(
String pageId, boolean viewMode, AclPermission permission, User currentUser) {
return this.findByPageIdAndViewMode(pageId, viewMode, permission, currentUser);
}

@Override
public List<IdPoliciesOnly> findIdsAndPolicyMapByApplicationIdIn(List<String> applicationIds) {
return queryBuilder()
.criteria(Bridge.in(NewAction.Fields.applicationId, applicationIds))
.all(IdPoliciesOnly.class);
}
}
3 changes: 3 additions & 0 deletions ...server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.appsmith.server.acl.AclPermission;
import com.appsmith.server.domains.NewAction;
import com.appsmith.server.domains.User;
import com.appsmith.server.projections.IdPoliciesOnly;
import com.appsmith.server.repositories.AppsmithRepository;
import org.springframework.data.domain.Sort;

Expand Down Expand Up @@ -90,4 +91,6 @@ List<NewAction> findAllPublishedActionsByContextIdAndContextType(
boolean includeJs);

List<NewAction> findAllByApplicationIds(List<String> branchedArtifactIds, List<String> includedFields);

List<IdPoliciesOnly> findIdsAndPolicyMapByApplicationIdIn(List<String> applicationIds);
}
8 changes: 8 additions & 0 deletions ...er/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.appsmith.server.domains.User;
import com.appsmith.server.helpers.ce.bridge.Bridge;
import com.appsmith.server.helpers.ce.bridge.BridgeQuery;
import com.appsmith.server.projections.IdPoliciesOnly;
import com.appsmith.server.repositories.BaseAppsmithRepositoryImpl;
import io.micrometer.observation.ObservationRegistry;
import jakarta.transaction.Transactional;
Expand Down Expand Up @@ -485,4 +486,11 @@ public List<NewAction> findAllByApplicationIds(List<String> applicationIds, List
.fields(includedFields)
.all();
}

@Override
public List<IdPoliciesOnly> findIdsAndPolicyMapByApplicationIdIn(List<String> applicationIds) {
return queryBuilder()
.criteria(Bridge.in(NewAction.Fields.applicationId, applicationIds))
.all(IdPoliciesOnly.class);
}
}
3 changes: 3 additions & 0 deletions ...h-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.appsmith.server.acl.AclPermission;
import com.appsmith.server.domains.NewPage;
import com.appsmith.server.domains.User;
import com.appsmith.server.projections.IdPoliciesOnly;
import com.appsmith.server.repositories.AppsmithRepository;

import java.util.Collection;
Expand Down Expand Up @@ -48,4 +49,6 @@ Optional<NewPage> findPageByBranchNameAndBasePageId(
List<NewPage> findAllByApplicationIdsWithoutPermission(List<String> applicationIds, List<String> includeFields);

Optional<Integer> updateDependencyMap(String pageId, Map<String, List<String>> dependencyMap);

List<IdPoliciesOnly> findIdsAndPolicyMapByApplicationIdIn(List<String> applicationIds);
}
8 changes: 8 additions & 0 deletions ...rver/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import com.appsmith.server.helpers.ce.bridge.BridgeQuery;
import com.appsmith.server.helpers.ce.bridge.BridgeUpdate;
import com.appsmith.server.projections.IdOnly;
import com.appsmith.server.projections.IdPoliciesOnly;
import com.appsmith.server.repositories.BaseAppsmithRepositoryImpl;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
Expand Down Expand Up @@ -263,4 +264,11 @@ public Optional<Integer> updateDependencyMap(String pageId, Map<String, List<Str
update.set(NewPage.Fields.unpublishedPage_dependencyMap, dependencyMap);
return Optional.of(queryBuilder().criteria(q).updateFirst(update));
}

@Override
public List<IdPoliciesOnly> findIdsAndPolicyMapByApplicationIdIn(List<String> applicationIds) {
return queryBuilder()
.criteria(Bridge.in(NewPage.Fields.applicationId, applicationIds))
.all(IdPoliciesOnly.class);
}
}
3 changes: 0 additions & 3 deletions ...smith-server/src/main/java/com/appsmith/server/repositories/ce/NewActionRepositoryCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import com.appsmith.server.domains.NewAction;
import com.appsmith.server.dtos.PluginTypeAndCountDTO;
import com.appsmith.server.newactions.projections.IdAndDatasourceIdNewActionView;
import com.appsmith.server.projections.IdPoliciesOnly;
import com.appsmith.server.repositories.BaseRepository;
import com.appsmith.server.repositories.CustomNewActionRepository;
import org.springframework.data.jpa.repository.Query;
Expand All @@ -21,8 +20,6 @@ public interface NewActionRepositoryCE extends BaseRepository<NewAction, String>

Optional<Long> countByDeletedAtNull();

List<IdPoliciesOnly> findIdsAndPolicyMapByApplicationIdIn(List<String> applicationIds);

List<IdAndDatasourceIdNewActionView> findIdAndDatasourceIdByApplicationIdIn(List<String> applicationIds);

@Query(
Expand Down
3 changes: 0 additions & 3 deletions ...ppsmith-server/src/main/java/com/appsmith/server/repositories/ce/NewPageRepositoryCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.appsmith.server.repositories.ce;

import com.appsmith.server.domains.NewPage;
import com.appsmith.server.projections.IdPoliciesOnly;
import com.appsmith.server.repositories.BaseRepository;
import com.appsmith.server.repositories.CustomNewPageRepository;

Expand All @@ -13,6 +12,4 @@ public interface NewPageRepositoryCE extends BaseRepository<NewPage, String>, Cu
List<NewPage> findByApplicationId(String applicationId);

Optional<Long> countByDeletedAtNull();

List<IdPoliciesOnly> findIdsAndPolicyMapByApplicationIdIn(List<String> applicationIds);
}