From 4e20b4c496606d88af65dd51919f0a5ab7043b0e Mon Sep 17 00:00:00 2001 From: subratadeypappu Date: Sat, 19 Nov 2022 09:49:36 +0600 Subject: [PATCH] Fix JS object duplicate name issue (#18198) --- .../ce/LayoutActionServiceCEImpl.java | 18 +++++++++++ .../services/LayoutActionServiceTest.java | 32 +++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java index 9958ee4c15..3e241540e2 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java @@ -9,6 +9,7 @@ import com.appsmith.external.helpers.MustacheHelper; import com.appsmith.external.models.ActionDTO; import com.appsmith.external.models.Datasource; import com.appsmith.external.models.DefaultResources; +import com.appsmith.external.models.PluginType; import com.appsmith.server.constants.FieldName; import com.appsmith.server.domains.ActionDependencyEdge; import com.appsmith.server.domains.Layout; @@ -405,6 +406,23 @@ public class LayoutActionServiceCEImpl implements LayoutActionServiceCE { Mono> actionNamesInPageMono = newActionService .getUnpublishedActions(params) + .flatMap( + actionDTO -> { + /* + This is unexpected. Every action inside a JS collection should have a collectionId. + But there are a few documents found for plugin type JS inside newAction collection that don't have any collectionId. + The reason could be due to the lack of transactional behaviour when multiple inserts/updates that take place + during JS action creation. A detailed RCA is documented here + https://www.notion.so/appsmith/RCA-JSObject-name-already-exists-Please-use-a-different-name-e09c407f0ddb4653bd3974f3703408e6 + */ + if (actionDTO.getPluginType().equals(PluginType.JS) && !StringUtils.hasLength(actionDTO.getCollectionId())) { + log.debug("JS Action with Id: {} doesn't have any collection Id under pageId: {}", actionDTO.getId(), pageId); + return Mono.empty(); + } else { + return Mono.just(actionDTO); + } + } + ) .map(ActionDTO::getValidName) .collect(toSet()); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java index 239bb8a445..4ffb69e243 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/LayoutActionServiceTest.java @@ -1147,4 +1147,36 @@ public class LayoutActionServiceTest { } + @Test + @WithUserDetails(value = "api_user") + public void jsActionWithoutCollectionIdShouldBeIgnoredDuringNameChecking() { + ActionDTO firstAction = new ActionDTO(); + firstAction.setPluginType(PluginType.JS); + firstAction.setName("foo"); + firstAction.setFullyQualifiedName("testCollection.foo"); + firstAction.setCollectionId("collectionId"); + + ActionDTO secondAction = new ActionDTO(); + secondAction.setPluginType(PluginType.JS); + secondAction.setName("bar"); + secondAction.setFullyQualifiedName("testCollection.bar"); + secondAction.setCollectionId(null); + + + Mockito.doReturn(Flux.just(firstAction, secondAction)).when(newActionService).getUnpublishedActions(Mockito.any()); + + ActionCollectionDTO mockActionCollectionDTO = new ActionCollectionDTO(); + mockActionCollectionDTO.setName("testCollection"); + mockActionCollectionDTO.setActions(List.of(firstAction, secondAction)); + + Mockito.when(actionCollectionService.getActionCollectionsByViewMode(Mockito.any(), Mockito.anyBoolean())) + .thenReturn(Flux.just(mockActionCollectionDTO)); + + Mono nameAllowedMono = layoutActionService.isNameAllowed(testPage.getId(), testPage.getLayouts().get(0).getId(), "testCollection.bar"); + + StepVerifier.create(nameAllowedMono) + .assertNext(Assertions::assertTrue) + .verifyComplete(); + } + }