Protei/PromucFlow_constructor
2
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

Fix race condition in setting default page in application (#394)

Browse Source 
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*.
This commit is contained in:
Shrikant Sharat Kandula 2020-08-21 16:31:40 +05:30 committed by GitHub
parent d6dbabf920
commit 2a2dda0ab0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 30 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomApplicationRepository.java
View File

@ -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<Applicat
Flux<Application> findByClonedFromApplicationId(String applicationId, AclPermission permission);
Mono<UpdateResult> addPageToApplication(Application application, Page page, boolean isDefault);
Mono<UpdateResult> addPageToApplication(String applicationId, String pageId, boolean isDefault);
Mono<UpdateResult> setDefaultPage(String applicationId, String pageId);
}

25
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomApplicationRepositoryImpl.java
View File

@ -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<UpdateResult> addPageToApplication(Application application, Page page, boolean isDefault) {
final ApplicationPage applicationPage = new ApplicationPage(page.getId(), isDefault);
public Mono<UpdateResult> 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<UpdateResult> setDefaultPage(String applicationId, String pageId) {
final Mono<UpdateResult> setAllAsNonDefaultMono = mongoOperations.updateFirst(
Query.query(getIdCriteria(applicationId)).addCriteria(Criteria.where("pages.isDefault").is(true)),
new Update().set("pages.$.isDefault", false),
Application.class
);
final Mono<UpdateResult> 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);
}
}

15
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java
View File

@ -124,7 +124,7 @@ public class ApplicationPageServiceImpl implements ApplicationPageService {
*/
@Override
public Mono<UpdateResult> 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<ApplicationPage> 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