From b67182638c5c0accf565c47925cafe555cda6ac3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Thea?= Date: Fri, 27 Oct 2023 18:20:09 -0300 Subject: [PATCH] Post review changes (#548) --- .../android/client/FlagSetsFilterImpl.java | 10 +++------- .../splits/FeatureFlagProcessStrategy.java | 17 +---------------- .../validators/FlagSetsValidatorImpl.java | 14 +++++++++++++- .../client/validators/SplitFilterValidator.java | 2 +- .../client/validators/TreatmentManagerImpl.java | 12 ++++++------ .../TreatmentManagerWithFlagSetsTest.java | 6 +++--- .../splits/SplitChangeProcessorTest.java | 8 ++++---- 7 files changed, 31 insertions(+), 38 deletions(-) diff --git a/src/main/java/io/split/android/client/FlagSetsFilterImpl.java b/src/main/java/io/split/android/client/FlagSetsFilterImpl.java index d8c731280..68c8d5501 100644 --- a/src/main/java/io/split/android/client/FlagSetsFilterImpl.java +++ b/src/main/java/io/split/android/client/FlagSetsFilterImpl.java @@ -1,5 +1,7 @@ package io.split.android.client; +import com.google.common.collect.Sets; + import java.util.Collection; import java.util.HashSet; import java.util.Set; @@ -24,13 +26,7 @@ public boolean intersect(Set sets) { return false; } - for (String set : sets) { - if (mFlagSets.contains(set)) { - return true; - } - } - - return false; + return !Sets.intersection(mFlagSets, sets).isEmpty(); } @Override diff --git a/src/main/java/io/split/android/client/service/splits/FeatureFlagProcessStrategy.java b/src/main/java/io/split/android/client/service/splits/FeatureFlagProcessStrategy.java index 1cffb6669..8628203a2 100644 --- a/src/main/java/io/split/android/client/service/splits/FeatureFlagProcessStrategy.java +++ b/src/main/java/io/split/android/client/service/splits/FeatureFlagProcessStrategy.java @@ -2,9 +2,7 @@ import androidx.annotation.NonNull; -import java.util.HashSet; import java.util.List; -import java.util.Set; import io.split.android.client.FlagSetsFilter; import io.split.android.client.dtos.Split; @@ -64,22 +62,9 @@ public void process(List activeFeatureFlags, List archivedFeatureF return; } - boolean shouldArchive = true; - Set newSets = new HashSet<>(); - for (String set : featureFlag.sets) { - if (mFlagSetsFilter.intersect(set)) { - newSets.add(set); // Add the flag set to the valid group - // Since the feature flag has at least one set that matches the configured sets, - // we process it according to its status - shouldArchive = false; - } - } - - if (shouldArchive) { + if (!mFlagSetsFilter.intersect(featureFlag.sets)) { archivedFeatureFlags.add(featureFlag); } else { - // Replace the feature flag sets with the intersection of the configured sets and the feature flag sets - featureFlag.sets = newSets; mStatusProcessStrategy.process(activeFeatureFlags, archivedFeatureFlags, featureFlag); } } diff --git a/src/main/java/io/split/android/client/validators/FlagSetsValidatorImpl.java b/src/main/java/io/split/android/client/validators/FlagSetsValidatorImpl.java index 5553eaa37..57642a6ec 100644 --- a/src/main/java/io/split/android/client/validators/FlagSetsValidatorImpl.java +++ b/src/main/java/io/split/android/client/validators/FlagSetsValidatorImpl.java @@ -66,7 +66,7 @@ public boolean isValid(String value) { } @Override - public Set items(List values, FlagSetsFilter flagSetsFilter) { + public Set items(String method, List values, FlagSetsFilter flagSetsFilter) { Set setsToReturn = new HashSet<>(); if (values == null || values.isEmpty()) { @@ -74,11 +74,23 @@ public Set items(List values, FlagSetsFilter flagSetsFilter) { } for (String flagSet : values) { + if (flagSet.trim().length() != flagSet.length()) { + Logger.w(method + ": Flag Set name " + flagSet + " has extra whitespace, trimming"); + flagSet = flagSet.trim(); + } + + if (!flagSet.toLowerCase().equals(flagSet)) { + Logger.w(method + ": Flag Set name "+flagSet+" should be all lowercase - converting string to lowercase"); + flagSet = flagSet.toLowerCase(); + } + if (!isValid(flagSet)) { + Logger.w(method + ": you passed "+ flagSet +", Flag Set must adhere to the regular expressions "+ FLAG_SET_REGEX +". This means a Flag Set must be start with a letter, be in lowercase, alphanumeric and have a max length of 50 characters. "+ flagSet +" was discarded."); continue; } if (flagSetsFilter != null && !flagSetsFilter.intersect(flagSet)) { + Logger.w(method + ": you passed Flag Set: "+ flagSet +" and is not part of the configured Flag set list, ignoring the request."); continue; } diff --git a/src/main/java/io/split/android/client/validators/SplitFilterValidator.java b/src/main/java/io/split/android/client/validators/SplitFilterValidator.java index b5f0d1b2b..1e3255b84 100644 --- a/src/main/java/io/split/android/client/validators/SplitFilterValidator.java +++ b/src/main/java/io/split/android/client/validators/SplitFilterValidator.java @@ -11,7 +11,7 @@ public interface SplitFilterValidator { boolean isValid(String value); - Set items(List values, FlagSetsFilter flagSetsFilter); + Set items(String method, List values, FlagSetsFilter flagSetsFilter); class ValidationResult { diff --git a/src/main/java/io/split/android/client/validators/TreatmentManagerImpl.java b/src/main/java/io/split/android/client/validators/TreatmentManagerImpl.java index 8ac4e1343..05469a2bf 100644 --- a/src/main/java/io/split/android/client/validators/TreatmentManagerImpl.java +++ b/src/main/java/io/split/android/client/validators/TreatmentManagerImpl.java @@ -187,11 +187,11 @@ public Map getTreatmentsByFlagSet(@NonNull String flagSet, @Null String validationTag = ValidationTag.GET_TREATMENTS_BY_FLAG_SET; Set names = new HashSet<>(); try { - names = getNamesFromSet(Collections.singletonList(flagSet)); if (isClientDestroyed) { mValidationLogger.e(CLIENT_DESTROYED_MESSAGE, validationTag); return controlTreatmentsForSplits(new ArrayList<>(names), validationTag); } + names = getNamesFromSet("getTreatmentsByFlagSet", Collections.singletonList(flagSet)); long start = System.currentTimeMillis(); try { @@ -212,11 +212,11 @@ public Map getTreatmentsByFlagSets(@NonNull List flagSet String validationTag = ValidationTag.GET_TREATMENTS_BY_FLAG_SETS; Set names = new HashSet<>(); try { - names = getNamesFromSet(flagSets); if (isClientDestroyed) { mValidationLogger.e(CLIENT_DESTROYED_MESSAGE, validationTag); return controlTreatmentsForSplits(new ArrayList<>(names), validationTag); } + names = getNamesFromSet("getTreatmentsByFlagSets", flagSets); long start = System.currentTimeMillis(); try { @@ -237,7 +237,7 @@ public Map getTreatmentsWithConfigByFlagSet(@NonNull String String validationTag = ValidationTag.GET_TREATMENTS_WITH_CONFIG_BY_FLAG_SET; Set names = new HashSet<>(); try { - names = getNamesFromSet(Collections.singletonList(flagSet)); + names = getNamesFromSet("getTreatmentsWithConfigByFlagSet", Collections.singletonList(flagSet)); if (isClientDestroyed) { mValidationLogger.e(CLIENT_DESTROYED_MESSAGE, validationTag); return controlTreatmentsForSplitsWithConfig(new ArrayList<>(names), validationTag); @@ -262,11 +262,11 @@ public Map getTreatmentsWithConfigByFlagSets(@NonNull List< String validationTag = ValidationTag.GET_TREATMENTS_WITH_CONFIG_BY_FLAG_SETS; Set names = new HashSet<>(); try { - names = getNamesFromSet(flagSets); if (isClientDestroyed) { mValidationLogger.e(CLIENT_DESTROYED_MESSAGE, validationTag); return controlTreatmentsForSplitsWithConfig(new ArrayList<>(names), validationTag); } + names = getNamesFromSet("getTreatmentsWithConfigByFlagSets", flagSets); long start = System.currentTimeMillis(); try { @@ -403,9 +403,9 @@ private void recordLatency(Method treatment, long startTime) { } @NonNull - private Set getNamesFromSet(@NonNull List flagSets) { + private Set getNamesFromSet(@NonNull String method, @NonNull List flagSets) { - Set setsToEvaluate = mFlagSetsValidator.items(flagSets, mFlagSetsFilter); + Set setsToEvaluate = mFlagSetsValidator.items(method, flagSets, mFlagSetsFilter); if (setsToEvaluate.isEmpty()) { return new HashSet<>(); diff --git a/src/test/java/io/split/android/client/TreatmentManagerWithFlagSetsTest.java b/src/test/java/io/split/android/client/TreatmentManagerWithFlagSetsTest.java index b732f466e..88d6ffbfa 100644 --- a/src/test/java/io/split/android/client/TreatmentManagerWithFlagSetsTest.java +++ b/src/test/java/io/split/android/client/TreatmentManagerWithFlagSetsTest.java @@ -89,7 +89,7 @@ public void tearDown() { public void getTreatmentsByFlagSetDestroyedDoesNotUseEvaluator() { mTreatmentManager.getTreatmentsByFlagSet("set_1", null, true); - verify(mSplitsStorage).getNamesByFlagSets(any()); + verify(mSplitsStorage, times(0)).getNamesByFlagSets(any()); verify(mEvaluator, times(0)).getTreatment(any(), any(), any(), anyMap()); } @@ -186,7 +186,7 @@ public void getTreatmentsByFlagSetRecordsTelemetry() { public void getTreatmentsByFlagSetsDestroyedDoesNotUseEvaluator() { mTreatmentManager.getTreatmentsByFlagSets(Collections.singletonList("set_1"), null, true); - verify(mSplitsStorage).getNamesByFlagSets(any()); + verify(mSplitsStorage, times(0)).getNamesByFlagSets(any()); verify(mEvaluator, times(0)).getTreatment(any(), any(), any(), anyMap()); } @@ -361,7 +361,7 @@ public void getTreatmentsWithConfigByFlagSetRecordsTelemetry() { public void getTreatmentsWithConfigByFlagSetsDestroyedDoesNotUseEvaluator() { mTreatmentManager.getTreatmentsWithConfigByFlagSets(Collections.singletonList("set_1"), null, true); - verify(mSplitsStorage).getNamesByFlagSets(any()); + verify(mSplitsStorage, times(0)).getNamesByFlagSets(any()); verify(mEvaluator, times(0)).getTreatment(any(), any(), any(), anyMap()); } diff --git a/src/test/java/io/split/android/client/service/splits/SplitChangeProcessorTest.java b/src/test/java/io/split/android/client/service/splits/SplitChangeProcessorTest.java index 191896074..66ca13210 100644 --- a/src/test/java/io/split/android/client/service/splits/SplitChangeProcessorTest.java +++ b/src/test/java/io/split/android/client/service/splits/SplitChangeProcessorTest.java @@ -246,7 +246,7 @@ public void creatingWithFilterWithEmptyConfiguredValuesProcessesEverything() { } @Test - public void nonConfiguredSetsAreRemovedFromSplit() { + public void nonConfiguredSetsAreNotRemovedFromSplit() { Set configuredSets = new HashSet<>(); configuredSets.add("set_1"); configuredSets.add("set_2"); @@ -270,14 +270,14 @@ public void nonConfiguredSetsAreRemovedFromSplit() { Split processedSplit1 = result.getActiveSplits().get(0); Assert.assertEquals(split1.name, processedSplit1.name); Assert.assertEquals(2, initialSplit1Sets); - Assert.assertEquals(1, processedSplit1.sets.size()); + Assert.assertEquals(2, processedSplit1.sets.size()); Assert.assertTrue(processedSplit1.sets.contains("set_1")); - Assert.assertFalse(processedSplit1.sets.contains("set_3")); + Assert.assertTrue(processedSplit1.sets.contains("set_3")); Split processedSplit2 = result.getActiveSplits().get(1); Assert.assertEquals(split2.name, processedSplit2.name); Assert.assertEquals(2, initialSplit2Sets); - Assert.assertEquals(1, processedSplit2.sets.size()); + Assert.assertEquals(2, processedSplit2.sets.size()); Assert.assertTrue(processedSplit2.sets.contains("set_2")); }