chore: Dynamic class based projections for nested fields in jsonb column#36988
chore: Dynamic class based projections for nested fields in jsonb column#36988
Conversation
abhvsn
requested review from
a team,
AnaghHegde,
ApekshaBhosale,
NilanshBansal,
nidhi-nair,
pratapaprasanna,
sagar-qa007,
sharat87 and
sondermanish
as code owners
github-actions
bot
added
the
skip-changelog
WalkthroughThe changes introduce new functionalities and enhancements across several classes in the Appsmith server. A new record Changes
Suggested labels
Suggested reviewers
Poem
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: 3
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/AppsmithClassUtils.java (2)
3-3: Add a private constructor to prevent instantiation.As this is a utility class with only static methods, consider adding a private constructor to prevent instantiation.
public class AppsmithClassUtils { + private AppsmithClassUtils() { + // Prevent instantiation + } // ... existing methods }
5-7: Consider using a more specific regex pattern.The current regex might match unintended package names. A more specific pattern could ensure better accuracy.
- return clazz.getPackageName().matches(".*appsmith.*projections"); + return clazz.getPackageName().matches(".*appsmith\\..*\\.projections");This change ensures "appsmith" and "projections" are separate path components.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/FieldInfo.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/AppsmithClassUtils.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/bridge/BridgeQuery.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/FieldInfo.java (1)
1-3: LGTM! Excellent use of Java record for FieldInfo.The introduction of the
FieldInforecord is a clean and efficient way to represent field metadata. Using a record here provides automatic implementations ofequals(),hashCode(), andtoString(), which is perfect for this use case.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/AppsmithClassUtils.java (1)
9-11: LGTM!The method is well-named and correctly implemented.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/bridge/BridgeQuery.java (1)
240-240: Visibility change:keyToExpressionmethod is now publicThe visibility of the
keyToExpressionmethod has been changed from private to public. This change allows external access to the method, which could potentially impact how it's used across the application.Please ensure that:
- This change is intentional and necessary.
- The method's documentation (if any) is updated to reflect its public nature.
- Any security implications of exposing this method have been considered.
✅ Verification successful
Visibility change:
keyToExpressionmethod is now publicThe
keyToExpressionmethod has been made public to allow external access, as evidenced by its usage inBaseAppsmithRepositoryCEImpl.java.Please ensure that:
- This change is intentional and necessary for the application's functionality.
- The method's documentation has been updated to reflect its public accessibility.
- Any potential security implications of exposing this method have been reviewed and addressed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of keyToExpression method outside the BridgeQuery class rg "BridgeQuery\.keyToExpression" --type javaLength of output: 239
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (2)
11-11: Import statements are appropriateThe added imports for
FieldInfo,extractFieldPaths, andmapare necessary and correctly included.Also applies to: 66-66, 68-68
303-309: Dynamic field path extraction implemented correctlyThe use of
extractFieldPathsandkeyToExpressionto handle nested fields in projections is well-integrated.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (1)
303-309: LGTM: Improved dynamic field handling for projections.The changes enhance the flexibility of query construction by dynamically extracting field paths and using
FieldInfoobjects. This approach better supports nested fields and complex projections.Consider adding a comment explaining the purpose of the
extractFieldPathsmethod for better code documentation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/AppsmithClassUtils.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/AppsmithClassUtils.java
🧰 Additional context used
🔇 Additional comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (2)
11-11: LGTM: New imports for improved field handling.The new imports for
FieldInfo,extractFieldPaths, and the modified import forkeyToExpressionare appropriate for the changes implemented in the class.Also applies to: 66-66, 68-68
Line range hint
1-824: Overall assessment: Improved flexibility and robustness.The changes in this file significantly enhance the handling of dynamic field paths and projections in the repository implementation. The use of
FieldInfoand dynamic field path extraction improves support for nested fields and complex projections. While there are minor opportunities for improvement in documentation and code structure, the overall changes are well-implemented and consistent.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java (2)
160-161: Correct Logical Condition inextractFieldPathsRecursivelyThe condition may not work as intended. Use logical AND (
&&) instead of OR (||) to proceed when the field type is neither an Appsmith-defined class nor an Appsmith projection.
152-175: Add Safeguard AgainstStackOverflowErrorConsider implementing a recursion depth limit or tracking visited classes in
extractFieldPathsRecursivelyto prevent a potentialStackOverflowError.
...h-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java
Outdated
Show resolved
Hide resolved
Failed server tests
|
abhvsn
removed request for
a team,
AnaghHegde,
ApekshaBhosale,
NilanshBansal,
nidhi-nair,
pratapaprasanna,
sagar-qa007,
sharat87 and
sondermanish
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java (1)
146-186: Improve cyclic dependency error messageThe cyclic dependency detection is a good safeguard. Consider improving the error message to include the field name that caused the cycle.
Suggested improvement:
- throw new RuntimeException("Cyclical dependency detected for: " + cyclicChain); + throw new RuntimeException("Cyclical dependency detected at field '" + fieldName + "' in chain: " + cyclicChain);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/FieldInfo.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/AppsmithClassUtils.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/FieldInfo.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/AppsmithClassUtils.java
🧰 Additional context used
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java (5)
3-18: LGTM: Import statements and class declarationThe new imports and static imports are appropriate for the added functionality and improve code readability.
81-83: LGTM: UpdatedmapmethodThe changes are consistent with the modifications in the private
mapmethod and improve type safety.
94-102: LGTM: Optimizedmapmethod for multiple recordsThe changes improve performance by fetching field types only once for all records. The implementation is consistent with other modifications.
111-127: LGTM: NewfetchAllFieldTypesmethodThe method correctly traverses the class hierarchy and handles private fields, providing a robust way to fetch all field types.
135-138: LGTM: NewisCollectionTypemethodThe method correctly checks for Collection and Map types, providing a useful utility for type checking.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java
Show resolved
Hide resolved
Failed server tests
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11452370160. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/RecentlyUsedEntityDTO.java (0 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/projections/UserRecentlyUsedEntitiesProjection.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserDataRepositoryCEImpl.java (1 hunks)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/RecentlyUsedEntityDTO.java
🧰 Additional context used
🔇 Additional comments (9)
app/server/appsmith-server/src/main/java/com/appsmith/server/projections/UserRecentlyUsedEntitiesProjection.java (3)
10-11: Approved: Change from record to class enhances flexibility.The transition from a record to a class with
@Getterannotation maintains the immutability while allowing for custom constructor logic.
12-12: LGTM: Proper field initialization.Good use of interface for declaration and concrete class for initialization.
14-23: Consider exception handling and future refactoring.The constructor logic looks sound, but consider these points:
- Handle potential exceptions from ObjectMapper conversion.
- The TODO comment suggests a need for refactoring. Consider creating a ticket for this.
To ensure all usages are updated, run:
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java (5)
3-23: LGTM: Import statements and class setup look good.The new imports and static final
ObjectMapperare appropriate for the added functionality.
81-83: LGTM: Consistent with privatemapmethod changes.The use of
ArrayListaligns with the modifications in the privatemapmethod.
94-103: LGTM: Good performance optimization.Fetching field types once for multiple records is an excellent optimization, especially for large datasets.
111-127: LGTM: Comprehensive field type fetching.The method correctly handles the class hierarchy and private fields.
135-138: LGTM: Correct implementation for checking collection types.The method accurately identifies Collection and Map types.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserDataRepositoryCEImpl.java (1)
73-75: Code change looks goodThe method correctly retrieves the most recently used workspace ID.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java
Show resolved
Hide resolved
| public static List<FieldInfo> extractFieldPaths(Class<?> projectionClass) { | ||
| List<FieldInfo> fieldPaths = new ArrayList<>(); | ||
| List<Class<?>> visitedClasses = new ArrayList<>(); | ||
| extractFieldPathsRecursively(projectionClass, "", fieldPaths, visitedClasses); | ||
| return fieldPaths; | ||
| } | ||
|
|
||
| private static void extractFieldPathsRecursively( | ||
| Class<?> clazz, String parentPath, List<FieldInfo> fieldPaths, List<Class<?>> visitedClasses) { | ||
| // Check if the class has already been visited to prevent cyclic dependencies | ||
| if (visitedClasses.contains(clazz)) { | ||
| String cyclicChain = String.join( | ||
| " -> ", visitedClasses.stream().map(Class::getName).toArray(String[]::new)); | ||
| throw new RuntimeException("Cyclical dependency detected for: " + cyclicChain); | ||
| } | ||
| // Process the class and its superclasses | ||
| while (clazz != null && clazz != Object.class) { | ||
| for (Field field : clazz.getDeclaredFields()) { | ||
| field.setAccessible(true); // Ensure access to private fields | ||
| String fieldName = field.getName(); | ||
| String fullPath = parentPath.isEmpty() ? fieldName : dotted(parentPath, fieldName); | ||
| Class<?> fieldType = field.getType(); | ||
|
|
||
| if (isAppsmithProjections(fieldType)) { | ||
| visitedClasses.add(fieldType); | ||
| extractFieldPathsRecursively(field.getType(), fullPath, fieldPaths, visitedClasses); | ||
| } else { | ||
| // Check if the field type is part of JdbcType.getDdlTypeCode if not assign the Object as the type | ||
| if (isPrimitiveWrapper(field.getType()) || String.class.equals(field.getType())) { | ||
| fieldPaths.add(new FieldInfo(fullPath, field.getType())); | ||
| } else { | ||
| fieldPaths.add(new FieldInfo(fullPath, Object.class)); | ||
| } | ||
| } | ||
| } | ||
| // Move to superclass (if any) | ||
| clazz = clazz.getSuperclass(); | ||
| } | ||
| // Remove the class from the visited set to allow revisiting in different branches | ||
| visitedClasses.remove(clazz); | ||
| } |
There was a problem hiding this comment.
Consider more graceful handling of cyclic dependencies
While the cyclic dependency detection is good, throwing a runtime exception might be too severe. Consider logging a warning and skipping the cyclic path instead.
Suggested modification:
- throw new RuntimeException("Cyclical dependency detected for: " + cyclicChain);
+ System.err.println("Warning: Cyclical dependency detected for: " + cyclicChain);
+ return;📝 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.
| public static List<FieldInfo> extractFieldPaths(Class<?> projectionClass) { | |
| List<FieldInfo> fieldPaths = new ArrayList<>(); | |
| List<Class<?>> visitedClasses = new ArrayList<>(); | |
| extractFieldPathsRecursively(projectionClass, "", fieldPaths, visitedClasses); | |
| return fieldPaths; | |
| } | |
| private static void extractFieldPathsRecursively( | |
| Class<?> clazz, String parentPath, List<FieldInfo> fieldPaths, List<Class<?>> visitedClasses) { | |
| // Check if the class has already been visited to prevent cyclic dependencies | |
| if (visitedClasses.contains(clazz)) { | |
| String cyclicChain = String.join( | |
| " -> ", visitedClasses.stream().map(Class::getName).toArray(String[]::new)); | |
| throw new RuntimeException("Cyclical dependency detected for: " + cyclicChain); | |
| } | |
| // Process the class and its superclasses | |
| while (clazz != null && clazz != Object.class) { | |
| for (Field field : clazz.getDeclaredFields()) { | |
| field.setAccessible(true); // Ensure access to private fields | |
| String fieldName = field.getName(); | |
| String fullPath = parentPath.isEmpty() ? fieldName : dotted(parentPath, fieldName); | |
| Class<?> fieldType = field.getType(); | |
| if (isAppsmithProjections(fieldType)) { | |
| visitedClasses.add(fieldType); | |
| extractFieldPathsRecursively(field.getType(), fullPath, fieldPaths, visitedClasses); | |
| } else { | |
| // Check if the field type is part of JdbcType.getDdlTypeCode if not assign the Object as the type | |
| if (isPrimitiveWrapper(field.getType()) || String.class.equals(field.getType())) { | |
| fieldPaths.add(new FieldInfo(fullPath, field.getType())); | |
| } else { | |
| fieldPaths.add(new FieldInfo(fullPath, Object.class)); | |
| } | |
| } | |
| } | |
| // Move to superclass (if any) | |
| clazz = clazz.getSuperclass(); | |
| } | |
| // Remove the class from the visited set to allow revisiting in different branches | |
| visitedClasses.remove(clazz); | |
| } | |
| public static List<FieldInfo> extractFieldPaths(Class<?> projectionClass) { | |
| List<FieldInfo> fieldPaths = new ArrayList<>(); | |
| List<Class<?>> visitedClasses = new ArrayList<>(); | |
| extractFieldPathsRecursively(projectionClass, "", fieldPaths, visitedClasses); | |
| return fieldPaths; | |
| } | |
| private static void extractFieldPathsRecursively( | |
| Class<?> clazz, String parentPath, List<FieldInfo> fieldPaths, List<Class<?>> visitedClasses) { | |
| // Check if the class has already been visited to prevent cyclic dependencies | |
| if (visitedClasses.contains(clazz)) { | |
| String cyclicChain = String.join( | |
| " -> ", visitedClasses.stream().map(Class::getName).toArray(String[]::new)); | |
| System.err.println("Warning: Cyclical dependency detected for: " + cyclicChain); | |
| return; | |
| } | |
| // Process the class and its superclasses | |
| while (clazz != null && clazz != Object.class) { | |
| for (Field field : clazz.getDeclaredFields()) { | |
| field.setAccessible(true); // Ensure access to private fields | |
| String fieldName = field.getName(); | |
| String fullPath = parentPath.isEmpty() ? fieldName : dotted(parentPath, fieldName); | |
| Class<?> fieldType = field.getType(); | |
| if (isAppsmithProjections(fieldType)) { | |
| visitedClasses.add(fieldType); | |
| extractFieldPathsRecursively(field.getType(), fullPath, fieldPaths, visitedClasses); | |
| } else { | |
| // Check if the field type is part of JdbcType.getDdlTypeCode if not assign the Object as the type | |
| if (isPrimitiveWrapper(field.getType()) || String.class.equals(field.getType())) { | |
| fieldPaths.add(new FieldInfo(fullPath, field.getType())); | |
| } else { | |
| fieldPaths.add(new FieldInfo(fullPath, Object.class)); | |
| } | |
| } | |
| } | |
| // Move to superclass (if any) | |
| clazz = clazz.getSuperclass(); | |
| } | |
| // Remove the class from the visited set to allow revisiting in different branches | |
| visitedClasses.remove(clazz); | |
| } |
There was a problem hiding this comment.
It's better to fail early in such scenario so that devs can come up with better datastructure.
There was a problem hiding this comment.
@abhvsn, understood. Failing early in such scenarios makes sense to encourage better data structures. Thanks for clarifying.
(_/)
( -_-) thinking...
✏️ Learnings added
Learnt from: abhvsn
PR: appsmithorg/appsmith#36988
File: app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java:149-189
Timestamp: 2024-10-22T03:15:41.280Z
Learning: In methods like `extractFieldPathsRecursively` in `ReflectionHelpers.java`, when detecting cyclic dependencies, we prefer to fail early by throwing a `RuntimeException` to encourage developers to design better data structures.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Deploy-Preview-URL: https://ce-36988.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java (2)
35-70: LGTM: Improved object mapping logic.The new implementation handles Appsmith projections and performs deep copying, enhancing flexibility. Consider using
List<Object>instead ofArrayList<Object>in the method signature for better abstraction.
149-189: LGTM: New methods for extracting field paths.The
extractFieldPathsandextractFieldPathsRecursivelymethods provide crucial functionality for working with projection classes. However, consider logging a warning instead of throwing an exception for cyclic dependencies to allow for more graceful handling.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java (5)
3-4: LGTM: New imports added for enhanced functionality.The new imports support the added features in the class.
Also applies to: 10-11, 13-18
84-86: LGTM: Updated to use new mapping logic.The changes are consistent with the updates to the main
mapmethod.
97-105: LGTM: Performance improvement in mapping multiple records.Fetching field types once for all records reduces redundant operations. Good optimization!
114-130: LGTM: New method for fetching field types.The
fetchAllFieldTypesmethod correctly handles class hierarchy, supporting the improved mapping logic.
138-141: LGTM: Utility method for checking collection types.The
isCollectionTypemethod provides a clean way to identify collection types, supporting the mapping logic.
1 participant
Description
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 Sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11452551547
Commit: 63fb7bf
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 22 Oct 2024 03:51:40 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
FieldInforecord for better representation of field metadata.AppsmithClassUtilsfor class type identification.ReflectionHelperswith new methods for extracting field paths and improved tuple mapping.keyToExpressionmethod public inBridgeQueryfor broader access.BaseAppsmithRepositoryCEImplto dynamically handle field paths.UserRecentlyUsedEntitiesProjectionto a class structure with enhanced constructor logic.CustomUserDataRepositoryCEImplfor accessing and modifying recently used entities.Bug Fixes