From 2a2dda0ab0c9d4c6133907ae06422c0c992cc3e8 Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Fri, 21 Aug 2020 16:31:40 +0530 Subject: [PATCH] Fix race condition in setting default page in application (#394) We are currently getting the *list* of all pages, updating the `isDefault` fields inside, and then saving the whole *list* of all pages. If a new page got added to that list in the DB during this process, that page would be lost. This commit fixes this problem. This race condition was causing tests for cloning applications to fail *sometimes*. --- .../CustomApplicationRepository.java | 5 ++-- .../CustomApplicationRepositoryImpl.java | 25 ++++++++++++++++--- .../services/ApplicationPageServiceImpl.java | 15 +++++------ 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomApplicationRepository.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomApplicationRepository.java index 4ace303e6b..435c489799 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomApplicationRepository.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomApplicationRepository.java @@ -2,7 +2,6 @@ package com.appsmith.server.repositories; import com.appsmith.server.acl.AclPermission; import com.appsmith.server.domains.Application; -import com.appsmith.server.domains.Page; import com.mongodb.client.result.UpdateResult; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -21,6 +20,8 @@ public interface CustomApplicationRepository extends AppsmithRepository findByClonedFromApplicationId(String applicationId, AclPermission permission); - Mono addPageToApplication(Application application, Page page, boolean isDefault); + Mono addPageToApplication(String applicationId, String pageId, boolean isDefault); + + Mono setDefaultPage(String applicationId, String pageId); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomApplicationRepositoryImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomApplicationRepositoryImpl.java index 0d41f9bb90..3718cce1fb 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomApplicationRepositoryImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomApplicationRepositoryImpl.java @@ -4,11 +4,11 @@ import com.appsmith.server.acl.AclPermission; import com.appsmith.server.constants.FieldName; import com.appsmith.server.domains.Application; import com.appsmith.server.domains.ApplicationPage; -import com.appsmith.server.domains.Page; import com.appsmith.server.domains.QApplication; import com.mongodb.client.result.UpdateResult; import lombok.NonNull; import lombok.extern.slf4j.Slf4j; +import org.bson.types.ObjectId; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.mongodb.core.ReactiveMongoOperations; import org.springframework.data.mongodb.core.convert.MongoConverter; @@ -73,13 +73,30 @@ public class CustomApplicationRepositoryImpl extends BaseAppsmithRepositoryImpl< } @Override - public Mono addPageToApplication(Application application, Page page, boolean isDefault) { - final ApplicationPage applicationPage = new ApplicationPage(page.getId(), isDefault); + public Mono addPageToApplication(String applicationId, String pageId, boolean isDefault) { + final ApplicationPage applicationPage = new ApplicationPage(pageId, isDefault); return mongoOperations.updateFirst( - Query.query(getIdCriteria(application.getId())), + Query.query(getIdCriteria(applicationId)), new Update().addToSet(FieldName.PAGES, applicationPage), Application.class ); } + @Override + public Mono setDefaultPage(String applicationId, String pageId) { + final Mono setAllAsNonDefaultMono = mongoOperations.updateFirst( + Query.query(getIdCriteria(applicationId)).addCriteria(Criteria.where("pages.isDefault").is(true)), + new Update().set("pages.$.isDefault", false), + Application.class + ); + + final Mono setDefaultMono = mongoOperations.updateFirst( + Query.query(getIdCriteria(applicationId)).addCriteria(Criteria.where("pages._id").is(new ObjectId(pageId))), + new Update().set("pages.$.isDefault", true), + Application.class + ); + + return setAllAsNonDefaultMono.then(setDefaultMono); + } + } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java index 3cf4f4368e..b475ed18af 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java @@ -124,7 +124,7 @@ public class ApplicationPageServiceImpl implements ApplicationPageService { */ @Override public Mono addPageToApplication(Application application, Page page, Boolean isDefault) { - return applicationRepository.addPageToApplication(application, page, isDefault) + return applicationRepository.addPageToApplication(application.getId(), page.getId(), isDefault) .doOnSuccess(result -> { if (result.getModifiedCount() != 1) { log.error("Add page to application didn't update anything, probably because application wasn't found."); @@ -195,14 +195,11 @@ public class ApplicationPageServiceImpl implements ApplicationPageService { }) .then(applicationService.findById(applicationId)) .switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.APPLICATION_ID, applicationId))) - .flatMap(application -> { - List pages = application.getPages(); - - // We are guaranteed to find the pageId in this list. - pages.forEach(page -> page.setIsDefault(page.getId().equals(pageId))); - - return applicationService.save(application); - }); + .flatMap(application -> + applicationRepository + .setDefaultPage(applicationId, pageId) + .then(applicationService.getById(applicationId)) + ); } @Override