From 9b144b685b8e88550fd3c64a1076e6935c191ebe Mon Sep 17 00:00:00 2001 From: nilansh Date: Thu, 9 May 2024 11:47:54 +0530 Subject: [PATCH 01/15] initial setup --- .../com/appsmith/server/domains/Tenant.java | 4 ++- .../server/domains/TenantConfiguration.java | 4 ++- .../ce/CacheableRepositoryHelperCE.java | 5 +++ .../ce/CacheableRepositoryHelperCEImpl.java | 26 ++++++++++++++ .../server/services/TenantServiceImpl.java | 13 +++++-- .../services/ce/TenantServiceCEImpl.java | 35 +++++++++---------- 6 files changed, 65 insertions(+), 22 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Tenant.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Tenant.java index 2790bc63f175..962048a7ec79 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Tenant.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Tenant.java @@ -10,13 +10,15 @@ import org.springframework.data.annotation.Transient; import org.springframework.data.mongodb.core.mapping.Document; +import java.io.Serializable; + @Getter @Setter @ToString @NoArgsConstructor @Document @FieldNameConstants -public class Tenant extends BaseDomain { +public class Tenant extends BaseDomain implements Serializable { @Unique String slug; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/TenantConfiguration.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/TenantConfiguration.java index e4b293b814a7..1496e345a9d2 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/TenantConfiguration.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/TenantConfiguration.java @@ -4,6 +4,8 @@ import lombok.Data; import lombok.EqualsAndHashCode; +import java.io.Serializable; + @Data @EqualsAndHashCode(callSuper = true) -public class TenantConfiguration extends TenantConfigurationCE {} +public class TenantConfiguration extends TenantConfigurationCE implements Serializable {} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java index b86255051243..507bb066c376 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java @@ -1,5 +1,6 @@ package com.appsmith.server.repositories.ce; +import com.appsmith.server.domains.Tenant; import com.appsmith.server.domains.User; import reactor.core.publisher.Mono; @@ -18,4 +19,8 @@ public interface CacheableRepositoryHelperCE { Mono getDefaultTenantId(); Mono getInstanceAdminPermissionGroupId(); + + Mono fetchCachedTenant(String tenantId); + + Mono evictCachedTenant(String tenantId); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java index 1415d880d9cc..a5cf30f275a1 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java @@ -6,6 +6,7 @@ import com.appsmith.server.domains.Config; import com.appsmith.server.domains.PermissionGroup; import com.appsmith.server.domains.Tenant; +import com.appsmith.server.domains.TenantConfiguration; import com.appsmith.server.domains.User; import com.appsmith.server.domains.Workspace; import com.appsmith.server.exceptions.AppsmithError; @@ -166,4 +167,29 @@ public Mono getInstanceAdminPermissionGroupId() { .doOnSuccess(permissionGroupId -> inMemoryCacheableRepositoryHelper.setInstanceAdminPermissionGroupId(permissionGroupId)); } + + @Cache(cacheName = "defaultTenant", key = "{#tenantId}") + @Override + public Mono fetchCachedTenant(String tenantId) { + // Get the default tenant object from the DB and then populate the relevant user permissions in that + // We are doing this differently because `findBySlug` is a Mongo JPA query and not a custom Appsmith query + BridgeQuery defaultTenantCriteria = Bridge.equal(Tenant.Fields.slug, FieldName.DEFAULT); + Query query = new Query(); + query.addCriteria(defaultTenantCriteria); + + return mongoOperations.findOne(query, Tenant.class).map(tenant -> { + if (tenant.getTenantConfiguration() == null) { + tenant.setTenantConfiguration(new TenantConfiguration()); + } + return tenant; + }); + // .flatMap(tenant -> + // setUserPermissionsInObject(tenant).switchIfEmpty(Mono.just(tenant))); + } + + @CacheEvict(cacheName = "defaultTenant", key = "{#tenantId}") + @Override + public Mono evictCachedTenant(String tenantId) { + return Mono.empty(); + } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/TenantServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/TenantServiceImpl.java index ba038be16b12..874f46f2f34a 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/TenantServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/TenantServiceImpl.java @@ -1,6 +1,7 @@ package com.appsmith.server.services; import com.appsmith.server.helpers.FeatureFlagMigrationHelper; +import com.appsmith.server.repositories.CacheableRepositoryHelper; import com.appsmith.server.repositories.TenantRepository; import com.appsmith.server.services.ce.TenantServiceCEImpl; import com.appsmith.server.solutions.EnvManager; @@ -19,7 +20,15 @@ public TenantServiceImpl( AnalyticsService analyticsService, ConfigService configService, @Lazy EnvManager envManager, - FeatureFlagMigrationHelper featureFlagMigrationHelper) { - super(validator, repository, analyticsService, configService, envManager, featureFlagMigrationHelper); + FeatureFlagMigrationHelper featureFlagMigrationHelper, + CacheableRepositoryHelper cacheableRepositoryHelper) { + super( + validator, + repository, + analyticsService, + configService, + envManager, + featureFlagMigrationHelper, + cacheableRepositoryHelper); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java index c40c0f27e843..8c279de4c756 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java @@ -12,6 +12,7 @@ import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.CollectionUtils; import com.appsmith.server.helpers.FeatureFlagMigrationHelper; +import com.appsmith.server.repositories.CacheableRepositoryHelper; import com.appsmith.server.repositories.TenantRepository; import com.appsmith.server.services.AnalyticsService; import com.appsmith.server.services.BaseService; @@ -39,17 +40,21 @@ public class TenantServiceCEImpl extends BaseService getDefaultTenantId() { @Override public Mono updateTenantConfiguration(String tenantId, TenantConfiguration tenantConfiguration) { - return repository - .findById(tenantId, MANAGE_TENANT) + Mono evictTenantCache = cacheableRepositoryHelper.evictCachedTenant(tenantId); + return evictTenantCache + .then(repository.findById(tenantId, MANAGE_TENANT)) .switchIfEmpty(Mono.error( new AppsmithException(AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.TENANT, tenantId))) .flatMap(tenant -> { @@ -89,11 +95,13 @@ public Mono updateTenantConfiguration(String tenantId, TenantConfigurati return Mono.empty(); }); } - return envMono.then(Mono.zip(Mono.just(oldtenantConfiguration), Mono.just(tenant))); + + return envMono.then( + Mono.zip(evictTenantCache, Mono.just(oldtenantConfiguration), Mono.just(tenant))); }) - .flatMap(tuple2 -> { - Tenant tenant = tuple2.getT2(); - TenantConfiguration oldConfig = tuple2.getT1(); + .flatMap(tuple3 -> { + Tenant tenant = tuple3.getT3(); + TenantConfiguration oldConfig = tuple3.getT2(); AppsmithBeanUtils.copyNestedNonNullProperties(tenantConfiguration, oldConfig); tenant.setTenantConfiguration(oldConfig); return repository.updateById(tenantId, tenant, MANAGE_TENANT); @@ -151,17 +159,8 @@ public Mono getTenantConfiguration() { @Override public Mono getDefaultTenant() { - // Get the default tenant object from the DB and then populate the relevant user permissions in that - // We are doing this differently because `findBySlug` is a Mongo JPA query and not a custom Appsmith query - return repository - .findBySlug(FieldName.DEFAULT) - .map(tenant -> { - if (tenant.getTenantConfiguration() == null) { - tenant.setTenantConfiguration(new TenantConfiguration()); - } - return tenant; - }) - .flatMap(tenant -> repository.setUserPermissionsInObject(tenant).switchIfEmpty(Mono.just(tenant))); + // Fetching Tenant from cache + return getDefaultTenantId().flatMap(tenantId -> cacheableRepositoryHelper.fetchCachedTenant(tenantId)); } @Override From 9f372344f21da6cfe118e39cc8717c0cf4505b64 Mon Sep 17 00:00:00 2001 From: nilansh Date: Thu, 9 May 2024 13:12:31 +0530 Subject: [PATCH 02/15] feat: added tenant configuration to redis cache (#33083) --- .../ce/CacheableRepositoryHelperCEImpl.java | 2 -- .../server/services/ce/TenantServiceCEImpl.java | 13 +++++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java index a5cf30f275a1..5e7afe6fe456 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java @@ -183,8 +183,6 @@ public Mono fetchCachedTenant(String tenantId) { } return tenant; }); - // .flatMap(tenant -> - // setUserPermissionsInObject(tenant).switchIfEmpty(Mono.just(tenant))); } @CacheEvict(cacheName = "defaultTenant", key = "{#tenantId}") diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java index 8c279de4c756..cd15139a21e0 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java @@ -96,12 +96,11 @@ public Mono updateTenantConfiguration(String tenantId, TenantConfigurati }); } - return envMono.then( - Mono.zip(evictTenantCache, Mono.just(oldtenantConfiguration), Mono.just(tenant))); + return envMono.then(Mono.zip(Mono.just(oldtenantConfiguration), Mono.just(tenant))); }) - .flatMap(tuple3 -> { - Tenant tenant = tuple3.getT3(); - TenantConfiguration oldConfig = tuple3.getT2(); + .flatMap(tuple2 -> { + Tenant tenant = tuple2.getT2(); + TenantConfiguration oldConfig = tuple2.getT1(); AppsmithBeanUtils.copyNestedNonNullProperties(tenantConfiguration, oldConfig); tenant.setTenantConfiguration(oldConfig); return repository.updateById(tenantId, tenant, MANAGE_TENANT); @@ -160,7 +159,9 @@ public Mono getTenantConfiguration() { @Override public Mono getDefaultTenant() { // Fetching Tenant from cache - return getDefaultTenantId().flatMap(tenantId -> cacheableRepositoryHelper.fetchCachedTenant(tenantId)); + return getDefaultTenantId() + .flatMap(tenantId -> cacheableRepositoryHelper.fetchCachedTenant(tenantId)) + .flatMap(tenant -> repository.setUserPermissionsInObject(tenant).switchIfEmpty(Mono.just(tenant))); } @Override From e889d834532dab696c14899279efe22562486a33 Mon Sep 17 00:00:00 2001 From: nilansh Date: Thu, 9 May 2024 13:25:33 +0530 Subject: [PATCH 03/15] code optimised to replace usage of tenantRepository to cached function --- .../repositories/ce/CacheableRepositoryHelperCEImpl.java | 8 ++++++-- .../server/repositories/ce/TenantRepositoryCE.java | 2 +- .../services/ce/CacheableFeatureFlagHelperCEImpl.java | 8 +++----- .../appsmith/server/services/ce/TenantServiceCEImpl.java | 4 ++-- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java index 5e7afe6fe456..c60230e8e527 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java @@ -168,11 +168,15 @@ public Mono getInstanceAdminPermissionGroupId() { inMemoryCacheableRepositoryHelper.setInstanceAdminPermissionGroupId(permissionGroupId)); } + /** + * Returns the default tenant from the cache if present. + * If not present in cache, then it fetches the default tenant from the database and adds to redis. + * @param tenantId + * @return + */ @Cache(cacheName = "defaultTenant", key = "{#tenantId}") @Override public Mono fetchCachedTenant(String tenantId) { - // Get the default tenant object from the DB and then populate the relevant user permissions in that - // We are doing this differently because `findBySlug` is a Mongo JPA query and not a custom Appsmith query BridgeQuery defaultTenantCriteria = Bridge.equal(Tenant.Fields.slug, FieldName.DEFAULT); Query query = new Query(); query.addCriteria(defaultTenantCriteria); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java index 5f6cbe099db7..59fecb5b890e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java @@ -5,6 +5,6 @@ import reactor.core.publisher.Mono; public interface TenantRepositoryCE extends BaseRepository, CustomTenantRepositoryCE { - + // Use tenantService.getDefaultTenant() instead of this method as it is cached to redis. Mono findBySlug(String slug); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCEImpl.java index 08742ad940f4..7f18b84041fe 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCEImpl.java @@ -16,8 +16,8 @@ import com.appsmith.server.featureflags.FeatureFlagIdentityTraits; import com.appsmith.server.helpers.CollectionUtils; import com.appsmith.server.helpers.SignatureVerifier; -import com.appsmith.server.repositories.TenantRepository; import com.appsmith.server.services.ConfigService; +import com.appsmith.server.services.TenantService; import com.appsmith.server.services.UserIdentifierService; import com.appsmith.server.solutions.ReleaseNotesService; import com.appsmith.util.WebClientUtils; @@ -40,13 +40,11 @@ import java.util.Set; import static com.appsmith.server.constants.ApiConstants.CLOUD_SERVICES_SIGNATURE; -import static com.appsmith.server.constants.ce.FieldNameCE.DEFAULT; @Slf4j @RequiredArgsConstructor public class CacheableFeatureFlagHelperCEImpl implements CacheableFeatureFlagHelperCE { - - private final TenantRepository tenantRepository; + private final TenantService tenantService; private final ConfigService configService; private final CloudServicesConfig cloudServicesConfig; private final CommonConfig commonConfig; @@ -112,7 +110,7 @@ public Mono evictUserCachedFlags(String userIdentifier) { private Mono> forceAllRemoteFeatureFlagsForUser(String userIdentifier, User user) { Mono instanceIdMono = configService.getInstanceId(); // TODO: Convert to current tenant when the feature is enabled - Mono defaultTenantMono = tenantRepository.findBySlug(DEFAULT); + Mono defaultTenantMono = tenantService.getDefaultTenant(); return Mono.zip(instanceIdMono, defaultTenantMono, getUserDefaultTraits(user)) .flatMap(objects -> { String tenantId = objects.getT2().getId(); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java index cd15139a21e0..9d4456d0c5a8 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java @@ -65,7 +65,7 @@ public Mono getDefaultTenantId() { return Mono.just(tenantId); } - return repository.findBySlug(FieldName.DEFAULT).map(Tenant::getId).map(tenantId -> { + return this.getDefaultTenant().map(Tenant::getId).map(tenantId -> { // Set the cache value before returning. this.tenantId = tenantId; return tenantId; @@ -158,7 +158,7 @@ public Mono getTenantConfiguration() { @Override public Mono getDefaultTenant() { - // Fetching Tenant from cache + // Fetching Tenant from redis cache return getDefaultTenantId() .flatMap(tenantId -> cacheableRepositoryHelper.fetchCachedTenant(tenantId)) .flatMap(tenant -> repository.setUserPermissionsInObject(tenant).switchIfEmpty(Mono.just(tenant))); From b2c301e6c3c33667e2a9030fda60ed580be3655a Mon Sep 17 00:00:00 2001 From: nilansh Date: Thu, 9 May 2024 15:38:34 +0530 Subject: [PATCH 04/15] fixed cyclic dependency --- .../services/ce/CacheableFeatureFlagHelperCEImpl.java | 7 ++++--- .../appsmith/server/services/ce/TenantServiceCEImpl.java | 3 +-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCEImpl.java index 7f18b84041fe..b773e6c6b548 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CacheableFeatureFlagHelperCEImpl.java @@ -16,8 +16,8 @@ import com.appsmith.server.featureflags.FeatureFlagIdentityTraits; import com.appsmith.server.helpers.CollectionUtils; import com.appsmith.server.helpers.SignatureVerifier; +import com.appsmith.server.repositories.TenantRepository; import com.appsmith.server.services.ConfigService; -import com.appsmith.server.services.TenantService; import com.appsmith.server.services.UserIdentifierService; import com.appsmith.server.solutions.ReleaseNotesService; import com.appsmith.util.WebClientUtils; @@ -40,11 +40,12 @@ import java.util.Set; import static com.appsmith.server.constants.ApiConstants.CLOUD_SERVICES_SIGNATURE; +import static com.appsmith.server.constants.ce.FieldNameCE.DEFAULT; @Slf4j @RequiredArgsConstructor public class CacheableFeatureFlagHelperCEImpl implements CacheableFeatureFlagHelperCE { - private final TenantService tenantService; + private final TenantRepository tenantRepository; private final ConfigService configService; private final CloudServicesConfig cloudServicesConfig; private final CommonConfig commonConfig; @@ -110,7 +111,7 @@ public Mono evictUserCachedFlags(String userIdentifier) { private Mono> forceAllRemoteFeatureFlagsForUser(String userIdentifier, User user) { Mono instanceIdMono = configService.getInstanceId(); // TODO: Convert to current tenant when the feature is enabled - Mono defaultTenantMono = tenantService.getDefaultTenant(); + Mono defaultTenantMono = tenantRepository.findBySlug(DEFAULT); return Mono.zip(instanceIdMono, defaultTenantMono, getUserDefaultTraits(user)) .flatMap(objects -> { String tenantId = objects.getT2().getId(); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java index 9d4456d0c5a8..11e830820699 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java @@ -64,8 +64,7 @@ public Mono getDefaultTenantId() { if (StringUtils.hasLength(tenantId)) { return Mono.just(tenantId); } - - return this.getDefaultTenant().map(Tenant::getId).map(tenantId -> { + return repository.findBySlug(FieldName.DEFAULT).map(Tenant::getId).map(tenantId -> { // Set the cache value before returning. this.tenantId = tenantId; return tenantId; From 883b05a775093337c3f1419ff97d653164fd0481 Mon Sep 17 00:00:00 2001 From: Nilansh Bansal Date: Fri, 10 May 2024 13:14:20 +0530 Subject: [PATCH 05/15] Update app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java Co-authored-by: Arpit Mohan --- .../com/appsmith/server/services/ce/TenantServiceCEImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java index 11e830820699..2dd5fa0205ae 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java @@ -40,7 +40,7 @@ public class TenantServiceCEImpl extends BaseService Date: Fri, 10 May 2024 14:19:33 +0530 Subject: [PATCH 06/15] cache evicted after db update to avoid inconsistency --- .../server/services/ce/TenantServiceCEImpl.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java index 11e830820699..cab3a5e3a489 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java @@ -74,8 +74,8 @@ public Mono getDefaultTenantId() { @Override public Mono updateTenantConfiguration(String tenantId, TenantConfiguration tenantConfiguration) { Mono evictTenantCache = cacheableRepositoryHelper.evictCachedTenant(tenantId); - return evictTenantCache - .then(repository.findById(tenantId, MANAGE_TENANT)) + return repository + .findById(tenantId, MANAGE_TENANT) .switchIfEmpty(Mono.error( new AppsmithException(AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.TENANT, tenantId))) .flatMap(tenant -> { @@ -102,7 +102,13 @@ public Mono updateTenantConfiguration(String tenantId, TenantConfigurati TenantConfiguration oldConfig = tuple2.getT1(); AppsmithBeanUtils.copyNestedNonNullProperties(tenantConfiguration, oldConfig); tenant.setTenantConfiguration(oldConfig); - return repository.updateById(tenantId, tenant, MANAGE_TENANT); + Mono updatedTenantMono = repository + .updateById(tenantId, tenant, MANAGE_TENANT) + .cache(); + // Firstly updating the Tenant object in the database and then evicting the cache. + // returning the updatedTenant, notice the updatedTenantMono is cached using .cache() + // hence it will not be evaluated again + return updatedTenantMono.then(evictTenantCache).then(updatedTenantMono); }); } From 9543f944208b6958d6658508e9a761c54e20b1f6 Mon Sep 17 00:00:00 2001 From: nilansh Date: Mon, 13 May 2024 10:13:02 +0530 Subject: [PATCH 07/15] fixed tests --- .../services/ce/TenantServiceCETest.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java index 4710801319ff..1fe539b9ff14 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java @@ -196,6 +196,8 @@ void setEmailVerificationEnabled_WithInvalidSMTPHost_ReturnsError() { @Test @WithUserDetails("api_user") void setEmailVerificationEnabled_WithValidSMTPHost_Success() { + Tenant tenant = tenantService.getDefaultTenant().block(); + String tenantId = tenant.getId(); final TenantConfiguration changes = new TenantConfiguration(); changes.setEmailVerificationEnabled(TRUE); @@ -207,7 +209,7 @@ void setEmailVerificationEnabled_WithValidSMTPHost_Success() { Mockito.when(envManager.getAllNonEmpty()).thenReturn(Mono.just(envVars)); final Mono resultMono = tenantService - .updateDefaultTenantConfiguration(changes) + .updateTenantConfiguration(tenantId, changes) .then(tenantService.getTenantConfiguration()) .map(Tenant::getTenantConfiguration); @@ -325,19 +327,17 @@ void checkAndExecuteMigrationsForTenantFeatureFlags_withPendingMigration_getUpda void updateTenantConfiguration_updateStrongPasswordPolicy_success() { // Ensure that the default tenant does not have strong password policy setup - Mono tenantMono = tenantService.getDefaultTenant(); - StepVerifier.create(tenantMono) - .assertNext(tenant -> { - assertThat(tenant.getTenantConfiguration().getIsStrongPasswordPolicyEnabled()) - .isNull(); - }) - .verifyComplete(); + Tenant tenant = tenantService.getDefaultTenant().block(); + String tenantId = tenant.getId(); + assertThat(tenant.getTenantConfiguration().getIsStrongPasswordPolicyEnabled()) + .isNull(); // Ensure that the strong password policy is enabled after the update final TenantConfiguration changes = new TenantConfiguration(); changes.setIsStrongPasswordPolicyEnabled(TRUE); + Mono resultMono = tenantService - .updateDefaultTenantConfiguration(changes) + .updateTenantConfiguration(tenantId, changes) .then(tenantService.getTenantConfiguration()) .map(Tenant::getTenantConfiguration); @@ -350,8 +350,9 @@ void updateTenantConfiguration_updateStrongPasswordPolicy_success() { // Ensure that the strong password policy is disabled after the update changes.setIsStrongPasswordPolicyEnabled(FALSE); + resultMono = tenantService - .updateDefaultTenantConfiguration(changes) + .updateTenantConfiguration(tenantId, changes) .then(tenantService.getTenantConfiguration()) .map(Tenant::getTenantConfiguration); From 958f5e26f939eef74a043d60fb3a60d911a19869 Mon Sep 17 00:00:00 2001 From: nilansh Date: Mon, 13 May 2024 13:55:51 +0530 Subject: [PATCH 08/15] added implements serializable for tenant configuration --- .../domains/ce/TenantConfigurationCE.java | 3 ++- .../ce/CacheableRepositoryHelperCEImpl.java | 2 +- .../services/ce/TenantServiceCEImpl.java | 5 ++++- .../services/ce/TenantServiceCETest.java | 21 +++++++++---------- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/TenantConfigurationCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/TenantConfigurationCE.java index 4d11d668be64..39f8a3a28825 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/TenantConfigurationCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/TenantConfigurationCE.java @@ -10,12 +10,13 @@ import lombok.Data; import org.apache.commons.lang3.ObjectUtils; +import java.io.Serializable; import java.util.ArrayList; import java.util.List; import java.util.Map; @Data -public class TenantConfigurationCE { +public class TenantConfigurationCE implements Serializable { private String googleMapsKey; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java index c60230e8e527..50ac49d92e9b 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java @@ -192,6 +192,6 @@ public Mono fetchCachedTenant(String tenantId) { @CacheEvict(cacheName = "defaultTenant", key = "{#tenantId}") @Override public Mono evictCachedTenant(String tenantId) { - return Mono.empty(); + return Mono.empty().then(); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java index b3ddbf54cdc3..24ef373a4dfe 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java @@ -108,7 +108,10 @@ public Mono updateTenantConfiguration(String tenantId, TenantConfigurati // Firstly updating the Tenant object in the database and then evicting the cache. // returning the updatedTenant, notice the updatedTenantMono is cached using .cache() // hence it will not be evaluated again - return updatedTenantMono.then(evictTenantCache).then(updatedTenantMono); + // return updatedTenantMono.then(evictTenantCache).then(updatedTenantMono); + return updatedTenantMono + .then(Mono.defer(() -> evictTenantCache)) + .then(updatedTenantMono); }); } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java index 1fe539b9ff14..4710801319ff 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java @@ -196,8 +196,6 @@ void setEmailVerificationEnabled_WithInvalidSMTPHost_ReturnsError() { @Test @WithUserDetails("api_user") void setEmailVerificationEnabled_WithValidSMTPHost_Success() { - Tenant tenant = tenantService.getDefaultTenant().block(); - String tenantId = tenant.getId(); final TenantConfiguration changes = new TenantConfiguration(); changes.setEmailVerificationEnabled(TRUE); @@ -209,7 +207,7 @@ void setEmailVerificationEnabled_WithValidSMTPHost_Success() { Mockito.when(envManager.getAllNonEmpty()).thenReturn(Mono.just(envVars)); final Mono resultMono = tenantService - .updateTenantConfiguration(tenantId, changes) + .updateDefaultTenantConfiguration(changes) .then(tenantService.getTenantConfiguration()) .map(Tenant::getTenantConfiguration); @@ -327,17 +325,19 @@ void checkAndExecuteMigrationsForTenantFeatureFlags_withPendingMigration_getUpda void updateTenantConfiguration_updateStrongPasswordPolicy_success() { // Ensure that the default tenant does not have strong password policy setup - Tenant tenant = tenantService.getDefaultTenant().block(); - String tenantId = tenant.getId(); - assertThat(tenant.getTenantConfiguration().getIsStrongPasswordPolicyEnabled()) - .isNull(); + Mono tenantMono = tenantService.getDefaultTenant(); + StepVerifier.create(tenantMono) + .assertNext(tenant -> { + assertThat(tenant.getTenantConfiguration().getIsStrongPasswordPolicyEnabled()) + .isNull(); + }) + .verifyComplete(); // Ensure that the strong password policy is enabled after the update final TenantConfiguration changes = new TenantConfiguration(); changes.setIsStrongPasswordPolicyEnabled(TRUE); - Mono resultMono = tenantService - .updateTenantConfiguration(tenantId, changes) + .updateDefaultTenantConfiguration(changes) .then(tenantService.getTenantConfiguration()) .map(Tenant::getTenantConfiguration); @@ -350,9 +350,8 @@ void updateTenantConfiguration_updateStrongPasswordPolicy_success() { // Ensure that the strong password policy is disabled after the update changes.setIsStrongPasswordPolicyEnabled(FALSE); - resultMono = tenantService - .updateTenantConfiguration(tenantId, changes) + .updateDefaultTenantConfiguration(changes) .then(tenantService.getTenantConfiguration()) .map(Tenant::getTenantConfiguration); From d4690479d363ac86b87b8abb4cbf73312182ef2a Mon Sep 17 00:00:00 2001 From: nilansh Date: Mon, 13 May 2024 18:35:38 +0530 Subject: [PATCH 09/15] addressed review comments and other optimisations --- .../repositories/ce/CacheableRepositoryHelperCE.java | 2 +- .../repositories/ce/CacheableRepositoryHelperCEImpl.java | 2 +- .../server/repositories/ce/TenantRepositoryCE.java | 1 + .../appsmith/server/services/ce/TenantServiceCEImpl.java | 8 +++++--- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java index 507bb066c376..8bf2c469b9aa 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java @@ -20,7 +20,7 @@ public interface CacheableRepositoryHelperCE { Mono getInstanceAdminPermissionGroupId(); - Mono fetchCachedTenant(String tenantId); + Mono fetchDefaultTenant(String tenantId); Mono evictCachedTenant(String tenantId); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java index 50ac49d92e9b..b7de9d94c972 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java @@ -176,7 +176,7 @@ public Mono getInstanceAdminPermissionGroupId() { */ @Cache(cacheName = "defaultTenant", key = "{#tenantId}") @Override - public Mono fetchCachedTenant(String tenantId) { + public Mono fetchDefaultTenant(String tenantId) { BridgeQuery defaultTenantCriteria = Bridge.equal(Tenant.Fields.slug, FieldName.DEFAULT); Query query = new Query(); query.addCriteria(defaultTenantCriteria); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java index 59fecb5b890e..76ba3c1e797d 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java @@ -6,5 +6,6 @@ public interface TenantRepositoryCE extends BaseRepository, CustomTenantRepositoryCE { // Use tenantService.getDefaultTenant() instead of this method as it is cached to redis. + @Deprecated Mono findBySlug(String slug); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java index 24ef373a4dfe..ea40d985479a 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java @@ -108,7 +108,6 @@ public Mono updateTenantConfiguration(String tenantId, TenantConfigurati // Firstly updating the Tenant object in the database and then evicting the cache. // returning the updatedTenant, notice the updatedTenantMono is cached using .cache() // hence it will not be evaluated again - // return updatedTenantMono.then(evictTenantCache).then(updatedTenantMono); return updatedTenantMono .then(Mono.defer(() -> evictTenantCache)) .then(updatedTenantMono); @@ -168,7 +167,7 @@ public Mono getTenantConfiguration() { public Mono getDefaultTenant() { // Fetching Tenant from redis cache return getDefaultTenantId() - .flatMap(tenantId -> cacheableRepositoryHelper.fetchCachedTenant(tenantId)) + .flatMap(tenantId -> cacheableRepositoryHelper.fetchDefaultTenant(tenantId)) .flatMap(tenant -> repository.setUserPermissionsInObject(tenant).switchIfEmpty(Mono.just(tenant))); } @@ -200,9 +199,12 @@ protected Mono getClientPertinentTenant(Tenant dbTenant, Tenant clientTe return Mono.just(clientTenant); } + // This function is used to save the tenant object in the database and evict the cache @Override public Mono save(Tenant tenant) { - return repository.save(tenant); + Mono evictTenantCache = cacheableRepositoryHelper.evictCachedTenant(tenantId); + Mono savedTenantMono = repository.save(tenant).cache(); + return savedTenantMono.then(Mono.defer(() -> evictTenantCache)).then(savedTenantMono); } /** From 2d1115d850ab85309a77b6c1e079487c5739f6bb Mon Sep 17 00:00:00 2001 From: nilansh Date: Tue, 14 May 2024 09:45:31 +0530 Subject: [PATCH 10/15] updated cacheName and added notDeleted criteria --- .../repositories/ce/CacheableRepositoryHelperCEImpl.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java index b7de9d94c972..9db94b33ccf5 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java @@ -174,12 +174,14 @@ public Mono getInstanceAdminPermissionGroupId() { * @param tenantId * @return */ - @Cache(cacheName = "defaultTenant", key = "{#tenantId}") + @Cache(cacheName = "tenant", key = "{#tenantId}") @Override public Mono fetchDefaultTenant(String tenantId) { BridgeQuery defaultTenantCriteria = Bridge.equal(Tenant.Fields.slug, FieldName.DEFAULT); + BridgeQuery notDeletedCriteria = notDeleted(); + BridgeQuery andCriteria = Bridge.and(defaultTenantCriteria, notDeletedCriteria); Query query = new Query(); - query.addCriteria(defaultTenantCriteria); + query.addCriteria(andCriteria); return mongoOperations.findOne(query, Tenant.class).map(tenant -> { if (tenant.getTenantConfiguration() == null) { @@ -189,7 +191,7 @@ public Mono fetchDefaultTenant(String tenantId) { }); } - @CacheEvict(cacheName = "defaultTenant", key = "{#tenantId}") + @CacheEvict(cacheName = "tenant", key = "{#tenantId}") @Override public Mono evictCachedTenant(String tenantId) { return Mono.empty().then(); From 4d1deed2cf1baf9f8267732f38f77056b21a1a33 Mon Sep 17 00:00:00 2001 From: nilansh Date: Tue, 14 May 2024 10:24:23 +0530 Subject: [PATCH 11/15] cache evicted from update func --- .../server/services/ce/TenantServiceCEImpl.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java index ea40d985479a..9656023e773e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java @@ -252,6 +252,19 @@ public Mono retrieveById(String id) { return repository.findById(id); } + /** + * This function updates the tenant object in the database and evicts the cache + * @param tenantId + * @param tenant + * @return + */ + @Override + public Mono update(String tenantId, Tenant tenant) { + Mono evictTenantCache = cacheableRepositoryHelper.evictCachedTenant(tenantId); + Mono updatedTenantMono = super.update(tenantId, tenant).cache(); + return updatedTenantMono.then(Mono.defer(() -> evictTenantCache)).then(updatedTenantMono); + } + /** * This function checks if the tenant needs to be restarted and restarts after the feature flag migrations are * executed. From da5af5895e94216c2a1ab364ea7b295930ff2a9e Mon Sep 17 00:00:00 2001 From: nilansh Date: Tue, 14 May 2024 12:42:42 +0530 Subject: [PATCH 12/15] moved cache invalidation to repository layer --- .../CustomTenantRepositoryImpl.java | 9 ++++- .../ce/CustomTenantRepositoryCE.java | 7 +++- .../ce/CustomTenantRepositoryCEImpl.java | 34 ++++++++++++++++++- .../services/ce/TenantServiceCEImpl.java | 24 ++++++++++--- 4 files changed, 66 insertions(+), 8 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomTenantRepositoryImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomTenantRepositoryImpl.java index b56a9559f6e4..cfa0cd0b8d66 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomTenantRepositoryImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomTenantRepositoryImpl.java @@ -1,5 +1,12 @@ package com.appsmith.server.repositories; import com.appsmith.server.repositories.ce.CustomTenantRepositoryCEImpl; +import org.springframework.data.mongodb.core.ReactiveMongoOperations; -public class CustomTenantRepositoryImpl extends CustomTenantRepositoryCEImpl implements CustomTenantRepository {} +public class CustomTenantRepositoryImpl extends CustomTenantRepositoryCEImpl implements CustomTenantRepository { + + public CustomTenantRepositoryImpl( + CacheableRepositoryHelper cacheableRepositoryHelper, ReactiveMongoOperations mongoOperations) { + super(cacheableRepositoryHelper, mongoOperations); + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCE.java index e9cc3b92c097..19c7b2aa0a58 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCE.java @@ -2,5 +2,10 @@ import com.appsmith.server.domains.Tenant; import com.appsmith.server.repositories.AppsmithRepository; +import reactor.core.publisher.Mono; -public interface CustomTenantRepositoryCE extends AppsmithRepository {} +public interface CustomTenantRepositoryCE extends AppsmithRepository { + Mono save(Tenant tenant); + + Mono update(String tenantId, Tenant tenant); +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCEImpl.java index 2b56879bdef8..be27ae547a7f 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCEImpl.java @@ -2,8 +2,40 @@ import com.appsmith.server.domains.Tenant; import com.appsmith.server.repositories.BaseAppsmithRepositoryImpl; +import com.appsmith.server.repositories.CacheableRepositoryHelper; +import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.springframework.data.mongodb.core.ReactiveMongoOperations; +import reactor.core.publisher.Mono; + +import static org.springframework.data.mongodb.core.query.Criteria.where; +import static org.springframework.data.mongodb.core.query.Query.query; @Slf4j +@RequiredArgsConstructor public class CustomTenantRepositoryCEImpl extends BaseAppsmithRepositoryImpl - implements CustomTenantRepositoryCE {} + implements CustomTenantRepositoryCE { + private final CacheableRepositoryHelper cacheableRepositoryHelper; + private final ReactiveMongoOperations mongoOperations; + + @Override + public Mono save(Tenant tenant) { + Mono savedTenantMono = mongoOperations.save(tenant).cache(); + return savedTenantMono.flatMap(tenant1 -> { + Mono evictTenantCache = cacheableRepositoryHelper.evictCachedTenant(tenant1.getId()); + return evictTenantCache.then(savedTenantMono); + }); + } + + @Override + public Mono update(String tenantId, Tenant tenant) { + Mono evictTenantCache = cacheableRepositoryHelper.evictCachedTenant(tenantId); + Mono updatedTenantMono = mongoOperations + .update(Tenant.class) + .matching(query(where("_id").is(tenantId))) + .replaceWith(tenant) + .findAndReplace() + .cache(); + return updatedTenantMono.then(Mono.defer(() -> evictTenantCache)).then(updatedTenantMono); + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java index 9656023e773e..647cedb3b2e5 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java @@ -59,7 +59,6 @@ public TenantServiceCEImpl( @Override public Mono getDefaultTenantId() { - // If the value exists in cache, return it as is if (StringUtils.hasLength(tenantId)) { return Mono.just(tenantId); @@ -199,8 +198,16 @@ protected Mono getClientPertinentTenant(Tenant dbTenant, Tenant clientTe return Mono.just(clientTenant); } - // This function is used to save the tenant object in the database and evict the cache + /** + * This function saves the tenant object in the database and evicts the cache. This function is specifically created + * and deprecated to ensure that only repository level function is called. + * Instead of this function, please consider using repository.save(tenant) directly + * This is done to ensure that even if we have an EE override, the cache eviction is done from the repository layer + * @param tenant + * @return + */ @Override + @Deprecated public Mono save(Tenant tenant) { Mono evictTenantCache = cacheableRepositoryHelper.evictCachedTenant(tenantId); Mono savedTenantMono = repository.save(tenant).cache(); @@ -232,7 +239,8 @@ public Mono checkAndExecuteMigrationsForTenantFeatureFlags(Tenant tenant } else { tenant.getTenantConfiguration().setMigrationStatus(MigrationStatus.IN_PROGRESS); } - return this.save(tenant) + return repository + .save(tenant) // Fetch the tenant again from DB to make sure the downstream chain is consuming the // latest // DB object and not the modified one because of the client pertinent changes @@ -253,12 +261,16 @@ public Mono retrieveById(String id) { } /** - * This function updates the tenant object in the database and evicts the cache + * This function updates the tenant object in the database and evicts the cache. + * This function is specifically created and deprecated to ensure that only repository level function is called. + * Instead of this function, please consider using repository.update(tenantId, tenant) directly + * This is done to ensure that even if we have an EE override, the cache eviction is done from the repository layer * @param tenantId * @param tenant * @return */ @Override + @Deprecated public Mono update(String tenantId, Tenant tenant) { Mono evictTenantCache = cacheableRepositoryHelper.evictCachedTenant(tenantId); Mono updatedTenantMono = super.update(tenantId, tenant).cache(); @@ -280,7 +292,9 @@ public Mono restartTenant() { log.debug("Triggering tenant restart after the feature flag migrations are executed"); TenantConfiguration tenantConfiguration = updatedTenant.getTenantConfiguration(); tenantConfiguration.setIsRestartRequired(false); - return this.update(updatedTenant.getId(), updatedTenant).then(envManager.restartWithoutAclCheck()); + return repository + .update(updatedTenant.getId(), updatedTenant) + .then(envManager.restartWithoutAclCheck()); } return Mono.empty(); }); From 1910c6130a142128b92040483ae82307889c6c8a Mon Sep 17 00:00:00 2001 From: nilansh Date: Tue, 14 May 2024 16:48:05 +0530 Subject: [PATCH 13/15] Revert "moved cache invalidation to repository layer" This reverts commit da5af5895e94216c2a1ab364ea7b295930ff2a9e. --- .../CustomTenantRepositoryImpl.java | 9 +---- .../ce/CustomTenantRepositoryCE.java | 7 +--- .../ce/CustomTenantRepositoryCEImpl.java | 34 +------------------ .../services/ce/TenantServiceCEImpl.java | 24 +++---------- 4 files changed, 8 insertions(+), 66 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomTenantRepositoryImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomTenantRepositoryImpl.java index cfa0cd0b8d66..b56a9559f6e4 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomTenantRepositoryImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CustomTenantRepositoryImpl.java @@ -1,12 +1,5 @@ package com.appsmith.server.repositories; import com.appsmith.server.repositories.ce.CustomTenantRepositoryCEImpl; -import org.springframework.data.mongodb.core.ReactiveMongoOperations; -public class CustomTenantRepositoryImpl extends CustomTenantRepositoryCEImpl implements CustomTenantRepository { - - public CustomTenantRepositoryImpl( - CacheableRepositoryHelper cacheableRepositoryHelper, ReactiveMongoOperations mongoOperations) { - super(cacheableRepositoryHelper, mongoOperations); - } -} +public class CustomTenantRepositoryImpl extends CustomTenantRepositoryCEImpl implements CustomTenantRepository {} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCE.java index 19c7b2aa0a58..e9cc3b92c097 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCE.java @@ -2,10 +2,5 @@ import com.appsmith.server.domains.Tenant; import com.appsmith.server.repositories.AppsmithRepository; -import reactor.core.publisher.Mono; -public interface CustomTenantRepositoryCE extends AppsmithRepository { - Mono save(Tenant tenant); - - Mono update(String tenantId, Tenant tenant); -} +public interface CustomTenantRepositoryCE extends AppsmithRepository {} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCEImpl.java index be27ae547a7f..2b56879bdef8 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomTenantRepositoryCEImpl.java @@ -2,40 +2,8 @@ import com.appsmith.server.domains.Tenant; import com.appsmith.server.repositories.BaseAppsmithRepositoryImpl; -import com.appsmith.server.repositories.CacheableRepositoryHelper; -import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; -import org.springframework.data.mongodb.core.ReactiveMongoOperations; -import reactor.core.publisher.Mono; - -import static org.springframework.data.mongodb.core.query.Criteria.where; -import static org.springframework.data.mongodb.core.query.Query.query; @Slf4j -@RequiredArgsConstructor public class CustomTenantRepositoryCEImpl extends BaseAppsmithRepositoryImpl - implements CustomTenantRepositoryCE { - private final CacheableRepositoryHelper cacheableRepositoryHelper; - private final ReactiveMongoOperations mongoOperations; - - @Override - public Mono save(Tenant tenant) { - Mono savedTenantMono = mongoOperations.save(tenant).cache(); - return savedTenantMono.flatMap(tenant1 -> { - Mono evictTenantCache = cacheableRepositoryHelper.evictCachedTenant(tenant1.getId()); - return evictTenantCache.then(savedTenantMono); - }); - } - - @Override - public Mono update(String tenantId, Tenant tenant) { - Mono evictTenantCache = cacheableRepositoryHelper.evictCachedTenant(tenantId); - Mono updatedTenantMono = mongoOperations - .update(Tenant.class) - .matching(query(where("_id").is(tenantId))) - .replaceWith(tenant) - .findAndReplace() - .cache(); - return updatedTenantMono.then(Mono.defer(() -> evictTenantCache)).then(updatedTenantMono); - } -} + implements CustomTenantRepositoryCE {} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java index 647cedb3b2e5..9656023e773e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java @@ -59,6 +59,7 @@ public TenantServiceCEImpl( @Override public Mono getDefaultTenantId() { + // If the value exists in cache, return it as is if (StringUtils.hasLength(tenantId)) { return Mono.just(tenantId); @@ -198,16 +199,8 @@ protected Mono getClientPertinentTenant(Tenant dbTenant, Tenant clientTe return Mono.just(clientTenant); } - /** - * This function saves the tenant object in the database and evicts the cache. This function is specifically created - * and deprecated to ensure that only repository level function is called. - * Instead of this function, please consider using repository.save(tenant) directly - * This is done to ensure that even if we have an EE override, the cache eviction is done from the repository layer - * @param tenant - * @return - */ + // This function is used to save the tenant object in the database and evict the cache @Override - @Deprecated public Mono save(Tenant tenant) { Mono evictTenantCache = cacheableRepositoryHelper.evictCachedTenant(tenantId); Mono savedTenantMono = repository.save(tenant).cache(); @@ -239,8 +232,7 @@ public Mono checkAndExecuteMigrationsForTenantFeatureFlags(Tenant tenant } else { tenant.getTenantConfiguration().setMigrationStatus(MigrationStatus.IN_PROGRESS); } - return repository - .save(tenant) + return this.save(tenant) // Fetch the tenant again from DB to make sure the downstream chain is consuming the // latest // DB object and not the modified one because of the client pertinent changes @@ -261,16 +253,12 @@ public Mono retrieveById(String id) { } /** - * This function updates the tenant object in the database and evicts the cache. - * This function is specifically created and deprecated to ensure that only repository level function is called. - * Instead of this function, please consider using repository.update(tenantId, tenant) directly - * This is done to ensure that even if we have an EE override, the cache eviction is done from the repository layer + * This function updates the tenant object in the database and evicts the cache * @param tenantId * @param tenant * @return */ @Override - @Deprecated public Mono update(String tenantId, Tenant tenant) { Mono evictTenantCache = cacheableRepositoryHelper.evictCachedTenant(tenantId); Mono updatedTenantMono = super.update(tenantId, tenant).cache(); @@ -292,9 +280,7 @@ public Mono restartTenant() { log.debug("Triggering tenant restart after the feature flag migrations are executed"); TenantConfiguration tenantConfiguration = updatedTenant.getTenantConfiguration(); tenantConfiguration.setIsRestartRequired(false); - return repository - .update(updatedTenant.getId(), updatedTenant) - .then(envManager.restartWithoutAclCheck()); + return this.update(updatedTenant.getId(), updatedTenant).then(envManager.restartWithoutAclCheck()); } return Mono.empty(); }); From 371f2d17e0388abd36ce3570909d88550fab9f79 Mon Sep 17 00:00:00 2001 From: nilansh Date: Tue, 14 May 2024 17:21:55 +0530 Subject: [PATCH 14/15] optimisations --- .../server/repositories/ce/TenantRepositoryCE.java | 2 +- .../appsmith/server/services/ce/TenantServiceCETest.java | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java index 76ba3c1e797d..7cad8f0f12a4 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/TenantRepositoryCE.java @@ -6,6 +6,6 @@ public interface TenantRepositoryCE extends BaseRepository, CustomTenantRepositoryCE { // Use tenantService.getDefaultTenant() instead of this method as it is cached to redis. - @Deprecated + @Deprecated(forRemoval = true) Mono findBySlug(String slug); } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java index 4710801319ff..b9af9ede7645 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java @@ -88,12 +88,11 @@ public void setup() throws IOException { @AfterEach public void cleanup() { + Tenant updatedTenant = new Tenant(); + updatedTenant.setTenantConfiguration(originalTenantConfiguration); tenantService - .getDefaultTenant() - .flatMap(tenant -> tenantRepository.updateAndReturn( - tenant.getId(), - Bridge.update().set(Tenant.Fields.tenantConfiguration, originalTenantConfiguration), - Optional.empty())) + .getDefaultTenantId() + .flatMap(tenantId -> tenantService.update(tenantId, updatedTenant)) .block(); } From bb21eaebb75e0b7dc24ecfcbddc9f609eef50ce9 Mon Sep 17 00:00:00 2001 From: nilansh Date: Tue, 14 May 2024 17:43:09 +0530 Subject: [PATCH 15/15] code rabbit suggestion --- .../com/appsmith/server/services/ce/TenantServiceCETest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java index b9af9ede7645..bca2dcf205a5 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.java @@ -93,6 +93,9 @@ public void cleanup() { tenantService .getDefaultTenantId() .flatMap(tenantId -> tenantService.update(tenantId, updatedTenant)) + .doOnError(error -> { + System.err.println("Error during cleanup: " + error.getMessage()); + }) .block(); }