Protei/PromucFlow_constructor
2
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

Fixing the move action API by removing invocations to subscribe (#17)

Browse Source 
* Fixing the move action API by removing invocations to subscribe

Calling subscribe() inside function calls is an anti-pattern and we shouldn't be doing it.
The reactiveContext is not called if the subscribe() function is called in the middle of execution flows. This breaks DB queries.

* Added test case for move action.

Co-authored-by: Trisha Anand <trisha@appsmith.com>
This commit is contained in:
Arpit Mohan 2020-07-02 15:41:45 +05:30 committed by GitHub
parent 5df9395003
commit fdeed757ff
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 56 additions and 92 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
app/server/appsmith-server/src/main/java/com/appsmith/server/services/LayoutActionServiceImpl.java
View File

@ -217,30 +217,23 @@ public class LayoutActionServiceImpl implements LayoutActionService {
.switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, actionMoveDTO.getAction().getId())))
.flatMap(savedAction -> pageService
.findById(oldPageId, MANAGE_PAGES)
.map(page -> {
.flatMap(page -> {
if (page.getLayouts() == null) {
return Mono.empty();
}
return page.getLayouts()
.stream()
/*
* subscribe() is being used here because within a stream, the master subscriber provided
* by spring framework does not get attached here leading to the updateLayout mono not
* emitting. The same is true for the updateLayout call for the new page.
*/
.map(layout -> updateLayout(oldPageId, layout.getId(), layout).subscribe())
return Flux.fromIterable(page.getLayouts())
.flatMap(layout -> updateLayout(oldPageId, layout.getId(), layout))
.collect(toSet());
})
.then(pageService.findById(actionMoveDTO.getDestinationPageId(), MANAGE_PAGES))
.map(page -> {
.flatMap(page -> {
if (page.getLayouts() == null) {
return Mono.empty();
}
return page.getLayouts()
.stream()
.map(layout -> updateLayout(actionMoveDTO.getDestinationPageId(), layout.getId(), layout).subscribe())
return Flux.fromIterable(page.getLayouts())
.flatMap(layout -> updateLayout(actionMoveDTO.getDestinationPageId(), layout.getId(), layout))
.collect(toSet());
})
.thenReturn(savedAction));

129
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionServiceTest.java
View File

@ -16,6 +16,7 @@ import com.appsmith.server.domains.Organization;
import com.appsmith.server.domains.Page;
import com.appsmith.server.domains.Plugin;
import com.appsmith.server.domains.User;
import com.appsmith.server.dtos.ActionMoveDTO;
import com.appsmith.server.dtos.ExecuteActionDTO;
import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException;
@ -43,6 +44,7 @@ import reactor.test.StepVerifier;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.UUID;
import static com.appsmith.server.acl.AclPermission.MANAGE_ACTIONS;
import static com.appsmith.server.acl.AclPermission.READ_ACTIONS;
@ -83,12 +85,13 @@ public class ActionServiceTest {
@Autowired
ObjectMapper objectMapper;
@Autowired
LayoutActionService layoutActionService;
Application testApp = null;
Page testPage = null;
int i = 0;
Datasource datasource;
@Before
@ -102,8 +105,7 @@ public class ActionServiceTest {
if (testApp == null && testPage == null) {
//Create application and page which will be used by the tests to create actions for.
Application application = new Application();
application.setName("ActionServiceTest-App-" + String.valueOf(i));
i++;
application.setName(UUID.randomUUID().toString());
testApp = applicationPageService.createApplication(application, organization.getId()).block();
testPage = pageService.getById(testApp.getPages().get(0).getId()).block();
}
@ -158,8 +160,51 @@ public class ActionServiceTest {
.verifyComplete();
}
@Test
@WithUserDetails(value = "api_user")
public void validMoveAction() {
Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(new MockPluginExecutor()));
@Test
Page newPage = new Page();
newPage.setName("Destination Page");
newPage.setApplicationId(testApp.getId());
Page destinationPage = applicationPageService.createPage(newPage).block();
Action action = new Action();
action.setName("validAction");
action.setPageId(testPage.getId());
ActionConfiguration actionConfiguration = new ActionConfiguration();
actionConfiguration.setHttpMethod(HttpMethod.GET);
action.setActionConfiguration(actionConfiguration);
action.setDatasource(datasource);
Mono<Action> createActionMono = actionService.create(action).cache();
Mono<Action> movedActionMono = createActionMono
.flatMap(savedAction -> {
ActionMoveDTO actionMoveDTO = new ActionMoveDTO();
actionMoveDTO.setAction(savedAction);
actionMoveDTO.setDestinationPageId(destinationPage.getId());
return layoutActionService.moveAction(actionMoveDTO);
});
StepVerifier
.create(Mono.zip(createActionMono, movedActionMono))
.assertNext(tuple -> {
Action originalAction = tuple.getT1();
Action movedAction = tuple.getT2();
assertThat(movedAction.getId()).isEqualTo(originalAction.getId());
assertThat(movedAction.getName()).isEqualTo(originalAction.getName());
assertThat(movedAction.getPolicies()).containsAll(originalAction.getPolicies());
assertThat(movedAction.getPageId()).isEqualTo(destinationPage.getId());
})
.verifyComplete();
}
@Test
@WithUserDetails(value = "api_user")
public void createValidActionWithJustName() {
Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(new MockPluginExecutor()));
@ -376,72 +421,6 @@ public class ActionServiceTest {
executeAndAssertAction(executeActionDTO, actionConfiguration, mockResult);
}
// @Test
// @WithUserDetails(value = "api_user")
// public void testActionExecuteDuplicateRequestHeader() {
// ActionExecutionResult mockResult = new ActionExecutionResult();
// mockResult.setIsExecutionSuccess(true);
// mockResult.setBody("response-body");
//
// Action action = new Action();
// ActionConfiguration actionConfiguration = new ActionConfiguration();
// actionConfiguration.setHeaders(List.of(
// new Property("random-header-key", "random-header-value"),
// new Property("dup-key", "dup-value1"),
// new Property("DUP-key", "dup-value2")
// ));
// action.setActionConfiguration(actionConfiguration);
//
// ExecuteActionDTO executeActionDTO = new ExecuteActionDTO();
// executeActionDTO.setAction(action);
//
// Mono<ActionExecutionResult> executionResultMono = executeAction(executeActionDTO, actionConfiguration, mockResult);
// StepVerifier.create(executionResultMono)
// .assertNext(result -> {
// // Assert that the fxn should pick up the latest key based on the case-insensitive values
// ActionExecutionRequest request = result.getRequest();
// assertThat(request.getRequestHeaders().size()).isEqualTo(2);
// MultiValueMap<String, String> resultHeader = CollectionUtils.toMultiValueMap(Map.of(
// "random-header-key", Arrays.asList("random-header-value"),
// "DUP-key", Arrays.asList("dup-value2")));
// assertThat(request.getRequestHeaders()).isEqualTo(objectMapper.valueToTree(resultHeader));
// })
// .verifyComplete();
// }
// @Test
// @WithUserDetails(value = "api_user")
// public void testActionExecuteEmptyRequestHeader() {
// ActionExecutionResult mockResult = new ActionExecutionResult();
// mockResult.setIsExecutionSuccess(true);
// mockResult.setBody("response-body");
//
// Action action = new Action();
// ActionConfiguration actionConfiguration = new ActionConfiguration();
// actionConfiguration.setHeaders(List.of(
// new Property("random-header-key", "random-header-value"),
// new Property("", "")
// ));
// action.setActionConfiguration(actionConfiguration);
//
// ExecuteActionDTO executeActionDTO = new ExecuteActionDTO();
// executeActionDTO.setAction(action);
//
// Mono<ActionExecutionResult> executionResultMono = executeAction(executeActionDTO, actionConfiguration, mockResult);
// StepVerifier.create(executionResultMono)
// .assertNext(result -> {
// // The fxn should have ignored all duplicate headers
// ActionExecutionRequest request = result.getRequest();
// assertThat(request.getRequestHeaders().size()).isEqualTo(1);
// MultiValueMap<String, String> resultHeader = CollectionUtils.toMultiValueMap(Map.of(
// "random-header-key", Arrays.asList("random-header-value")));
//
// assertThat(request.getRequestHeaders()).isEqualTo(objectMapper.valueToTree(resultHeader));
// })
// .verifyComplete();
// }
@Test
@WithUserDetails(value = "api_user")
public void testActionExecuteErrorResponse() {
@ -483,14 +462,6 @@ public class ActionServiceTest {
.assertNext(result -> {
assertThat(result).isNotNull();
assertThat(result.getBody()).isEqualTo(mockResult.getBody());
// ActionExecutionRequest request = result.getRequest();
// if (actionConfiguration.getHeaders() != null) {
// assertThat(request.getRequestHeaders().size()).isEqualTo(actionConfiguration.getHeaders().size());
// }
//
// assertThat(request.getRequestBody() == actionConfiguration.getBody()).isTrue();
// assertThat(result.getHeaders()).isEqualTo(mockResult.getHeaders());
})
.verifyComplete();
}