diff --git a/.github/workflows/sdk-spec.yml b/.github/workflows/sdk-spec.yml index 7ad90ac0..ec972bbc 100644 --- a/.github/workflows/sdk-spec.yml +++ b/.github/workflows/sdk-spec.yml @@ -11,7 +11,8 @@ permissions: jobs: sdk-spec: # Keep the ref in `uses:` and `spec-ref` in sync. - uses: Flagsmith/sdk-spec/.github/workflows/run-spec.yml@v0.3.0 + uses: Flagsmith/sdk-spec/.github/workflows/run-spec.yml@v0.7.0 with: - spec-ref: v0.3.0 + spec-ref: v0.7.0 language: java + tags: core or local-evaluation diff --git a/.gitmodules b/.gitmodules index e55e7996..dacf09bd 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,7 +1,7 @@ [submodule "src/test/java/com/flagsmith/flagengine/enginetestdata"] path = src/test/java/com/flagsmith/flagengine/enginetestdata url = git@github.com:Flagsmith/engine-test-data.git - branch = v3.7.0 + branch = v3.11.0 [submodule "third-party/codspeed-jvm"] path = third-party/codspeed-jvm url = https://github.com/CodSpeedHQ/codspeed-jvm.git diff --git a/sdk-spec/Steps.java b/sdk-spec/Steps.java index f5d80d49..74f05c65 100644 --- a/sdk-spec/Steps.java +++ b/sdk-spec/Steps.java @@ -1,4 +1,5 @@ import com.flagsmith.FlagsmithClient; +import com.flagsmith.config.FlagsmithConfig; import com.flagsmith.exceptions.FlagsmithClientError; import com.flagsmith.models.BaseFlag; import com.flagsmith.models.Flags; @@ -35,6 +36,17 @@ static FlagsmithClient createClient(String environmentKey, String apiUrl) { return FlagsmithClient.newBuilder().setApiKey(environmentKey).withApiUrl(apiUrl).build(); } + // Create the SDK client with local evaluation + static FlagsmithClient createClientWithLocalEvaluation(String environmentKey, String apiUrl) { + return FlagsmithClient.newBuilder() + .setApiKey(environmentKey) + .withConfiguration(FlagsmithConfig.newBuilder() + .baseUri(apiUrl) + .withLocalEvaluation(true) + .build()) + .build(); + } + // When the application gets the environment flags static List getEnvironmentFlags(FlagsmithClient flagsmith) throws FlagsmithClientError { diff --git a/src/main/java/com/flagsmith/FlagsmithClient.java b/src/main/java/com/flagsmith/FlagsmithClient.java index 8233f53e..9e61f4df 100644 --- a/src/main/java/com/flagsmith/FlagsmithClient.java +++ b/src/main/java/com/flagsmith/FlagsmithClient.java @@ -9,6 +9,8 @@ import com.flagsmith.flagengine.Engine; import com.flagsmith.flagengine.EvaluationContext; import com.flagsmith.flagengine.EvaluationResult; +import com.flagsmith.flagengine.SegmentRule; +import com.flagsmith.flagengine.Segments; import com.flagsmith.interfaces.FlagsmithCache; import com.flagsmith.interfaces.FlagsmithSdk; import com.flagsmith.mappers.EngineMappers; @@ -30,9 +32,11 @@ import java.util.stream.Collectors; import lombok.AccessLevel; import lombok.Data; +import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.NonNull; import lombok.Setter; +import lombok.ToString; import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -43,9 +47,17 @@ @Data public class FlagsmithClient { + private static final List IDENTITY_FREE_PROPERTY_PREFIXES = List.of( + "$.environment.", "$.environment[", "$.flags.", "$.flags["); + private final FlagsmithLogger logger = new FlagsmithLogger(); private FlagsmithSdk flagsmithSdk; private EvaluationContext evaluationContext; + @Getter(AccessLevel.NONE) + @Setter(AccessLevel.NONE) + @EqualsAndHashCode.Exclude + @ToString.Exclude + private EvaluationContext environmentEvaluationContext; private PollingManager pollingManager; @Getter(AccessLevel.PACKAGE) @Setter(AccessLevel.NONE) @@ -58,6 +70,19 @@ public static FlagsmithClient.Builder newBuilder() { return new FlagsmithClient.Builder(); } + /** + * Set the evaluation context, and derive the one environment flags evaluate against. + * + * @param evaluationContext the evaluation context + */ + public void setEvaluationContext(EvaluationContext evaluationContext) { + this.environmentEvaluationContext = evaluationContext == null + ? null + : new EvaluationContext(evaluationContext) + .withSegments(getIdentityFreeSegments(evaluationContext.getSegments())); + this.evaluationContext = evaluationContext; + } + /** * Load the environment flags in the environment variable from the API. */ @@ -68,7 +93,7 @@ public void updateEnvironment() { // if we didn't get an environment from the API, // then don't overwrite the copy we already have. if (updatedEvaluationContext != null) { - this.evaluationContext = updatedEvaluationContext; + setEvaluationContext(updatedEvaluationContext); } else { logger.error(getEnvironmentUpdateErrorMessage()); } @@ -411,17 +436,15 @@ private EventProcessor requireEventProcessor(String action) { } private Flags getEnvironmentFlagsFromEvaluationContext() throws FlagsmithClientError { - if (evaluationContext == null) { + final EvaluationContext context = environmentEvaluationContext; + if (context == null) { if (getConfig().getFlagsmithFlagDefaults() == null) { throw new FlagsmithClientError("Unable to get flags. No environment present."); } return getDefaultFlags(); } - final EvaluationResult result = Engine.getEvaluationResult( - new EvaluationContext(evaluationContext) - .withSegments(null) - ); + final EvaluationResult result = Engine.getEvaluationResult(context); return Flags.fromEvaluationResult( result, @@ -429,6 +452,32 @@ private Flags getEnvironmentFlagsFromEvaluationContext() throws FlagsmithClientE getConfig().getFlagsmithFlagDefaults()); } + /* + * The segments that resolve without an identity, i.e. those whose every + * condition reads only the environment or other flags. + */ + private static Segments getIdentityFreeSegments(Segments segments) { + if (segments == null) { + return null; + } + Segments identityFreeSegments = new Segments(); + segments.getAdditionalProperties().forEach((segmentKey, segmentContext) -> { + if (isIdentityFree(segmentContext.getRules())) { + identityFreeSegments.setAdditionalProperty(segmentKey, segmentContext); + } + }); + return identityFreeSegments; + } + + private static boolean isIdentityFree(List rules) { + return rules == null || rules.stream().allMatch(rule -> + (rule.getConditions() == null || rule.getConditions().stream().allMatch(condition -> + condition.getProperty() != null + && IDENTITY_FREE_PROPERTY_PREFIXES.stream() + .anyMatch(condition.getProperty()::startsWith))) + && isIdentityFree(rule.getRules())); + } + private Flags getIdentityFlagsFromEvaluationContext( String identifier, Map traits) throws FlagsmithClientError { @@ -748,8 +797,8 @@ public FlagsmithClient build() { throw new FlagsmithRuntimeError( "Cannot use both default flag handler and offline handler."); } - client.evaluationContext = EngineMappers.mapEnvironmentToContext( - configuration.getOfflineHandler().getEnvironment()); + client.setEvaluationContext(EngineMappers.mapEnvironmentToContext( + configuration.getOfflineHandler().getEnvironment())); } EventProcessor processor = null; diff --git a/src/main/java/com/flagsmith/flagengine/Engine.java b/src/main/java/com/flagsmith/flagengine/Engine.java index 269c1006..43791d82 100644 --- a/src/main/java/com/flagsmith/flagengine/Engine.java +++ b/src/main/java/com/flagsmith/flagengine/Engine.java @@ -1,15 +1,26 @@ package com.flagsmith.flagengine; +import com.fasterxml.jackson.databind.ObjectMapper; import com.flagsmith.flagengine.segments.SegmentEvaluator; import com.flagsmith.flagengine.utils.Hashing; import java.math.BigDecimal; +import java.util.AbstractMap; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.tuple.ImmutablePair; -public class Engine { +public class Engine { + public static final String CIRCULAR_DEPENDENCY_REASON = "ERROR; code=CIRCULAR_DEPENDENCY"; + + private static final ObjectMapper mapper = new ObjectMapper(); + private static class SegmentEvaluationResult { List segments; HashMap> segmentFeatureContexts; @@ -30,6 +41,164 @@ public HashMap> getSegmentFeatureC } } + /** + * The `$.flags` mapping, resolving a flag when a condition first reads it. + */ + private static class LazyFlags extends AbstractMap { + private final EvaluationContext context; + private final Map resolved = new LinkedHashMap<>(); + private DependencyResolver resolver; + + /** Names of the flags found to be in a dependency cycle. */ + private final Set cyclic = new HashSet<>(); + + /** Whether a condition has read a flag, even one that resolved to nothing. */ + private boolean used = false; + + /** Incremented whenever a cycle is cut, so callers can tell a verdict rests on one. */ + private int cycleHits = 0; + + LazyFlags(EvaluationContext context) { + this.context = context; + } + + @Override + public Object get(Object key) { + if (!(key instanceof String)) { + return null; + } + String featureName = (String) key; + if (!resolved.containsKey(featureName)) { + used = true; + if (resolver == null) { + resolver = new DependencyResolver(context, this); + } + resolver.resolveFeature(featureName); + } + FlagResult flagResult = resolved.get(featureName); + return flagResult == null ? null : mapper.convertValue(flagResult, Map.class); + } + + @Override + public boolean containsKey(Object key) { + return get(key) != null; + } + + @Override + public Set> entrySet() { + return resolved.keySet().stream() + .map(featureName -> new SimpleImmutableEntry<>(featureName, get(featureName))) + .collect(Collectors.toCollection(HashSet::new)); + } + } + + /** + * Resolves overrides for a context with flag dependencies, memoising the results. + */ + private static class DependencyResolver { + private final EvaluationContext context; + private final LazyFlags flags; + private final Map segmentMatches = new HashMap<>(); + private final List resolving = new ArrayList<>(); + private final Map> segmentKeysByFeatureName = new HashMap<>(); + + DependencyResolver(EvaluationContext context, LazyFlags flags) { + this.context = context; + this.flags = flags; + context.getSegments().getAdditionalProperties().forEach((segmentKey, segmentContext) -> { + List overrides = segmentContext.getOverrides(); + if (overrides != null) { + for (FeatureContext override : overrides) { + segmentKeysByFeatureName + .computeIfAbsent(override.getName(), name -> new ArrayList<>()) + .add(segmentKey); + } + } + }); + } + + void resolveFeature(String featureName) { + int cycleStart = resolving.indexOf(featureName); + if (cycleStart != -1) { + flags.cyclic.addAll(resolving.subList(cycleStart, resolving.size())); + flags.cycleHits++; + return; + } + Features features = context.getFeatures(); + FeatureContext featureContext = features == null + ? null + : features.getAdditionalProperties().get(featureName); + if (featureContext == null) { + return; + } + + resolving.add(featureName); + ImmutablePair segmentOverride; + try { + segmentOverride = getSegmentOverride(featureName); + } finally { + resolving.remove(resolving.size() - 1); + } + + if (flags.cyclic.contains(featureName)) { + return; + } + + String identityKey = getIdentityKey(context); + if (segmentOverride != null) { + flags.resolved.put(featureName, getFlagResultFromFeatureContext( + segmentOverride.getRight(), + identityKey, + "TARGETING_MATCH; segment=" + segmentOverride.getLeft())); + } else { + flags.resolved.put(featureName, getFlagResultFromFeatureContext( + featureContext, identityKey, "DEFAULT")); + } + } + + private boolean matchesSegment(String segmentKey) { + Boolean memoised = segmentMatches.get(segmentKey); + if (memoised != null) { + return memoised; + } + + int cycleHits = flags.cycleHits; + boolean matches = SegmentEvaluator.isContextInSegment( + context, context.getSegments().getAdditionalProperties().get(segmentKey), flags); + + if (flags.cycleHits != cycleHits) { + return false; // The verdict rests on a flag left unresolved by a cycle. + } + + segmentMatches.put(segmentKey, matches); + return matches; + } + + private ImmutablePair getSegmentOverride(String featureName) { + ImmutablePair segmentOverride = null; + + for (String segmentKey : segmentKeysByFeatureName.getOrDefault(featureName, List.of())) { + if (!matchesSegment(segmentKey)) { + continue; + } + SegmentContext segmentContext = context.getSegments().getAdditionalProperties() + .get(segmentKey); + for (FeatureContext overrideFeatureContext : segmentContext.getOverrides()) { + if (!overrideFeatureContext.getName().equals(featureName)) { + continue; + } + if (winsOver(overrideFeatureContext, + segmentOverride == null ? null : segmentOverride.getRight())) { + segmentOverride = new ImmutablePair<>( + segmentContext.getName(), overrideFeatureContext); + } + } + } + + return segmentOverride; + } + } + /** * Get evaluation result for a given evaluation context. * @@ -38,9 +207,20 @@ public HashMap> getSegmentFeatureC */ public static EvaluationResult getEvaluationResult(EvaluationContext context) { context = getEnrichedEvaluationContext(context); - SegmentEvaluationResult segmentEvaluationResult = evaluateSegments(context); + LazyFlags resolved = new LazyFlags(context); + SegmentEvaluationResult segmentEvaluationResult = evaluateSegments(context, resolved); Flags flags = evaluateFeatures(context, segmentEvaluationResult.getSegmentFeatureContexts()); + if (resolved.used) { + flags.getAdditionalProperties().putAll(resolved.resolved); + for (String featureName : resolved.cyclic) { + FlagResult flagResult = flags.getAdditionalProperties().get(featureName); + if (flagResult != null) { + flagResult.setReason(CIRCULAR_DEPENDENCY_REASON); + } + } + } + return new EvaluationResult() .withFlags(flags) .withSegments(segmentEvaluationResult.getSegments()); @@ -48,7 +228,7 @@ public static EvaluationResult getEvaluationResult(EvaluationContext context) { /* * Get a version of the evaluation context enriched with derived data. - * + * * @param context Evaluation context. * @return Enriched evaluation context, or the original if no enrichment was needed. */ @@ -64,16 +244,41 @@ private static EvaluationContext getEnrichedEvaluationContext(EvaluationContext return context; } + /* + * Whether a segment override takes precedence over the one held so far. + * + * Lower priority wins, and the first seen wins a tie, so that precedence + * follows context order rather than the order segments are evaluated in. + */ + private static boolean winsOver(FeatureContext candidate, FeatureContext incumbent) { + return incumbent == null || getPriority(candidate) < getPriority(incumbent); + } + + private static double getPriority(FeatureContext featureContext) { + return featureContext.getPriority() == null + ? EngineConstants.WEAKEST_PRIORITY + : featureContext.getPriority(); + } + private static SegmentEvaluationResult evaluateSegments( - EvaluationContext context) { + EvaluationContext context, LazyFlags flags) { List segments = new ArrayList<>(); HashMap> segmentFeatureContexts = new HashMap<>(); Segments contextSegments = context.getSegments(); if (contextSegments != null) { + int cycleHits = flags.cycleHits; + for (SegmentContext segmentContext : contextSegments.getAdditionalProperties().values()) { - if (SegmentEvaluator.isContextInSegment(context, segmentContext)) { + boolean matches = SegmentEvaluator.isContextInSegment(context, segmentContext, flags); + + if (flags.cycleHits != cycleHits) { + cycleHits = flags.cycleHits; // The verdict rests on a flag left unresolved by a cycle. + continue; + } + + if (matches) { segments.add(new SegmentResult() .withName(segmentContext.getName()) .withMetadata(segmentContext.getMetadata())); @@ -83,26 +288,14 @@ private static SegmentEvaluationResult evaluateSegments( if (segmentOverrides != null) { for (FeatureContext featureContext : segmentOverrides) { String featureName = featureContext.getName(); + ImmutablePair incumbent = segmentFeatureContexts + .get(featureName); - if (segmentFeatureContexts.containsKey(featureName)) { - ImmutablePair existing = segmentFeatureContexts - .get(featureName); - FeatureContext existingFeatureContext = existing.getRight(); - - Double existingPriority = existingFeatureContext.getPriority() == null - ? EngineConstants.WEAKEST_PRIORITY - : existingFeatureContext.getPriority(); - Double featurePriority = featureContext.getPriority() == null - ? EngineConstants.WEAKEST_PRIORITY - : featureContext.getPriority(); - - if (existingPriority < featurePriority) { - continue; - } + if (winsOver(featureContext, incumbent == null ? null : incumbent.getRight())) { + segmentFeatureContexts.put(featureName, + new ImmutablePair( + segmentContext.getName(), featureContext)); } - segmentFeatureContexts.put(featureName, - new ImmutablePair( - segmentContext.getName(), featureContext)); } } } @@ -118,9 +311,7 @@ private static Flags evaluateFeatures( Features contextFeatures = context.getFeatures(); Flags flags = new Flags(); - String identityKey = context.getIdentity() != null - ? context.getIdentity().getKey() - : null; + String identityKey = getIdentityKey(context); if (contextFeatures != null) { for (FeatureContext featureContext : contextFeatures.getAdditionalProperties().values()) { @@ -144,45 +335,54 @@ private static Flags evaluateFeatures( return flags; } + private static String getIdentityKey(EvaluationContext context) { + return context.getIdentity() != null + ? context.getIdentity().getKey() + : null; + } + private static FlagResult getFlagResultFromFeatureContext( FeatureContext featureContext, String identityKey, String reason) { - if (identityKey != null) { - List variants = featureContext.getVariants(); - if (variants != null) { - Float percentageValue = Hashing.getInstance() - .getHashedPercentageForObjectIds(List.of(featureContext.getKey(), identityKey)); - - Float startPercentage = 0.0f; - - ArrayList sortedVariants = new ArrayList<>(variants); - sortedVariants.sort((a, b) -> { - Double priority = a.getPriority(); - Double comparedPriority = b.getPriority(); - return priority.compareTo(comparedPriority); - }); - for (FeatureValue variant : sortedVariants) { - Double weight = variant.getWeight(); - Float limit = startPercentage + weight.floatValue(); - if (startPercentage <= percentageValue && percentageValue < limit) { - return new FlagResult().withEnabled(featureContext.getEnabled()) - .withName(featureContext.getName()) - .withValue(variant.getValue()) - .withReason("SPLIT; weight=" + BigDecimal.valueOf(weight) - .stripTrailingZeros() - .toPlainString()) - .withMetadata(featureContext.getMetadata()); - } - startPercentage = limit; + Object value = featureContext.getValue(); + String variant = null; + + List variants = featureContext.getVariants(); + if (identityKey != null && variants != null && !variants.isEmpty()) { + variant = "control"; // A matched variant overrides the control bucket. + + Float percentageValue = Hashing.getInstance() + .getHashedPercentageForObjectIds(List.of(featureContext.getKey(), identityKey)); + + Float startPercentage = 0.0f; + + ArrayList sortedVariants = new ArrayList<>(variants); + sortedVariants.sort((a, b) -> { + Double priority = a.getPriority(); + Double comparedPriority = b.getPriority(); + return priority.compareTo(comparedPriority); + }); + for (FeatureValue featureValue : sortedVariants) { + Double weight = featureValue.getWeight(); + Float limit = startPercentage + weight.floatValue(); + if (startPercentage <= percentageValue && percentageValue < limit) { + value = featureValue.getValue(); + variant = featureValue.getKey(); + reason = "SPLIT; weight=" + BigDecimal.valueOf(weight) + .stripTrailingZeros() + .toPlainString(); + break; } + startPercentage = limit; } } return new FlagResult().withEnabled(featureContext.getEnabled()) .withName(featureContext.getName()) - .withValue(featureContext.getValue()) + .withValue(value) .withReason(reason) + .withVariant(variant) .withMetadata(featureContext.getMetadata()); } -} \ No newline at end of file +} diff --git a/src/main/java/com/flagsmith/flagengine/segments/SegmentEvaluator.java b/src/main/java/com/flagsmith/flagengine/segments/SegmentEvaluator.java index cbfb741e..d6722059 100644 --- a/src/main/java/com/flagsmith/flagengine/segments/SegmentEvaluator.java +++ b/src/main/java/com/flagsmith/flagengine/segments/SegmentEvaluator.java @@ -16,6 +16,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.function.Predicate; @@ -31,6 +32,9 @@ public class SegmentEvaluator { .setOptions(Option.SUPPRESS_EXCEPTIONS); private static TypeReference> stringListTypeRef = new TypeReference>() { }; + private static TypeReference> contextMapTypeRef = + new TypeReference>() { + }; /** * Check if context is in segment. @@ -40,15 +44,28 @@ public class SegmentEvaluator { * @return true if context is in segment. */ public static Boolean isContextInSegment(EvaluationContext context, SegmentContext segment) { + return isContextInSegment(context, segment, Map.of()); + } + + /** + * Check if context is in segment, with conditions reading `$.flags` from the given mapping. + * + * @param context Evaluation context. + * @param segment Segment context. + * @param flags Flag results by feature name, as serialised maps. + * @return true if context is in segment. + */ + public static Boolean isContextInSegment(EvaluationContext context, SegmentContext segment, + Map flags) { List rules = segment.getRules(); return !rules.isEmpty() && rules.stream() - .allMatch((rule) -> contextMatchesRule(context, rule, segment.getKey())); + .allMatch((rule) -> contextMatchesRule(context, rule, segment.getKey(), flags)); } private static Boolean contextMatchesRule(EvaluationContext context, SegmentRule rule, - String segmentKey) { + String segmentKey, Map flags) { Predicate conditionPredicate = (condition) -> contextMatchesCondition( - context, condition, segmentKey); + context, condition, segmentKey, flags); Boolean isMatch; List conditions = rule.getConditions(); @@ -81,7 +98,7 @@ private static Boolean contextMatchesRule(EvaluationContext context, SegmentRule } Predicate subRulePredicate = (subRule) -> contextMatchesRule( - context, subRule, segmentKey); + context, subRule, segmentKey, flags); switch (rule.getType()) { case ALL: return subRules.stream().allMatch(subRulePredicate); @@ -97,7 +114,8 @@ private static Boolean contextMatchesRule(EvaluationContext context, SegmentRule private static Boolean contextMatchesCondition( EvaluationContext context, SegmentCondition condition, - String segmentKey) { + String segmentKey, + Map flags) { Object contextValue = null; Object conditionValue = condition.getValue(); String conditionProperty = condition.getProperty(); @@ -114,7 +132,7 @@ private static Boolean contextMatchesCondition( contextValue = identity.getKey(); } } else { - contextValue = getContextValue(context, conditionProperty); + contextValue = getContextValue(context, conditionProperty, flags); } switch (operator) { @@ -216,9 +234,11 @@ private static Boolean contextMatchesCondition( * * @param context Evaluation context. * @param property Property name. + * @param flags Flag results by feature name, as serialised maps. * @return Property value. */ - private static Object getContextValue(EvaluationContext context, String property) { + private static Object getContextValue(EvaluationContext context, String property, + Map flags) { Object result; if (context.getIdentity() != null && context.getIdentity().getTraits() != null) { result = context.getIdentity().getTraits().getAdditionalProperties().get(property); @@ -227,9 +247,19 @@ private static Object getContextValue(EvaluationContext context, String property } } if (property.startsWith("$.")) { + Map contextMap; + if (property.startsWith("$.flags.") || property.startsWith("$.flags[")) { + contextMap = Map.of("flags", flags); // Spares serialising a context the path can't reach. + } else if (property.startsWith("$.environment.") || property.startsWith("$.environment[")) { + contextMap = Collections.singletonMap("environment", + mapper.convertValue(context.getEnvironment(), contextMapTypeRef)); + } else { + contextMap = mapper.convertValue(context, contextMapTypeRef); + contextMap.put("flags", flags); // Replaces any client-supplied flags. + } result = JsonPath .using(jsonPathConfiguration) - .parse(mapper.convertValue(context, Map.class)) + .parse(contextMap) .read(property); if (result instanceof List || result instanceof Map) { return null; diff --git a/src/main/java/com/flagsmith/flagengine/utils/types/TypeCasting.java b/src/main/java/com/flagsmith/flagengine/utils/types/TypeCasting.java index 9b05b2f3..04f093b4 100644 --- a/src/main/java/com/flagsmith/flagengine/utils/types/TypeCasting.java +++ b/src/main/java/com/flagsmith/flagengine/utils/types/TypeCasting.java @@ -62,15 +62,50 @@ public static Boolean compare(SegmentConditions condition, Comparable value1, Co return value1.compareTo(value2) == 0; } + /* + * Whether a string may parse as a decimal, sparing a failed parse's exception when it can't. + * Accepts a superset of Float.parseFloat and Double.parseDouble inputs. + */ + private static boolean mayBeDecimal(String value) { + int index = 0; + while (index < value.length() && value.charAt(index) <= ' ') { + index++; // Skips the whitespace the parsers trim. + } + if (index == value.length()) { + return false; + } + char first = value.charAt(index); + return Character.isDigit(first) + || first == '+' || first == '-' || first == '.' || first == 'N' || first == 'I'; + } + + /* + * Whether a string may parse as an integer, sparing a failed parse's exception when it can't. + * Accepts a superset of Integer.valueOf inputs. + */ + private static boolean mayBeInteger(String value) { + if (value.isEmpty()) { + return false; + } + char first = value.charAt(0); + return Character.isDigit(first) || first == '+' || first == '-'; + } + /** * Convert the object to Double. * * @param number Object to convert to Double. */ public static Double toDouble(Object number) { + if (number instanceof Double) { + return (Double) number; + } + String asString = String.valueOf(number); + if (!mayBeDecimal(asString)) { + return null; + } try { - String asString = String.valueOf(number); - return number instanceof Double ? ((Double) number) : Double.parseDouble(asString); + return Double.parseDouble(asString); } catch (Exception nfe) { return null; } @@ -91,8 +126,15 @@ public static Boolean isDouble(Object number) { * @param number Object to convert to Float. */ public static Float toFloat(Object number) { + if (number instanceof Float) { + return (Float) number; + } + String asString = String.valueOf(number); + if (!mayBeDecimal(asString)) { + return null; + } try { - return number instanceof Float ? ((Float) number) : Float.parseFloat(String.valueOf(number)); + return Float.parseFloat(asString); } catch (Exception nfe) { return null; } @@ -113,9 +155,15 @@ public static Boolean isFloat(Object number) { * @param number Object to convert to Integer. */ public static Integer toInteger(Object number) { + if (number instanceof Integer) { + return (Integer) number; + } + String asString = String.valueOf(number); + if (!mayBeInteger(asString)) { + return null; + } try { - String asString = String.valueOf(number); - return number instanceof Integer ? ((Integer) number) : Integer.valueOf(asString); + return Integer.valueOf(asString); } catch (Exception nfe) { return null; } diff --git a/src/test/java/com/flagsmith/FlagsmithClientTest.java b/src/test/java/com/flagsmith/FlagsmithClientTest.java index 4e2df3ab..e4888b50 100644 --- a/src/test/java/com/flagsmith/FlagsmithClientTest.java +++ b/src/test/java/com/flagsmith/FlagsmithClientTest.java @@ -22,7 +22,6 @@ import com.flagsmith.exceptions.FlagsmithClientError; import com.flagsmith.exceptions.FlagsmithRuntimeError; import com.flagsmith.flagengine.EvaluationContext; -import com.flagsmith.flagengine.EvaluationResult; import com.flagsmith.interfaces.FlagsmithCache; import com.flagsmith.models.BaseFlag; import com.flagsmith.models.DefaultFlag; @@ -55,13 +54,11 @@ import okhttp3.ResponseBody; import okhttp3.mock.MockInterceptor; import okio.Buffer; -import com.flagsmith.flagengine.Engine; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.mockito.ArgumentCaptor; -import org.mockito.MockedStatic; import org.mockito.Mockito; import org.mockito.invocation.Invocation; import org.slf4j.Logger; @@ -786,49 +783,6 @@ public void testLocalEvaluation_ReturnsIdentityOverrides() throws FlagsmithClien assertEquals(flagsWithOverride.getFeatureValue("some_feature"), "overridden-value"); } - @Test - public void testLocalEvaluation_getEnvironmentFlags_NoTargeting() throws FlagsmithClientError { - // Given - EvaluationContext evaluationContext = FlagsmithTestHelper.evaluationContext(); - EvaluationResult evaluationResult = Engine.getEvaluationResult( - new EvaluationContext(evaluationContext) - .withSegments(null) - ); - - FlagsmithConfig config = FlagsmithConfig.newBuilder().withLocalEvaluation(true).build(); - - FlagsmithApiWrapper mockedApiWrapper = mock(FlagsmithApiWrapper.class); - when(mockedApiWrapper.getEvaluationContext()) - .thenReturn(evaluationContext); - when(mockedApiWrapper.getConfig()).thenReturn(config); - - FlagsmithClient client = FlagsmithClient.newBuilder() - .withFlagsmithApiWrapper(mockedApiWrapper) - .withConfiguration(config) - .setApiKey("ser.dummy-key") - .build(); - - // When - try (MockedStatic mockedEngine = mockStatic(Engine.class)) { - mockedEngine.when( - () -> Engine.getEvaluationResult( - new EvaluationContext(evaluationContext) - .withSegments(null) - ) - ).thenReturn(evaluationResult); - - client.getEnvironmentFlags(); - - // Then - mockedEngine.verify( - () -> Engine.getEvaluationResult( - new EvaluationContext(evaluationContext) - .withSegments(null) - ) - ); - } - } - @Test public void testGetEnvironmentFlags_UsesDefaultFlags_IfLocalEvaluationEnvironmentNull() throws FlagsmithClientError { diff --git a/src/test/java/com/flagsmith/flagengine/enginetestdata b/src/test/java/com/flagsmith/flagengine/enginetestdata index 4b29dc77..28363b38 160000 --- a/src/test/java/com/flagsmith/flagengine/enginetestdata +++ b/src/test/java/com/flagsmith/flagengine/enginetestdata @@ -1 +1 @@ -Subproject commit 4b29dc772a764364af2dd504ecefbdf74cf5473f +Subproject commit 28363b388de2850c7c3f1b6e7a8bdd41d8da433c diff --git a/src/test/java/com/flagsmith/flagengine/unit/segments/SegmentEvaluatorTest.java b/src/test/java/com/flagsmith/flagengine/unit/segments/SegmentEvaluatorTest.java index a11007ec..63cc4ee1 100644 --- a/src/test/java/com/flagsmith/flagengine/unit/segments/SegmentEvaluatorTest.java +++ b/src/test/java/com/flagsmith/flagengine/unit/segments/SegmentEvaluatorTest.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Map; import java.util.stream.Stream; public class SegmentEvaluatorTest { @@ -62,6 +63,46 @@ public void testContextInSegment(SegmentContext segment, List identi Assertions.assertEquals(expectedResponse, actualResult); } + private static Stream numericTraitComparisons() { + return Stream.of( + Arguments.of(7, SegmentConditions.EQUAL, "7", true), + Arguments.of("7", SegmentConditions.EQUAL, "7", true), + Arguments.of("+7", SegmentConditions.EQUAL, "7", true), + Arguments.of("007", SegmentConditions.EQUAL, "7", true), + Arguments.of("-7", SegmentConditions.LESS_THAN, "0", true), + Arguments.of("7.5", SegmentConditions.GREATER_THAN, "7", true), + Arguments.of(".5", SegmentConditions.LESS_THAN, "1", true), + Arguments.of(" 7 ", SegmentConditions.EQUAL, "7.0", true), + Arguments.of("7e2", SegmentConditions.EQUAL, "700", true), + Arguments.of("NaN", SegmentConditions.EQUAL, "NaN", true), + Arguments.of("-Infinity", SegmentConditions.LESS_THAN, "0", true), + Arguments.of("plan-7", SegmentConditions.EQUAL, "7", false), + Arguments.of("", SegmentConditions.EQUAL, "0", false), + Arguments.of(" ", SegmentConditions.EQUAL, "0", false) + ); + } + + @ParameterizedTest + @MethodSource("numericTraitComparisons") + public void testTraitValuesThatReadAsNumbersAreComparedAsNumbers(Object traitValue, + SegmentConditions conditionOperator, String conditionValue, Boolean expectedResult) { + // Given + final EvaluationContext context = EngineMappers.mapContextAndIdentityDataToContext( + FlagsmithTestHelper.evaluationContext(), "foo", Map.of("score", traitValue)); + SegmentContext segment = new SegmentContext().withName("testSegment").withRules( + Arrays.asList(new SegmentRule().withType(SegmentRule.Type.ALL).withConditions( + Arrays.asList(new SegmentCondition() + .withOperator(conditionOperator) + .withProperty("score") + .withValue(conditionValue))))); + + // When + Boolean inSegment = SegmentEvaluator.isContextInSegment(context, segment); + + // Then + Assertions.assertEquals(expectedResult, inSegment); + } + private static Stream traitExistenceChecks() { return Stream.of( Arguments.of(SegmentConditions.IS_SET, "foo", new ArrayList<>(), false),