From b831a3943b474961e7417e697e6f3d9dfe409d64 Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Wed, 22 May 2024 11:36:20 +0530 Subject: [PATCH] chore: Add ApplicationCreationDTO for Application creation API body (#33200) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR cleans up the application creation API with stricter validations on the input. We're also moving the `workspaceId` from the query parameter into the body, so it can be validated togather and all input data is in one place. Simplifies code both in client and server. No additional changes for EE, and no conflicts either, and al unit and Cypress tests pass. /ok-to-test tags="@tag.Sanity" > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 916b54c802a163910c738e3f8ceb203314a38a6c > Cypress dashboard url: Click here! --- app/client/cypress/support/commands.js | 4 +- app/client/src/ce/api/ApplicationApi.tsx | 32 ++++------------ .../ce/ApplicationControllerCE.java | 13 ++----- .../appsmith/server/domains/Application.java | 5 ++- .../server/dtos/ApplicationCreationDTO.java | 37 +++++++++++++++++++ .../appsmith/server/dtos/PageCreationDTO.java | 4 +- .../appsmith/server/dtos/PageUpdateDTO.java | 8 ++-- .../server/meta/validations/IconName.java | 22 +++++++++++ .../meta/validations/IconNameValidator.java | 16 ++++++++ .../appsmith/server/domains/EqualityTest.java | 9 ++--- 10 files changed, 101 insertions(+), 49 deletions(-) create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ApplicationCreationDTO.java create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/IconName.java create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/IconNameValidator.java diff --git a/app/client/cypress/support/commands.js b/app/client/cypress/support/commands.js index a888f44863..c666246cb6 100644 --- a/app/client/cypress/support/commands.js +++ b/app/client/cypress/support/commands.js @@ -954,9 +954,7 @@ Cypress.Commands.add("startServerAndRoutes", () => { cy.intercept("GET", "/api/v1/plugins/*/form").as("getPluginForm"); cy.intercept("DELETE", "/api/v1/applications/*").as("deleteApplication"); - cy.intercept("POST", "/api/v1/applications?workspaceId=*").as( - "createNewApplication", - ); + cy.intercept("POST", "/api/v1/applications").as("createNewApplication"); cy.intercept("PUT", "/api/v1/applications/*").as("updateApplication"); cy.intercept("PUT", "/api/v1/actions/*").as("saveAction"); cy.intercept("PUT", "/api/v1/actions/move").as("moveAction"); diff --git a/app/client/src/ce/api/ApplicationApi.tsx b/app/client/src/ce/api/ApplicationApi.tsx index a019cf6220..2ffa8cc883 100644 --- a/app/client/src/ce/api/ApplicationApi.tsx +++ b/app/client/src/ce/api/ApplicationApi.tsx @@ -283,8 +283,6 @@ export class ApplicationApi extends Api { static baseURL = "v1/applications"; static publishURLPath = (applicationId: string) => `/publish/${applicationId}`; - static createApplicationPath = (workspaceId: string) => - `?workspaceId=${workspaceId}`; static changeAppViewAccessPath = (applicationId: string) => `/${applicationId}/changeAccess`; static setDefaultPagePath = (request: SetDefaultPageRequest) => @@ -341,28 +339,14 @@ export class ApplicationApi extends Api { static async createApplication( request: CreateApplicationRequest, ): Promise> { - const applicationDetail = { - appPositioning: { - type: request.layoutSystemType, - }, - } as any; - - if (request.showNavbar !== undefined) { - applicationDetail.navigationSetting = { - showNavbar: request.showNavbar, - }; - } - - return Api.post( - ApplicationApi.baseURL + - ApplicationApi.createApplicationPath(request.workspaceId), - { - name: request.name, - color: request.color, - icon: request.icon, - applicationDetail, - }, - ); + return Api.post(ApplicationApi.baseURL, { + workspaceId: request.workspaceId, + name: request.name, + color: request.color, + icon: request.icon, + positioningType: request.layoutSystemType, + showNavbar: request.showNavbar ?? null, + }); } static async setDefaultApplicationPage( diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ApplicationControllerCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ApplicationControllerCE.java index 5fdb997c6f..872c9ab606 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ApplicationControllerCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ApplicationControllerCE.java @@ -9,6 +9,7 @@ import com.appsmith.server.domains.Application; import com.appsmith.server.domains.GitAuth; import com.appsmith.server.domains.Theme; import com.appsmith.server.dtos.ApplicationAccessDTO; +import com.appsmith.server.dtos.ApplicationCreationDTO; import com.appsmith.server.dtos.ApplicationImportDTO; import com.appsmith.server.dtos.ApplicationJson; import com.appsmith.server.dtos.ApplicationPagesDTO; @@ -19,8 +20,6 @@ import com.appsmith.server.dtos.GitAuthDTO; import com.appsmith.server.dtos.PartialExportFileDTO; import com.appsmith.server.dtos.ReleaseItemsDTO; import com.appsmith.server.dtos.ResponseDTO; -import com.appsmith.server.exceptions.AppsmithError; -import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.exports.internal.ExportService; import com.appsmith.server.exports.internal.partial.PartialExportService; import com.appsmith.server.fork.internal.ApplicationForkingService; @@ -77,14 +76,10 @@ public class ApplicationControllerCE { @JsonView(Views.Public.class) @PostMapping @ResponseStatus(HttpStatus.CREATED) - public Mono> create( - @Valid @RequestBody Application resource, @RequestParam String workspaceId) { - if (workspaceId == null) { - return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "workspace id")); - } - log.debug("Going to create application in workspace {}", workspaceId); + public Mono> create(@Valid @RequestBody ApplicationCreationDTO resource) { + log.debug("Going to create application in workspace {}", resource.workspaceId()); return applicationPageService - .createApplication(resource, workspaceId) + .createApplication(resource.toApplication()) .map(created -> new ResponseDTO<>(HttpStatus.CREATED.value(), created, null)); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Application.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Application.java index b8edc1f34b..3b88521ed6 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Application.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Application.java @@ -428,12 +428,13 @@ public class Application extends BaseDomain implements Artifact { */ @Data @NoArgsConstructor + @AllArgsConstructor public static class AppPositioning { @JsonView({Views.Public.class, Git.class}) Type type; - public AppPositioning(Type type) { - this.type = type; + public AppPositioning(String type) { + setType(Type.valueOf(type)); } public enum Type { diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ApplicationCreationDTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ApplicationCreationDTO.java new file mode 100644 index 0000000000..2ee5349daf --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ApplicationCreationDTO.java @@ -0,0 +1,37 @@ +package com.appsmith.server.dtos; + +import com.appsmith.server.domains.Application; +import com.appsmith.server.domains.ApplicationDetail; +import com.appsmith.server.meta.validations.IconName; +import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.Pattern; +import jakarta.validation.constraints.Size; +import org.apache.commons.lang3.StringUtils; + +public record ApplicationCreationDTO( + @NotBlank @Size(max = 99) String workspaceId, + @NotBlank @Size(max = 99) String name, + @IconName String icon, + @Pattern(regexp = "#[A-F0-9]{6}") String color, + Application.AppPositioning positioningType, + Boolean showNavBar) { + + public Application toApplication() { + final Application application = new Application(); + application.setWorkspaceId(workspaceId); + application.setName(name.trim()); + application.setIcon(StringUtils.isBlank(icon) ? null : icon.trim()); + application.setColor(color); + + final ApplicationDetail applicationDetail = new ApplicationDetail(); + application.setApplicationDetail(applicationDetail); + + applicationDetail.setAppPositioning(positioningType); + + final Application.NavigationSetting navigationSetting = new Application.NavigationSetting(); + navigationSetting.setShowNavbar(showNavBar); + applicationDetail.setNavigationSetting(navigationSetting); + + return application; + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageCreationDTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageCreationDTO.java index 455bb91273..cf567a80a2 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageCreationDTO.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageCreationDTO.java @@ -8,13 +8,13 @@ import jakarta.validation.constraints.Size; import java.util.List; public record PageCreationDTO( - @FileName(message = "Page names must be valid file names") @Size(max = 30) String name, + @FileName(message = "Page names must be valid file names", isNullValid = false) @Size(max = 30) String name, @NotEmpty @Size(min = 24, max = 50) String applicationId, @NotEmpty List layouts) { public PageDTO toPageDTO() { final PageDTO page = new PageDTO(); - page.setName(name); + page.setName(name.trim()); page.setApplicationId(applicationId); page.setLayouts(layouts); return page; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageUpdateDTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageUpdateDTO.java index 45f089e1cf..b0490f7c41 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageUpdateDTO.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageUpdateDTO.java @@ -1,19 +1,21 @@ package com.appsmith.server.dtos; import com.appsmith.server.meta.validations.FileName; +import com.appsmith.server.meta.validations.IconName; import jakarta.validation.constraints.Pattern; import jakarta.validation.constraints.Size; +import org.apache.commons.lang3.StringUtils; public record PageUpdateDTO( @FileName(message = "Page names must be valid file names") @Size(max = 30) String name, - @Pattern(regexp = "[-a-z]+") @Size(max = 20) String icon, + @IconName String icon, @Pattern(regexp = "[-\\w]*") String customSlug, Boolean isHidden) { public PageDTO toPageDTO() { final PageDTO page = new PageDTO(); - page.setName(name); - page.setIcon(icon); + page.setName(name == null ? null : name.trim()); + page.setIcon(StringUtils.isBlank(icon) ? null : icon.trim()); page.setCustomSlug(customSlug); page.setIsHidden(isHidden); return page; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/IconName.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/IconName.java new file mode 100644 index 0000000000..7a12f1447b --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/IconName.java @@ -0,0 +1,22 @@ +package com.appsmith.server.meta.validations; + +import jakarta.validation.Constraint; +import jakarta.validation.Payload; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +@Documented +@Constraint(validatedBy = {IconNameValidator.class}) +@Target({ElementType.FIELD, ElementType.METHOD}) +@Retention(RetentionPolicy.RUNTIME) +public @interface IconName { + String message() default "Invalid icon name"; + + Class[] groups() default {}; + + Class[] payload() default {}; +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/IconNameValidator.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/IconNameValidator.java new file mode 100644 index 0000000000..77a95aad21 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/meta/validations/IconNameValidator.java @@ -0,0 +1,16 @@ +package com.appsmith.server.meta.validations; + +import jakarta.validation.ConstraintValidator; +import jakarta.validation.ConstraintValidatorContext; + +import java.util.regex.Pattern; + +public class IconNameValidator implements ConstraintValidator { + + private static final Pattern PATTERN = Pattern.compile("[-a-z]{1,20}"); + + @Override + public boolean isValid(String value, ConstraintValidatorContext context) { + return value == null || PATTERN.matcher(value).matches(); + } +} diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/domains/EqualityTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/domains/EqualityTest.java index 4b1062dc01..e15667024b 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/domains/EqualityTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/domains/EqualityTest.java @@ -58,12 +58,9 @@ public class EqualityTest { @Test void testApplicationDetail() { - Application.AppPositioning p1 = new Application.AppPositioning(); - p1.setType(Application.AppPositioning.Type.AUTO); - Application.AppPositioning p2 = new Application.AppPositioning(); - p2.setType(Application.AppPositioning.Type.AUTO); - Application.AppPositioning p3 = new Application.AppPositioning(); - p3.setType(Application.AppPositioning.Type.FIXED); + Application.AppPositioning p1 = new Application.AppPositioning(Application.AppPositioning.Type.AUTO); + Application.AppPositioning p2 = new Application.AppPositioning(Application.AppPositioning.Type.AUTO); + Application.AppPositioning p3 = new Application.AppPositioning(Application.AppPositioning.Type.FIXED); assertThat(p1).isEqualTo(p2).isNotEqualTo(p3); ApplicationDetail d1 = new ApplicationDetail();