From 7e9eb58f03d2c88e4e8c328fd50862cf859230ec Mon Sep 17 00:00:00 2001 From: Sumit Kumar Date: Tue, 26 Apr 2022 16:51:30 +0530 Subject: [PATCH] fix: fix action objects missing plugin id and plugin type info in database (#13263) * add plugin id and type info to action object if found missing * this fix is currently added to the read and update action flows --- .../services/ce/NewActionServiceCE.java | 1 + .../services/ce/NewActionServiceCEImpl.java | 116 +++++++++++++++--- .../ce/NewActionServiceCEImplTest.java | 65 ++++++++++ 3 files changed, 168 insertions(+), 14 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/NewActionServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/NewActionServiceCE.java index 13599dbe02..4e1c531ed6 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/NewActionServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/NewActionServiceCE.java @@ -99,4 +99,5 @@ public interface NewActionServiceCE extends CrudService { Mono findBranchedIdByBranchNameAndDefaultActionId(String branchName, String defaultActionId, AclPermission permission); + public Mono sanitizeAction(NewAction action); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/NewActionServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/NewActionServiceCEImpl.java index 836b49e2c5..853e690700 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/NewActionServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/NewActionServiceCEImpl.java @@ -120,6 +120,8 @@ public class NewActionServiceCEImpl extends BaseService { // In case of external datasource (not embedded) instead of storing the entire datasource // again inside the action, instead replace it with just the datasource ID. This is so that @@ -1100,7 +1102,8 @@ public class NewActionServiceCEImpl extends BaseService findUnpublishedOnLoadActionsExplicitSetByUserInPage(String pageId) { return repository - .findUnpublishedActionsByPageIdAndExecuteOnLoadSetByUserTrue(pageId, MANAGE_ACTIONS); + .findUnpublishedActionsByPageIdAndExecuteOnLoadSetByUserTrue(pageId, MANAGE_ACTIONS) + .flatMap(this::sanitizeAction); } /** @@ -1113,27 +1116,32 @@ public class NewActionServiceCEImpl extends BaseService findUnpublishedActionsInPageByNames(Set names, String pageId) { return repository - .findUnpublishedActionsByNameInAndPageId(names, pageId, MANAGE_ACTIONS); + .findUnpublishedActionsByNameInAndPageId(names, pageId, MANAGE_ACTIONS) + .flatMap(this::sanitizeAction); } @Override public Mono findById(String id) { - return repository.findById(id); + return repository.findById(id) + .flatMap(this::sanitizeAction); } @Override public Mono findById(String id, AclPermission aclPermission) { - return repository.findById(id, aclPermission); + return repository.findById(id, aclPermission) + .flatMap(this::sanitizeAction); } @Override public Flux findByPageId(String pageId, AclPermission permission) { - return repository.findByPageId(pageId, permission); + return repository.findByPageId(pageId, permission) + .flatMap(this::sanitizeAction); } @Override public Flux findByPageIdAndViewMode(String pageId, Boolean viewMode, AclPermission permission) { - return repository.findByPageIdAndViewMode(pageId, viewMode, permission); + return repository.findByPageIdAndViewMode(pageId, viewMode, permission) + .flatMap(this::sanitizeAction); } @Override @@ -1151,7 +1159,8 @@ public class NewActionServiceCEImpl extends BaseService repository.findByApplicationIdAndViewMode(application.getId(), false, READ_ACTIONS)) + .flatMap(this::sanitizeAction) .flatMap(this::setTransientFieldsInUnpublishedAction); } return repository.findAllActionsByNameAndPageIdsAndViewMode(name, pageIds, false, READ_ACTIONS, sort) + .flatMap(this::sanitizeAction) .flatMap(this::setTransientFieldsInUnpublishedAction); } @@ -1365,6 +1376,75 @@ public class NewActionServiceCEImpl extends BaseService !PluginType.JS.equals(actionDTO.getPluginType())); } + /** + * This method is meant to be used to check for any missing or bad values in NewAction object and attempt to fix it. + * + * This method is added in response to certain cases where it was found that pluginId and pluginType keys + * were missing from the NewAction object in the database.Since it is currently not know what exactly causes + * these values to go missing, this check will serve as a workaround by fetching and setting pluginId and + * pluginType using the datasource object contained in the ActionDTO object. + * Ref: https://github.com/appsmithorg/appsmith/issues/11927 + * + */ + public Mono sanitizeAction(NewAction action) { + Mono actionMono = Mono.just(action); + if (isPluginTypeOrPluginIdMissing(action)) { + actionMono = providePluginTypeAndIdToNewActionObjectUsingJSTypeOrDatasource(action); + } + + return actionMono; + } + + private boolean isPluginTypeOrPluginIdMissing(NewAction action) { + return action.getPluginId() == null || action.getPluginType() == null; + } + + private Mono providePluginTypeAndIdToNewActionObjectUsingJSTypeOrDatasource(NewAction action) { + ActionDTO actionDTO = action.getUnpublishedAction(); + if (actionDTO == null) { + return Mono.just(action); + } + + /** + * if path: + * In case an action object is related to a JS Object then it must have a non-null collectionId. + * + * else path: + * Otherwise, check if the datasource object has the pluginId. If so, use this pluginId to fetch the correct + * pluginType. + */ + Datasource datasource = actionDTO.getDatasource(); + if (actionDTO.getCollectionId() != null) { + return setPluginIdAndTypeForJSAction(action); + } + else if (datasource != null && datasource.getPluginId() != null) { + String pluginId = datasource.getPluginId(); + action.setPluginId(pluginId); + + return setPluginTypeFromId(action, pluginId); + } + + return Mono.just(action); + } + + private Mono setPluginTypeFromId(NewAction action, String pluginId) { + return pluginService.findById(pluginId) + .flatMap(plugin -> { + action.setPluginType(plugin.getType()); + return Mono.just(action); + }); + } + + private Mono setPluginIdAndTypeForJSAction(NewAction action) { + action.setPluginType(JS_PLUGIN_TYPE); + + return pluginService.findByPackageName(JS_PLUGIN_PACKAGE_NAME) + .flatMap(plugin -> { + action.setPluginId(plugin.getId()); + return Mono.just(action); + }); + } + // We can afford to make this call all the time since we already have all the info we need in context private Mono getRemoteDatasourceContext(Plugin plugin, Datasource datasource) { final DatasourceContext datasourceContext = new DatasourceContext(); @@ -1388,7 +1468,9 @@ public class NewActionServiceCEImpl extends BaseService repository.save(sanitizedAction)); } @Override @@ -1396,12 +1478,17 @@ public class NewActionServiceCEImpl extends BaseService action.getGitSyncId() == null) .forEach(action -> action.setGitSyncId(action.getApplicationId() + "_" + Instant.now().toString())); - return repository.saveAll(actions); + + return Flux.fromIterable(actions) + .flatMap(this::sanitizeAction) + .collectList() + .flatMapMany(actionList -> repository.saveAll(actionList)); } @Override public Flux findByPageId(String pageId) { - return repository.findByPageId(pageId); + return repository.findByPageId(pageId) + .flatMap(this::sanitizeAction); } /** @@ -1642,7 +1729,8 @@ public class NewActionServiceCEImpl extends BaseService findBranchedIdByBranchNameAndDefaultActionId(String branchName, String defaultActionId, AclPermission permission) { diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/NewActionServiceCEImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/NewActionServiceCEImplTest.java index 87c141b66a..f1f809d037 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/NewActionServiceCEImplTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/NewActionServiceCEImplTest.java @@ -1,8 +1,13 @@ package com.appsmith.server.services.ce; import com.appsmith.external.models.ActionExecutionResult; +import com.appsmith.external.models.Datasource; import com.appsmith.server.acl.PolicyGenerator; import com.appsmith.server.constants.FieldName; +import com.appsmith.server.domains.NewAction; +import com.appsmith.server.domains.Plugin; +import com.appsmith.server.domains.PluginType; +import com.appsmith.server.dtos.ActionDTO; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.PluginExecutorHelper; @@ -23,6 +28,7 @@ import lombok.extern.slf4j.Slf4j; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.Mockito; import org.springframework.boot.test.mock.mockito.MockBean; import org.springframework.core.codec.ByteBufferDecoder; import org.springframework.core.codec.StringDecoder; @@ -56,6 +62,12 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.spy; + @RunWith(SpringRunner.class) @Slf4j @@ -215,4 +227,57 @@ public class NewActionServiceCEImplTest { .verify(); } + @Test + public void testMissingPluginIdAndTypeFixForNonJSPluginType() { + /* Mock `findById` method of pluginService to return `testPlugin` */ + Plugin testPlugin = new Plugin(); + testPlugin.setId("testId"); + testPlugin.setType(PluginType.DB); + Mockito.when(pluginService.findById(anyString())) + .thenReturn(Mono.just(testPlugin)); + + NewAction action = new NewAction(); + action.setPluginId(null); + action.setPluginType(null); + ActionDTO actionDTO = new ActionDTO(); + Datasource datasource = new Datasource(); + /* Datasource has correct plugin id */ + datasource.setPluginId(testPlugin.getId()); + actionDTO.setDatasource(datasource); + action.setUnpublishedAction(actionDTO); + + Mono updatedActionFlux = newActionService.sanitizeAction(action); + StepVerifier.create(updatedActionFlux) + .assertNext(updatedAction -> { + assertEquals("testId", updatedAction.getPluginId()); + assertEquals(PluginType.DB, updatedAction.getPluginType()); + }) + .verifyComplete(); + } + + @Test + public void testMissingPluginIdAndTypeFixForJSPluginType() { + /* Mock `findByPackageName` method of pluginService to return `testPlugin` */ + Plugin testPlugin = new Plugin(); + testPlugin.setId("testId"); + testPlugin.setType(PluginType.JS); + Mockito.when(pluginService.findByPackageName(anyString())) + .thenReturn(Mono.just(testPlugin)); + + NewAction action = new NewAction(); + action.setPluginId(null); + action.setPluginType(null); + ActionDTO actionDTO = new ActionDTO(); + /* Non-null collection id to indicate a JS action */ + actionDTO.setCollectionId("testId"); + action.setUnpublishedAction(actionDTO); + + Mono updatedActionFlux = newActionService.sanitizeAction(action); + StepVerifier.create(updatedActionFlux) + .assertNext(updatedAction -> { + assertEquals("testId", updatedAction.getPluginId()); + assertEquals(PluginType.JS, updatedAction.getPluginType()); + }) + .verifyComplete(); + } } \ No newline at end of file