From 64d27e4723082ab8e2d7c61a6b61abd3081ea79a Mon Sep 17 00:00:00 2001 From: Håkon Hallingstad Date: Wed, 23 Oct 2019 11:13:01 +0200 Subject: Revert "Revert "Support flag conditions based on Vespa release "" --- .../configserver/flags/db/FlagsDbImplTest.java | 6 +- flags/pom.xml | 6 ++ .../java/com/yahoo/vespa/flags/FetchVector.java | 41 +++++++----- .../src/main/java/com/yahoo/vespa/flags/Flags.java | 11 +++- .../yahoo/vespa/flags/json/BlacklistCondition.java | 10 +++ .../java/com/yahoo/vespa/flags/json/Condition.java | 72 ++++++++++++---------- .../yahoo/vespa/flags/json/DimensionHelper.java | 1 + .../com/yahoo/vespa/flags/json/ListCondition.java | 43 +++++++++++++ .../vespa/flags/json/RelationalCondition.java | 64 +++++++++++++++++++ .../yahoo/vespa/flags/json/RelationalOperator.java | 39 ++++++++++++ .../vespa/flags/json/RelationalPredicate.java | 43 +++++++++++++ .../yahoo/vespa/flags/json/WhitelistCondition.java | 10 +++ .../yahoo/vespa/flags/json/wire/WireCondition.java | 1 + .../com/yahoo/vespa/flags/json/ConditionTest.java | 40 ++++++++++-- .../yahoo/vespa/flags/json/SerializationTest.java | 12 +++- 15 files changed, 344 insertions(+), 55 deletions(-) create mode 100644 flags/src/main/java/com/yahoo/vespa/flags/json/BlacklistCondition.java create mode 100644 flags/src/main/java/com/yahoo/vespa/flags/json/ListCondition.java create mode 100644 flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java create mode 100644 flags/src/main/java/com/yahoo/vespa/flags/json/RelationalOperator.java create mode 100644 flags/src/main/java/com/yahoo/vespa/flags/json/RelationalPredicate.java create mode 100644 flags/src/main/java/com/yahoo/vespa/flags/json/WhitelistCondition.java diff --git a/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java b/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java index 7460e42c866..4f2d5ae13d4 100644 --- a/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java +++ b/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java @@ -8,12 +8,15 @@ import com.yahoo.vespa.flags.JsonNodeRawFlag; import com.yahoo.vespa.flags.json.Condition; import com.yahoo.vespa.flags.json.FlagData; import com.yahoo.vespa.flags.json.Rule; +import com.yahoo.vespa.flags.json.WhitelistCondition; import org.junit.Test; +import java.util.List; import java.util.Map; import java.util.Optional; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.notNullValue; import static org.junit.Assert.assertEquals; @@ -29,7 +32,8 @@ public class FlagsDbImplTest { MockCurator curator = new MockCurator(); FlagsDbImpl db = new FlagsDbImpl(curator); - Condition condition1 = new Condition(Condition.Type.WHITELIST, FetchVector.Dimension.HOSTNAME, "host1"); + var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME, List.of("host1"), Optional.empty()); + Condition condition1 = WhitelistCondition.create(params); Rule rule1 = new Rule(Optional.of(JsonNodeRawFlag.fromJson("13")), condition1); FlagId flagId = new FlagId("id"); FlagData data = new FlagData(flagId, new FetchVector().with(FetchVector.Dimension.ZONE_ID, "zone-a"), rule1); diff --git a/flags/pom.xml b/flags/pom.xml index c1e9eca20ab..6afa920e261 100644 --- a/flags/pom.xml +++ b/flags/pom.xml @@ -25,6 +25,12 @@ ${project.version} provided + + com.yahoo.vespa + component + ${project.version} + provided + com.yahoo.vespa defaults diff --git a/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java b/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java index d2228c58c51..4cce7a9bc2a 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java @@ -24,27 +24,40 @@ public class FetchVector { * Note: If this enum is changed, you must also change {@link DimensionHelper}. */ public enum Dimension { - /** - * WARNING: DO NOT USE - * - *

ALL flags can be set differently in different zones: This dimension is ONLY useful for the controller - * that needs to handle multiple zones. - * - *

Value from ZoneId::value is of the form environment.region. - */ - ZONE_ID, - /** Value from ApplicationId::serializedForm of the form tenant:applicationName:instance. */ APPLICATION_ID, - /** Fully qualified hostname */ - HOSTNAME, - /** Node type from com.yahoo.config.provision.NodeType::name, e.g. tenant, host, confighost, controller, etc. */ NODE_TYPE, /** Cluster type from com.yahoo.config.provision.ClusterSpec.Type::name, e.g. content, container, admin */ - CLUSTER_TYPE + CLUSTER_TYPE, + + /** + * Fully qualified hostname. + * + *

NOTE: There is seldom any need to set HOSTNAME, as it is always set implicitly (in {@link Flags}) + * from {@code Defaults.getDefaults().vespaHostname()}. The hostname may e.g. be overridden when + * fetching flag value for a Docker container node. + */ + HOSTNAME, + + /** + * Vespa version from Version::toFullString of the form Major.Minor.Micro. + * + *

NOTE: There is seldom any need to set VESPA_VERSION, as it is always set implicitly + * (in {@link Flags}) from {@link com.yahoo.component.Vtag#currentVersion}. The version COULD e.g. + * be overridden when fetching flag value for a Docker container node. + */ + VESPA_VERSION, + + /** + * Zone from ZoneId::value of the form environment.region. + * + *

NOTE: There is seldom any need to set ZONE_ID, as all flags are set per zone anyways. The controller + * could PERHAPS use this where it handles multiple zones. + */ + ZONE_ID } private final Map map; diff --git a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java index a27171f29a2..f4b223ead82 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -1,6 +1,7 @@ // Copyright 2019 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.flags; +import com.yahoo.component.Vtag; import com.yahoo.vespa.defaults.Defaults; import com.yahoo.vespa.flags.custom.PreprovisionCapacity; @@ -11,6 +12,7 @@ import java.util.TreeMap; import static com.yahoo.vespa.flags.FetchVector.Dimension.APPLICATION_ID; import static com.yahoo.vespa.flags.FetchVector.Dimension.HOSTNAME; import static com.yahoo.vespa.flags.FetchVector.Dimension.NODE_TYPE; +import static com.yahoo.vespa.flags.FetchVector.Dimension.VESPA_VERSION; /** * Definitions of feature flags. @@ -235,7 +237,8 @@ public class Flags { * from the FlagSource. * @param The boxed type of the flag value, e.g. Boolean for flags guarding features. * @param The type of the unbound flag, e.g. UnboundBooleanFlag. - * @return An unbound flag with {@link FetchVector.Dimension#HOSTNAME HOSTNAME} environment. The ZONE environment + * @return An unbound flag with {@link FetchVector.Dimension#HOSTNAME HOSTNAME} and + * {@link FetchVector.Dimension#VESPA_VERSION VESPA_VERSION} already set. The ZONE environment * is typically implicit. */ private static > U define(TypedUnboundFlagFactory factory, @@ -245,7 +248,11 @@ public class Flags { String modificationEffect, FetchVector.Dimension[] dimensions) { FlagId id = new FlagId(flagId); - FetchVector vector = new FetchVector().with(HOSTNAME, Defaults.getDefaults().vespaHostname()); + FetchVector vector = new FetchVector() + .with(HOSTNAME, Defaults.getDefaults().vespaHostname()) + // Warning: In unit tests and outside official Vespa releases, the currentVersion is e.g. 7.0.0 + // (determined by the current major version). Consider not setting VESPA_VERSION if minor = micro = 0. + .with(VESPA_VERSION, Vtag.currentVersion.toFullString()); U unboundFlag = factory.create(id, defaultValue, vector); FlagDefinition definition = new FlagDefinition(unboundFlag, description, modificationEffect, dimensions); flags.put(id, definition); diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/BlacklistCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/BlacklistCondition.java new file mode 100644 index 00000000000..435ddbb3e19 --- /dev/null +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/BlacklistCondition.java @@ -0,0 +1,10 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.flags.json; + +/** + * @author hakonhall + */ +public class BlacklistCondition extends ListCondition { + public static BlacklistCondition create(CreateParams params) { return new BlacklistCondition(params); } + private BlacklistCondition(CreateParams params) { super(Type.BLACKLIST, params); } +} diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java index a0ad08fb0b3..96dbc8197c1 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java @@ -1,62 +1,72 @@ -// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.flags.json; import com.yahoo.vespa.flags.FetchVector; import com.yahoo.vespa.flags.json.wire.WireCondition; -import java.util.Arrays; import java.util.List; import java.util.Objects; +import java.util.Optional; import java.util.function.Predicate; /** * @author hakonhall */ -public class Condition implements Predicate { - public enum Type { WHITELIST, BLACKLIST } +public interface Condition extends Predicate { + enum Type { + WHITELIST, + BLACKLIST, + RELATIONAL; - private final Type type; - private final FetchVector.Dimension dimension; - private final List values; + public String toWire() { return name().toLowerCase(); } - public Condition(Type type, FetchVector.Dimension dimension, String... values) { - this(type, dimension, Arrays.asList(values)); - } + public static Type fromWire(String typeString) { + for (Type type : values()) { + if (type.name().equalsIgnoreCase(typeString)) { + return type; + } + } - public Condition(Type type, FetchVector.Dimension dimension, List values) { - this.type = type; - this.dimension = dimension; - this.values = values; + throw new IllegalArgumentException("Unknown type: '" + typeString + "'"); + } } - @Override - public boolean test(FetchVector vector) { - boolean isMember = vector.getValue(dimension).filter(values::contains).isPresent(); + class CreateParams { + private final FetchVector.Dimension dimension; + private final List values; + private final Optional predicate; - switch (type) { - case WHITELIST: return isMember; - case BLACKLIST: return !isMember; - default: throw new IllegalArgumentException("Unknown type " + type); + public CreateParams(FetchVector.Dimension dimension, List values, Optional predicate) { + this.dimension = Objects.requireNonNull(dimension); + this.values = Objects.requireNonNull(values); + this.predicate = Objects.requireNonNull(predicate); } + + public FetchVector.Dimension dimension() { return dimension; } + public List values() { return values; } + public Optional predicate() { return predicate; } } - public static Condition fromWire(WireCondition wireCondition) { + static Condition fromWire(WireCondition wireCondition) { Objects.requireNonNull(wireCondition.type); - Type type = Type.valueOf(wireCondition.type.toUpperCase()); + Condition.Type type = Condition.Type.fromWire(wireCondition.type); Objects.requireNonNull(wireCondition.dimension); FetchVector.Dimension dimension = DimensionHelper.fromWire(wireCondition.dimension); List values = wireCondition.values == null ? List.of() : wireCondition.values; + Optional predicate = Optional.ofNullable(wireCondition.predicate); - return new Condition(type, dimension, values); - } + var params = new CreateParams(dimension, values, predicate); - public WireCondition toWire() { - WireCondition wire = new WireCondition(); - wire.type = type.name().toLowerCase(); - wire.dimension = DimensionHelper.toWire(dimension); - wire.values = values.isEmpty() ? null : values; - return wire; + switch (type) { + case WHITELIST: return WhitelistCondition.create(params); + case BLACKLIST: return BlacklistCondition.create(params); + case RELATIONAL: return RelationalCondition.create(params); + } + + throw new IllegalArgumentException("Unknown type '" + type + "'"); } + + WireCondition toWire(); } diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/DimensionHelper.java b/flags/src/main/java/com/yahoo/vespa/flags/json/DimensionHelper.java index 4fe27e81f2b..c7081ca72ab 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/DimensionHelper.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/DimensionHelper.java @@ -18,6 +18,7 @@ public class DimensionHelper { serializedDimensions.put(FetchVector.Dimension.APPLICATION_ID, "application"); serializedDimensions.put(FetchVector.Dimension.NODE_TYPE, "node-type"); serializedDimensions.put(FetchVector.Dimension.CLUSTER_TYPE, "cluster-type"); + serializedDimensions.put(FetchVector.Dimension.VESPA_VERSION, "vespa-version"); if (serializedDimensions.size() != FetchVector.Dimension.values().length) { throw new IllegalStateException(FetchVectorHelper.class.getName() + " is not in sync with " + diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/ListCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/ListCondition.java new file mode 100644 index 00000000000..c2c76529833 --- /dev/null +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/ListCondition.java @@ -0,0 +1,43 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.flags.json; + +import com.yahoo.vespa.flags.FetchVector; +import com.yahoo.vespa.flags.json.wire.WireCondition; + +import java.util.List; + +/** + * @author hakonhall + */ +public abstract class ListCondition implements Condition { + private final Condition.Type type; + private final FetchVector.Dimension dimension; + private final List values; + private final boolean isWhitelist; + + protected ListCondition(Type type, CreateParams params) { + this.type = type; + this.dimension = params.dimension(); + this.values = List.copyOf(params.values()); + this.isWhitelist = type == Type.WHITELIST; + + if (params.predicate().isPresent()) { + throw new IllegalArgumentException(getClass().getSimpleName() + " does not support the 'predicate' field"); + } + } + + @Override + public boolean test(FetchVector fetchVector) { + boolean listContainsValue = fetchVector.getValue(dimension).map(values::contains).orElse(false); + return isWhitelist == listContainsValue; + } + + @Override + public WireCondition toWire() { + var condition = new WireCondition(); + condition.type = type.toWire(); + condition.dimension = DimensionHelper.toWire(dimension); + condition.values = values.isEmpty() ? null : values; + return condition; + } +} diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java new file mode 100644 index 00000000000..afaf94b26b6 --- /dev/null +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java @@ -0,0 +1,64 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.flags.json; + +import com.yahoo.component.Version; +import com.yahoo.vespa.flags.FetchVector; +import com.yahoo.vespa.flags.json.wire.WireCondition; + +import java.util.List; +import java.util.function.Predicate; + +/** + * @author hakonhall + */ +public class RelationalCondition implements Condition { + private final RelationalPredicate relationalPredicate; + private final Predicate predicate; + private final FetchVector.Dimension dimension; + + public static RelationalCondition create(CreateParams params) { + if (!params.values().isEmpty()) { + throw new IllegalArgumentException(RelationalCondition.class.getSimpleName() + + " does not support the 'values' field"); + } + + String predicate = params.predicate().orElseThrow(() -> + new IllegalArgumentException(RelationalCondition.class.getSimpleName() + + " requires the predicate field in the condition")); + RelationalPredicate relationalPredicate = RelationalPredicate.fromWire(predicate); + + switch (params.dimension()) { + case VESPA_VERSION: + final Version rightVersion = Version.fromString(relationalPredicate.rightOperand()); + Predicate p = (String leftString) -> { + Version leftVersion = Version.fromString(leftString); + return relationalPredicate.operator().evaluate(leftVersion, rightVersion); + }; + return new RelationalCondition(relationalPredicate, p, params.dimension()); + default: + throw new IllegalArgumentException(RelationalCondition.class.getSimpleName() + + " not supported for dimension " + FetchVector.Dimension.VESPA_VERSION.name()); + } + } + + private RelationalCondition(RelationalPredicate relationalPredicate, Predicate predicate, + FetchVector.Dimension dimension) { + this.relationalPredicate = relationalPredicate; + this.predicate = predicate; + this.dimension = dimension; + } + + @Override + public boolean test(FetchVector fetchVector) { + return fetchVector.getValue(dimension).map(predicate::test).orElse(false); + } + + @Override + public WireCondition toWire() { + var condition = new WireCondition(); + condition.type = Condition.Type.RELATIONAL.toWire(); + condition.dimension = DimensionHelper.toWire(dimension); + condition.predicate = relationalPredicate.toWire(); + return condition; + } +} diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalOperator.java b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalOperator.java new file mode 100644 index 00000000000..ca7a997f447 --- /dev/null +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalOperator.java @@ -0,0 +1,39 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.flags.json; + +import java.util.Objects; +import java.util.function.Function; + +/** + * @author hakonhall + */ +public enum RelationalOperator { + EQUAL ("==", compareToValue -> compareToValue == 0), + NOT_EQUAL ("!=", compareToValue -> compareToValue != 0), + LESS_EQUAL ("<=", compareToValue -> compareToValue <= 0), + LESS ("<" , compareToValue -> compareToValue < 0), + GREATER_EQUAL(">=", compareToValue -> compareToValue >= 0), + GREATER (">" , compareToValue -> compareToValue > 0); + + private String text; + private final Function compareToValuePredicate; + + RelationalOperator(String text, Function compareToValuePredicate) { + this.text = text; + this.compareToValuePredicate = compareToValuePredicate; + } + + public String toText() { return text; } + + /** Returns true if 'left op right' is true, with 'op' being the operator represented by this. */ + public > boolean evaluate(T left, T right) { + Objects.requireNonNull(left); + Objects.requireNonNull(right); + return evaluate(left.compareTo(right)); + } + + public boolean evaluate(int compareToValue) { + return compareToValuePredicate.apply(compareToValue); + } +} + diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalPredicate.java b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalPredicate.java new file mode 100644 index 00000000000..c5ad195e0d2 --- /dev/null +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalPredicate.java @@ -0,0 +1,43 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.flags.json; + +import java.util.Comparator; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +/** + * @author hakonhall + */ +public class RelationalPredicate { + private final String originalPredicateString; + private final RelationalOperator operator; + private final String rightOperand; + + /** @param predicateString is e.g. "> SUFFIX" or "<=SUFFIX". The first part is {@link RelationalOperator}. */ + public static RelationalPredicate fromWire(String predicateString) { + // Make sure we try to match e.g. "<=" before "<" as the prefix of predicateString. + List operatorsByDecendingLength = Stream.of(RelationalOperator.values()) + .sorted(Comparator.comparing(operator -> - operator.toText().length())) + .collect(Collectors.toList()); + + for (var operator : operatorsByDecendingLength) { + if (predicateString.startsWith(operator.toText())) { + String suffix = predicateString.substring(operator.toText().length()); + return new RelationalPredicate(predicateString, operator, suffix); + } + } + + throw new IllegalArgumentException("Predicate string '" + predicateString + "' does not start with a relation operator"); + } + + private RelationalPredicate(String originalPredicateString, RelationalOperator operator, String rightOperand) { + this.originalPredicateString = originalPredicateString; + this.operator = operator; + this.rightOperand = rightOperand; + } + + public RelationalOperator operator() { return operator; } + public String rightOperand() { return rightOperand; } + public String toWire() { return originalPredicateString; } +} diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/WhitelistCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/WhitelistCondition.java new file mode 100644 index 00000000000..ab266fdaf1d --- /dev/null +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/WhitelistCondition.java @@ -0,0 +1,10 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.flags.json; + +/** + * @author hakonhall + */ +public class WhitelistCondition extends ListCondition { + public static WhitelistCondition create(CreateParams params) { return new WhitelistCondition(params); } + private WhitelistCondition(CreateParams params) { super(Type.WHITELIST, params); } +} diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/wire/WireCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/wire/WireCondition.java index 2020ce1e49f..1729444fcf2 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/wire/WireCondition.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/wire/WireCondition.java @@ -16,4 +16,5 @@ public class WireCondition { @JsonProperty("type") public String type; @JsonProperty("dimension") public String dimension; @JsonProperty("values") public List values; + @JsonProperty("predicate") public String predicate; } diff --git a/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java b/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java index d19442ae0f0..618bc86baaf 100644 --- a/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java +++ b/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java @@ -4,9 +4,10 @@ package com.yahoo.vespa.flags.json; import com.yahoo.vespa.flags.FetchVector; import org.junit.Test; -import java.util.stream.Collectors; -import java.util.stream.Stream; +import java.util.List; +import java.util.Optional; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -17,8 +18,8 @@ public class ConditionTest { @Test public void testWhitelist() { String hostname1 = "host1"; - Condition condition = new Condition(Condition.Type.WHITELIST, FetchVector.Dimension.HOSTNAME, - Stream.of(hostname1).collect(Collectors.toList())); + var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME, List.of(hostname1), Optional.empty()); + Condition condition = WhitelistCondition.create(params); assertFalse(condition.test(new FetchVector())); assertFalse(condition.test(new FetchVector().with(FetchVector.Dimension.APPLICATION_ID, "foo"))); assertFalse(condition.test(new FetchVector().with(FetchVector.Dimension.HOSTNAME, "bar"))); @@ -28,11 +29,38 @@ public class ConditionTest { @Test public void testBlacklist() { String hostname1 = "host1"; - Condition condition = new Condition(Condition.Type.BLACKLIST, FetchVector.Dimension.HOSTNAME, - Stream.of(hostname1).collect(Collectors.toList())); + var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME, List.of(hostname1), Optional.empty()); + Condition condition = BlacklistCondition.create(params); assertTrue(condition.test(new FetchVector())); assertTrue(condition.test(new FetchVector().with(FetchVector.Dimension.APPLICATION_ID, "foo"))); assertTrue(condition.test(new FetchVector().with(FetchVector.Dimension.HOSTNAME, "bar"))); assertFalse(condition.test(new FetchVector().with(FetchVector.Dimension.HOSTNAME, hostname1))); } + + @Test + public void testRelational() { + verifyVespaVersionFor("<", true, false, false); + verifyVespaVersionFor("<=", true, true, false); + verifyVespaVersionFor(">", false, false, true); + verifyVespaVersionFor(">=", false, true, true); + + // Test with empty fetch vector along vespa version dimension (this should never happen as the + // version is always available through Vtag, although Vtag has a dummy version number for e.g. + // locally run unit tests that hasn't set the release Vespa version). + var params = new Condition.CreateParams(FetchVector.Dimension.VESPA_VERSION, List.of(), Optional.of(">=7.1.2")); + Condition condition = RelationalCondition.create(params); + assertFalse(condition.test(new FetchVector())); + } + + private void verifyVespaVersionFor(String operator, boolean whenLess, boolean whenEqual, boolean whenGreater) { + assertEquals(whenLess, vespaVersionCondition("7.2.4", operator + "7.3.4")); + assertEquals(whenEqual, vespaVersionCondition("7.3.4", operator + "7.3.4")); + assertEquals(whenGreater, vespaVersionCondition("7.4.4", operator + "7.3.4")); + } + + private boolean vespaVersionCondition(String vespaVersion, String predicate) { + var params = new Condition.CreateParams(FetchVector.Dimension.VESPA_VERSION, List.of(), Optional.of(predicate)); + Condition condition = RelationalCondition.create(params); + return condition.test(new FetchVector().with(FetchVector.Dimension.VESPA_VERSION, vespaVersion)); + } } diff --git a/flags/src/test/java/com/yahoo/vespa/flags/json/SerializationTest.java b/flags/src/test/java/com/yahoo/vespa/flags/json/SerializationTest.java index b0e4cd0f682..8326b14fcbf 100644 --- a/flags/src/test/java/com/yahoo/vespa/flags/json/SerializationTest.java +++ b/flags/src/test/java/com/yahoo/vespa/flags/json/SerializationTest.java @@ -48,6 +48,11 @@ public class SerializationTest { " \"type\": \"blacklist\",\n" + " \"dimension\": \"hostname\",\n" + " \"values\": [ \"h1\" ]\n" + + " },\n" + + " {\n" + + " \"type\": \"relational\",\n" + + " \"dimension\": \"vespa-version\",\n" + + " \"predicate\": \">=7.3.4\"\n" + " }\n" + " ],\n" + " \"value\": true\n" + @@ -66,7 +71,7 @@ public class SerializationTest { assertThat(wireData.id, equalTo("id2")); // rule assertThat(wireData.rules.size(), equalTo(1)); - assertThat(wireData.rules.get(0).andConditions.size(), equalTo(2)); + assertThat(wireData.rules.get(0).andConditions.size(), equalTo(3)); assertThat(wireData.rules.get(0).value.getNodeType(), equalTo(JsonNodeType.BOOLEAN)); assertThat(wireData.rules.get(0).value.asBoolean(), equalTo(true)); // first condition @@ -79,6 +84,11 @@ public class SerializationTest { assertThat(blacklistCondition.type, equalTo("blacklist")); assertThat(blacklistCondition.dimension, equalTo("hostname")); assertThat(blacklistCondition.values, equalTo(List.of("h1"))); + // third condition + WireCondition relationalCondition = wireData.rules.get(0).andConditions.get(2); + assertThat(relationalCondition.type, equalTo("relational")); + assertThat(relationalCondition.dimension, equalTo("vespa-version")); + assertThat(relationalCondition.predicate, equalTo(">=7.3.4")); // attributes assertThat(wireData.defaultFetchVector, notNullValue()); -- cgit v1.2.3 From ff250e41e38e7bb8ac91b3bcc72d605fc2ec483c Mon Sep 17 00:00:00 2001 From: Håkon Hallingstad Date: Wed, 23 Oct 2019 11:44:13 +0200 Subject: Make fluent-style params --- .../configserver/flags/db/FlagsDbImplTest.java | 4 +-- .../java/com/yahoo/vespa/flags/json/Condition.java | 29 +++++++++++++++------- .../com/yahoo/vespa/flags/json/ConditionTest.java | 11 +++----- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java b/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java index 4f2d5ae13d4..75a66d581a0 100644 --- a/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java +++ b/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java @@ -11,12 +11,10 @@ import com.yahoo.vespa.flags.json.Rule; import com.yahoo.vespa.flags.json.WhitelistCondition; import org.junit.Test; -import java.util.List; import java.util.Map; import java.util.Optional; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.notNullValue; import static org.junit.Assert.assertEquals; @@ -32,7 +30,7 @@ public class FlagsDbImplTest { MockCurator curator = new MockCurator(); FlagsDbImpl db = new FlagsDbImpl(curator); - var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME, List.of("host1"), Optional.empty()); + var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME).setValues("host1"); Condition condition1 = WhitelistCondition.create(params); Rule rule1 = new Rule(Optional.of(JsonNodeRawFlag.fromJson("13")), condition1); FlagId flagId = new FlagId("id"); diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java index 96dbc8197c1..93ccb773de6 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java @@ -33,13 +33,20 @@ public interface Condition extends Predicate { class CreateParams { private final FetchVector.Dimension dimension; - private final List values; - private final Optional predicate; + private List values = List.of(); + private Optional predicate = Optional.empty(); - public CreateParams(FetchVector.Dimension dimension, List values, Optional predicate) { - this.dimension = Objects.requireNonNull(dimension); - this.values = Objects.requireNonNull(values); - this.predicate = Objects.requireNonNull(predicate); + public CreateParams(FetchVector.Dimension dimension) { this.dimension = Objects.requireNonNull(dimension); } + + public CreateParams setValues(String... values) { return setValues(List.of(values)); } + public CreateParams setValues(List values) { + this.values = List.copyOf(values); + return this; + } + + public CreateParams setPredicate(String predicate) { + this.predicate = Optional.of(predicate); + return this; } public FetchVector.Dimension dimension() { return dimension; } @@ -53,11 +60,15 @@ public interface Condition extends Predicate { Objects.requireNonNull(wireCondition.dimension); FetchVector.Dimension dimension = DimensionHelper.fromWire(wireCondition.dimension); + var params = new CreateParams(dimension); - List values = wireCondition.values == null ? List.of() : wireCondition.values; - Optional predicate = Optional.ofNullable(wireCondition.predicate); + if (wireCondition.values != null) { + params.setValues(wireCondition.values); + } - var params = new CreateParams(dimension, values, predicate); + if (wireCondition.predicate != null) { + params.setPredicate(wireCondition.predicate); + } switch (type) { case WHITELIST: return WhitelistCondition.create(params); diff --git a/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java b/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java index 618bc86baaf..b46828b6691 100644 --- a/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java +++ b/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java @@ -4,9 +4,6 @@ package com.yahoo.vespa.flags.json; import com.yahoo.vespa.flags.FetchVector; import org.junit.Test; -import java.util.List; -import java.util.Optional; - import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -18,7 +15,7 @@ public class ConditionTest { @Test public void testWhitelist() { String hostname1 = "host1"; - var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME, List.of(hostname1), Optional.empty()); + var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME).setValues(hostname1); Condition condition = WhitelistCondition.create(params); assertFalse(condition.test(new FetchVector())); assertFalse(condition.test(new FetchVector().with(FetchVector.Dimension.APPLICATION_ID, "foo"))); @@ -29,7 +26,7 @@ public class ConditionTest { @Test public void testBlacklist() { String hostname1 = "host1"; - var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME, List.of(hostname1), Optional.empty()); + var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME).setValues(hostname1); Condition condition = BlacklistCondition.create(params); assertTrue(condition.test(new FetchVector())); assertTrue(condition.test(new FetchVector().with(FetchVector.Dimension.APPLICATION_ID, "foo"))); @@ -47,7 +44,7 @@ public class ConditionTest { // Test with empty fetch vector along vespa version dimension (this should never happen as the // version is always available through Vtag, although Vtag has a dummy version number for e.g. // locally run unit tests that hasn't set the release Vespa version). - var params = new Condition.CreateParams(FetchVector.Dimension.VESPA_VERSION, List.of(), Optional.of(">=7.1.2")); + var params = new Condition.CreateParams(FetchVector.Dimension.VESPA_VERSION).setPredicate(">=7.1.2"); Condition condition = RelationalCondition.create(params); assertFalse(condition.test(new FetchVector())); } @@ -59,7 +56,7 @@ public class ConditionTest { } private boolean vespaVersionCondition(String vespaVersion, String predicate) { - var params = new Condition.CreateParams(FetchVector.Dimension.VESPA_VERSION, List.of(), Optional.of(predicate)); + var params = new Condition.CreateParams(FetchVector.Dimension.VESPA_VERSION).setPredicate(predicate); Condition condition = RelationalCondition.create(params); return condition.test(new FetchVector().with(FetchVector.Dimension.VESPA_VERSION, vespaVersion)); } -- cgit v1.2.3 From ceca416d6c98edf98dd44e03eb151adb3a7221ad Mon Sep 17 00:00:00 2001 From: Håkon Hallingstad Date: Wed, 23 Oct 2019 12:34:25 +0200 Subject: Use withX instead of setX --- .../com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java | 2 +- flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java | 10 +++++----- .../test/java/com/yahoo/vespa/flags/json/ConditionTest.java | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java b/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java index 75a66d581a0..150fcc502d4 100644 --- a/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java +++ b/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java @@ -30,7 +30,7 @@ public class FlagsDbImplTest { MockCurator curator = new MockCurator(); FlagsDbImpl db = new FlagsDbImpl(curator); - var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME).setValues("host1"); + var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME).withValues("host1"); Condition condition1 = WhitelistCondition.create(params); Rule rule1 = new Rule(Optional.of(JsonNodeRawFlag.fromJson("13")), condition1); FlagId flagId = new FlagId("id"); diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java index 93ccb773de6..46961fbd8cc 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java @@ -38,13 +38,13 @@ public interface Condition extends Predicate { public CreateParams(FetchVector.Dimension dimension) { this.dimension = Objects.requireNonNull(dimension); } - public CreateParams setValues(String... values) { return setValues(List.of(values)); } - public CreateParams setValues(List values) { + public CreateParams withValues(String... values) { return withValues(List.of(values)); } + public CreateParams withValues(List values) { this.values = List.copyOf(values); return this; } - public CreateParams setPredicate(String predicate) { + public CreateParams withPredicate(String predicate) { this.predicate = Optional.of(predicate); return this; } @@ -63,11 +63,11 @@ public interface Condition extends Predicate { var params = new CreateParams(dimension); if (wireCondition.values != null) { - params.setValues(wireCondition.values); + params.withValues(wireCondition.values); } if (wireCondition.predicate != null) { - params.setPredicate(wireCondition.predicate); + params.withPredicate(wireCondition.predicate); } switch (type) { diff --git a/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java b/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java index b46828b6691..b41da9f567c 100644 --- a/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java +++ b/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java @@ -15,7 +15,7 @@ public class ConditionTest { @Test public void testWhitelist() { String hostname1 = "host1"; - var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME).setValues(hostname1); + var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME).withValues(hostname1); Condition condition = WhitelistCondition.create(params); assertFalse(condition.test(new FetchVector())); assertFalse(condition.test(new FetchVector().with(FetchVector.Dimension.APPLICATION_ID, "foo"))); @@ -26,7 +26,7 @@ public class ConditionTest { @Test public void testBlacklist() { String hostname1 = "host1"; - var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME).setValues(hostname1); + var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME).withValues(hostname1); Condition condition = BlacklistCondition.create(params); assertTrue(condition.test(new FetchVector())); assertTrue(condition.test(new FetchVector().with(FetchVector.Dimension.APPLICATION_ID, "foo"))); @@ -44,7 +44,7 @@ public class ConditionTest { // Test with empty fetch vector along vespa version dimension (this should never happen as the // version is always available through Vtag, although Vtag has a dummy version number for e.g. // locally run unit tests that hasn't set the release Vespa version). - var params = new Condition.CreateParams(FetchVector.Dimension.VESPA_VERSION).setPredicate(">=7.1.2"); + var params = new Condition.CreateParams(FetchVector.Dimension.VESPA_VERSION).withPredicate(">=7.1.2"); Condition condition = RelationalCondition.create(params); assertFalse(condition.test(new FetchVector())); } @@ -56,7 +56,7 @@ public class ConditionTest { } private boolean vespaVersionCondition(String vespaVersion, String predicate) { - var params = new Condition.CreateParams(FetchVector.Dimension.VESPA_VERSION).setPredicate(predicate); + var params = new Condition.CreateParams(FetchVector.Dimension.VESPA_VERSION).withPredicate(predicate); Condition condition = RelationalCondition.create(params); return condition.test(new FetchVector().with(FetchVector.Dimension.VESPA_VERSION, vespaVersion)); } -- cgit v1.2.3