Gracefully handle the timeout overflow / out of expected range errors (…

…#3411)

- Expect max value of timeout as 60000 ms.
- If value exceeds max value then add error message to the list of invalids. This list is returned to the client in response body.
- Detect integer overflow exception (Number format exception) and override the value to 60000 ms.

app/server/appsmith-interfaces/pom.xml

@@ -115,6 +115,11 @@
  <artifactId>assertj-core</artifactId>
  <scope>test</scope>
  </dependency>
+ <dependency>
+ <groupId>org.hibernate.validator</groupId>
+ <artifactId>hibernate-validator</artifactId>
+ <version>6.0.18.Final</version>
+ </dependency>
 
  </dependencies>
 

app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/ActionConfiguration.java

@@ -4,9 +4,11 @@
 import lombok.NoArgsConstructor;
 import lombok.Setter;
 import lombok.ToString;
+import org.hibernate.validator.constraints.Range;
 import org.springframework.data.mongodb.core.mapping.Document;
 import org.springframework.http.HttpMethod;
 
+import java.sql.Statement;
 import java.util.List;
 
 import static com.appsmith.external.constants.ActionConstants.DEFAULT_ACTION_EXECUTION_TIMEOUT_MS;
@@ -17,6 +19,10 @@
 @NoArgsConstructor
 @Document
 public class ActionConfiguration {
+ private static final int MIN_TIMEOUT_VALUE = 0; // in Milliseconds
+ private static final int MAX_TIMEOUT_VALUE = 60000; // in Milliseconds
+ private static final String TIMEOUT_OUT_OF_RANGE_MESSAGE = "'Query timeout' field must be an integer between "
+ + MIN_TIMEOUT_VALUE + " and " + MAX_TIMEOUT_VALUE;
  /*
  * Any of the fields mentioned below could be represented in mustache
  * template. If the mustache template is found, it would be replaced
@@ -26,6 +32,9 @@ public class ActionConfiguration {
  * action execution.
  */
 
+ @Range(min=MIN_TIMEOUT_VALUE,
+ max=MAX_TIMEOUT_VALUE,
+ message=TIMEOUT_OUT_OF_RANGE_MESSAGE)
  Integer timeoutInMillisecond;
  PaginationType paginationType = PaginationType.NONE;
 
@@ -56,6 +65,17 @@ public class ActionConfiguration {
  */
  List<Property> pluginSpecifiedTemplates;
 
+ public void setTimeoutInMillisecond(String timeoutInMillisecond) {
+ try {
+ this.timeoutInMillisecond = Integer.valueOf(timeoutInMillisecond);
+ }
+ catch (NumberFormatException e) {
+ System.out.println("Failed to convert timeout request parameter to Integer. Setting it to max valid " +
+ "value.");
+ this.timeoutInMillisecond = MAX_TIMEOUT_VALUE;
+ }
+ }
+
  public Integer getTimeoutInMillisecond() {
  return (timeoutInMillisecond == null || timeoutInMillisecond <= 0) ?
  DEFAULT_ACTION_EXECUTION_TIMEOUT_MS : timeoutInMillisecond;

app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ActionController.java

@@ -62,7 +62,7 @@ public Mono<ResponseDTO<ActionDTO>> createAction(@Valid @RequestBody ActionDTO r
  }
 
  @PutMapping("/{id}")
- public Mono<ResponseDTO<ActionDTO>> updateAction(@PathVariable String id, @RequestBody ActionDTO resource) {
+ public Mono<ResponseDTO<ActionDTO>> updateAction(@PathVariable String id, @Valid @RequestBody ActionDTO resource) {
  log.debug("Going to update resource with id: {}", id);
  return actionCollectionService.updateAction(id, resource)
  .map(updatedResource -> new ResponseDTO<>(HttpStatus.OK.value(), updatedResource, null));

app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog.java

@@ -1866,4 +1866,32 @@ public void fixDynamicBindingPathListForExistingActions(MongoTemplate mongoTempl
  }
  }
  }
+
+ @ChangeSet(order = "057", id = "update-database-action-configuration-timeout", author = "")
+ public void updateActionConfigurationTimeout(MongoTemplate mongoTemplate) {
+
+ for (NewAction action : mongoTemplate.findAll(NewAction.class)) {
+ boolean updateTimeout = false;
+
+ if (action.getUnpublishedAction() != null
+ && action.getUnpublishedAction().getActionConfiguration() != null
+ && action.getUnpublishedAction().getActionConfiguration().getTimeoutInMillisecond() != null
+ && action.getUnpublishedAction().getActionConfiguration().getTimeoutInMillisecond() > 60000) {
+ action.getUnpublishedAction().getActionConfiguration().setTimeoutInMillisecond("60000");
+ updateTimeout = true;
+ }
+
+ if (action.getPublishedAction() != null
+ && action.getPublishedAction().getActionConfiguration() != null
+ && action.getPublishedAction().getActionConfiguration().getTimeoutInMillisecond() != null
+ && action.getPublishedAction().getActionConfiguration().getTimeoutInMillisecond() > 60000) {
+ action.getPublishedAction().getActionConfiguration().setTimeoutInMillisecond("60000");
+ updateTimeout = true;
+ }
+
+ if(updateTimeout) {
+ mongoTemplate.save(action);
+ }
+ }
+ }
 }

app/server/appsmith-server/src/main/java/com/appsmith/server/services/NewActionServiceImpl.java

@@ -14,6 +14,7 @@
 import com.appsmith.external.models.Param;
 import com.appsmith.external.models.Policy;
 import com.appsmith.external.models.Provider;
+import com.appsmith.external.models.QActionConfiguration;
 import com.appsmith.external.plugins.PluginExecutor;
 import com.appsmith.server.acl.AclPermission;
 import com.appsmith.server.acl.PolicyGenerator;
@@ -283,6 +284,14 @@ private Mono<ActionDTO> validateAndSaveActionToRepository(NewAction newAction) {
  .flatMap(savedAction -> generateActionByViewMode(savedAction, false));
  }
 
+ // Validate actionConfiguration
+ ActionConfiguration actionConfig = action.getActionConfiguration();
+ if(actionConfig != null) {
+ validator.validate(actionConfig)
+ .stream()
+ .forEach(x -> invalids.add(x.getMessage()));
+ }
+
  Mono<Datasource> datasourceMono;
  if (action.getDatasource().getId() == null) {
  if (action.getDatasource().getDatasourceConfiguration() != null &&

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

@@ -589,7 +589,7 @@ public void testActionExecuteSecondaryStaleConnection() {
  new Property("random-header-key", "random-header-value"),
  new Property("", "")
  ));
- actionConfiguration.setTimeoutInMillisecond(1000);
+ actionConfiguration.setTimeoutInMillisecond(String.valueOf(1000));
  action.setActionConfiguration(actionConfiguration);
  action.setPageId(testPage.getId());
  action.setName("testActionExecuteSecondaryStaleConnection");
@@ -630,7 +630,7 @@ public void testActionExecuteTimeout() {
  new Property("random-header-key", "random-header-value"),
  new Property("", "")
  ));
- actionConfiguration.setTimeoutInMillisecond(10);
+ actionConfiguration.setTimeoutInMillisecond(String.valueOf(10));
  action.setActionConfiguration(actionConfiguration);
  action.setPageId(testPage.getId());
  action.setName("testActionExecuteTimeout");
@@ -676,7 +676,7 @@ public void checkActionInViewMode() {
  action1.setPageId(testPage.getId());
  ActionConfiguration actionConfiguration1 = new ActionConfiguration();
  actionConfiguration1.setHttpMethod(HttpMethod.GET);
- actionConfiguration1.setTimeoutInMillisecond(20000);
+ actionConfiguration1.setTimeoutInMillisecond(String.valueOf(20000));
  action1.setActionConfiguration(actionConfiguration1);
  action1.setDatasource(datasource);
 
@@ -1029,4 +1029,142 @@ public void testExecuteOnLoadParamOnActionCreateWithClonePageContext() {
  })
  .verifyComplete();
  }
+
+ @Test
+ @WithUserDetails(value = "api_user")
+ public void testUpdateActionWithOutOfRangeTimeout() {
+ Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(new MockPluginExecutor()));
+
+ ActionDTO action = new ActionDTO();
+ action.setName("testAction");
+ action.setPageId(testPage.getId());
+ ActionConfiguration actionConfiguration = new ActionConfiguration();
+ actionConfiguration.setTimeoutInMillisecond("60001");
+ action.setActionConfiguration(actionConfiguration);
+ action.setDatasource(datasource);
+
+ Mono<ActionDTO> newActionMono = newActionService
+ .createAction(action);
+
+ Mono<ActionDTO> updateActionMono = newActionMono
+ .flatMap(preUpdateAction -> {
+ ActionDTO actionUpdate = action;
+ actionUpdate.getActionConfiguration().setBody("New Body");
+ return actionCollectionService.updateAction(preUpdateAction.getId(), actionUpdate);
+ });
+
+ StepVerifier
+ .create(updateActionMono)
+ .assertNext(updatedAction -> {
+ assertThat(updatedAction).isNotNull();
+ assertThat(updatedAction
+ .getInvalids()
+ .stream()
+ .anyMatch(errorMsg -> errorMsg.contains("'Query timeout' field must be an integer between" +
+ " 0 and 60000"))
+ ).isTrue();
+ })
+ .verifyComplete();
+ }
+
+ @Test
+ @WithUserDetails(value = "api_user")
+ public void testUpdateActionWithValidRangeTimeout() {
+ Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(new MockPluginExecutor()));
+
+ ActionDTO action = new ActionDTO();
+ action.setName("testAction");
+ action.setPageId(testPage.getId());
+ ActionConfiguration actionConfiguration = new ActionConfiguration();
+ actionConfiguration.setTimeoutInMillisecond("6000");
+ action.setActionConfiguration(actionConfiguration);
+ action.setDatasource(datasource);
+
+ Mono<ActionDTO> newActionMono = newActionService
+ .createAction(action);
+
+ Mono<ActionDTO> updateActionMono = newActionMono
+ .flatMap(preUpdateAction -> {
+ ActionDTO actionUpdate = action;
+ actionUpdate.getActionConfiguration().setBody("New Body");
+ return actionCollectionService.updateAction(preUpdateAction.getId(), actionUpdate);
+ });
+
+ StepVerifier
+ .create(updateActionMono)
+ .assertNext(updatedAction -> {
+ assertThat(updatedAction).isNotNull();
+ assertThat(updatedAction
+ .getInvalids()
+ .stream()
+ .anyMatch(errorMsg -> errorMsg.contains("'Query timeout' field must be an integer between"))
+ ).isFalse();
+ })
+ .verifyComplete();
+ }
+
+ @Test
+ @WithUserDetails(value = "api_user")
+ public void testCreateActionWithOutOfRangeTimeout() {
+ Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(new MockPluginExecutor()));
+
+ ActionDTO action = new ActionDTO();
+ action.setName("validAction");
+ action.setPageId(testPage.getId());
+ action.setExecuteOnLoad(true);
+ ActionConfiguration actionConfiguration = new ActionConfiguration();
+ actionConfiguration.setHttpMethod(HttpMethod.GET);
+ actionConfiguration.setTimeoutInMillisecond("60001");
+ action.setActionConfiguration(actionConfiguration);
+ action.setDatasource(datasource);
+
+ Mono<ActionDTO> actionMono = newActionService.createAction(action)
+ .flatMap(createdAction -> newActionService.findById(createdAction.getId(), READ_ACTIONS))
+ .flatMap(newAction -> newActionService.generateActionByViewMode(newAction, false));
+
+ StepVerifier
+ .create(actionMono)
+ .assertNext(createdAction -> {
+ assertThat(createdAction).isNotNull();
+ assertThat(createdAction
+ .getInvalids()
+ .stream()
+ .anyMatch(errorMsg -> errorMsg.contains("'Query timeout' field must be an integer between" +
+ " 0 and 60000"))
+ ).isTrue();
+ })
+ .verifyComplete();
+ }
+
+ @Test
+ @WithUserDetails(value = "api_user")
+ public void testCreateActionWithValidRangeTimeout() {
+ Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(new MockPluginExecutor()));
+
+ ActionDTO action = new ActionDTO();
+ action.setName("validAction");
+ action.setPageId(testPage.getId());
+ action.setExecuteOnLoad(true);
+ ActionConfiguration actionConfiguration = new ActionConfiguration();
+ actionConfiguration.setHttpMethod(HttpMethod.GET);
+ actionConfiguration.setTimeoutInMillisecond("6000");
+ action.setActionConfiguration(actionConfiguration);
+ action.setDatasource(datasource);
+
+ Mono<ActionDTO> actionMono = newActionService.createAction(action)
+ .flatMap(createdAction -> newActionService.findById(createdAction.getId(), READ_ACTIONS))
+ .flatMap(newAction -> newActionService.generateActionByViewMode(newAction, false));
+
+ StepVerifier
+ .create(actionMono)
+ .assertNext(createdAction -> {
+ assertThat(createdAction).isNotNull();
+ assertThat(createdAction
+ .getInvalids()
+ .stream()
+ .anyMatch(errorMsg -> errorMsg.contains("'Query timeout' field must be an integer between"))
+ ).isFalse();
+ })
+ .verifyComplete();
+ }
 }