Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: Add API for storing dependencyMap in page object #33296

Merged
merged 6 commits into from
May 31, 2024
Merged

chore: Add API for storing dependencyMap in page object #33296

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf2269d
Add API for storing dependencyMap in page object
AnaghHegde May 8, 2024
bec496f
add tests
AnaghHegde May 10, 2024
98ee8a2
make dependencyMap optional
AnaghHegde May 13, 2024
6180f98
use bridge query
AnaghHegde May 17, 2024
ebd8bf5
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
AnaghHegde May 17, 2024
03e9f01
add sanitisation for git
AnaghHegde May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions ...er/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/PageControllerCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
import org.springframework.web.bind.annotation.ResponseStatus;
import reactor.core.publisher.Mono;

import java.util.List;
import java.util.Map;

@RequestMapping(Url.PAGE_URL)
@RequiredArgsConstructor
@Slf4j
Expand Down Expand Up @@ -196,4 +199,15 @@ public Mono<ResponseDTO<ApplicationPagesDTO>> getAllPages(
.findApplicationPages(applicationId, pageId, branchName, mode)
.map(resources -> new ResponseDTO<>(HttpStatus.OK.value(), resources, null));
}

@JsonView(Views.Public.class)
@PutMapping("/{defaultPageId}/dependencyMap")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my information, any reason we chose to do this as a separate endpoint instead of part of update layout API? Would having a separate endpoint cause a race condition between saving the updated page and saving the new dependency map?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a valid concern and we don't have a robust approach at the moment. We are reducing the scope to use the dependency map cache only in the view mode for now. When a user publishes the application, we will trigger the updateDependencyMap API followed by the publish API. The dependency map in the unpublished version will reflect the last published version, and it will be updated with each publish.
We will get back to edit mode dependency maps later when we have a better approach for invalidation of older dependency maps.
@AnaghHegde For the time being we can go ahead with this approach.
cc: @rajatagrawal

public Mono<ResponseDTO<String>> updateDependencyMap(
@PathVariable String defaultPageId,
@RequestBody(required = false) Map<String, List<String>> dependencyMap,
@RequestHeader(name = FieldName.BRANCH_NAME, required = false) String branchName) {
return newPageService
.updateDependencyMap(defaultPageId, dependencyMap, branchName)
.map(updatedResource -> new ResponseDTO<>(HttpStatus.OK.value(), updatedResource, null));
}
}
1 change: 1 addition & 0 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/domains/NewPage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public static class Fields extends BranchAwareDomain.Fields {
public static String unpublishedPage_slug = unpublishedPage + "." + PageDTO.Fields.slug;
public static String unpublishedPage_customSlug = unpublishedPage + "." + PageDTO.Fields.customSlug;
public static String unpublishedPage_deletedAt = unpublishedPage + "." + PageDTO.Fields.deletedAt;
public static String unpublishedPage_dependencyMap = unpublishedPage + "." + PageDTO.Fields.dependencyMap;
AnaghHegde marked this conversation as resolved.
Show resolved Hide resolved

public static String publishedPage_layouts = publishedPage + "." + PageDTO.Fields.layouts;
public static String publishedPage_name = publishedPage + "." + PageDTO.Fields.name;
Expand Down
4 changes: 4 additions & 0 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/PageDTO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.time.Instant;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

@Getter
Expand Down Expand Up @@ -77,6 +78,9 @@ public class PageDTO {
@JsonView(Views.Public.class)
DefaultResources defaultResources;

@JsonView(Views.Public.class)
Map<String, List<String>> dependencyMap;
AnaghHegde marked this conversation as resolved.
Show resolved Hide resolved

public void sanitiseToExportDBObject() {
this.getLayouts().forEach(Layout::sanitiseToExportDBObject);
}
Expand Down
3 changes: 3 additions & 0 deletions ...ver/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Optional;

public interface NewPageServiceCE extends CrudService<NewPage, String> {
Expand Down Expand Up @@ -98,4 +99,6 @@ Mono<NewPage> findByGitSyncIdAndDefaultApplicationId(

Mono<ApplicationPagesDTO> createApplicationPagesDTO(
Application branchedApplication, List<NewPage> newPages, boolean viewMode, boolean isRecentlyAccessed);

Mono<String> updateDependencyMap(String pageId, Map<String, List<String>> dependencyMap, String branchName);
AnaghHegde marked this conversation as resolved.
Show resolved Hide resolved
}
23 changes: 20 additions & 3 deletions ...appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,10 +584,9 @@ public Mono<String> findBranchedPageId(String branchName, String defaultPageId,
return Mono.just(defaultPageId);
}
return repository
.findPageByBranchNameAndDefaultPageId(branchName, defaultPageId, permission)
.findBranchedPageId(branchName, defaultPageId, permission)
.switchIfEmpty(Mono.error(new AppsmithException(
AppsmithError.NO_RESOURCE_FOUND, FieldName.PAGE_ID, defaultPageId + ", " + branchName)))
.map(NewPage::getId);
AppsmithError.NO_RESOURCE_FOUND, FieldName.PAGE_ID, defaultPageId + ", " + branchName)));
}

