Skip to content

Commit

Permalink
chore: optimise pages API in view mode (#35915)
Browse files Browse the repository at this point in the history
## Description
This PR addresses issue
#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 #35881

## Automation

/ok-to-test tags="@tag.All"

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/10610633615>
> Commit: 696b778
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10610633615&attempt=3"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.All`
> Spec:
> <hr>Thu, 29 Aug 2024 08:52:24 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## 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.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
subrata71 authored Aug 29, 2024
1 parent f3374bd commit 83326ef
Show file tree
Hide file tree
Showing 8 changed files with 60 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ public interface NewPageServiceCE extends CrudService<NewPage, String> {

Flux<NewPage> findNewPagesByApplicationId(String applicationId, AclPermission permission);

Flux<NewPage> findNewPagesByApplicationId(
String applicationId, AclPermission permission, List<String> includeFields);

Mono<NewPage> findByIdAndBranchName(String id, String branchName);

Mono<PageDTO> saveUnpublishedPage(PageDTO page);
Expand All @@ -52,8 +55,6 @@ Mono<PageDTO> findByNameAndApplicationIdAndViewMode(

Mono<List<NewPage>> archivePagesByApplicationId(String applicationId, AclPermission permission);

Mono<List<String>> findAllPageIdsInApplication(String applicationId, AclPermission permission, Boolean view);

Mono<PageDTO> updatePage(String pageId, PageDTO page);

Mono<NewPage> save(NewPage page);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -291,24 +292,25 @@ public Mono<ApplicationPagesDTO> findApplicationPagesByBranchedApplicationIdAndV
public Mono<ApplicationPagesDTO> createApplicationPagesDTO(
Application branchedApplication, List<NewPage> newPages, boolean viewMode, boolean markRecentlyAccessed) {

Mono<Void> markedRecentlyAccessedMono = Mono.empty();

if (Boolean.TRUE.equals(markRecentlyAccessed)) {
markedRecentlyAccessedMono = userDataService
ApplicationMode applicationMode = viewMode ? ApplicationMode.PUBLISHED : ApplicationMode.EDIT;
Mono<ApplicationPagesDTO> 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<UserData> 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<ApplicationPage> getApplicationPages(Application application, boolean viewMode) {
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -427,32 +429,15 @@ public Flux<NewPage> findNewPagesByApplicationId(String applicationId, AclPermis
}

@Override
public Mono<List<NewPage>> archivePagesByApplicationId(String applicationId, AclPermission permission) {
return findNewPagesByApplicationId(applicationId, permission)
.flatMap(repository::archive)
.collectList();
public Flux<NewPage> findNewPagesByApplicationId(
String applicationId, AclPermission permission, List<String> includeFields) {
return repository.findByApplicationId(applicationId, permission, includeFields);
}

@Override
// Remove if not used
public Mono<List<String>> 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<List<NewPage>> archivePagesByApplicationId(String applicationId, AclPermission permission) {
return findNewPagesByApplicationId(applicationId, permission)
.flatMap(repository::archive)
.collectList();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ public interface CustomNewPageRepositoryCE extends AppsmithRepository<NewPage> {

Flux<NewPage> findByApplicationId(String applicationId, AclPermission aclPermission);

Flux<NewPage> findByApplicationId(String applicationId, AclPermission aclPermission, List<String> includeFields);

Flux<NewPage> findByApplicationIdAndNonDeletedEditMode(String applicationId, AclPermission aclPermission);

Mono<NewPage> findByIdAndLayoutsIdAndViewMode(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,16 @@ public Flux<NewPage> findByApplicationId(String applicationId, AclPermission acl
.all();
}

@Override
public Flux<NewPage> findByApplicationId(
String applicationId, AclPermission aclPermission, List<String> includeFields) {
return queryBuilder()
.criteria(Bridge.equal(NewPage.Fields.applicationId, applicationId))
.permission(aclPermission)
.fields(includeFields)
.all();
}

@Override
public Flux<NewPage> findByApplicationIdAndNonDeletedEditMode(String applicationId, AclPermission aclPermission) {
BridgeQuery<NewPage> q = Bridge.<NewPage>equal(NewPage.Fields.applicationId, applicationId)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,22 @@ public Mono<PageDTO> getPage(String pageId, boolean viewMode) {
public Mono<List<NewPage>> getPagesBasedOnApplicationMode(
Application branchedApplication, ApplicationMode applicationMode) {

Boolean viewMode = ApplicationMode.PUBLISHED.equals(applicationMode) ? Boolean.TRUE : Boolean.FALSE;
Boolean viewMode = Boolean.FALSE;
List<String> 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<ApplicationPage> applicationPages = Boolean.TRUE.equals(viewMode)
? branchedApplication.getPublishedPages()
Expand All @@ -265,7 +280,8 @@ public Mono<List<NewPage>> 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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,8 @@ public Mono<ConsolidatedAPIResponseDTO> getConsolidatedInfoForPageLoad(
} else {
branchedApplicationMonoCached = applicationService
.findByBaseIdBranchNameAndApplicationMode(baseApplicationId, branchName, mode)
.name(getQualifiedSpanName(APPLICATION_ID_SPAN, mode))
.tap(Micrometer.observation(observationRegistry))
.cache();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ public void verifyGetPagesBasedOnApplicationMode_ReturnsRigthNumberOfPages_Based
.assertNext(pages -> {
assertThat(pages.size()).isEqualTo(2);
Set<String> pageNames = pages.stream()
.map(page -> page.getUnpublishedPage().getName())
.map(page -> page.getPublishedPage().getName())
.collect(Collectors.toSet());
assertThat(pageNames).contains(pageName);
assertThat(pageNames).doesNotContain(unpublishedPageName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,8 @@ public void verifyCreateApplicationPagesDTO_ReturnsRightNumberOfPages_BasedOnVie
})
.verifyComplete();

Mono<ApplicationPagesDTO> viewModeTrueMono = newPageService.createApplicationPagesDTO(
applicationService.findById(applicationId).block(), allPages, true, true);
Mono<ApplicationPagesDTO> viewModeTrueMono = Mono.defer(() -> newPageService.createApplicationPagesDTO(
applicationService.findById(applicationId).block(), allPages, true, true));

StepVerifier.create(viewModeTrueMono).verifyErrorSatisfies(error -> {
assertThat(error).isInstanceOf(AppsmithException.class);
Expand Down

0 comments on commit 83326ef

Please sign in to comment.