diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java index 3980530087..658c9390cc 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java @@ -14,7 +14,9 @@ import reactor.core.publisher.Mono; import java.time.Instant; import java.time.temporal.ChronoUnit; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; import static com.appsmith.server.constants.FeatureMigrationType.DISABLE; @@ -136,6 +138,9 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel } catch (Exception e) { // Ignore IllegalArgumentException as all the feature flags are not added on // server side + if (!(e instanceof IllegalArgumentException)) { + log.error("Error while parsing the feature flag {} with value {}", key, value, e); + } } } }); @@ -157,13 +162,13 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel // featuresWithPendingMigrationDB => {feature1 : enable, feature2 : disable} // latestFeatureDiffsWithMigrationType => {feature1 : enable, feature2 : enable, feature3 : disable} // updatedFlagsForMigrations => {feature1 : enable, feature3 : disable} - + List featureFlagsToBeRemoved = new ArrayList<>(); updatedFlagsForMigrations.forEach((featureFlagEnum, featureMigrationType) -> { if (latestFeatureDiffsWithMigrationType.containsKey(featureFlagEnum) && !featureMigrationType.equals(latestFeatureDiffsWithMigrationType.get(featureFlagEnum))) { /* Scenario when the migrations will be blocked on user input and may end up in a case where we just have - to remove the entry as migration it's no longer needed: + to remove the entry as migration is no longer needed: Step 1: Feature gets enabled by adding a valid licence and enable migration gets registered Step 2: License expires which results in feature getting disabled so migration entry gets registered with disable type (This will happen via cron to check the license status) @@ -172,10 +177,16 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel Step 4: User adds the valid key or renews the subscription again which results in enabling the feature and ends up in nullifying the effect for step 2 */ - updatedFlagsForMigrations.remove(featureFlagEnum); - latestFeatureDiffsWithMigrationType.remove(featureFlagEnum); + featureFlagsToBeRemoved.add(featureFlagEnum); } }); + + // Added a separate loop to remove the flags as we cannot remove the entry while iterating over the map to + // avoid the ConcurrentModificationException + featureFlagsToBeRemoved.forEach(featureFlagEnum -> { + updatedFlagsForMigrations.remove(featureFlagEnum); + latestFeatureDiffsWithMigrationType.remove(featureFlagEnum); + }); // Add the latest flags which were not part of earlier check. updatedFlagsForMigrations.putAll(latestFeatureDiffsWithMigrationType); return updatedFlagsForMigrations; diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/FeatureFlagMigrationHelperTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/FeatureFlagMigrationHelperTest.java index c39f5cd35b..c1ca84100e 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/FeatureFlagMigrationHelperTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/FeatureFlagMigrationHelperTest.java @@ -226,4 +226,50 @@ class FeatureFlagMigrationHelperTest { }) .verifyComplete(); } + + @Test + void + getUpdatedFlagsWithPendingMigration_diffForExistingAndLatestFlag_sameFlagIsFlippedAsPerDBState_flagGetsRemovedFromPendingMigrationList() { + + // Mock DB state to have the feature flag in pending migration list with DISABLE status which means the feature + // flag flipped from true to false + Tenant defaultTenant = new Tenant(); + defaultTenant.setId(UUID.randomUUID().toString()); + TenantConfiguration tenantConfiguration = new TenantConfiguration(); + tenantConfiguration.setFeaturesWithPendingMigration(Map.of(TENANT_TEST_FEATURE, DISABLE)); + defaultTenant.setTenantConfiguration(tenantConfiguration); + + // Mock CS calls to fetch the feature flags to have the feature flag in pending migration list with ENABLE + // status + // This means the feature flag flipped from false to true again with latest check + CachedFeatures existingCachedFeatures = new CachedFeatures(); + Map featureMap = new HashMap<>(); + featureMap.put(TENANT_TEST_FEATURE.name(), false); + existingCachedFeatures.setFeatures(featureMap); + existingCachedFeatures.setRefreshedAt(Instant.now().minus(1, ChronoUnit.DAYS)); + + CachedFeatures latestCachedFeatures = new CachedFeatures(); + Map latestFeatureMap = new HashMap<>(); + latestFeatureMap.put(TENANT_TEST_FEATURE.name(), true); + latestCachedFeatures.setFeatures(latestFeatureMap); + latestCachedFeatures.setRefreshedAt(Instant.now()); + + Mockito.when(cacheableFeatureFlagHelper.fetchCachedTenantFeatures(any())) + .thenReturn(Mono.just(existingCachedFeatures)) + .thenReturn(Mono.just(latestCachedFeatures)); + + Mockito.when(cacheableFeatureFlagHelper.evictCachedTenantFeatures(any())) + .thenReturn(Mono.empty()); + + Mono> getUpdatedFlagsWithPendingMigration = + featureFlagMigrationHelper.getUpdatedFlagsWithPendingMigration(defaultTenant); + + StepVerifier.create(getUpdatedFlagsWithPendingMigration) + .assertNext(featureFlagEnumFeatureMigrationTypeMap -> { + // As the feature flag is flipped back to true, the feature flag should be removed from the pending + // migration entries as the migration is no longer required + assertThat(featureFlagEnumFeatureMigrationTypeMap).isEmpty(); + }) + .verifyComplete(); + } }