fix: Add custom deserialiser for idPolicies projection #38127
Conversation
WalkthroughThe changes in this pull request involve the transformation of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/projections/IdPoliciesOnly.java (1)
11-14: Make fields final and consider builder patternClass structure could be improved for immutability.
@Getter public class IdPoliciesOnly { - String id; - Map<String, Policy> policyMap = new HashMap<>(); + private final String id; + private final Map<String, Policy> policyMap = new HashMap<>();app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCE.java (1)
47-48: Add JavaDoc for the new repository methodDocument the method's purpose, parameters, and return value.
+ /** + * Retrieves ID and policies for collections matching the given application IDs + * @param applicationIds List of application IDs to search for + * @return List of ID and policy mappings + */ List<IdPoliciesOnly> findIdsAndPolicyMapByApplicationIdIn(List<String> applicationIds);app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java (1)
52-53: Maintain consistent documentation across repositoriesAdd JavaDoc similar to CustomActionCollectionRepositoryCE.
+ /** + * Retrieves ID and policies for pages matching the given application IDs + * @param applicationIds List of application IDs to search for + * @return List of ID and policy mappings + */ List<IdPoliciesOnly> findIdsAndPolicyMapByApplicationIdIn(List<String> applicationIds);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/server/appsmith-server/src/main/java/com/appsmith/server/projections/IdPoliciesOnly.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/ActionCollectionRepositoryCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/NewActionRepositoryCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/NewPageRepositoryCE.java(0 hunks)
💤 Files with no reviewable changes (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/NewActionRepositoryCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/NewPageRepositoryCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/ActionCollectionRepositoryCE.java
👮 Files not reviewed due to content moderation or server errors (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java (1)
52-53: Verify consistent implementation across repositories
Ensure all repositories implement the new method consistently.
✅ Verification successful
Implementation is consistent across repositories
The search results show the method findIdsAndPolicyMapByApplicationIdIn is consistently implemented in:
- CustomNewPageRepositoryCEImpl
- CustomNewActionRepositoryCEImpl
- CustomActionCollectionRepositoryCEImpl
All implementations follow the same pattern using queryBuilder() with appropriate criteria. The method is also properly defined in corresponding interfaces.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all implementations of findIdsAndPolicyMapByApplicationIdIn
ast-grep --pattern 'findIdsAndPolicyMapByApplicationIdIn($$$)'
Length of output: 130877
Script:
#!/bin/bash
# Search for implementations of findIdsAndPolicyMapByApplicationIdIn in Java files
rg -t java "findIdsAndPolicyMapByApplicationIdIn" -C 3
# Search for interface definitions and implementations
ast-grep --pattern 'interface $_ {
$$$
findIdsAndPolicyMapByApplicationIdIn($$$)
$$$
}'
Length of output: 13194
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (2)
10-10: LGTM!
The import is correctly placed and used by the new method.
490-495: LGTM! Clean and consistent implementation.
The method follows the repository pattern and maintains consistency with existing query patterns in the codebase.
Let's verify the interface method declaration and IdPoliciesOnly projection usage:
✅ Verification successful
Implementation verified and consistent across the codebase
The method is properly declared in the interface and follows the same pattern used in related repositories (CustomNewPageRepository, CustomActionCollectionRepository). The IdPoliciesOnly projection is a valid class with proper id and policyMap fields, and the method is actively used in ApplicationPageService and ApplicationForkingService.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the method is declared in the interface
ast-grep --pattern 'interface CustomNewActionRepositoryCE {
$$$
List<IdPoliciesOnly> findIdsAndPolicyMapByApplicationIdIn(List<String> $_);
$$$
}'
# Check IdPoliciesOnly projection implementation
ast-grep --pattern 'class IdPoliciesOnly {
$$$
}'
Length of output: 208
Script:
#!/bin/bash
# Search for interface declaration with ripgrep
rg -l "interface\s+CustomNewActionRepositoryCE" --type java
# Search for IdPoliciesOnly class or interface
rg -l "(?:class|interface)\s+IdPoliciesOnly" --type java
# If files are found, check their content
rg "(?:class|interface)\s+CustomNewActionRepositoryCE" -A 10 --type java
rg "(?:class|interface)\s+IdPoliciesOnly" -A 10 --type java
Length of output: 6994
Script:
#!/bin/bash
# Let's check if the method is declared in the interface
rg "findIdsAndPolicyMapByApplicationIdIn" app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java -A 2
# Check for any other implementations or usages of this method
rg "findIdsAndPolicyMapByApplicationIdIn" --type java
Length of output: 2751
| // 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)); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Handle ObjectMapper conversion exceptions and add parameter validation
The constructor contains potential issues:
- ObjectMapper conversion could throw exceptions
- Missing validation for id parameter
- 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.
| // 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); | |
| } | |
| } | |
| }); | |
| } |
Failed server tests
|
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Description
PR to move projection methods with IdPoliciesOnly class to custom repository.
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.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation