diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FeatureFlagConfig.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FeatureFlagConfig.java deleted file mode 100644 index 9a8b4807d3..0000000000 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/FeatureFlagConfig.java +++ /dev/null @@ -1,17 +0,0 @@ -package com.appsmith.server.configurations; - -import org.ff4j.FF4j; -import org.ff4j.conf.XmlParser; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Configuration; - -@Configuration -public class FeatureFlagConfig { - - // Since we rely on cloud services to retrieve the most up-to-date flags, we avoid automatically generating the same - // flag in the FF4J context. - @Bean - public FF4j ff4j() { - return new FF4j(new XmlParser(), "features/init-flags.xml").audit(true).autoCreate(false); - } -} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/featureflags/FeatureFlagEnum.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/featureflags/FeatureFlagEnum.java index b8742b34a2..2e9fbc1936 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/featureflags/FeatureFlagEnum.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/featureflags/FeatureFlagEnum.java @@ -1,19 +1,5 @@ package com.appsmith.server.featureflags; -import org.ff4j.core.FlippingStrategy; -import org.ff4j.strategy.PonderationStrategy; -import org.ff4j.strategy.time.OfficeHourStrategy; - -/** - * This enum lists all the feature flags available along with their flipping strategy. - * In order to create a new feature flag, create another enum entry and add the same string to {@link features/init-flags.xml} - *

- * If you wish to define a custom flipping strategy, define a class that implements {@link FlippingStrategy} and - * ensure that you've mentioned this custom class when defining the feature in {@link features/init-flags.xml} - *

- * The feature flag implementation class should extend an existing feature flag implementation like {@link PonderationStrategy}, - * {@link OfficeHourStrategy} etc. These default classes provide a lot of basic functionality out of the box. - */ public enum FeatureFlagEnum { // ------------------- These features are only for JUnit testing. DO NOT use these features in your code path.--- // // ------------------- Couldn't find a better way to do this ---------------------------------------------------- // diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/featureflags/strategies/AppsmithUserStrategy.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/featureflags/strategies/AppsmithUserStrategy.java deleted file mode 100644 index 52682333bc..0000000000 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/featureflags/strategies/AppsmithUserStrategy.java +++ /dev/null @@ -1,24 +0,0 @@ -package com.appsmith.server.featureflags.strategies; - -import com.appsmith.server.constants.FieldName; -import com.appsmith.server.domains.User; -import lombok.extern.slf4j.Slf4j; -import org.apache.commons.lang3.StringUtils; -import org.ff4j.core.FeatureStore; -import org.ff4j.core.FlippingExecutionContext; -import org.ff4j.strategy.AbstractFlipStrategy; - -/** - * This strategy enables a given feature for Appsmith users only. Useful when features are under development and not - * ready for prime-time - */ -@Slf4j -public class AppsmithUserStrategy extends AbstractFlipStrategy { - - @Override - public boolean evaluate(String featureName, FeatureStore store, FlippingExecutionContext executionContext) { - User user = (User) executionContext.getValue(FieldName.USER, true); - - return StringUtils.endsWith(user.getEmail(), "@appsmith.com"); - } -} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/featureflags/strategies/EmailBasedRolloutStrategy.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/featureflags/strategies/EmailBasedRolloutStrategy.java deleted file mode 100644 index 3171aa420f..0000000000 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/featureflags/strategies/EmailBasedRolloutStrategy.java +++ /dev/null @@ -1,63 +0,0 @@ -package com.appsmith.server.featureflags.strategies; - -import com.appsmith.server.constants.FieldName; -import com.appsmith.server.domains.User; -import lombok.extern.slf4j.Slf4j; -import org.apache.commons.lang3.StringUtils; -import org.ff4j.core.FeatureStore; -import org.ff4j.core.FlippingExecutionContext; -import org.ff4j.strategy.AbstractFlipStrategy; - -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.Map; - -@Slf4j -public class EmailBasedRolloutStrategy extends AbstractFlipStrategy { - - List validDomains = new ArrayList<>(); - List validEmails = new ArrayList<>(); - - private static final String PARAM_EMAIL_DOMAINS = "emailDomains"; - private static final String PARAM_EMAILS = "emails"; - - /** - * {@inheritDoc} - */ - @Override - public void init(String featureName, Map initParam) { - super.init(featureName, initParam); - if (!initParam.containsKey(PARAM_EMAIL_DOMAINS) && !initParam.containsKey(PARAM_EMAILS)) { - String msg = String.format( - "Either '%s' or '%s' is required for EmailBasedRolloutStrategy", PARAM_EMAIL_DOMAINS, PARAM_EMAILS); - throw new IllegalArgumentException(msg); - } - if (!StringUtils.isEmpty(initParam.get(PARAM_EMAIL_DOMAINS))) { - this.validDomains = Arrays.asList(initParam.get(PARAM_EMAIL_DOMAINS).split(",")); - } - if (!StringUtils.isEmpty(initParam.get(PARAM_EMAILS))) { - this.validEmails = Arrays.asList(initParam.get(PARAM_EMAILS).split(",")); - } - } - - /** - * {@inheritDoc} - */ - @Override - public boolean evaluate(String featureName, FeatureStore store, FlippingExecutionContext executionContext) { - User user = (User) executionContext.getValue(FieldName.USER, true); - int atIndex = user.getEmail().indexOf("@"); - - if (atIndex > 0) { - // If the email domain is valid, check the user's email ID against the list of validated domains - String domain = user.getEmail().substring(atIndex + 1).toLowerCase(); - if (validDomains.contains(domain)) { - return true; - } else { - return validEmails.contains(user.getEmail()); - } - } - return false; - } -} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/FeatureFlagServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/FeatureFlagServiceImpl.java index 8ba5d4bdae..103a3fd5df 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/FeatureFlagServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/FeatureFlagServiceImpl.java @@ -2,21 +2,18 @@ package com.appsmith.server.services; import com.appsmith.server.helpers.FeatureFlagMigrationHelper; import com.appsmith.server.services.ce.FeatureFlagServiceCEImpl; -import org.ff4j.FF4j; import org.springframework.stereotype.Component; @Component public class FeatureFlagServiceImpl extends FeatureFlagServiceCEImpl implements FeatureFlagService { public FeatureFlagServiceImpl( SessionUserService sessionUserService, - FF4j ff4j, TenantService tenantService, UserIdentifierService userIdentifierService, CacheableFeatureFlagHelper cacheableFeatureFlagHelper, FeatureFlagMigrationHelper featureFlagMigrationHelper) { super( sessionUserService, - ff4j, tenantService, userIdentifierService, cacheableFeatureFlagHelper, 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 369aa3f1f1..cc612e32dc 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 @@ -152,7 +152,7 @@ public class CacheableFeatureFlagHelperCEImpl implements CacheableFeatureFlagHel // We're gobbling up errors here so that all feature flags are turned off by default // This will be problematic if we do not maintain code to reflect validity of flags log.debug("Received error from CS for feature flags: {}", error.getMessage()); - return Mono.just(Map.of()); + return Mono.just(new HashMap<>()); }); } @@ -166,8 +166,14 @@ public class CacheableFeatureFlagHelperCEImpl implements CacheableFeatureFlagHel public Mono fetchCachedTenantFeatures(String tenantId) { return this.forceAllRemoteFeaturesForTenant(tenantId).flatMap(flags -> { CachedFeatures cachedFeatures = new CachedFeatures(); - cachedFeatures.setRefreshedAt(Instant.now()); cachedFeatures.setFeatures(flags); + // If CS is down we expect the empty flags, from upstream method. Hence, setting the refreshed at to past + // so that the next call will have the force refresh. + if (CollectionUtils.isNullOrEmpty(flags)) { + cachedFeatures.setRefreshedAt(Instant.now().minus(1, ChronoUnit.DAYS)); + } else { + cachedFeatures.setRefreshedAt(Instant.now()); + } return Mono.just(cachedFeatures); }); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCE.java index 806726ea94..f617e92971 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCE.java @@ -1,7 +1,6 @@ package com.appsmith.server.services.ce; import com.appsmith.server.domains.Tenant; -import com.appsmith.server.domains.User; import com.appsmith.server.featureflags.CachedFeatures; import com.appsmith.server.featureflags.FeatureFlagEnum; import reactor.core.publisher.Mono; @@ -10,16 +9,6 @@ import java.util.Map; public interface FeatureFlagServiceCE { - /** - * Used to check if a particular feature is enabled for a given user. Useful in contexts where we already have the - * User object and simply wish to do a boolean check - * - * @param featureEnum - * @param user - * @return Boolean - */ - Mono check(FeatureFlagEnum featureEnum, User user); - /** * Check if a particular feature is enabled for the current logged in user. Useful in chaining reactive functions * while writing business logic that may depend on a feature flag @@ -29,8 +18,6 @@ public interface FeatureFlagServiceCE { */ Mono check(FeatureFlagEnum featureEnum); - Boolean check(String featureName, User user); - /** * Fetch all the flags and their values for the current logged in user * diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java index 6c761ce537..d73a419d60 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java @@ -1,6 +1,5 @@ package com.appsmith.server.services.ce; -import com.appsmith.server.constants.FieldName; import com.appsmith.server.constants.MigrationStatus; import com.appsmith.server.domains.Tenant; import com.appsmith.server.domains.TenantConfiguration; @@ -16,19 +15,13 @@ import com.appsmith.server.services.TenantService; import com.appsmith.server.services.UserIdentifierService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; -import org.ff4j.FF4j; -import org.ff4j.core.FlippingExecutionContext; -import org.ff4j.exception.FeatureNotFoundException; -import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; -import reactor.util.function.Tuple2; import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.HashMap; import java.util.Map; - -import static java.lang.Boolean.TRUE; +import java.util.Objects; @Slf4j @RequiredArgsConstructor @@ -36,8 +29,6 @@ public class FeatureFlagServiceCEImpl implements FeatureFlagServiceCE { private final SessionUserService sessionUserService; - private final FF4j ff4j; - private final TenantService tenantService; private final UserIdentifierService userIdentifierService; @@ -49,26 +40,6 @@ public class FeatureFlagServiceCEImpl implements FeatureFlagServiceCE { private CachedFeatures cachedTenantFeatureFlags; - private Mono checkAll(String featureName, User user) { - Boolean check = check(featureName, user); - - if (TRUE.equals(check)) { - return Mono.just(true); - } - - return getAllFeatureFlagsForUser() - .flatMap(featureMap -> Mono.justOrEmpty(featureMap.get(featureName))) - .switchIfEmpty(Mono.just(false)); - } - - @Override - public Mono check(FeatureFlagEnum featureEnum, User user) { - if (featureEnum == null) { - return Mono.just(false); - } - return checkAll(featureEnum.toString(), user); - } - /** * This function checks if the feature is enabled for the current user. In case the user object is not present, * i.e. when the method is getting called internally via cron or other mechanism check for tenant level flag and @@ -79,32 +50,11 @@ public class FeatureFlagServiceCEImpl implements FeatureFlagServiceCE { */ @Override public Mono check(FeatureFlagEnum featureEnum) { - // Check if the feature is supported at the tenant level and provide a fallback as falsy value - // i.e. not supported - Mono isTenantFeatureSupported = this.getTenantFeatures() - .flatMap(featureMap -> Mono.justOrEmpty(featureMap.get(featureEnum.name()))) - .switchIfEmpty(Mono.just(false)); - - return sessionUserService - .getCurrentUser() - .flatMap(user -> check(featureEnum, user)) - .switchIfEmpty(isTenantFeatureSupported); - } - - @Override - public Boolean check(String featureName, User user) { - try { - return ff4j.check(featureName, new FlippingExecutionContext(Map.of(FieldName.USER, user))); - } catch (Exception e) { - // FF4J is configured not to auto-generate a flag if it's not present in init-flags.xml - // (see FeatureFlagConfig.java). - // Consequently, we anticipate that the flag may not exist in the FF4J context and need to handle any - // related exceptions silently. - if (!(e instanceof FeatureNotFoundException)) { - log.error("Error checking feature flag: {}", featureName, e); - } + if (Objects.isNull(featureEnum)) { + return Mono.just(Boolean.FALSE); } - return false; + return this.getAllFeatureFlagsForUser() + .map(featureMap -> featureMap.getOrDefault(featureEnum.name(), Boolean.FALSE)); } /** @@ -115,27 +65,16 @@ public class FeatureFlagServiceCEImpl implements FeatureFlagServiceCE { */ @Override public Mono> getAllFeatureFlagsForUser() { - Mono currentUser = sessionUserService.getCurrentUser().cache(); - Flux> featureUserTuple = Flux.fromIterable( - ff4j.getFeatures().keySet()) - .flatMap(featureName -> Mono.just(featureName).zipWith(currentUser)); - - // Filter out anonymous users, then collect feature flags into a Map - Mono> localFlagsForUser = featureUserTuple - .filter(objects -> !objects.getT2().isAnonymous()) - .collectMap(Tuple2::getT1, tuple -> check(tuple.getT1(), tuple.getT2())); - // Combine local flags, remote flags, and tenant features, and merge them into a single map - return localFlagsForUser.flatMap(localFlags -> this.getAllRemoteFeatureFlagsForUser() - .zipWith(this.getTenantFeatures()) + return Mono.zip(this.getAllRemoteFeatureFlagsForUser(), this.getTenantFeatures()) .map(remoteAndTenantFlags -> { - Map combinedFlags = new HashMap<>(localFlags); + Map combinedFlags = new HashMap<>(); combinedFlags.putAll(remoteAndTenantFlags.getT1()); // Always add the tenant level flags after the user flags to make sure tenant flags gets the // precedence combinedFlags.putAll(remoteAndTenantFlags.getT2()); return combinedFlags; - })); + }); } /** @@ -146,33 +85,33 @@ public class FeatureFlagServiceCEImpl implements FeatureFlagServiceCE { private Mono> getAllRemoteFeatureFlagsForUser() { Mono userMono = sessionUserService.getCurrentUser().cache(); return userMono.flatMap(user -> { - String userIdentifier = userIdentifierService.getUserIdentifier(user); - // Checks for flags present in cache and if the cache is not expired - return cacheableFeatureFlagHelper - .fetchUserCachedFlags(userIdentifier, user) - .flatMap(cachedFlags -> { - if (cachedFlags.getRefreshedAt().until(Instant.now(), ChronoUnit.MINUTES) - < FEATURE_FLAG_CACHE_TIME_MIN) { - return Mono.just(cachedFlags.getFlags()); - } else { - // empty the cache for the userIdentifier as expired - return cacheableFeatureFlagHelper - .evictUserCachedFlags(userIdentifier) - .then(cacheableFeatureFlagHelper.fetchUserCachedFlags(userIdentifier, user)) - .flatMap(cachedFlagsUpdated -> { - // In case the retrieval of the latest flags from CS encounters an error, the - // previous flags will serve as a fallback value. - if (cachedFlagsUpdated == null - || CollectionUtils.isNullOrEmpty(cachedFlagsUpdated.getFlags())) { - return cacheableFeatureFlagHelper - .updateUserCachedFlags(userIdentifier, cachedFlags) - .map(CachedFlags::getFlags); - } - return Mono.just(cachedFlagsUpdated.getFlags()); - }); - } - }); - }); + String userIdentifier = userIdentifierService.getUserIdentifier(user); + // Checks for flags present in cache and if the cache is not expired + return cacheableFeatureFlagHelper + .fetchUserCachedFlags(userIdentifier, user) + .flatMap(cachedFlags -> { + if (cachedFlags.getRefreshedAt().until(Instant.now(), ChronoUnit.MINUTES) + < FEATURE_FLAG_CACHE_TIME_MIN) { + return Mono.just(cachedFlags.getFlags()); + } + // empty the cache for the userIdentifier as expired + return cacheableFeatureFlagHelper + .evictUserCachedFlags(userIdentifier) + .then(cacheableFeatureFlagHelper.fetchUserCachedFlags(userIdentifier, user)) + .flatMap(cachedFlagsUpdated -> { + // In case the retrieval of the latest flags from CS encounters an error, + // the previous flags will serve as a fallback value. + if (cachedFlagsUpdated == null + || CollectionUtils.isNullOrEmpty(cachedFlagsUpdated.getFlags())) { + return cacheableFeatureFlagHelper + .updateUserCachedFlags(userIdentifier, cachedFlags) + .map(CachedFlags::getFlags); + } + return Mono.just(cachedFlagsUpdated.getFlags()); + }); + }); + }) + .switchIfEmpty(Mono.just(new HashMap<>())); } /** @@ -223,7 +162,8 @@ public class FeatureFlagServiceCEImpl implements FeatureFlagServiceCE { .map(cachedFeatures -> { cachedTenantFeatureFlags = cachedFeatures; return cachedFeatures.getFeatures(); - }); + }) + .switchIfEmpty(Mono.just(new HashMap<>())); } /** diff --git a/app/server/appsmith-server/src/main/resources/features/init-flags.xml b/app/server/appsmith-server/src/main/resources/features/init-flags.xml deleted file mode 100644 index 71944bda5e..0000000000 --- a/app/server/appsmith-server/src/main/resources/features/init-flags.xml +++ /dev/null @@ -1,13 +0,0 @@ - - - false - false - - - - - - - - \ No newline at end of file diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspectTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspectTest.java index 2a31a2002e..8a75128197 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspectTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/FeatureFlaggedMethodInvokerAspectTest.java @@ -4,6 +4,7 @@ import com.appsmith.server.aspect.component.TestComponent; import com.appsmith.server.featureflags.CachedFeatures; import com.appsmith.server.featureflags.FeatureFlagEnum; import com.appsmith.server.services.FeatureFlagService; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mockito; @@ -34,6 +35,16 @@ class FeatureFlaggedMethodInvokerAspectTest { private static final String CE_COMPATIBLE_RESPONSE = "ce_compatible_impl_method"; private static final String CE_RESPONSE = "ce_impl_method"; + @BeforeEach + void setUp() { + Mockito.when(featureFlagService.check(eq(FeatureFlagEnum.TENANT_TEST_FEATURE))) + .thenReturn(Mono.just(false)); + + CachedFeatures cachedFeatures = new CachedFeatures(); + cachedFeatures.setFeatures(Map.of(FeatureFlagEnum.TENANT_TEST_FEATURE.name(), Boolean.FALSE)); + Mockito.when(featureFlagService.getCachedTenantFeatureFlags()).thenReturn(cachedFeatures); + } + @Test void testEEOnlyMethod() { Mono resultMono = testComponent.eeOnlyMethod(); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/featureflags/strategies/EmailBasedRolloutStrategyTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/featureflags/strategies/EmailBasedRolloutStrategyTest.java deleted file mode 100644 index 1d23b82614..0000000000 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/featureflags/strategies/EmailBasedRolloutStrategyTest.java +++ /dev/null @@ -1,62 +0,0 @@ -package com.appsmith.server.featureflags.strategies; - -import com.appsmith.server.domains.User; -import lombok.extern.slf4j.Slf4j; -import org.ff4j.core.FlippingExecutionContext; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mockito; -import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.security.test.context.support.WithUserDetails; -import org.springframework.test.context.junit.jupiter.SpringExtension; - -import java.util.HashMap; -import java.util.Map; - -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; - -@ExtendWith(SpringExtension.class) -@SpringBootTest -@Slf4j -public class EmailBasedRolloutStrategyTest { - - EmailBasedRolloutStrategy strategy = new EmailBasedRolloutStrategy(); - - @BeforeEach - @WithUserDetails(value = "api_user") - public void setup() { - Map initParams = new HashMap<>(); - initParams.put("emailDomains", "example.com,another-example.com"); - strategy.init("test-feature", initParams); - } - - @Test - @WithUserDetails(value = "api_user") - public void testValidFeatureCheck() { - FlippingExecutionContext executionContext = Mockito.mock(FlippingExecutionContext.class); - - User user = new User(); - user.setEmail("test@EXAMPLE.com"); - Mockito.when(executionContext.getValue(Mockito.anyString(), Mockito.anyBoolean())) - .thenReturn(user); - - boolean evaluate = strategy.evaluate("test-feature", null, executionContext); - assertTrue(evaluate); - } - - @Test - @WithUserDetails(value = "api_user") - public void testInvalidFeatureCheck() { - FlippingExecutionContext executionContext = Mockito.mock(FlippingExecutionContext.class); - - User user = new User(); - user.setEmail("test@random.com"); - Mockito.when(executionContext.getValue(Mockito.anyString(), Mockito.anyBoolean())) - .thenReturn(user); - - boolean evaluate = strategy.evaluate("test-feature", null, executionContext); - assertFalse(evaluate); - } -} diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/MockCacheableFeatureFlagHelper.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/MockCacheableFeatureFlagHelper.java index 97caf19520..1e045796f5 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/MockCacheableFeatureFlagHelper.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/MockCacheableFeatureFlagHelper.java @@ -15,6 +15,11 @@ import reactor.core.publisher.Mono; import java.time.Instant; import java.util.HashMap; +import java.util.Map; + +import static com.appsmith.server.featureflags.FeatureFlagEnum.TENANT_TEST_FEATURE; +import static com.appsmith.server.featureflags.FeatureFlagEnum.TEST_FEATURE_1; +import static com.appsmith.server.featureflags.FeatureFlagEnum.TEST_FEATURE_2; @Profile("test") @Primary @@ -26,7 +31,10 @@ public class MockCacheableFeatureFlagHelper implements CacheableFeatureFlagHelpe public Mono fetchUserCachedFlags(String userIdentifier, User user) { CachedFlags cachedFlags = new CachedFlags(); cachedFlags.setRefreshedAt(Instant.now()); - cachedFlags.setFlags(new HashMap<>()); + Map flags = new HashMap<>(); + flags.put(TEST_FEATURE_1.name(), true); + flags.put(TEST_FEATURE_2.name(), false); + cachedFlags.setFlags(flags); return Mono.just(cachedFlags); } @@ -68,7 +76,9 @@ public class MockCacheableFeatureFlagHelper implements CacheableFeatureFlagHelpe @Override public Mono getRemoteFeaturesForTenant(FeaturesRequestDTO featuresRequestDTO) { FeaturesResponseDTO responseDTO = new FeaturesResponseDTO(); - responseDTO.setFeatures(new HashMap<>()); + Map features = new HashMap<>(); + features.put(TENANT_TEST_FEATURE.name(), true); + responseDTO.setFeatures(features); return Mono.just(responseDTO); } } 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 31e105de9c..d0ef5a9e12 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 @@ -11,8 +11,6 @@ import com.appsmith.server.services.CacheableFeatureFlagHelper; import com.appsmith.server.services.FeatureFlagService; import com.appsmith.server.services.TenantService; import lombok.extern.slf4j.Slf4j; -import org.ff4j.FF4j; -import org.ff4j.conf.XmlParser; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -21,10 +19,8 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mockito; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.boot.test.context.TestConfiguration; import org.springframework.boot.test.mock.mockito.MockBean; import org.springframework.boot.test.mock.mockito.SpyBean; -import org.springframework.context.annotation.Bean; import org.springframework.data.redis.core.ReactiveRedisTemplate; import org.springframework.security.test.context.support.WithUserDetails; import org.springframework.test.annotation.DirtiesContext; @@ -45,7 +41,6 @@ import static com.appsmith.server.featureflags.FeatureFlagEnum.TENANT_TEST_FEATU import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; @@ -93,30 +88,7 @@ public class FeatureFlagServiceCETest { @WithUserDetails(value = "api_user") public void testNullFeatureCheck() { StepVerifier.create(featureFlagService.check(null)) - .assertNext(result -> { - assertFalse(result); - }) - .verifyComplete(); - } - - @Test - @WithUserDetails(value = "api_user") - public void testFeatureCheckForPonderationStrategy() { - Math.random(); - StepVerifier.create(featureFlagService.check(FeatureFlagEnum.TEST_FEATURE_2)) - .assertNext(result -> { - assertTrue(result); - }) - .verifyComplete(); - } - - @Test - @WithUserDetails(value = "api_user") - public void testFeatureCheckForAppsmithUserStrategy() { - StepVerifier.create(featureFlagService.check(FeatureFlagEnum.TEST_FEATURE_1)) - .assertNext(result -> { - assertFalse(result); - }) + .assertNext(Assertions::assertFalse) .verifyComplete(); } @@ -131,17 +103,6 @@ public class FeatureFlagServiceCETest { .verifyComplete(); } - @Test - @WithUserDetails(value = "api_user") - public void testFeatureCheckForEmailStrategy() { - StepVerifier.create(featureFlagService.getAllFeatureFlagsForUser()) - .assertNext(result -> { - assertNotNull(result); - assertTrue(result.containsKey(FeatureFlagEnum.TEST_FEATURE_3.toString())); - }) - .verifyComplete(); - } - @Test @WithUserDetails(value = "api_user") public void testGetFeaturesForUser_overrideWithTenantFeature() { @@ -150,12 +111,12 @@ public class FeatureFlagServiceCETest { StepVerifier.create(featureFlagService.getAllFeatureFlagsForUser()) .assertNext(result -> { assertNotNull(result); - assertNull(result.get(TENANT_TEST_FEATURE.toString())); + assertTrue(result.get(TENANT_TEST_FEATURE.toString())); }) .verifyComplete(); Map tenantFeatures = new HashMap<>(); - tenantFeatures.put(TENANT_TEST_FEATURE.toString(), true); + tenantFeatures.put(TENANT_TEST_FEATURE.toString(), false); FeaturesResponseDTO responseDTO = new FeaturesResponseDTO(); responseDTO.setFeatures(tenantFeatures); doReturn(Mono.just(responseDTO)).when(cacheableFeatureFlagHelper).getRemoteFeaturesForTenant(any()); @@ -163,7 +124,7 @@ public class FeatureFlagServiceCETest { StepVerifier.create(featureFlagService.getAllFeatureFlagsForUser()) .assertNext(result -> { assertNotNull(result); - assertTrue(result.get(TENANT_TEST_FEATURE.toString())); + assertFalse(result.get(TENANT_TEST_FEATURE.toString())); }) .verifyComplete(); } @@ -175,9 +136,7 @@ public class FeatureFlagServiceCETest { Mono cachedFlagsMono = cacheableFeatureFlagHelper.fetchUserCachedFlags(userIdentifier, dummyUser); Mono hasKeyMono = reactiveRedisTemplate.hasKey("featureFlag:" + userIdentifier); StepVerifier.create(cachedFlagsMono.then(hasKeyMono)) - .assertNext(isKeyPresent -> { - assertTrue(isKeyPresent); - }) + .assertNext(Assertions::assertTrue) .verifyComplete(); } @@ -187,9 +146,7 @@ public class FeatureFlagServiceCETest { Mono evictCache = cacheableFeatureFlagHelper.evictUserCachedFlags(userIdentifier); Mono hasKeyMono = reactiveRedisTemplate.hasKey("featureFlag:" + userIdentifier); StepVerifier.create(evictCache.then(hasKeyMono)) - .assertNext(isKeyPresent -> { - assertFalse(isKeyPresent); - }) + .assertNext(Assertions::assertFalse) .verifyComplete(); } @@ -324,34 +281,23 @@ public class FeatureFlagServiceCETest { // Assert that the cached feature flags are empty before the remote fetch CachedFeatures cachedFeaturesBeforeRemoteCall = featureFlagService.getCachedTenantFeatureFlags(); - assertTrue(cachedFeaturesBeforeRemoteCall.getFeatures().isEmpty()); + assertThat(cachedFeaturesBeforeRemoteCall.getFeatures().size()).isEqualTo(1); + assertTrue(cachedFeaturesBeforeRemoteCall.getFeatures().get(TENANT_TEST_FEATURE.name())); Map tenantFeatures = new HashMap<>(); - tenantFeatures.put(TENANT_TEST_FEATURE.name(), true); + tenantFeatures.put(TENANT_TEST_FEATURE.name(), false); FeaturesResponseDTO responseDTO = new FeaturesResponseDTO(); responseDTO.setFeatures(tenantFeatures); doReturn(Mono.just(responseDTO)).when(cacheableFeatureFlagHelper).getRemoteFeaturesForTenant(any()); StepVerifier.create(featureFlagService.getTenantFeatures()) .assertNext(result -> { assertNotNull(result); - assertTrue(result.get(TENANT_TEST_FEATURE.name())); + assertFalse(result.get(TENANT_TEST_FEATURE.name())); // Check if the cached feature flags are updated after the remote fetch CachedFeatures cachedFeaturesAfterRemoteCall = featureFlagService.getCachedTenantFeatureFlags(); - assertTrue(cachedFeaturesAfterRemoteCall.getFeatures().get(TENANT_TEST_FEATURE.name())); + assertFalse(cachedFeaturesAfterRemoteCall.getFeatures().get(TENANT_TEST_FEATURE.name())); }) .verifyComplete(); } - - @TestConfiguration - static class TestFeatureFlagConfig { - - @Bean - FF4j ff4j() { - FF4j ff4j = new FF4j(new XmlParser(), "features/init-flags-test.xml") - .audit(true) - .autoCreate(false); - return ff4j; - } - } } diff --git a/app/server/appsmith-server/src/test/resources/features/init-flags-test.xml b/app/server/appsmith-server/src/test/resources/features/init-flags-test.xml deleted file mode 100644 index 5273ee9d2b..0000000000 --- a/app/server/appsmith-server/src/test/resources/features/init-flags-test.xml +++ /dev/null @@ -1,24 +0,0 @@ - - - false - false - - - - - - - - - - - - - - - - - -