From c14a896a7a9d8a0360a37755daa2e8b16f1b9a03 Mon Sep 17 00:00:00 2001 From: Abhijeet <41686026+abhvsn@users.noreply.github.com> Date: Mon, 28 Apr 2025 14:44:43 +0530 Subject: [PATCH] refactor: Remove strong dependency from feature flag enum to (de)serialise organization object (#40374) --- .../ce/OrganizationConfigurationCE.java | 3 +- .../ce/FeatureFlagMigrationHelperCE.java | 5 +- .../ce/FeatureFlagMigrationHelperCEImpl.java | 51 +++++++++---------- .../ce/OrganizationServiceCEImpl.java | 21 ++++++-- .../FeatureFlagMigrationHelperTest.java | 19 ++++--- .../services/ce/FeatureFlagServiceCETest.java | 8 +-- .../ce/OrganizationServiceCETest.java | 13 +++-- 7 files changed, 61 insertions(+), 59 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java index bc9d275b65..0b6ee27834 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java @@ -1,6 +1,5 @@ package com.appsmith.server.domains.ce; -import com.appsmith.external.enums.FeatureFlagEnum; import com.appsmith.server.constants.FeatureMigrationType; import com.appsmith.server.constants.LicensePlan; import com.appsmith.server.constants.MigrationStatus; @@ -58,7 +57,7 @@ public class OrganizationConfigurationCE implements Serializable { // the feature flags. This can happen for 2 reasons: // 1. The license plan changes // 2. Because of grandfathering via cron where organization level feature flags are fetched - Map featuresWithPendingMigration; + Map featuresWithPendingMigration; Boolean isStrongPasswordPolicyEnabled; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCE.java index 3df6f2d057..90ae98cc18 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCE.java @@ -9,10 +9,9 @@ import java.util.Map; public interface FeatureFlagMigrationHelperCE { - Mono> getUpdatedFlagsWithPendingMigration( - Organization defaultOrganization); + Mono> getUpdatedFlagsWithPendingMigration(Organization defaultOrganization); - Mono> getUpdatedFlagsWithPendingMigration( + Mono> getUpdatedFlagsWithPendingMigration( Organization organization, boolean forceUpdate); Mono checkAndExecuteMigrationsForFeatureFlag(Organization organization, FeatureFlagEnum featureFlagEnum); 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 f2f2990acc..aa83a698de 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 @@ -7,6 +7,7 @@ import com.appsmith.server.domains.OrganizationConfiguration; import com.appsmith.server.featureflags.CachedFeatures; import com.appsmith.server.helpers.CollectionUtils; import com.appsmith.server.services.CacheableFeatureFlagHelper; +import com.appsmith.server.solutions.ce.ScheduledTaskCEImpl; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import reactor.core.publisher.Mono; @@ -36,7 +37,7 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel private static final long ORGANIZATION_FEATURES_CACHE_TIME_MIN = 115; @Override - public Mono> getUpdatedFlagsWithPendingMigration( + public Mono> getUpdatedFlagsWithPendingMigration( Organization defaultOrganization) { return getUpdatedFlagsWithPendingMigration(defaultOrganization, FALSE); } @@ -49,7 +50,7 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel * @return Map of feature flags with pending migrations */ @Override - public Mono> getUpdatedFlagsWithPendingMigration( + public Mono> getUpdatedFlagsWithPendingMigration( Organization organization, boolean forceUpdate) { /* @@ -127,50 +128,43 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel * @param existingCachedFlags Flags which are already stored in cache * @return updated organization with the required flags with pending migrations */ - private Map getUpdatedFlagsWithPendingMigration( + private Map getUpdatedFlagsWithPendingMigration( Organization organization, CachedFeatures latestFlags, CachedFeatures existingCachedFlags) { // 1. Check if there are any diffs for the feature flags // 2. Update the flags for pending migration within provided organization object - Map featureDiffsWithMigrationType = new HashMap<>(); + Map featureDiffsWithMigrationType = new HashMap<>(); Map existingFeatureMap = existingCachedFlags.getFeatures(); latestFlags.getFeatures().forEach((key, value) -> { if (value != null && !value.equals(existingFeatureMap.get(key))) { try { - featureDiffsWithMigrationType.put( - FeatureFlagEnum.valueOf(key), Boolean.TRUE.equals(value) ? ENABLE : DISABLE); + featureDiffsWithMigrationType.put(key, Boolean.TRUE.equals(value) ? ENABLE : DISABLE); } 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); - } + log.error("Error while processing the feature flag {} with value {}", key, value, e); } } }); return getUpdatedFlagsWithPendingMigration(featureDiffsWithMigrationType, organization); } - private Map getUpdatedFlagsWithPendingMigration( - Map latestFeatureDiffsWithMigrationType, - Organization dbOrganization) { + private Map getUpdatedFlagsWithPendingMigration( + Map latestFeatureDiffsWithMigrationType, Organization dbOrganization) { - Map featuresWithPendingMigrationDB = + Map featuresWithPendingMigrationDB = dbOrganization.getOrganizationConfiguration().getFeaturesWithPendingMigration() == null ? new HashMap<>() : dbOrganization.getOrganizationConfiguration().getFeaturesWithPendingMigration(); - Map updatedFlagsForMigrations = - new HashMap<>(featuresWithPendingMigrationDB); + Map updatedFlagsForMigrations = new HashMap<>(featuresWithPendingMigrationDB); // We should expect the following state after the latest run: // 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))) { + List featureFlagsToBeRemoved = new ArrayList<>(); + updatedFlagsForMigrations.forEach((featureFlag, featureMigrationType) -> { + if (latestFeatureDiffsWithMigrationType.containsKey(featureFlag) + && !featureMigrationType.equals(latestFeatureDiffsWithMigrationType.get(featureFlag))) { /* 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 is no longer needed: @@ -182,7 +176,7 @@ 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 */ - featureFlagsToBeRemoved.add(featureFlagEnum); + featureFlagsToBeRemoved.add(featureFlag); } }); @@ -217,16 +211,16 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel return Mono.just(TRUE); } - Map featuresWithPendingMigration = + Map featuresWithPendingMigration = organizationConfiguration.getFeaturesWithPendingMigration(); if (CollectionUtils.isNullOrEmpty(featuresWithPendingMigration) - || !featuresWithPendingMigration.containsKey(featureFlagEnum)) { + || !featuresWithPendingMigration.containsKey(featureFlagEnum.name())) { return Mono.just(TRUE); } log.debug( "Running the migration for flag {} with migration type {}", featureFlagEnum.name(), - featuresWithPendingMigration.get(featureFlagEnum)); + featuresWithPendingMigration.get(featureFlagEnum.name())); return this.executeMigrationsBasedOnFeatureFlag(organization, featureFlagEnum); }); } @@ -238,7 +232,7 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel * @return Boolean indicating if the migrations is required or not */ private Mono isMigrationRequired(Organization organization, FeatureFlagEnum featureFlagEnum) { - Map featureMigrationTypeMap = + Map featureMigrationTypeMap = organization.getOrganizationConfiguration().getFeaturesWithPendingMigration(); if (CollectionUtils.isNullOrEmpty(featureMigrationTypeMap)) { return Mono.just(FALSE); @@ -250,10 +244,10 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel if (featureFlags.containsKey(featureFlagEnum.name())) { return (TRUE.equals(featureFlags.get(featureFlagEnum.name())) && FeatureMigrationType.ENABLE.equals( - featureMigrationTypeMap.get(featureFlagEnum))) + featureMigrationTypeMap.get(featureFlagEnum.name()))) || (FALSE.equals(featureFlags.get(featureFlagEnum.name())) && FeatureMigrationType.DISABLE.equals( - featureMigrationTypeMap.get(featureFlagEnum))); + featureMigrationTypeMap.get(featureFlagEnum.name()))); } return FALSE; }); @@ -268,6 +262,7 @@ public class FeatureFlagMigrationHelperCEImpl implements FeatureFlagMigrationHel @Override public Mono executeMigrationsBasedOnFeatureFlag( Organization organization, FeatureFlagEnum featureFlagEnum) { + // Placeholder implementation for extending classes return Mono.just(TRUE); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java index 1c85ddc95c..ddec20291c 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java @@ -290,16 +290,27 @@ public class OrganizationServiceCEImpl extends BaseService featureMigrationTypeMap = + Map featureMigrationTypeMap = organization.getOrganizationConfiguration().getFeaturesWithPendingMigration(); - FeatureFlagEnum featureFlagEnum = + final String featureFlagKey = featureMigrationTypeMap.keySet().stream().findFirst().orElse(null); + + FeatureFlagEnum featureFlagEnum = null; + if (featureFlagKey != null) { + try { + featureFlagEnum = FeatureFlagEnum.valueOf(featureFlagKey); + } catch (IllegalArgumentException e) { + log.warn("Unknown feature flag: {}", featureFlagKey); + } + } + + final FeatureFlagEnum finalFeatureFlagEnum = featureFlagEnum; return featureFlagMigrationHelper - .checkAndExecuteMigrationsForFeatureFlag(organization, featureFlagEnum) + .checkAndExecuteMigrationsForFeatureFlag(organization, finalFeatureFlagEnum) .flatMap(isSuccessful -> { if (TRUE.equals(isSuccessful)) { - featureMigrationTypeMap.remove(featureFlagEnum); + featureMigrationTypeMap.remove(featureFlagKey); if (CollectionUtils.isNullOrEmpty(featureMigrationTypeMap)) { organization.getOrganizationConfiguration().setMigrationStatus(MigrationStatus.COMPLETED); } else { @@ -312,7 +323,7 @@ public class OrganizationServiceCEImpl extends BaseService> getUpdatedFlagsWithPendingMigration = + Mono> getUpdatedFlagsWithPendingMigration = featureFlagMigrationHelper.getUpdatedFlagsWithPendingMigration(defaultOrganization); StepVerifier.create(getUpdatedFlagsWithPendingMigration) .assertNext(featureFlagEnumFeatureMigrationTypeMap -> { assertThat(featureFlagEnumFeatureMigrationTypeMap).isNotEmpty(); assertThat(featureFlagEnumFeatureMigrationTypeMap).hasSize(1); - assertThat(featureFlagEnumFeatureMigrationTypeMap.get(ORGANIZATION_TEST_FEATURE)) + assertThat(featureFlagEnumFeatureMigrationTypeMap.get(ORGANIZATION_TEST_FEATURE.name())) .isEqualTo(DISABLE); }) .verifyComplete(); @@ -103,14 +102,14 @@ class FeatureFlagMigrationHelperTest { Mockito.when(cacheableFeatureFlagHelper.evictCachedOrganizationFeatures(any())) .thenReturn(Mono.empty()); - Mono> getUpdatedFlagsWithPendingMigration = + Mono> getUpdatedFlagsWithPendingMigration = featureFlagMigrationHelper.getUpdatedFlagsWithPendingMigration(defaultOrganization); StepVerifier.create(getUpdatedFlagsWithPendingMigration) .assertNext(featureFlagEnumFeatureMigrationTypeMap -> { assertThat(featureFlagEnumFeatureMigrationTypeMap).isNotEmpty(); assertThat(featureFlagEnumFeatureMigrationTypeMap).hasSize(1); - assertThat(featureFlagEnumFeatureMigrationTypeMap.get(ORGANIZATION_TEST_FEATURE)) + assertThat(featureFlagEnumFeatureMigrationTypeMap.get(ORGANIZATION_TEST_FEATURE.name())) .isEqualTo(ENABLE); }) .verifyComplete(); @@ -134,7 +133,7 @@ class FeatureFlagMigrationHelperTest { Mockito.when(cacheableFeatureFlagHelper.evictCachedOrganizationFeatures(any())) .thenReturn(Mono.empty()); - Mono> getUpdatedFlagsWithPendingMigration = + Mono> getUpdatedFlagsWithPendingMigration = featureFlagMigrationHelper.getUpdatedFlagsWithPendingMigration(defaultOrganization); StepVerifier.create(getUpdatedFlagsWithPendingMigration) @@ -172,7 +171,7 @@ class FeatureFlagMigrationHelperTest { Mockito.when(cacheableFeatureFlagHelper.evictCachedOrganizationFeatures(any())) .thenReturn(Mono.empty()); - Mono> getUpdatedFlagsWithPendingMigration = + Mono> getUpdatedFlagsWithPendingMigration = featureFlagMigrationHelper.getUpdatedFlagsWithPendingMigration(defaultOrganization); StepVerifier.create(getUpdatedFlagsWithPendingMigration) @@ -198,7 +197,7 @@ class FeatureFlagMigrationHelperTest { void checkAndExecuteMigrationsForFeatureFlag_validFeatureFlag_success() { Organization defaultOrganization = new Organization(); OrganizationConfiguration organizationConfiguration = new OrganizationConfiguration(); - organizationConfiguration.setFeaturesWithPendingMigration(Map.of(ORGANIZATION_TEST_FEATURE, ENABLE)); + organizationConfiguration.setFeaturesWithPendingMigration(Map.of(ORGANIZATION_TEST_FEATURE.name(), ENABLE)); organizationConfiguration.setMigrationStatus(PENDING); defaultOrganization.setOrganizationConfiguration(organizationConfiguration); @@ -231,7 +230,7 @@ class FeatureFlagMigrationHelperTest { Organization defaultOrganization = new Organization(); defaultOrganization.setId(UUID.randomUUID().toString()); OrganizationConfiguration organizationConfiguration = new OrganizationConfiguration(); - organizationConfiguration.setFeaturesWithPendingMigration(Map.of(ORGANIZATION_TEST_FEATURE, DISABLE)); + organizationConfiguration.setFeaturesWithPendingMigration(Map.of(ORGANIZATION_TEST_FEATURE.name(), DISABLE)); defaultOrganization.setOrganizationConfiguration(organizationConfiguration); // Mock CS calls to fetch the feature flags to have the feature flag in pending migration list with ENABLE @@ -256,7 +255,7 @@ class FeatureFlagMigrationHelperTest { Mockito.when(cacheableFeatureFlagHelper.evictCachedOrganizationFeatures(any())) .thenReturn(Mono.empty()); - Mono> getUpdatedFlagsWithPendingMigration = + Mono> getUpdatedFlagsWithPendingMigration = featureFlagMigrationHelper.getUpdatedFlagsWithPendingMigration(defaultOrganization); StepVerifier.create(getUpdatedFlagsWithPendingMigration) diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java index 9538a834d3..ee7a691433 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java @@ -226,7 +226,7 @@ public class FeatureFlagServiceCETest { getAllRemoteFeaturesForOrganizationAndUpdateFeatureFlagsWithPendingMigrations_disableMigration_statesUpdate() { Mockito.when(featureFlagMigrationHelper.getUpdatedFlagsWithPendingMigration(any())) - .thenReturn(Mono.just(Map.of(ORGANIZATION_TEST_FEATURE, DISABLE))); + .thenReturn(Mono.just(Map.of(ORGANIZATION_TEST_FEATURE.name(), DISABLE))); organizationService .retrieveAll() @@ -237,7 +237,7 @@ public class FeatureFlagServiceCETest { StepVerifier.create(organizationService.getCurrentUserOrganization()) .assertNext(organization -> { assertThat(organization.getOrganizationConfiguration().getFeaturesWithPendingMigration()) - .isEqualTo(Map.of(ORGANIZATION_TEST_FEATURE, DISABLE)); + .isEqualTo(Map.of(ORGANIZATION_TEST_FEATURE.name(), DISABLE)); assertThat(organization.getOrganizationConfiguration().getMigrationStatus()) .isEqualTo(PENDING); }) @@ -249,7 +249,7 @@ public class FeatureFlagServiceCETest { getAllRemoteFeaturesForOrganizationAndUpdateFeatureFlagsWithPendingMigrations_enableMigration_statesUpdate() { Mockito.when(featureFlagMigrationHelper.getUpdatedFlagsWithPendingMigration(any())) - .thenReturn(Mono.just(Map.of(ORGANIZATION_TEST_FEATURE, ENABLE))); + .thenReturn(Mono.just(Map.of(ORGANIZATION_TEST_FEATURE.name(), ENABLE))); organizationService .retrieveAll() @@ -260,7 +260,7 @@ public class FeatureFlagServiceCETest { StepVerifier.create(organizationService.getCurrentUserOrganization()) .assertNext(organization -> { assertThat(organization.getOrganizationConfiguration().getFeaturesWithPendingMigration()) - .isEqualTo(Map.of(ORGANIZATION_TEST_FEATURE, ENABLE)); + .isEqualTo(Map.of(ORGANIZATION_TEST_FEATURE.name(), ENABLE)); assertThat(organization.getOrganizationConfiguration().getMigrationStatus()) .isEqualTo(PENDING); }) diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java index 58d3cacad4..555083644d 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java @@ -1,7 +1,6 @@ package com.appsmith.server.services.ce; import com.appsmith.caching.components.CacheManager; -import com.appsmith.external.enums.FeatureFlagEnum; import com.appsmith.server.constants.FeatureMigrationType; import com.appsmith.server.constants.LicensePlan; import com.appsmith.server.domains.Organization; @@ -339,10 +338,10 @@ class OrganizationServiceCETest { organization.setId(UUID.randomUUID().toString()); organization.setSlug(UUID.randomUUID().toString()); OrganizationConfiguration config = new OrganizationConfiguration(); - Map featureMigrationTypeMap = new HashMap<>(); + Map featureMigrationTypeMap = new HashMap<>(); config.setFeaturesWithPendingMigration(featureMigrationTypeMap); - featureMigrationTypeMap.put(ORGANIZATION_TEST_FEATURE, FeatureMigrationType.ENABLE); - featureMigrationTypeMap.put(TEST_FEATURE_2, FeatureMigrationType.DISABLE); + featureMigrationTypeMap.put(ORGANIZATION_TEST_FEATURE.name(), FeatureMigrationType.ENABLE); + featureMigrationTypeMap.put(TEST_FEATURE_2.name(), FeatureMigrationType.DISABLE); organization.setOrganizationConfiguration(config); final Mono resultMono = organizationService.checkAndExecuteMigrationsForOrganizationFeatureFlags(organization); @@ -368,10 +367,10 @@ class OrganizationServiceCETest { organization.setId(UUID.randomUUID().toString()); organization.setSlug(UUID.randomUUID().toString()); OrganizationConfiguration config = new OrganizationConfiguration(); - Map featureMigrationTypeMap = new HashMap<>(); + Map featureMigrationTypeMap = new HashMap<>(); config.setFeaturesWithPendingMigration(featureMigrationTypeMap); - featureMigrationTypeMap.put(ORGANIZATION_TEST_FEATURE, FeatureMigrationType.DISABLE); - featureMigrationTypeMap.put(TEST_FEATURE_2, FeatureMigrationType.ENABLE); + featureMigrationTypeMap.put(ORGANIZATION_TEST_FEATURE.name(), FeatureMigrationType.DISABLE); + featureMigrationTypeMap.put(TEST_FEATURE_2.name(), FeatureMigrationType.ENABLE); organization.setOrganizationConfiguration(config); final Mono resultMono = organizationService.checkAndExecuteMigrationsForOrganizationFeatureFlags(organization);