Skip to content

chore: Dynamic class based projections for nested fields in jsonb column #36988

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
abhvsn merged 6 commits into from
Oct 22, 2024
Merged

chore: Dynamic class based projections for nested fields in jsonb column #36988

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8d16e63
chore: Add class based projections for nested jsonb fields
abhvsn Oct 21, 2024
c0b7010
chore: Minor refactors
abhvsn Oct 21, 2024
c242dae
chore: Refactors and comments to make code readable
abhvsn Oct 21, 2024
0765c47
chore: Throw exception in case of cyclical dependency within projecti…
abhvsn Oct 21, 2024
2629b66
chore: Fix serialisation in UserRecentlyUsedEntitiesProjection
abhvsn Oct 22, 2024
63fb7bf
chore: Add check for out of bound index exception
abhvsn Oct 22, 2024
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
3 changes: 3 additions & 0 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/FieldInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package com.appsmith.server.dtos;

public record FieldInfo(String fullPath, Class<?> type) {}
22 changes: 22 additions & 0 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/AppsmithClassUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package com.appsmith.server.helpers;

public class AppsmithClassUtils {

/**
* This method checks if the given class is a projection class. Projection classes are classes that are used to project data from one form to another.
* @param clazz The class to be checked
* @return True if the class is a projection class, false otherwise
*/
public static boolean isAppsmithProjections(Class<?> clazz) {
return clazz.getPackageName().matches(".*appsmith.*projections");
}

/**
* This method checks if the given class is an Appsmith defined class.
* @param clazz The class to be checked
* @return True if the class is an Appsmith defined class, false otherwise
*/
public static boolean isAppsmithDefinedClass(Class<?> clazz) {
return clazz.getPackageName().startsWith("com.appsmith");
}
}
156 changes: 136 additions & 20 deletions ...erver/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ReflectionHelpers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,60 +1,176 @@
package com.appsmith.server.helpers.ce;

import com.appsmith.server.dtos.FieldInfo;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.springframework.util.CollectionUtils;

import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.Map;

import static com.appsmith.external.helpers.StringUtils.dotted;
import static com.appsmith.server.helpers.AppsmithClassUtils.isAppsmithDefinedClass;
import static com.appsmith.server.helpers.AppsmithClassUtils.isAppsmithProjections;
import static org.modelmapper.internal.util.Primitives.isPrimitiveWrapper;