@Override
Expand Down Expand Up @@ -670,4 +669,22 @@ public Mono<ApplicationPagesDTO> findApplicationPages(
public Mono<Void> publishPages(Collection<String> pageIds, AclPermission permission) {
return repository.publishPages(pageIds, permission);
}

@Override
public Mono<String> updateDependencyMap(String pageId, Map<String, List<String>> dependencyMap, String branchName) {
Mono<Integer> updateResult;
if (branchName != null) {
updateResult = findBranchedPageId(branchName, pageId, AclPermission.MANAGE_PAGES)
.flatMap(branchPageId -> repository.updateDependencyMap(branchPageId, dependencyMap));
} else {
updateResult = repository.updateDependencyMap(pageId, dependencyMap);
}

return updateResult.flatMap(count -> {
if (count == 0) {
return Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.PAGE, pageId));
}
return Mono.just(count.toString());
});
}
AnaghHegde marked this conversation as resolved.
Show resolved Hide resolved
}
5 changes: 5 additions & 0 deletions ...h-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Optional;

public interface CustomNewPageRepositoryCE extends AppsmithRepository<NewPage> {
Expand Down Expand Up @@ -42,4 +43,8 @@ Mono<NewPage> findByGitSyncIdAndDefaultApplicationId(
Mono<Void> publishPages(Collection<String> pageIds, AclPermission permission);

Flux<NewPage> findAllByApplicationIdsWithoutPermission(List<String> applicationIds, List<String> includeFields);

Mono<String> findBranchedPageId(String branchName, String defaultPageId, AclPermission permission);
AnaghHegde marked this conversation as resolved.
Show resolved Hide resolved

Mono<Integer> updateDependencyMap(String pageId, Map<String, List<String>> dependencyMap);
AnaghHegde marked this conversation as resolved.
Show resolved Hide resolved
}
25 changes: 25 additions & 0 deletions ...rver/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.appsmith.server.dtos.PageDTO;
import com.appsmith.server.helpers.ce.bridge.Bridge;
import com.appsmith.server.helpers.ce.bridge.BridgeQuery;
import com.appsmith.server.helpers.ce.bridge.BridgeUpdate;
import com.appsmith.server.repositories.BaseAppsmithRepositoryImpl;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
Expand All @@ -21,6 +22,7 @@

import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;

Expand Down Expand Up @@ -160,6 +162,20 @@ public Mono<NewPage> findPageByBranchNameAndDefaultPageId(
return queryBuilder().criteria(q).permission(permission).one();
}

public Mono<String> findBranchedPageId(String branchName, String defaultPageId, AclPermission permission) {
final BridgeQuery<NewPage> q =
// defaultPageIdCriteria
Bridge.equal(NewPage.Fields.defaultResources_pageId, defaultPageId);
q.equal(NewPage.Fields.defaultResources_branchName, branchName);

return queryBuilder()
.criteria(q)
.permission(permission)
.fields("id")
.one()
.map(NewPage::getId);
}
AnaghHegde marked this conversation as resolved.
Show resolved Hide resolved

@Override
public Flux<NewPage> findSlugsByApplicationIds(List<String> applicationIds, AclPermission aclPermission) {
return queryBuilder()
Expand Down Expand Up @@ -238,4 +254,13 @@ public Flux<NewPage> findAllByApplicationIdsWithoutPermission(
.fields(includeFields)
.all();
}

@Override
public Mono<Integer> updateDependencyMap(String pageId, Map<String, List<String>> dependencyMap) {
final BridgeQuery<NewPage> q = Bridge.equal(NewPage.Fields.id, pageId);

BridgeUpdate update = Bridge.update();
update.set(NewPage.Fields.unpublishedPage_dependencyMap, dependencyMap);
return queryBuilder().criteria(q).updateFirst(update);
}
}
106 changes: 106 additions & 0 deletions ...server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,10 @@
import reactor.core.publisher.Mono;
import reactor.test.StepVerifier;

import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
import java.util.function.Function;
Expand Down Expand Up @@ -406,4 +408,108 @@ public void verifyCreateApplicationPagesDTO_ReturnsRightNumberOfPages_BasedOnVie
.isEqualTo(AppsmithError.ACL_NO_RESOURCE_FOUND.getAppErrorCode());
});
}

