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.

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

chore: Transaction PG - Insert/Save and Read ops #37313

Closed
wants to merge 1 commit into from
Closed

chore: Transaction PG - Insert/Save and Read ops #37313

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
273 changes: 89 additions & 184 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/TransactionAspect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
import com.appsmith.external.models.BaseDomain;
import com.appsmith.server.helpers.RepositoryFactory;
import com.appsmith.server.helpers.ce.bridge.BridgeUpdate;
import com.appsmith.server.repositories.AppsmithRepository;
import com.appsmith.server.repositories.cakes.BaseCake;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.Setter;
Expand All @@ -17,17 +15,11 @@
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;

import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
import java.util.Map;
import java.util.Optional;
import java.util.UUID;
import java.util.regex.Pattern;

import static com.appsmith.server.constants.ce.FieldNameCE.ARCHIVE;
import static com.appsmith.server.constants.ce.FieldNameCE.DELETE;
import static com.appsmith.server.constants.ce.FieldNameCE.FIND;
import static com.appsmith.server.constants.ce.FieldNameCE.TRANSACTION_CONTEXT;
import static com.appsmith.server.constants.ce.FieldNameCE.*;

@Aspect
@Component
Expand All @@ -37,111 +29,129 @@ public class TransactionAspect {

private final RepositoryFactory repositoryFactory;

Pattern OBJECTID_PATTERN = Pattern.compile("^[0-9a-fA-F]{24}$"); // 24 character hex string
Pattern OBJECTID_PATTERN = Pattern.compile("^[0-9a-fA-F]{24}$");

@Around("execution(* com.appsmith.server.repositories.cakes..*(..))")
public Object handleTransaction(ProceedingJoinPoint joinPoint) throws Throwable {

Class<?> returnType =
((MethodSignature) joinPoint.getSignature()).getMethod().getReturnType();

if (Mono.class.isAssignableFrom(returnType)) {

return Mono.deferContextual(context -> {
try {
// Check if the transaction context is available, if not execute the DB call and return
if (context.isEmpty() || !context.hasKey(TRANSACTION_CONTEXT)) {
return (Mono<?>) joinPoint.proceed(joinPoint.getArgs());
}
Map<String, DBOps> transactionContext = context.get(TRANSACTION_CONTEXT);
// Check if it's a write operation
boolean isWriteOp = isWriteOp((MethodSignature) joinPoint.getSignature());

Map<String, DBOps> transactionContext = context.get(TRANSACTION_CONTEXT);
EntityData entityData = getEntityData(joinPoint.getArgs(), joinPoint.getTarget());
boolean isInsertOp = isWriteOp(entityData, (MethodSignature) joinPoint.getSignature());

// TODO - remove this once the values are consistent with the new method to check write operation
if (isInsertOp != isWriteOp) {
log.error(
"Mismatch in write operation detection. isNewWriteOp: {}, isWriteOp: {}",
isInsertOp,
isWriteOp);
}
// If the operation is read, operation
// 1. Get the object from DB
// 2. Check if the object is already present in the context:
// - If not, store the object in the context and return
// - If yes, then return the object as is. We want to just maintain the initial state of the
// object as we are concerned with the object initial state before the transaction started
// We are considering the previous state of the db,
// because we want to revert the changes if the transaction fails, and we want to store the initial
// state of the object before the transaction started.
// This cleanup is done in the TransactionHandler class in Solution module
if (!isInsertOp) {
return ((Mono<?>) joinPoint.proceed(joinPoint.getArgs()))
.map(obj -> updateContextMapWithReadOperation(transactionContext, obj));
} else {
// If the operation is writing operation
// 1. Extract the id of the object
// 2. Check if the object is already present in the context
// a. If yes, execute the DB call
// b. If not get the initial state of the object from DB using findById method on the
// repository class
// - If end up in switchIfEmpty means no object is present in the DB and should mark this
// as a new object and store the object in DB
// - If object is present in the DB, then store the initial state in the context

BaseDomain domain = entityData.getBaseDomain();
addEntityToContextMap(transactionContext, domain, entityData);
return ((Mono<?>) joinPoint.proceed(joinPoint.getArgs()));
if (isReadOp(entityData, (MethodSignature) joinPoint.getSignature())) {
return handleReadOperation(joinPoint, transactionContext, entityData);
} else if (isInsertOrCreateOp(entityData, (MethodSignature) joinPoint.getSignature())) {
handleInsertOperation(transactionContext, entityData);
return Mono.just(
entityData.getBaseDomain()); // Return entity after persisting it to context map
} else if (isUpdateOp(entityData)) {
return (Mono<?>) joinPoint.proceed(joinPoint.getArgs());
} else if (isDeleteOp((MethodSignature) joinPoint.getSignature())) {
return (Mono<?>) joinPoint.proceed(joinPoint.getArgs());
}

} catch (Throwable e) {
log.error(
"Error while executing the function in the Transaction Aspect {}",
joinPoint.getSignature().getName(),
e);
return Mono.error(e);
}
return Mono.empty();
});
} else if (Flux.class.isAssignableFrom(returnType)) {
return Flux.deferContextual(context -> {
try {
if (!context.isEmpty() && context.hasKey(TRANSACTION_CONTEXT)) {
Map<String, DBOps> transactionContext = context.get(TRANSACTION_CONTEXT);
EntityData entityData = getEntityData(joinPoint.getArgs(), joinPoint.getTarget());

boolean isInsertOp = isWriteOp(entityData, (MethodSignature) joinPoint.getSignature());

Flux flux = (Flux<?>) joinPoint.proceed(joinPoint.getArgs());

if (!isInsertOp) {
return flux.map(obj -> updateContextMapWithReadOperation(transactionContext, obj));
} else {
BaseDomain domain = entityData.getBaseDomain();
addEntityToContextMap(transactionContext, domain, entityData);
return flux;
}
if (context.isEmpty() || !context.hasKey(TRANSACTION_CONTEXT)) {
return (Flux<?>) joinPoint.proceed(joinPoint.getArgs());
}

return (Flux<?>) joinPoint.proceed(joinPoint.getArgs());
Map<String, DBOps> transactionContext = context.get(TRANSACTION_CONTEXT);
EntityData entityData = getEntityData(joinPoint.getArgs(), joinPoint.getTarget());

if (isReadOp(entityData, (MethodSignature) joinPoint.getSignature())) {
return handleReadOperationFlux(joinPoint, transactionContext, entityData);
} else if (isInsertOrCreateOp(entityData, (MethodSignature) joinPoint.getSignature())) {
handleInsertOperation(transactionContext, entityData);
return Flux.just(
entityData.getBaseDomain()); // Return entity after persisting it to context map
} else if (isUpdateOp(entityData)) {
return (Flux<?>) joinPoint.proceed(joinPoint.getArgs());
} else if (isDeleteOp((MethodSignature) joinPoint.getSignature())) {
return (Flux<?>) joinPoint.proceed(joinPoint.getArgs());
}
} catch (Throwable e) {
log.error(
"Error while executing the function in the Transaction Aspect {}",
joinPoint.getSignature().getName(),
e);
return Flux.error(e);
}
return Flux.empty();
});
}
return joinPoint.proceed(joinPoint.getArgs());
}

private EntityData getEntityData(Object[] args, Object target) {
// To store the baseDomain and the id of the object and BridgeUpdate
// when the BaseDomain is not present, in the case of updateById methods
private Mono<?> handleReadOperation(
ProceedingJoinPoint joinPoint, Map<String, DBOps> transactionContext, EntityData entityData)
throws Throwable {
return ((Mono<?>) joinPoint.proceed(joinPoint.getArgs()))
.switchIfEmpty(Mono.defer(() -> {
String id = entityData.getId();
return Mono.justOrEmpty(transactionContext.get(id)).map(DBOps::getEntity);
}))
.map(obj -> updateContextMapWithReadOperation(transactionContext, obj));
}

private Flux<?> handleReadOperationFlux(
ProceedingJoinPoint joinPoint, Map<String, DBOps> transactionContext, EntityData entityData)
throws Throwable {
return ((Flux<?>) joinPoint.proceed(joinPoint.getArgs()))
.switchIfEmpty(Flux.defer(() -> {
String id = entityData.getId();
return Flux.justOrEmpty(transactionContext.get(id)).map(DBOps::getEntity);
}))
.map(obj -> updateContextMapWithReadOperation(transactionContext, obj));
}

private void handleInsertOperation(Map<String, DBOps> transactionContext, EntityData entityData) {
BaseDomain domain = entityData.getBaseDomain();
DBOps dbOps = new DBOps();
dbOps.setEntity(domain);
dbOps.setNew(true);
transactionContext.put(domain.getId(), dbOps);
}

private boolean isReadOp(EntityData entityData, MethodSignature signature) {
String methodName = signature.getMethod().getName();
return methodName.startsWith(FIND) || methodName.startsWith(GET);
}

private boolean isInsertOrCreateOp(EntityData entityData, MethodSignature signature) {
String methodName = signature.getMethod().getName();
return (methodName.contains("save") || methodName.contains("saveAll"))
&& (entityData.getBaseDomain() != null
&& entityData.getBaseDomain().getId() == null);
}

private boolean isUpdateOp(EntityData entityData) {
return entityData.getUpdate() instanceof BridgeUpdate;
}

private boolean isDeleteOp(MethodSignature signature) {
String methodName = signature.getMethod().getName();
return methodName.contains(DELETE);
}

private EntityData getEntityData(Object[] args, Object target) {
EntityData entityData = new EntityData();
for (Object arg : args) {
if (arg instanceof BaseDomain domain) {
Expand All @@ -154,22 +164,14 @@ private EntityData getEntityData(Object[] args, Object target) {
}

if (entityData.getBaseDomain() != null) {
// When the update method is called with id not present in baseDomain
if (entityData.getBaseDomain().getId() == null && entityData.getId() != null) {
entityData.getBaseDomain().setId(entityData.getId());
} else if (entityData.getBaseDomain().getId() == null && entityData.getId() == null) {
// When the object is new and not present in the DB, we need to generate the id
entityData.getBaseDomain().setId(generateId());
entityData.setId(entityData.getBaseDomain().getId());
} else if (entityData.getId() == null) {
entityData.setId(entityData.getBaseDomain().getId());
}
} else if (entityData.getUpdate() != null || entityData.getId() != null) {
// When the updateById is called with the BridgeUpdate object or
// When the id is passed as a parameter to the method for archiveById or deleteById
Object entityClass = createEntityTypeDomainObject(getEntityClassForBridgeUpdate(target));
entityData.setBaseDomain((BaseDomain) entityClass);
entityData.getBaseDomain().setId(entityData.getId());
}
return entityData;
}
Expand All @@ -188,7 +190,6 @@ private boolean isUUIDString(String id) {
}
}

// To Support the mongo _ids as well
private boolean isObjectIdString(String id) {
return OBJECTID_PATTERN.matcher(id).matches();
}
Expand All @@ -199,124 +200,28 @@ private Object updateContextMapWithReadOperation(Map<String, DBOps> transactionC
dbOps.setEntity(obj);
dbOps.setModified(false);
String id = getObjectId(dbOps);
if (!transactionContext.containsKey(id)) {
transactionContext.put(id, dbOps);
}
return obj;
transactionContext.putIfAbsent(id, dbOps);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add null check before updating transactionContext

Ensure id is not null before adding to transactionContext to prevent potential NullPointerException.

Apply this change:

-    transactionContext.putIfAbsent(id, dbOps);
+    if (id != null) {
+        transactionContext.putIfAbsent(id, dbOps);
+    }
📝 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
transactionContext.putIfAbsent(id, dbOps);
if (id != null) {
transactionContext.putIfAbsent(id, dbOps);
}

}
return obj;
}

private void addEntityToContextMap(Map<String, DBOps> transactionContext, Object obj, EntityData entityData) {
AppsmithRepository<?> repository = getDomainClassFromObject(entityData.getBaseDomain());
if (repository == null) {
log.error(" Unable to find the repository for the entity {}", obj.getClass());
return;
}
DBOps dbOps = new DBOps();
dbOps.setEntity(obj);
if (transactionContext.containsKey(getObjectId(dbOps))) {
return;
}

Optional<?> entity = repository.getById(((BaseDomain) obj).getId());
dbOps = new DBOps();
if (entity.isPresent()) {
dbOps.setEntity(entity.get());
dbOps.setModified(true);
} else {
dbOps.setEntity(obj);
dbOps.setNew(true);
}
transactionContext.put(getObjectId(dbOps), dbOps);
}

private boolean isWriteOp(EntityData entityData, MethodSignature signature) {
// Special case like findCustomJsLib accepting the BaseDomain object as parameter instead of the UUID,
// hence the need to check for method name as well
if (entityData.getBaseDomain() != null
&& isSaveOrCreateOp(entityData.getBaseDomain())
&& !signature.getMethod().getName().contains(FIND)) {
return true;
} else if (entityData.getUpdate() != null && isUpdateOp(entityData.getUpdate())) {
return true;
} else return isArchiveOp(signature);
}

private boolean isSaveOrCreateOp(Object obj) {
return obj instanceof BaseDomain;
}

private boolean isUpdateOp(Object obj) {
return obj instanceof BridgeUpdate;
}

private boolean isArchiveOp(MethodSignature signature) {
return signature.getMethod().getName().contains(ARCHIVE)
|| signature.getMethod().getName().contains(DELETE);
}

// TODO - remove this method, used only for testing the validity of the new method
private boolean isWriteOp(MethodSignature signature) {
String methodName = signature.getMethod().getName();
// save/create instance of BaseDomain
// update instance of BridgeUpdate
// archive is archive
return methodName.contains("save")
|| methodName.contains("update")
|| methodName.contains("delete")
|| methodName.contains("insert")
|| methodName.contains("archive");
}

private AppsmithRepository<?> getDomainClassFromObject(Object object) {
return repositoryFactory.getRepositoryFromEntity(object);
}

private String getObjectId(DBOps obj) {
return obj != null && obj.getEntity() instanceof BaseDomain ? ((BaseDomain) obj.getEntity()).getId() : null;
}

private Class<?> getEntityClassForBridgeUpdate(Object target) {
Class<?> targetClass = target.getClass();

if (BaseCake.class.isAssignableFrom(targetClass)) {
Type genericSuperclass = targetClass.getGenericSuperclass();
if (genericSuperclass instanceof ParameterizedType) {
ParameterizedType parameterizedType = (ParameterizedType) genericSuperclass;
Type[] actualTypeArguments = parameterizedType.getActualTypeArguments();
if (actualTypeArguments.length > 0) {
Class<?> entityClass = (Class<?>) actualTypeArguments[0];
return entityClass;
}
}
}
log.error(" No entity class exists for the given target {}", target.getClass());
return null;
}

private Object createEntityTypeDomainObject(Class<?> entityClass) {
try {
return entityClass.getDeclaredConstructor().newInstance();
} catch (Exception e) {
log.error("Error while creating the entity object for the class {}", entityClass, e);
}
return new BaseDomain() {};
@Getter
@Setter
public static class EntityData {
private BaseDomain baseDomain;
private BridgeUpdate update;
private String id;
}

@Getter
@Setter
public static class DBOps {
private Object entity;
private boolean isModified;
private boolean isNew;
}

@Getter
@Setter
public static class EntityData {
private BaseDomain baseDomain;
private String id;
private BridgeUpdate update;
private boolean isModified;
}
}
4 changes: 4 additions & 0 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,4 +214,8 @@ public class FieldNameCE {
public static final String ARCHIVE = "archive";

public static final String DELETE = "delete";

public static final String GET = "get";

public static final String SAVE = "save";
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove duplicate constant SAVE.

The constant SAVE duplicates the existing CREATE constant (line 217) with the same value "save". Having two constants for the same operation can lead to inconsistent usage.

Apply this diff to remove the duplicate:

-    public static final String SAVE = "save";

And update any new code to use the existing CREATE constant.

📝 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 final String SAVE = "save";

}
Loading