public class ReflectionHelpers {

private static final ObjectMapper objectMapper = new ObjectMapper();

/**
* Maps a tuple to an object of the given type using the constructor of the type. The order of the tuple should be
* the same as the order of the fields in the type constructor.
* Maps objects to the given type using the constructor of the type. The order of the objects should be the same as
* the order of the fields in the type constructor.
* @param type The type of the object to be created
* @param tuple The tuple to be mapped to the object
* @param tupleTypes The types of the tuple elements. If not provided, the types of the fields of the type are used.
* @param objects The objects to be mapped to the type
* @param tupleTypes The types of the objects elements. If not provided, the types of the fields of the type are
* used.
*
* @return The object of the given type
* @param <T> The type of the object to be created
*/
private static <T> T map(Object[] tuple, Class<T> type, List<Class<?>> tupleTypes) {
private static <T> T map(ArrayList<Object> objects, Class<T> type, List<Class<?>> tupleTypes) {
Comment thread
abhvsn marked this conversation as resolved.
Outdated
if (CollectionUtils.isEmpty(tupleTypes)) {
tupleTypes = new ArrayList<>();
for (Field field : type.getDeclaredFields()) {
tupleTypes.add(field.getType());
}
tupleTypes = fetchAllFieldTypes(type);
}
try {
Constructor<T> constructor = type.getConstructor(tupleTypes.toArray(new Class<?>[tuple.length]));
return constructor.newInstance(tuple);
// Create a deep copy of the objects
ArrayList<Object> modified = new ArrayList<>(objects.size());
for (Class<?> tupleType : tupleTypes) {
// In case of Appsmith based projection loop through each field to avoid mapping all the fields from
// the entity class
// e.g. class EntityClass {
// private String field1;
// private String field2;
// }
// class ProjectionClass {
// private String field1;
// }
// In the above example, we only need to map field1 from EntityClass to ProjectionClass. In the objects
// we expect only field1 value to be present.
if (isAppsmithProjections(tupleType)) {
modified.add(map(objects, tupleType, null));
} else {
Object value =
objects.get(0) != null && (isCollectionType(tupleType) || isAppsmithDefinedClass(tupleType))
? objectMapper.readValue(objects.get(0).toString(), tupleType)
: objects.get(0);
modified.add(value);
// Drop the first element from objects as it has been processed
objects.remove(0);
}
Comment thread
abhvsn marked this conversation as resolved.
Outdated
}
Constructor<T> constructor = type.getConstructor(tupleTypes.toArray(new Class<?>[0]));
return constructor.newInstance(modified.toArray());
Comment thread
abhvsn marked this conversation as resolved.
Comment thread
abhvsn marked this conversation as resolved.
} catch (Exception e) {
throw new RuntimeException(e);
}
}

public static <T> T map(Object[] tuple, Class<T> type) {
return map(tuple, type, null);
/**
* Maps a row from the database to an object of the given type using the constructor of the type.
* @param row The row to be mapped to the object
* @param type The type of the object to be created
*
* @return The object of the given type
* @param <T> The type of the object to be created
*/
public static <T> T map(Object[] row, Class<T> type) {
ArrayList<Object> update = new ArrayList<>(Arrays.asList(row));
return map(update, type, null);
}

/**
* Maps a list of tuples to a list of objects of the given type using the constructor of the type.
* @param type The type of the object to be created
* @param clazz The type of the object to be created
* @param records The list of tuples to be mapped to the objects
*
* @return The list of objects of the given type
* @param <T> The type of the object to be created
*/
public static <T> List<T> map(List<Object[]> records, Class<T> type) {
public static <T> List<T> map(List<Object[]> records, Class<T> clazz) {
List<T> result = new ArrayList<>();
List<Class<?>> tupleTypes = new ArrayList<>();
for (Field field : type.getDeclaredFields()) {
tupleTypes.add(field.getType());
}
// In case of multiple records avoid fetching the field types for each record
List<Class<?>> fieldTypes = fetchAllFieldTypes(clazz);
for (Object[] record : records) {
result.add(map(record, type, tupleTypes));
ArrayList<Object> update = new ArrayList<>(Arrays.asList(record));
result.add(map(update, clazz, fieldTypes));
}
return result;
}

/**
* Fetches all the field types of a class and its superclasses.
* @param clazz The class whose field types are to be fetched
*
* @return The list of field types
*/
private static List<Class<?>> fetchAllFieldTypes(Class<?> clazz) {
List<Class<?>> tupleTypes = new ArrayList<>();

// Traverse the class hierarchy to get fields from the class and its superclasses
while (clazz != null) {
// Get declared fields from the current class
for (Field field : clazz.getDeclaredFields()) {
// Ensure access to private fields
field.setAccessible(true);
tupleTypes.add(field.getType());
}
// Move to the superclass
clazz = clazz.getSuperclass();
}

return tupleTypes;
}

/**
* Check if the class is a Java container class e.g. List, Set, Map etc.
* @param clazz The class to be checked
*
* @return True if the class is a container class, false otherwise
*/
private static boolean isCollectionType(Class<?> clazz) {
// Check if the class is a subtype of Collection or Map
return Collection.class.isAssignableFrom(clazz) || Map.class.isAssignableFrom(clazz);
}

/**
* Extracts all the field paths along-with the field type of the projection class.
* @param projectionClass The projection class whose field paths are to be extracted
*
* @return The list of field paths
*/
public static List<FieldInfo> extractFieldPaths(Class<?> projectionClass) {
List<FieldInfo> fieldPaths = new ArrayList<>();
extractFieldPathsRecursively(projectionClass, "", fieldPaths);
return fieldPaths;
}

private static void extractFieldPathsRecursively(Class<?> clazz, String parentPath, List<FieldInfo> fieldPaths) {
// 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);

if (!isAppsmithDefinedClass(field.getType()) || !isAppsmithProjections(field.getType())) {
// 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));
}
} else {
Comment thread
abhvsn marked this conversation as resolved.
Outdated
// Recursively extract nested fields for complex types
extractFieldPathsRecursively(field.getType(), fullPath, fieldPaths);
}
}
// Move to superclass (if any)
clazz = clazz.getSuperclass();
}
}
Comment thread
abhvsn marked this conversation as resolved.
Outdated
Comment on lines +149 to +189

@coderabbitai coderabbitai Bot Oct 22, 2024 •
edited
Loading

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

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.

Suggested change
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);
}