@Test
@WithUserDetails("api_user")
public void updateDependencyMap_NotNullValue_shouldUpdateDependencyMap() {
String randomId = UUID.randomUUID().toString();
Application application = new Application();
application.setName("app_" + randomId);
Mono<NewPage> newPageMono = applicationPageService
.createApplication(application, workspaceId)
.flatMap(application1 -> {
PageDTO pageDTO = new PageDTO();
pageDTO.setName("page_" + randomId);
pageDTO.setApplicationId(application1.getId());
return applicationPageService.createPage(pageDTO);
})
.flatMap(pageDTO -> {
Map<String, List<String>> dependencyMap = new HashMap<>();
dependencyMap.put("key", List.of("val1", "val2"));
dependencyMap.put("key1", List.of("val1", "val2"));
dependencyMap.put("key2", List.of("val1", "val2"));
dependencyMap.put("key3", List.of("val1", "val2"));
return newPageService
.updateDependencyMap(pageDTO.getId(), dependencyMap, null)
.then(newPageService.findById(pageDTO.getId(), Optional.empty()));
});

StepVerifier.create(newPageMono)
.assertNext(newPage -> {
assertThat(newPage.getUnpublishedPage().getDependencyMap()).isNotNull();
assertThat(newPage.getUnpublishedPage().getDependencyMap().size())
.isEqualTo(4);
assertThat(newPage.getUnpublishedPage().getDependencyMap().get("key"))
.isEqualTo(List.of("val1", "val2"));
})
.verifyComplete();
}

@Test
@WithUserDetails("api_user")
public void updateDependencyMap_NotNullValueAndPublishApplication_shouldUpdateDependencyMap() {
String randomId = UUID.randomUUID().toString();
Application application = new Application();
application.setName("app_" + randomId);
Mono<NewPage> newPageMono = applicationPageService
.createApplication(application, workspaceId)
.flatMap(application1 -> {
PageDTO pageDTO = new PageDTO();
pageDTO.setName("page_" + randomId);
pageDTO.setApplicationId(application1.getId());
return applicationPageService.createPage(pageDTO);
})
.flatMap(pageDTO -> {
Map<String, List<String>> dependencyMap = new HashMap<>();
dependencyMap.put("key", List.of("val1", "val2"));
dependencyMap.put("key1", List.of("val1", "val2"));
dependencyMap.put("key2", List.of("val1", "val2"));
dependencyMap.put("key3", List.of("val1", "val2"));
return newPageService
.updateDependencyMap(pageDTO.getId(), dependencyMap, null)
.flatMap(page -> applicationPageService.publish(application.getId(), null, false))
.then(newPageService.findById(pageDTO.getId(), Optional.empty()));
});

StepVerifier.create(newPageMono)
.assertNext(newPage -> {
assertThat(newPage.getUnpublishedPage().getDependencyMap()).isNotNull();
assertThat(newPage.getUnpublishedPage().getDependencyMap().size())
.isEqualTo(4);
assertThat(newPage.getUnpublishedPage().getDependencyMap().get("key"))
.isEqualTo(List.of("val1", "val2"));

assertThat(newPage.getPublishedPage().getDependencyMap()).isNotNull();
assertThat(newPage.getPublishedPage().getDependencyMap().size())
.isEqualTo(4);
assertThat(newPage.getPublishedPage().getDependencyMap().get("key"))
.isEqualTo(List.of("val1", "val2"));
})
.verifyComplete();
}

@Test
@WithUserDetails("api_user")
public void updateDependencyMap_nullValue_shouldUpdateDependencyMap() {
String randomId = UUID.randomUUID().toString();
Application application = new Application();
application.setName("app_" + randomId);
Mono<NewPage> newPageMono = applicationPageService
.createApplication(application, workspaceId)
.flatMap(application1 -> {
PageDTO pageDTO = new PageDTO();
pageDTO.setName("page_" + randomId);
pageDTO.setApplicationId(application1.getId());
return applicationPageService.createPage(pageDTO);
})
.flatMap(pageDTO -> newPageService
.updateDependencyMap(pageDTO.getId(), null, null)
.then(newPageService.findById(pageDTO.getId(), Optional.empty())));

StepVerifier.create(newPageMono)
.assertNext(newPage -> {
assertThat(newPage.getUnpublishedPage().getDependencyMap()).isNull();
})
.verifyComplete();
}
}