From 0ad15b4cb6aaee9288e0c320081ed3ba295590d8 Mon Sep 17 00:00:00 2001 From: Shrikant Kandula Date: Wed, 8 Apr 2020 12:09:41 +0000 Subject: [PATCH] Fix: Duplicate key error when reusing the name of a deleted application --- .../appsmith/external/models/BaseDomain.java | 9 +++++ .../appsmith/server/constants/FieldName.java | 1 + .../server/migrations/DatabaseChangelog.java | 34 +++++++++++++++---- .../repositories/BaseRepositoryImpl.java | 18 +++++++--- .../services/ApplicationServiceTest.java | 29 ++++++++++++++++ 5 files changed, 81 insertions(+), 10 deletions(-) diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/BaseDomain.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/BaseDomain.java index 96452fd421..1089f1936e 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/BaseDomain.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/BaseDomain.java @@ -44,12 +44,21 @@ public abstract class BaseDomain implements Persistable { @LastModifiedBy protected String modifiedBy; + // Deprecating this so we can move on to using `deletedAt` for all domain models. + @Deprecated(forRemoval = true) protected Boolean deleted = false; + protected Instant deletedAt = null; + @JsonIgnore @Override public boolean isNew() { return this.getId() == null; } + @JsonIgnore + public boolean isDeleted() { + return this.getDeletedAt() != null || Boolean.TRUE.equals(getDeleted()); + } + } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/constants/FieldName.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/constants/FieldName.java index 41f3ce3d70..ce23e192ac 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/constants/FieldName.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/constants/FieldName.java @@ -4,6 +4,7 @@ public class FieldName { public static final String EMAIL = "email"; public static final String ORGANIZATION_ID = "organizationId"; public static final String DELETED = "deleted"; + public static final String DELETED_AT = "deletedAt"; public static String ORGANIZATION = "organization"; public static String ID = "id"; public static String NAME = "name"; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog.java index 647bccf1f8..04ea6c6663 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog.java @@ -1,6 +1,7 @@ package com.appsmith.server.migrations; import com.appsmith.server.domains.*; +import com.appsmith.server.services.ApplicationService; import com.appsmith.server.services.OrganizationService; import com.github.cloudyrock.mongock.ChangeLog; import com.github.cloudyrock.mongock.ChangeSet; @@ -49,6 +50,15 @@ public class DatabaseChangelog { } } + private static void dropIndexIfExists(MongoTemplate mongoTemplate, Class entityClass, String name) { + try { + mongoTemplate.indexOps(entityClass).dropIndex(name); + } catch (UncategorizedMongoDbException ignored) { + // The index probably doesn't exist. This happens if the database is created after the @Indexed annotation + // has been removed. + } + } + @ChangeSet(order = "001", id = "initial-plugins", author = "") public void initialPlugins(MongoTemplate mongoTemplate) { Plugin plugin1 = new Plugin(); @@ -78,12 +88,7 @@ public class DatabaseChangelog { @ChangeSet(order = "002", id = "remove-org-name-index", author = "") public void removeOrgNameIndex(MongoTemplate mongoTemplate) { - try { - mongoTemplate.indexOps(Organization.class).dropIndex("name"); - } catch (UncategorizedMongoDbException ignored) { - // The index probably doesn't exist. This happens if the database is created after the @Indexed annotation - // has been removed. - } + dropIndexIfExists(mongoTemplate, Organization.class, "name"); } @ChangeSet(order = "003", id = "add-org-slugs", author = "") @@ -188,4 +193,21 @@ public class DatabaseChangelog { ); } + @ChangeSet(order = "005", id = "application-deleted-at", author = "") + public void addApplicationDeletedAtFieldAndIndex(MongoTemplate mongoTemplate) { + dropIndexIfExists(mongoTemplate, Application.class, "organization_application_compound_index"); + + ensureIndexes(mongoTemplate, Application.class, + makeIndex("organizationId", "name", "deletedAt") + .unique().named("organization_application_deleted_compound_index") + ); + + for (Application application : mongoTemplate.findAll(Application.class)) { + if (application.isDeleted()) { + application.setDeletedAt(application.getUpdatedAt()); + mongoTemplate.save(application); + } + } + } + } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/BaseRepositoryImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/BaseRepositoryImpl.java index fb79a31e47..d35d8f134f 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/BaseRepositoryImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/BaseRepositoryImpl.java @@ -15,6 +15,7 @@ import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import java.io.Serializable; +import java.time.Instant; import java.util.List; import static org.springframework.data.mongodb.core.query.Criteria.where; @@ -48,9 +49,15 @@ public class BaseRepositoryImpl e } private Criteria notDeleted() { - return new Criteria().orOperator( - where("deleted").exists(false), - where("deleted").is(false) + return new Criteria().andOperator( + new Criteria().orOperator( + where(FieldName.DELETED).exists(false), + where(FieldName.DELETED).is(false) + ), + new Criteria().orOperator( + where(FieldName.DELETED_AT).exists(false), + where(FieldName.DELETED_AT).is(null) + ) ); } @@ -79,9 +86,10 @@ public class BaseRepositoryImpl e public Mono archive(T entity) { Assert.notNull(entity, "The given entity must not be null!"); Assert.notNull(entity.getId(), "The given entity's id must not be null!"); - Assert.isTrue(!entity.getDeleted(), "The given entity is already deleted"); + Assert.isTrue(!entity.isDeleted(), "The given entity is already deleted"); entity.setDeleted(true); + entity.setDeletedAt(Instant.now()); return mongoOperations.save(entity, entityInformation.getCollectionName()); } @@ -93,6 +101,7 @@ public class BaseRepositoryImpl e Update update = new Update(); update.set(FieldName.DELETED, true); + update.set(FieldName.DELETED_AT, Instant.now()); return mongoOperations.updateFirst(query, update, entityInformation.getJavaType()) .map(result -> result.getModifiedCount() > 0 ? true : false); } @@ -108,6 +117,7 @@ public class BaseRepositoryImpl e Update update = new Update(); update.set(FieldName.DELETED, true); + update.set(FieldName.DELETED_AT, Instant.now()); return mongoOperations.updateMulti(query, update, entityInformation.getJavaType()) .map(result -> result.getModifiedCount() > 0 ? true : false); } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java index 04cd2538a5..5016ae01e1 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationServiceTest.java @@ -124,4 +124,33 @@ public class ApplicationServiceTest { .verifyComplete(); } + @Test + @WithUserDetails(value = "api_user") + public void reuseDeletedAppName() { + Application firstApp = new Application(); + firstApp.setName("Ghost app"); + + Application secondApp = new Application(); + secondApp.setName("Ghost app"); + + Mono firstAppDeletion = applicationPageService + .createApplication(firstApp) + .flatMap(app -> applicationService.archive(app)) + .cache(); + + Mono secondAppCreation = firstAppDeletion.then( + applicationPageService.createApplication(secondApp)); + + StepVerifier + .create(Mono.zip(firstAppDeletion, secondAppCreation)) + .assertNext(tuple2 -> { + Application first = tuple2.getT1(), second = tuple2.getT2(); + assertThat(first.getName()).isEqualTo("Ghost app"); + assertThat(second.getName()).isEqualTo("Ghost app"); + assertThat(first.isDeleted()).isTrue(); + assertThat(second.isDeleted()).isFalse(); + }) + .verifyComplete(); + } + }