Skip to content

Commit

Permalink
[skip_changelog] Check datasource validity before creating a CRUD page (
Browse files Browse the repository at this point in the history 
appsmithorg#6562)

* Added check for valid datasource before generating a page

* Sanitise column names for binding in widgets

* Refactor

* Update TC
  • Loading branch information
@abhvsn
abhvsn authored Aug 13, 2021
1 parent c3b44aa commit a1398ad
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 36 deletions.
61 changes: 32 additions & 29 deletions ...ppsmith-server/src/main/java/com/appsmith/server/solutions/CreateDBTablePageSolution.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import com.appsmith.server.exceptions.AppsmithException;
import com.appsmith.server.services.AnalyticsService;
import com.appsmith.server.services.ApplicationPageService;
import com.appsmith.server.services.ApplicationService;
import com.appsmith.server.services.DatasourceService;
import com.appsmith.server.services.LayoutActionService;
import com.appsmith.server.services.NewPageService;
Expand Down Expand Up @@ -53,7 +52,6 @@
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
Expand All @@ -65,7 +63,6 @@ public class CreateDBTablePageSolution {

private final DatasourceService datasourceService;
private final NewPageService newPageService;
private final ApplicationService applicationService;
private final LayoutActionService layoutActionService;
private final ApplicationPageService applicationPageService;
private final PluginService pluginService;
Expand Down Expand Up @@ -142,7 +139,6 @@ public Mono<PageDTO> createPageFromDBTable(String pageId, CRUDPageResourceDTO pa
"snowflake-plugin"
);
StringBuffer templateAutogeneratedKey = new StringBuffer();
AtomicReference<String> savedPageId = new AtomicReference<>(pageId);
if (pageResourceDTO.getTableName() == null || pageResourceDTO.getDatasourceId() == null) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, ", tableName and datasourceId must be present"));
} else if (pageResourceDTO.getApplicationId() == null) {
Expand All @@ -159,19 +155,16 @@ public Mono<PageDTO> createPageFromDBTable(String pageId, CRUDPageResourceDTO pa
//Mapped columns along with table name between template and concerned DB table
Map<String, String> mappedColumnsAndTableName = new HashMap<>();

Mono<NewPage> pageMono = getOrCreatePage(applicationId, savedPageId.get(), tableName).cache();
Mono<NewPage> pageMono = getOrCreatePage(applicationId, pageId, tableName);

Mono<Datasource> datasourceMono = pageMono
.flatMap(newPage -> {
savedPageId.set(newPage.getId());
return applicationService.findById(newPage.getApplicationId());
})
.flatMap(application ->
datasourceService
.findById(datasourceId, AclPermission.MANAGE_DATASOURCES)
.switchIfEmpty(Mono.error(
new AppsmithException(AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.DATASOURCE, datasourceId))
)
Mono<Datasource> datasourceMono = datasourceService
.findById(datasourceId, AclPermission.MANAGE_DATASOURCES)
.switchIfEmpty(Mono.error(
new AppsmithException(AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.DATASOURCE, datasourceId))
)
.filter(datasource -> datasource.getIsValid())
.switchIfEmpty(Mono.error(
new AppsmithException(AppsmithError.INVALID_DATASOURCE, FieldName.DATASOURCE, datasourceId))
);

return datasourceMono
Expand All @@ -184,7 +177,8 @@ public Mono<PageDTO> createPageFromDBTable(String pageId, CRUDPageResourceDTO pa
Datasource datasource = tuple.getT1();
NewPage page = tuple.getT2().getT1();
Plugin plugin = tuple.getT2().getT2();
String layoutId = page.getUnpublishedPage().getLayouts().get(0).getId();
final String layoutId = page.getUnpublishedPage().getLayouts().get(0).getId();
final String savedPageId = page.getId();

ApplicationJson applicationJson = new ApplicationJson();
try {
Expand Down Expand Up @@ -321,14 +315,15 @@ public Mono<PageDTO> createPageFromDBTable(String pageId, CRUDPageResourceDTO pa
);
}

log.debug("Going to update layout for page {} and layout {}", savedPageId.get(), layoutId);
return layoutActionService.updateLayout(savedPageId.get(), layoutId, layout)
log.debug("Going to update layout for page {} and layout {}", savedPageId, layoutId);
return layoutActionService.updateLayout(savedPageId, layoutId, layout)
.then(Mono.zip(
Mono.just(datasource),
Mono.just(templateActionList),
Mono.just(deletedWidgets),
Mono.just(plugin),
Mono.just(tableNameUsedInAction)
Mono.just(tableNameUsedInAction),
Mono.just(savedPageId)
));
})
.flatMap(tuple -> {
Expand All @@ -338,10 +333,11 @@ public Mono<PageDTO> createPageFromDBTable(String pageId, CRUDPageResourceDTO pa
Set<String> deletedWidgets = tuple.getT3();
Plugin plugin = tuple.getT4();
String tableNameInAction = tuple.getT5();
String savedPageId = tuple.getT6();
log.debug("Going to clone actions from template application");
return cloneActionsFromTemplateApplication(datasource,
tableNameInAction,
savedPageId.get(),
savedPageId,
templateActionList,
mappedColumnsAndTableName,
deletedWidgets,
Expand All @@ -352,7 +348,7 @@ public Mono<PageDTO> createPageFromDBTable(String pageId, CRUDPageResourceDTO pa
|| StringUtils.equals(actionDTO.getName(), FIND_QUERY)
|| StringUtils.equals(actionDTO.getName(), LIST_QUERY)
? layoutActionService.setExecuteOnLoad(actionDTO.getId(), true) : Mono.just(actionDTO))
.then(applicationPageService.getPage(savedPageId.get(), false)
.then(applicationPageService.getPage(savedPageId, false)
.flatMap(pageDTO -> sendGenerateCRUDPageAnalyticsEvent(pageDTO, datasource, plugin.getName()))
);
});
Expand Down Expand Up @@ -794,25 +790,32 @@ private JSONObject updateTemplateWidgets(JSONObject widgetDsl, Map<String, Strin
widgetDsl.put(FieldName.PRIMARY_COLUMNS, newPrimaryColumns);
}
} else if (FieldName.DROP_DOWN_WIDGET.equals(widgetDsl.getAsString(FieldName.TYPE))
&& FieldName.OPTIONS.equals(key)
&& !(SQL_DEFAULT_DROPDOWN_VALUE.equalsIgnoreCase(defaultDropdownValue)
&& !(SQL_DEFAULT_DROPDOWN_VALUE.equalsIgnoreCase(defaultDropdownValue)
|| MONGO_DEFAULT_DROPDOWN_VALUE.equals(defaultDropdownValue))) {
// This will update the options field to include all the column names as label and value
// in SelectWidget except for SelectWidget with DefaultOptionValue SQL(DefaultValue : "ASC")
// and Mongo(DefaultValue : "1") check template application layout for more details

if (FieldName.OPTIONS.equals(key)) {
// This will update the options field to include all the column names as label and value
// in SelectWidget except for SelectWidget with DefaultOptionValue SQL(DefaultValue : "ASC")
// and Mongo(DefaultValue : "1") check template application layout for more details
List<String> dropdownOptions = new ArrayList<>();
mappedColumnsAndTableNames.forEach((colKey, colVal) -> {
if (colKey.toLowerCase().contains("col") && !colVal.equals(DELETE_FIELD)) {
dropdownOptions.add("\n{\n\t\"label\": \"" + colVal + "\",\n\t\"value\": \"" + colVal + "\"\n}");
dropdownOptions.add("\n{\n\t\"label\": \"" + colVal.replaceAll("\\W+", "_")
+ "\",\n\t\"value\": \"" + colVal + "\"\n}");
}
});
widgetDsl.put(FieldName.OPTIONS, dropdownOptions.toString());
} else if (FieldName.DEFAULT_OPTION.equals(key)) {
widgetDsl.put(key, mappedColumnsAndTableNames.get(widgetDsl.getAsString(key)));
}
} else {
//Get separate words and map to tableColumns from widgetDsl
Matcher matcher = WORD_PATTERN.matcher(widgetDsl.getAsString(key));
widgetDsl.put(key, matcher.replaceAll(field ->
// Replace any special characters with "_" as all the special characters are replaced with "_" in
// table column widget
mappedColumnsAndTableNames.get(field.group()) == null ?
field.group() : mappedColumnsAndTableNames.get(field.group())
field.group() : mappedColumnsAndTableNames.get(field.group()).replaceAll("\\W+", "_")
));
}
}
Expand Down
49 changes: 42 additions & 7 deletions ...th-server/src/test/java/com/appsmith/server/solutions/CreateDBTablePageSolutionTests.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.appsmith.server.solutions;

import com.appsmith.external.models.ActionConfiguration;
import com.appsmith.external.models.DatasourceConfiguration;
import com.appsmith.external.models.DatasourceStructure;
import com.appsmith.external.models.DatasourceStructure.Column;
import com.appsmith.external.models.DatasourceStructure.Key;
Expand Down Expand Up @@ -85,20 +86,22 @@ public class CreateDBTablePageSolutionTests {
private DatasourceStructure structure = new DatasourceStructure();

// Regex to break string in separate words
final static String specialCharactersRegex = "[^a-zA-Z0-9,;(){}*]+";
final static String specialCharactersRegex = "[^a-zA-Z0-9,;(){}*_]+";

private final String SELECT_QUERY = "SelectQuery";

private final String FIND_QUERY = "FindQuery";

private final String LIST_QUERY = "ListFiles";

DatasourceConfiguration datasourceConfiguration = new DatasourceConfiguration();

private final Map<String, String> actionNameToBodyMap = Map.of(
"DeleteQuery", "DELETE FROM sampleTable\n" +
" WHERE \"primaryKey\" = {{Table1.selectedRow.primaryKey}};",

"InsertQuery", "INSERT INTO sampleTable (\n" +
"\t\"field1\", \n" +
"\t\"field1.something\", \n" +
"\t\"field2\",\n" +
"\t\"field3\", \n" +
"\t\"field4\"\n" +
Expand All @@ -111,13 +114,13 @@ public class CreateDBTablePageSolutionTests {
");",

"SelectQuery", "SELECT * FROM sampleTable\n" +
"WHERE \"field1\" like '%{{Table1.searchText || \"\"}}%'\n" +
"WHERE \"field1.something\" like '%{{Table1.searchText || \"\"}}%'\n" +
"ORDER BY \"{{col_select.selectedOptionValue}}\" {{order_select.selectedOptionLabel}}\n" +
"LIMIT {{Table1.pageSize}}" +
"OFFSET {{(Table1.pageNo - 1) * Table1.pageSize}};",

"UpdateQuery", "UPDATE sampleTable SET\n" +
"\t\t\"field1\" = '{{update_col_2.text}}',\n" +
"\t\t\"field1.something\" = '{{update_col_2.text}}',\n" +
" \"field2\" = '{{update_col_3.text}}',\n" +
" \"field3\" = '{{update_col_4.text}}',\n" +
"\t\t\"field4\" = '{{update_col_5.text}}'\n" +
Expand All @@ -126,7 +129,7 @@ public class CreateDBTablePageSolutionTests {

private final String dropdownOptions = "options -> [\n" +
"{\n\t\"label\": \"field3\",\n\t\"value\": \"field3\"\n}, \n{\n\t\"label\": \"field4\",\n" +
"\t\"value\": \"field4\"\n}, \n{\n\t\"label\": \"field1\",\n\t\"value\": \"field1\"\n" +
"\t\"value\": \"field4\"\n}, \n{\n\t\"label\": \"field1_something\",\n\t\"value\": \"field1.something\"\n" +
"}, \n{\n\t\"label\": \"field2\",\n\t\"value\": \"field2\"\n}, \n{\n\t\"label\": \"primaryKey\",\n" +
"\t\"value\": \"primaryKey\"\n}]";

Expand All @@ -149,7 +152,7 @@ public void setup() {
List<Key> keys = List.of(new DatasourceStructure.PrimaryKey("pKey", List.of("primaryKey")));
List<Column> columns = List.of(
new Column("primaryKey", "type1", null, true),
new Column("field1", "VARCHAR(23)", null, false),
new Column("field1.something", "VARCHAR(23)", null, false),
new Column("field2", "type3", null, false),
new Column("field3", "type4", null, false),
new Column("field4", "type5", null, false)
Expand All @@ -160,6 +163,8 @@ public void setup() {
testDatasource.setOrganizationId(testOrg.getId());
testDatasource.setName("CRUD-Page-Table-DS");
testDatasource.setStructure(structure);
datasourceConfiguration.setUrl("http://test.com");
testDatasource.setDatasourceConfiguration(datasourceConfiguration);
datasourceService.create(testDatasource).block();

resource.setTableName(testDatasource.getStructure().getTables().get(0).getName());
Expand All @@ -185,6 +190,31 @@ public void createPageWithInvalidApplicationIdTest() {

}

@Test
@WithUserDetails(value = "api_user")
public void createPageWithInvalidDatasourceTest() {

Datasource invalidDatasource = new Datasource();
invalidDatasource.setOrganizationId(testOrg.getId());
invalidDatasource.setName("invalid_datasource");
invalidDatasource.setDatasourceConfiguration(new DatasourceConfiguration());

resource.setDatasourceId(invalidDatasource.getId());
Mono<PageDTO> resultMono = datasourceService.create(invalidDatasource)
.flatMap(datasource -> {
resource.setApplicationId(testApp.getId());
resource.setDatasourceId(datasource.getId());
return solution.createPageFromDBTable(testApp.getPages().get(0).getId(), resource);
});

StepVerifier
.create(resultMono)
.expectErrorMatches(throwable -> throwable instanceof AppsmithException &&
throwable.getMessage().equals(AppsmithError.INVALID_DATASOURCE.getMessage(FieldName.DATASOURCE, invalidDatasource.getId())))
.verify();

}

@Test
@WithUserDetails(value = "api_user")
public void createPageWithInvalidRequestBodyTest() {
Expand Down Expand Up @@ -279,6 +309,7 @@ public void createPageWithValidPageIdForMySqlDS() {
datasource.setOrganizationId(testOrg.getId());
datasource.setName("MySql-CRUD-Page-Table-DS");
datasource.setStructure(structure);
datasource.setDatasourceConfiguration(datasourceConfiguration);
return datasourceService.create(datasource);
});

Expand Down Expand Up @@ -338,6 +369,7 @@ public void createPageWithValidPageIdForRedshiftDS() {
datasource.setOrganizationId(testOrg.getId());
datasource.setName("Redshift-CRUD-Page-Table-DS");
datasource.setStructure(structure);
datasource.setDatasourceConfiguration(datasourceConfiguration);
return datasourceService.create(datasource);
});

Expand Down Expand Up @@ -391,6 +423,7 @@ public void createPageWithNullPageIdForMSSqlDS() {
datasource.setOrganizationId(testOrg.getId());
datasource.setName("MSSql-CRUD-Page-Table-DS");
datasource.setStructure(structure);
datasource.setDatasourceConfiguration(datasourceConfiguration);
return datasourceService.create(datasource);
});

Expand Down Expand Up @@ -443,6 +476,7 @@ public void createPageWithNullPageIdForSnowflake() {
datasource.setOrganizationId(testOrg.getId());
datasource.setName("Snowflake-CRUD-Page-Table-DS");
datasource.setStructure(structure);
datasource.setDatasourceConfiguration(datasourceConfiguration);
return datasourceService.create(datasource);
});

Expand Down Expand Up @@ -612,6 +646,7 @@ public void createPageWithValidPageIdForMongoDB() {
datasource.setOrganizationId(testOrg.getId());
datasource.setName("Mongo-CRUD-Page-Table-DS");
datasource.setStructure(structure);
datasource.setDatasourceConfiguration(datasourceConfiguration);
return datasourceService.create(datasource);
});

Expand Down Expand Up @@ -654,7 +689,7 @@ public void createPageWithValidPageIdForMongoDB() {
.isEqualTo("{ primaryKey: ObjectId('{{data_table.selectedRow.primaryKey}}') }");

assertThat(pluginSpecifiedTemplate.get(12).getValue().toString().replaceAll(specialCharactersRegex, ""))
.isEqualTo("{\"field2\" : {{update_col_1.text}},\"field1\" : {{update_col_2.text}},\"field3\" : {{update_col_3.text}},\"field4\" : {{update_col_4.text}}\"}"
.isEqualTo("{\"field2\" : {{update_col_1.text}},\"field1.something\" : {{update_col_2.text}},\"field3\" : {{update_col_3.text}},\"field4\" : {{update_col_4.text}}\"}"
.replaceAll(specialCharactersRegex, ""));
} else if (queryType.equals("DELETE")) {
assertThat(pluginSpecifiedTemplate.get(13).getValue().toString().replaceAll(specialCharactersRegex, ""))
Expand Down

0 comments on commit a1398ad

Please sign in to comment.