Skip to content

Commit

Permalink
Post review changes (#548)
Browse files Browse the repository at this point in the history
  • Loading branch information
gthea authored Oct 27, 2023
1 parent 2d1147d commit b671826
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 38 deletions.
10 changes: 3 additions & 7 deletions src/main/java/io/split/android/client/FlagSetsFilterImpl.java
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -24,13 +26,7 @@ public boolean intersect(Set<String> sets) {
return false;
}

for (String set : sets) {
if (mFlagSets.contains(set)) {
return true;
}
}

return false;
return !Sets.intersection(mFlagSets, sets).isEmpty();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -64,22 +62,9 @@ public void process(List<Split> activeFeatureFlags, List<Split> archivedFeatureF
return;
}

boolean shouldArchive = true;
Set<String> 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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,31 @@ public boolean isValid(String value) {
}

@Override
public Set<String> items(List<String> values, FlagSetsFilter flagSetsFilter) {
public Set<String> items(String method, List<String> values, FlagSetsFilter flagSetsFilter) {
Set<String> setsToReturn = new HashSet<>();

if (values == null || values.isEmpty()) {
return setsToReturn;
}

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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ public interface SplitFilterValidator {

boolean isValid(String value);

Set<String> items(List<String> values, FlagSetsFilter flagSetsFilter);
Set<String> items(String method, List<String> values, FlagSetsFilter flagSetsFilter);

class ValidationResult {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,11 +187,11 @@ public Map<String, String> getTreatmentsByFlagSet(@NonNull String flagSet, @Null
String validationTag = ValidationTag.GET_TREATMENTS_BY_FLAG_SET;
Set<String> 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 {
Expand All @@ -212,11 +212,11 @@ public Map<String, String> getTreatmentsByFlagSets(@NonNull List<String> flagSet
String validationTag = ValidationTag.GET_TREATMENTS_BY_FLAG_SETS;
Set<String> 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 {
Expand All @@ -237,7 +237,7 @@ public Map<String, SplitResult> getTreatmentsWithConfigByFlagSet(@NonNull String
String validationTag = ValidationTag.GET_TREATMENTS_WITH_CONFIG_BY_FLAG_SET;
Set<String> 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);
Expand All @@ -262,11 +262,11 @@ public Map<String, SplitResult> getTreatmentsWithConfigByFlagSets(@NonNull List<
String validationTag = ValidationTag.GET_TREATMENTS_WITH_CONFIG_BY_FLAG_SETS;
Set<String> 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 {
Expand Down Expand Up @@ -403,9 +403,9 @@ private void recordLatency(Method treatment, long startTime) {
}

@NonNull
private Set<String> getNamesFromSet(@NonNull List<String> flagSets) {
private Set<String> getNamesFromSet(@NonNull String method, @NonNull List<String> flagSets) {

Set<String> setsToEvaluate = mFlagSetsValidator.items(flagSets, mFlagSetsFilter);
Set<String> setsToEvaluate = mFlagSetsValidator.items(method, flagSets, mFlagSetsFilter);

if (setsToEvaluate.isEmpty()) {
return new HashSet<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ public void creatingWithFilterWithEmptyConfiguredValuesProcessesEverything() {
}

@Test
public void nonConfiguredSetsAreRemovedFromSplit() {
public void nonConfiguredSetsAreNotRemovedFromSplit() {
Set<String> configuredSets = new HashSet<>();
configuredSets.add("set_1");
configuredSets.add("set_2");
Expand All @@ -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"));
}

Expand Down

0 comments on commit b671826

Please sign in to comment.