@abhvsn abhvsn Oct 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's better to fail early in such scenario so that devs can come up with better datastructure.

@coderabbitai coderabbitai Bot Oct 22, 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.

@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.

}
2 changes: 1 addition & 1 deletion ...rver/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/bridge/BridgeQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ public BridgeQuery<T> jsonIn(@NonNull String needle, @NonNull String key) {
return this;
}

private static <R> Expression<R> keyToExpression(
public static <R> Expression<R> keyToExpression(
@NonNull Class<R> type, @NonNull Root<?> root, @NonNull CriteriaBuilder cb, @NonNull String key) {
if (key.contains(".")) {
final List<String> parts = List.of(key.split("\\."));
Expand Down
24 changes: 16 additions & 8 deletions ...erver/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.appsmith.server.acl.AclPermission;
import com.appsmith.server.constants.FieldName;
import com.appsmith.server.domains.User;
import com.appsmith.server.dtos.FieldInfo;
import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException;
import com.appsmith.server.helpers.ce.bridge.Bridge;
Expand Down Expand Up @@ -62,7 +63,9 @@

import static com.appsmith.external.helpers.ReflectionHelpers.getAllFields;
import static com.appsmith.server.constants.FieldName.PERMISSION_GROUPS;
import static com.appsmith.server.helpers.ce.ReflectionHelpers.extractFieldPaths;
import static com.appsmith.server.helpers.ce.ReflectionHelpers.map;
import static com.appsmith.server.helpers.ce.bridge.BridgeQuery.keyToExpression;

/**
* In case you are wondering why we have two different repository implementation classes i.e.
Expand Down Expand Up @@ -297,8 +300,13 @@ public <P> List<P> queryAllExecute(QueryAllParams<T> params, Class<P> projection

if (!projectionClass.getSimpleName().equals(genericDomain.getSimpleName())) {
List<Selection<?>> projectionFields = new ArrayList<>();
// TODO: Nested fields are not supported yet.
getAllFields(projectionClass).forEach(f -> projectionFields.add(root.get(f.getName())));
// Extract all field paths dynamically from the projection class
// Map of projection field path to the class type
List<FieldInfo> fieldPaths = extractFieldPaths(projectionClass);

for (FieldInfo fieldInfo : fieldPaths) {
projectionFields.add(keyToExpression(fieldInfo.type(), root, cb, fieldInfo.fullPath()));
}
cq.multiselect(projectionFields);
}

Expand All @@ -309,11 +317,11 @@ public <P> List<P> queryAllExecute(QueryAllParams<T> params, Class<P> projection
}

return Mono.fromSupplier(query::getResultList)
.map(tuple -> {
.map(rows -> {
if (genericDomain.getSimpleName().equals(projectionClass.getSimpleName())) {
return (List<P>) tuple;
return (List<P>) rows;
}
return map((List<Object[]>) tuple, projectionClass);
return map((List<Object[]>) rows, projectionClass);
})
.onErrorResume(NoResultException.class, e -> Mono.just(Collections.emptyList()));
})
Expand Down Expand Up @@ -371,11 +379,11 @@ public <P> Optional<P> queryOneExecute(QueryAllParams<T> params, Class<P> projec
}

return Mono.fromSupplier(entityManager.createQuery(cq)::getSingleResult)
.map(tuple -> {
.map(row -> {
if (genericDomain.getSimpleName().equals(projectionClass.getSimpleName())) {
return (P) tuple;
return (P) row;
}
return map((Object[]) tuple, projectionClass);
return map((Object[]) row, projectionClass);
Comment thread
abhvsn marked this conversation as resolved.
})
.onErrorResume(NoResultException.class, e -> Mono.empty());
})
Expand Down