From cb3cca0a85a6385ed4e451026909d6af4feb3b2f Mon Sep 17 00:00:00 2001 From: Abhijeet <41686026+abhvsn@users.noreply.github.com> Date: Wed, 11 Oct 2023 12:57:13 +0530 Subject: [PATCH] fix: Concurrent modification issue with pending migrations feature flags (#27926) ## Description In this code change, a separate loop has been introduced to handle the removal of specific flags from a map to avoid encountering a `ConcurrentModificationException` that can occur when attempting to remove an entry from a map while iterating over it directly. This new loop iterates through a collection of `featureFlagsToBeRemoved`, removing each corresponding flag from the mentioned maps, ensuring safe removal without disrupting the ongoing iteration process. #### PR fixes following issue(s) Fixes https://github.com/appsmithorg/appsmith-ee/issues/2608 #### Type of change - Bug fix (non-breaking change which fixes an issue) ## Testing > #### How Has This Been Tested? - [x] Manual - [x] JUnit - [ ] Jest - [ ] Cypress > > ## Checklist: #### Dev activity - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag #### QA activity: - [ ] [Speedbreak features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-) have been covered - [ ] Test plan covers all impacted features and [areas of interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-) - [ ] Test plan has been peer reviewed by project stakeholders and other QA members - [ ] Manually tested functionality on DP - [ ] We had an implementation alignment call with stakeholders post QA Round 2 - [ ] Cypress test cases have been added and approved by SDET/manual QA - [ ] Added `Test Plan Approved` label after Cypress tests were reviewed - [ ] Added `Test Plan Approved` label after JUnit tests were reviewed --- .../ce/FeatureFlagMigrationHelperCEImpl.java | 19 ++++++-- .../FeatureFlagMigrationHelperTest.java | 46 +++++++++++++++++++ 2 files changed, 61 insertions(+), 4 deletions(-) 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(); + } }