From 83326ef10d2e1c5cc8798f366bd1ea46a2b8f5a2 Mon Sep 17 00:00:00 2001 From: subratadeypappu Date: Thu, 29 Aug 2024 16:13:21 +0600 Subject: [PATCH] chore: optimise pages API in view mode (#35915) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description This PR addresses issue https://github.com/appsmithorg/appsmith/issues/35881 by optimizing the pages API specifically for view mode. The primary goal is to enhance the performance and efficiency of the API, ensuring a smoother and faster user experience when viewing pages. The changes involve refactoring existing methods, introducing new ones, and updating test cases to align with the new functionality. TL;DR It includes the following changes - Addition of projection for the query that fetches pages by applicationId in view mode - Removal of sorting workspaces and their resources in view mode Fixes https://github.com/appsmithorg/appsmith/issues/35881 ## Automation /ok-to-test tags="@tag.All" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 696b7787c0f72cc756122ee18620b0e14bfc6f4c > Cypress dashboard. > Tags: `@tag.All` > Spec: >
Thu, 29 Aug 2024 08:52:24 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Introduced a method to retrieve new pages by application ID with customizable field inclusion. - Enhanced data retrieval capabilities with more granular control over returned fields. - Added project-specific data handling in page retrieval logic. - **Bug Fixes** - Improved logic for handling view mode in page retrieval based on application mode. - **Documentation** - Updated tests to reflect changes in published page visibility, ensuring accurate assertions. - **Refactor** - Streamlined methods for creating application pages and improved control flow for data retrieval. - **Tests** - Enhanced asynchronous behavior in test methods for better execution control. --- .../newpages/base/NewPageServiceCE.java | 5 +- .../newpages/base/NewPageServiceCEImpl.java | 59 +++++++------------ .../ce/CustomNewPageRepositoryCE.java | 2 + .../ce/CustomNewPageRepositoryCEImpl.java | 10 ++++ .../ce/ApplicationPageServiceCEImpl.java | 20 ++++++- .../ce/ConsolidatedAPIServiceCEImpl.java | 2 + .../services/ApplicationPageServiceTest.java | 2 +- .../server/services/NewPageServiceTest.java | 4 +- 8 files changed, 60 insertions(+), 44 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java index 071ad9342ef..d8c1192de9b 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java @@ -27,6 +27,9 @@ public interface NewPageServiceCE extends CrudService { Flux findNewPagesByApplicationId(String applicationId, AclPermission permission); + Flux findNewPagesByApplicationId( + String applicationId, AclPermission permission, List includeFields); + Mono findByIdAndBranchName(String id, String branchName); Mono saveUnpublishedPage(PageDTO page); @@ -52,8 +55,6 @@ Mono findByNameAndApplicationIdAndViewMode( Mono> archivePagesByApplicationId(String applicationId, AclPermission permission); - Mono> findAllPageIdsInApplication(String applicationId, AclPermission permission, Boolean view); - Mono updatePage(String pageId, PageDTO page); Mono save(NewPage page); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java index ee3aca5221d..e8f418d3928 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java @@ -9,6 +9,7 @@ import com.appsmith.server.domains.ApplicationPage; import com.appsmith.server.domains.Layout; import com.appsmith.server.domains.NewPage; +import com.appsmith.server.domains.UserData; import com.appsmith.server.dtos.ApplicationPagesDTO; import com.appsmith.server.dtos.PageDTO; import com.appsmith.server.dtos.PageNameIdDTO; @@ -28,13 +29,13 @@ import net.minidev.json.parser.JSONParser; import net.minidev.json.parser.ParseException; import org.bson.types.ObjectId; -import org.jetbrains.annotations.NotNull; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.util.CollectionUtils; import org.springframework.util.StringUtils; import reactor.core.observability.micrometer.Micrometer; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import reactor.util.function.Tuple2; import java.util.ArrayList; import java.util.Collection; @@ -291,24 +292,25 @@ public Mono findApplicationPagesByBranchedApplicationIdAndV public Mono createApplicationPagesDTO( Application branchedApplication, List newPages, boolean viewMode, boolean markRecentlyAccessed) { - Mono markedRecentlyAccessedMono = Mono.empty(); - - if (Boolean.TRUE.equals(markRecentlyAccessed)) { - markedRecentlyAccessedMono = userDataService + ApplicationMode applicationMode = viewMode ? ApplicationMode.PUBLISHED : ApplicationMode.EDIT; + Mono getApplicationPagesDTOMono = Mono.just( + getApplicationPagesDTO(branchedApplication, newPages, viewMode)) + .name(getQualifiedSpanName(PREPARE_APPLICATION_PAGES_DTO_FROM_PAGES, applicationMode)) + .tap(Micrometer.observation(observationRegistry)); + if (Boolean.TRUE.equals(markRecentlyAccessed) && !viewMode) { + Mono markedRecentlyAccessedMono = userDataService .updateLastUsedResourceAndWorkspaceList( branchedApplication.getId(), branchedApplication.getWorkspaceId(), WorkspaceResourceContext.APPLICATIONS) - .then(); + .name(getQualifiedSpanName(MARK_RECENTLY_ACCESSED_RESOURCES_PAGES, applicationMode)) + .tap(Micrometer.observation(observationRegistry)); + + return Mono.zip(markedRecentlyAccessedMono, getApplicationPagesDTOMono) + .map(Tuple2::getT2); } - ApplicationMode applicationMode = viewMode ? ApplicationMode.PUBLISHED : ApplicationMode.EDIT; - return markedRecentlyAccessedMono - .name(getQualifiedSpanName(MARK_RECENTLY_ACCESSED_RESOURCES_PAGES, applicationMode)) - .tap(Micrometer.observation(observationRegistry)) - .then(Mono.fromCallable(() -> getApplicationPagesDTO(branchedApplication, newPages, viewMode)) - .name(getQualifiedSpanName(PREPARE_APPLICATION_PAGES_DTO_FROM_PAGES, applicationMode)) - .tap(Micrometer.observation(observationRegistry))); + return getApplicationPagesDTOMono; } private List getApplicationPages(Application application, boolean viewMode) { @@ -385,7 +387,7 @@ public ApplicationPagesDTO getApplicationPagesDTO( return applicationPagesDTO; } - private static @NotNull PageNameIdDTO getPageNameIdDTO(NewPage pageFromDb, String homePageId, boolean viewMode) { + private static PageNameIdDTO getPageNameIdDTO(NewPage pageFromDb, String homePageId, boolean viewMode) { PageNameIdDTO pageNameIdDTO = new PageNameIdDTO(); pageNameIdDTO.setId(pageFromDb.getId()); pageNameIdDTO.setBaseId(pageFromDb.getBaseIdOrFallback()); @@ -427,32 +429,15 @@ public Flux findNewPagesByApplicationId(String applicationId, AclPermis } @Override - public Mono> archivePagesByApplicationId(String applicationId, AclPermission permission) { - return findNewPagesByApplicationId(applicationId, permission) - .flatMap(repository::archive) - .collectList(); + public Flux findNewPagesByApplicationId( + String applicationId, AclPermission permission, List includeFields) { + return repository.findByApplicationId(applicationId, permission, includeFields); } @Override - // Remove if not used - public Mono> findAllPageIdsInApplication( - String applicationId, AclPermission aclPermission, Boolean view) { - return findNewPagesByApplicationId(applicationId, aclPermission) - .flatMap(newPage -> { - // Look if the page is migrated - // Real time migration - if (Boolean.TRUE.equals(view)) { - if (newPage.getPublishedPage().getDeletedAt() != null) { - return Mono.just(newPage.getId()); - } - } else { - if (newPage.getUnpublishedPage().getDeletedAt() != null) { - return Mono.just(newPage.getId()); - } - } - // Looks like the page has been deleted in the `view` mode. Don't return the id for this page. - return Mono.empty(); - }) + public Mono> archivePagesByApplicationId(String applicationId, AclPermission permission) { + return findNewPagesByApplicationId(applicationId, permission) + .flatMap(repository::archive) .collectList(); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java index 96bda4c7b98..43d30d5ea14 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java @@ -14,6 +14,8 @@ public interface CustomNewPageRepositoryCE extends AppsmithRepository { Flux findByApplicationId(String applicationId, AclPermission aclPermission); + Flux findByApplicationId(String applicationId, AclPermission aclPermission, List includeFields); + Flux findByApplicationIdAndNonDeletedEditMode(String applicationId, AclPermission aclPermission); Mono findByIdAndLayoutsIdAndViewMode( diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java index b4aae0c3601..c1d59515075 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java @@ -48,6 +48,16 @@ public Flux findByApplicationId(String applicationId, AclPermission acl .all(); } + @Override + public Flux findByApplicationId( + String applicationId, AclPermission aclPermission, List includeFields) { + return queryBuilder() + .criteria(Bridge.equal(NewPage.Fields.applicationId, applicationId)) + .permission(aclPermission) + .fields(includeFields) + .all(); + } + @Override public Flux findByApplicationIdAndNonDeletedEditMode(String applicationId, AclPermission aclPermission) { BridgeQuery q = Bridge.equal(NewPage.Fields.applicationId, applicationId) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java index e4a6a9ca494..cc3cfd047f0 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java @@ -255,7 +255,22 @@ public Mono getPage(String pageId, boolean viewMode) { public Mono> getPagesBasedOnApplicationMode( Application branchedApplication, ApplicationMode applicationMode) { - Boolean viewMode = ApplicationMode.PUBLISHED.equals(applicationMode) ? Boolean.TRUE : Boolean.FALSE; + Boolean viewMode = Boolean.FALSE; + List projectedFieldNames = null; + if (ApplicationMode.PUBLISHED.equals(applicationMode)) { + viewMode = Boolean.TRUE; + projectedFieldNames = List.of( + NewPage.Fields.id, + NewPage.Fields.baseId, + NewPage.Fields.publishedPage_name, + NewPage.Fields.publishedPage_icon, + NewPage.Fields.publishedPage_slug, + NewPage.Fields.publishedPage_customSlug, + NewPage.Fields.publishedPage_isHidden, + NewPage.Fields.userPermissions, + NewPage.Fields.policies, + NewPage.Fields.policyMap); + } List applicationPages = Boolean.TRUE.equals(viewMode) ? branchedApplication.getPublishedPages() @@ -265,7 +280,8 @@ public Mono> getPagesBasedOnApplicationMode( applicationPages.stream().map(ApplicationPage::getId).collect(Collectors.toSet()); return newPageService - .findNewPagesByApplicationId(branchedApplication.getId(), pagePermission.getReadPermission()) + .findNewPagesByApplicationId( + branchedApplication.getId(), pagePermission.getReadPermission(), projectedFieldNames) .filter(newPage -> pageIds.contains(newPage.getId())) .collectList() .name(getQualifiedSpanName(FETCH_PAGES_BY_APP_ID_DB, applicationMode)) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java index 15fc9b8e251..5716920b8d7 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java @@ -209,6 +209,8 @@ public Mono getConsolidatedInfoForPageLoad( } else { branchedApplicationMonoCached = applicationService .findByBaseIdBranchNameAndApplicationMode(baseApplicationId, branchName, mode) + .name(getQualifiedSpanName(APPLICATION_ID_SPAN, mode)) + .tap(Micrometer.observation(observationRegistry)) .cache(); } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java index 4082dfce510..9260c2672ec 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.java @@ -472,7 +472,7 @@ public void verifyGetPagesBasedOnApplicationMode_ReturnsRigthNumberOfPages_Based .assertNext(pages -> { assertThat(pages.size()).isEqualTo(2); Set pageNames = pages.stream() - .map(page -> page.getUnpublishedPage().getName()) + .map(page -> page.getPublishedPage().getName()) .collect(Collectors.toSet()); assertThat(pageNames).contains(pageName); assertThat(pageNames).doesNotContain(unpublishedPageName); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java index 3edbf823309..11237328ac6 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java @@ -395,8 +395,8 @@ public void verifyCreateApplicationPagesDTO_ReturnsRightNumberOfPages_BasedOnVie }) .verifyComplete(); - Mono viewModeTrueMono = newPageService.createApplicationPagesDTO( - applicationService.findById(applicationId).block(), allPages, true, true); + Mono viewModeTrueMono = Mono.defer(() -> newPageService.createApplicationPagesDTO( + applicationService.findById(applicationId).block(), allPages, true, true)); StepVerifier.create(viewModeTrueMono).verifyErrorSatisfies(error -> { assertThat(error).isInstanceOf(AppsmithException.class);