From cc1ce7ec4a002508a857fb8baab6987f5c4d024d Mon Sep 17 00:00:00 2001 From: Avishai Weissberg Date: Sat, 15 May 2021 09:51:36 -0700 Subject: [PATCH 1/3] add support for the filter tag also: deprecate usage of the tag parameter for filtering --- src/main/java/com/ecwid/consul/v1/Filter.java | 262 ++++++++++++++++++ .../v1/health/HealthServicesRequest.java | 61 +++- .../java/com/ecwid/consul/v1/FilterTest.java | 167 +++++++++++ .../com/ecwid/consul/v1/QueryParamsTest.java | 1 - .../v1/health/HealthServicesRequestTest.java | 9 +- 5 files changed, 491 insertions(+), 9 deletions(-) create mode 100644 src/main/java/com/ecwid/consul/v1/Filter.java create mode 100644 src/test/java/com/ecwid/consul/v1/FilterTest.java diff --git a/src/main/java/com/ecwid/consul/v1/Filter.java b/src/main/java/com/ecwid/consul/v1/Filter.java new file mode 100644 index 00000000..e54763e1 --- /dev/null +++ b/src/main/java/com/ecwid/consul/v1/Filter.java @@ -0,0 +1,262 @@ +package com.ecwid.consul.v1; + +import com.ecwid.consul.UrlParameters; + +import java.util.Arrays; +import java.util.Collections; +import java.util.LinkedList; +import java.util.List; +import java.util.Objects; +import java.util.stream.Collectors; + +/** + * Implements a query filter parameter. + * See https://www.consul.io/api-docs/features/filtering + */ +public class Filter implements UrlParameters { + public enum MatchingOperator { + EQUAL("=", false), + NOT_EQUAL("!=", false), + IS_EMPTY("is empty", true), + IS_NOT_EMPTY("is not empty", true), + IN("in", false), + NOT_IN("not in", false), + CONTAINS("contains", false), + NOT_CONTAINS("not contains", false), + MATCHES("matches", false), + NOT_MATCHES("not matches", false); + + private final String representation; + private final boolean unary; + + MatchingOperator(final String representation, final boolean unary) { + this.representation = representation; + this.unary = unary; + } + + @Override + public String toString() { + return representation; + } + } + + public static class Selector { + private final String s; + + private Selector(String s) { + this.s = s; + } + + public static Selector of(final String s) { + return new Selector(s); + } + } + + /** + * Creates a filter with a unary operator (e.g {@link MatchingOperator#IN} or {@link MatchingOperator#NOT_IN}) + * @throws IllegalArgumentException if used with a binary matching operator + */ + public static Filter of(final MatchingOperator matchingOperator, final Selector selector) { + if (!matchingOperator.unary) { + throw new IllegalArgumentException("operator " + matchingOperator.name() + " requires a value"); + } + return new Filter(Collections.emptyList(), BoolOp.AND, new Leaf(matchingOperator, selector.s, null), true); + } + + /** + * Creates a filter with a binary operator + * @throws IllegalArgumentException if used with a unary matching operator ({@link MatchingOperator#IN} or + * {@link MatchingOperator#NOT_IN}) specifying a value, or if used with a binary matching operator w/o specifying + * a value. + */ + public static Filter of(final MatchingOperator matchingOperator, final Selector selector, final String value) { + if (matchingOperator.unary && (value != null)) { + throw new IllegalArgumentException("operator " + matchingOperator.name() + " does not accept a value"); + } + if (!matchingOperator.unary && (value == null)) { + throw new IllegalArgumentException("operator " + matchingOperator.name() + " requires a value"); + } + if (matchingOperator == MatchingOperator.IN) { + return in(value, selector); + } + if (matchingOperator == MatchingOperator.NOT_IN) { + return notIn(value, selector); + } + return new Filter(Collections.emptyList(), BoolOp.AND, new Leaf(matchingOperator, selector.s, value), true); + } + + /** + * Creates a {@code "" in } query + */ + public static Filter in(final String value, final Selector selector) { + return new Filter(Collections.emptyList(), BoolOp.AND, new Leaf(MatchingOperator.IN, selector.s, value), true); + } + + /** + * Creates a {@code "" not in } query + */ + public static Filter notIn(final String value, final Selector selector) { + return new Filter(Collections.emptyList(), BoolOp.AND, new Leaf(MatchingOperator.NOT_IN, selector.s, value), true); + } + + /** + * Returns a new filter, with the specified filters added, all joined with 'and'. + */ + public Filter and(final Filter ...filters) { + return add(BoolOp.AND, filters); + } + + /** + * Creates a new filter with the specified filters, all joined with 'and'. + */ + public static Filter andAll(final List filters) { + return new Filter(filters, BoolOp.AND, true); + } + + /** + * Returns a new filter, with the specified filters added, all joined with 'or'. + */ + public Filter or(final Filter ...filters) { + return add(BoolOp.OR, filters); + } + + /** + * Creates a new filter with the specified filters, all joined with 'and'. + */ + public static Filter orAll(final List filters) { + return new Filter(filters, BoolOp.OR, true); + } + + /** + * Returns a negated copy of this filter. + * Calling this method on a negative filter will turn it into a positive filter, + * i.e {@code filter.not().not().equals(filter)} is true + */ + public Filter not() { + return new Filter(children, boolOp, leaf, !positive); + } + + @Override + public List toUrlParameters() { + return Collections.singletonList("filter=" + toString()); + } + + @Override + public boolean equals(final Object o) { + if (this == o) { + return true; + } + if ((o == null) || (getClass() != o.getClass())) { + return false; + } + final Filter filter = (Filter) o; + return (positive == filter.positive) && + Objects.equals(children, filter.children) && + (boolOp == filter.boolOp) && + Objects.equals(leaf, filter.leaf); + } + + @Override + public int hashCode() { + return Objects.hash(children, boolOp, leaf, positive); + } + + @Override + public String toString() { + final String prefix = positive ? "" : "not "; + if (leaf != null) { + if (leaf.value != null) { + if ((leaf.matchingOperator == MatchingOperator.IN) || (leaf.matchingOperator == MatchingOperator.NOT_IN)) { + return String.format(prefix + "\"%s\" %s %s", leaf.value, leaf.matchingOperator, leaf.selector); + } + return String.format(prefix + "%s %s \"%s\"", leaf.selector, leaf.matchingOperator, leaf.value); + } + return String.format(prefix + "%s %s", leaf.selector, leaf.matchingOperator); + } + + final String result = children.stream().map(Filter::toString).collect(Collectors.joining(" " + boolOp + " ")); + if ((parent == null) && positive) { + return result; + } + return prefix + "(" + result + ")"; + } + + private enum BoolOp { + OR("or"), + AND("and"); + + private final String representation; + + BoolOp(final String representation) { + this.representation = representation; + } + + @Override + public String toString() { + return representation; + } + + } + + private static class Leaf { + private final Filter.MatchingOperator matchingOperator; + private final String selector; + private final String value; + + private Leaf(final MatchingOperator matchingOperator, final String selector, final String value) { + this.matchingOperator = matchingOperator; + this.selector = selector; + this.value = value; + } + + @Override + public boolean equals(final Object o) { + if (this == o) { + return true; + } + if ((o == null) || (getClass() != o.getClass())) { + return false; + } + final Leaf leaf = (Leaf) o; + return (matchingOperator == leaf.matchingOperator) && + selector.equals(leaf.selector) && + value.equals(leaf.value); + } + + @Override + public int hashCode() { + return Objects.hash(matchingOperator, selector, value); + } + + } + private final List children; + + private final BoolOp boolOp; + private final Leaf leaf; + private Filter parent; + private final boolean positive; + private Filter(final List children, final BoolOp boolOp, final boolean positive) { + this.children = children; + this.boolOp = boolOp; + leaf = null; + this.positive = positive; + } + + private Filter(final List children, final BoolOp boolOp, final Leaf leaf, final boolean positive) { + this.children = children; + this.boolOp = boolOp; + this.leaf = leaf; + this.positive = positive; + } + + private Filter add(final BoolOp op, final Filter... filters) { + final List newChildren = new LinkedList<>(); + newChildren.add(this); + newChildren.addAll(Arrays.asList(filters)); + final Filter result = new Filter(newChildren, op, null, true); + for (final Filter child : newChildren) { + child.parent = result; + } + return result; + } +} diff --git a/src/main/java/com/ecwid/consul/v1/health/HealthServicesRequest.java b/src/main/java/com/ecwid/consul/v1/health/HealthServicesRequest.java index ececc494..df54d1ce 100644 --- a/src/main/java/com/ecwid/consul/v1/health/HealthServicesRequest.java +++ b/src/main/java/com/ecwid/consul/v1/health/HealthServicesRequest.java @@ -2,10 +2,11 @@ import com.ecwid.consul.ConsulRequest; import com.ecwid.consul.SingleUrlParameters; -import com.ecwid.consul.v1.TagsParameters; import com.ecwid.consul.UrlParameters; +import com.ecwid.consul.v1.Filter; import com.ecwid.consul.v1.NodeMetaParameters; import com.ecwid.consul.v1.QueryParams; +import com.ecwid.consul.v1.TagsParameters; import java.util.ArrayList; import java.util.Arrays; @@ -23,8 +24,18 @@ public final class HealthServicesRequest implements ConsulRequest { private final boolean passing; private final QueryParams queryParams; private final String token; - - private HealthServicesRequest(String datacenter, String near, String[] tags, Map nodeMeta, boolean passing, QueryParams queryParams, String token) { + private final Filter filter; + + private HealthServicesRequest( + String datacenter, + String near, + String[] tags, + Map nodeMeta, + boolean passing, + QueryParams queryParams, + String token, + Filter filter + ) { this.datacenter = datacenter; this.near = near; this.tags = tags; @@ -32,6 +43,7 @@ private HealthServicesRequest(String datacenter, String near, String[] tags, Map this.passing = passing; this.queryParams = queryParams; this.token = token; + this.filter = filter; } public String getDatacenter() { @@ -42,14 +54,28 @@ public String getNear() { return near; } + /** + * @deprecated use {@link HealthServicesRequest.Builder#setFilter(Filter)} to filter by tags + * e.g {@code * setFilter(Filter.in(tag, Filter.Selector.of("Service.tags")))} + */ + @Deprecated public String getTag() { - return tags != null && tags.length > 0 ? tags[0] : null; + return ((tags != null) && (tags.length > 0)) ? tags[0] : null; } + /** + * @deprecated use {@link HealthServicesRequest.Builder#setFilter(Filter)} to filter by tags + * e.g {@code * setFilter(Filter.in(tag, Filter.Selector.of("Service.tags")))} + */ + @Deprecated public String[] getTags() { return tags; } + public Filter getFilter() { + return filter; + } + public Map getNodeMeta() { return nodeMeta; } @@ -70,6 +96,7 @@ public static class Builder { private String datacenter; private String near; private String[] tags; + private Filter filter; private Map nodeMeta; private boolean passing; private QueryParams queryParams; @@ -88,16 +115,31 @@ public Builder setNear(String near) { return this; } + /** + * @deprecated use {@link #setFilter(Filter)} + * e.g {@code * setFilter(Filter.in(tag, Filter.Selector.of("Service.tags")))} + */ + @Deprecated public Builder setTag(String tag) { - this.tags = new String[]{tag}; + tags = new String[]{tag}; return this; } + /** + * @deprecated use {@link #setFilter(Filter)} + * e.g {@code * setFilter(Filter.in(tag, Filter.Selector.of("Service.tags")))} + */ + @Deprecated public Builder setTags(String[] tags) { this.tags = tags; return this; } + public Builder setFilter(Filter filter) { + this.filter = filter; + return this; + } + public Builder setNodeMeta(Map nodeMeta) { this.nodeMeta = nodeMeta != null ? Collections.unmodifiableMap(nodeMeta) : null; return this; @@ -119,7 +161,7 @@ public Builder setToken(String token) { } public HealthServicesRequest build() { - return new HealthServicesRequest(datacenter, near, tags, nodeMeta, passing, queryParams, token); + return new HealthServicesRequest(datacenter, near, tags, nodeMeta, passing, queryParams, token, filter); } } @@ -143,6 +185,10 @@ public List asUrlParameters() { params.add(new TagsParameters(tags)); } + if (filter != null) { + params.add(filter); + } + if (nodeMeta != null) { params.add(new NodeMetaParameters(nodeMeta)); } @@ -173,6 +219,7 @@ public boolean equals(Object o) { Objects.equals(datacenter, that.datacenter) && Objects.equals(near, that.near) && Arrays.equals(tags, that.tags) && + Objects.equals(filter, that.filter) && Objects.equals(nodeMeta, that.nodeMeta) && Objects.equals(queryParams, that.queryParams) && Objects.equals(token, that.token); @@ -180,7 +227,7 @@ public boolean equals(Object o) { @Override public int hashCode() { - int result = Objects.hash(datacenter, near, nodeMeta, passing, queryParams, token); + int result = Objects.hash(datacenter, near, nodeMeta, passing, queryParams, token, filter); result = 31 * result + Arrays.hashCode(tags); return result; } diff --git a/src/test/java/com/ecwid/consul/v1/FilterTest.java b/src/test/java/com/ecwid/consul/v1/FilterTest.java new file mode 100644 index 00000000..b7cd38e4 --- /dev/null +++ b/src/test/java/com/ecwid/consul/v1/FilterTest.java @@ -0,0 +1,167 @@ +package com.ecwid.consul.v1; + +import com.ecwid.consul.v1.Filter.Selector; +import nl.jqno.equalsverifier.EqualsVerifier; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class FilterTest { + @Nested + class EqualsAndHashCode { + @Test + void shouldVerify() { + EqualsVerifier.forClass(Filter.class) + .withPrefabValues( + Filter.class, + Filter.of(Filter.MatchingOperator.EQUAL, Filter.Selector.of("foo"), "bar"), + Filter.in("baz", Filter.Selector.of("fang")) + ) + .usingGetClass() + .withIgnoredFields("parent") + .verify(); + } + } + + @Test + public void of() { + Filter actual = Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo"), "bar"); + assertEquals("foo = \"bar\"", actual.toString()); + assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo"))); + + actual = Filter.of(Filter.MatchingOperator.NOT_EQUAL, Selector.of("foo"), "bar"); + assertEquals("foo != \"bar\"", actual.toString()); + assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.NOT_EQUAL, Selector.of("foo"))); + + actual = Filter.of(Filter.MatchingOperator.IS_EMPTY, Selector.of("foo"), null); + assertEquals("foo is empty", actual.toString()); + actual = Filter.of(Filter.MatchingOperator.IS_EMPTY, Selector.of("foo")); + assertEquals("foo is empty", actual.toString()); + assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.IS_EMPTY, Selector.of("foo"), "bar")); + + actual = Filter.of(Filter.MatchingOperator.IS_NOT_EMPTY, Selector.of("foo"), null); + assertEquals("foo is not empty", actual.toString()); + actual = Filter.of(Filter.MatchingOperator.IS_NOT_EMPTY, Selector.of("foo")); + assertEquals("foo is not empty", actual.toString()); + assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.IS_NOT_EMPTY, Selector.of("foo"), "bar")); + + actual = Filter.of(Filter.MatchingOperator.IN, Selector.of("foo"), "bar"); + assertEquals("\"bar\" in foo", actual.toString()); + assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.IN, Selector.of("foo"))); + + actual = Filter.of(Filter.MatchingOperator.NOT_IN, Selector.of("foo"), "bar"); + assertEquals("\"bar\" not in foo", actual.toString()); + assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.NOT_IN, Selector.of("foo"))); + + actual = Filter.of(Filter.MatchingOperator.CONTAINS, Selector.of("foo"), "bar"); + assertEquals("foo contains \"bar\"", actual.toString()); + assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.CONTAINS, Selector.of("foo"))); + + actual = Filter.of(Filter.MatchingOperator.NOT_CONTAINS, Selector.of("foo"), "bar"); + assertEquals("foo not contains \"bar\"", actual.toString()); + assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.NOT_CONTAINS, Selector.of("foo"))); + + actual = Filter.of(Filter.MatchingOperator.MATCHES, Selector.of("foo"), "bar"); + assertEquals("foo matches \"bar\"", actual.toString()); + assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.MATCHES, Selector.of("foo"))); + + actual = Filter.of(Filter.MatchingOperator.NOT_MATCHES, Selector.of("foo"), "bar"); + assertEquals("foo not matches \"bar\"", actual.toString()); + assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.NOT_MATCHES, Selector.of("foo"))); + } + + @Test + public void in() { + final Filter actual = Filter.in("bar", Selector.of("foo")); + assertEquals("\"bar\" in foo", actual.toString()); + } + + @Test + public void notIn() { + final Filter actual = Filter.notIn("bar", Selector.of("foo")); + assertEquals("\"bar\" not in foo", actual.toString()); + } + + @Test + public void and() { + final Filter f = Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo"), "bar"); + final Filter actual = f.and( + Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo1"), "bar1"), + Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo2"), "bar2") + ); + + assertEquals("foo = \"bar\" and foo1 = \"bar1\" and foo2 = \"bar2\"", actual.toString()); + + final Filter actual2 = f + .or(Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo1"), "bar1")) + .and(Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo3"), "bar3")); + assertEquals("(foo = \"bar\" or foo1 = \"bar1\") and foo3 = \"bar3\"", actual2.toString()); + + final Filter actual3 = f.and(); + assertEquals("foo = \"bar\"", actual3.toString()); + } + + @Test + public void addAll() { + final Filter actual = Filter.andAll(Arrays.asList(new Filter[]{ + Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo"), "bar"), + Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo1"), "bar1") + } + )); + assertEquals("foo = \"bar\" and foo1 = \"bar1\"", actual.toString()); + } + + @Test + public void or() { + final Filter f = Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo"), "bar"); + final Filter actual = f.or( + Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo1"), "bar1"), + Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo2"), "bar2") + ); + + assertEquals("foo = \"bar\" or foo1 = \"bar1\" or foo2 = \"bar2\"", actual.toString()); + + final Filter actual2 = f + .and(Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo1"), "bar1")) + .or(Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo3"), "bar3")); + assertEquals("(foo = \"bar\" and foo1 = \"bar1\") or foo3 = \"bar3\"", actual2.toString()); + + final Filter actual3 = f.or(); + assertEquals("foo = \"bar\"", actual3.toString()); + } + + @Test + public void orAll() { + final Filter actual = Filter.orAll(Arrays.asList(new Filter[]{ + Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo"), "bar"), + Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo1"), "bar1") + } + )); + assertEquals("foo = \"bar\" or foo1 = \"bar1\"", actual.toString()); + } + + @Test + public void not() { + final Filter f = Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo"), "bar"); + assertEquals("foo = \"bar\"", f.toString()); + assertEquals("not foo = \"bar\"", f.not().toString()); + assertEquals("foo = \"bar\"", f.not().not().toString()); + + final Filter f2 = f.and(Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo1"), "bar2")); + assertEquals("foo = \"bar\" and foo1 = \"bar2\"", f2.toString()); + assertEquals("not (foo = \"bar\" and foo1 = \"bar2\")", f2.not().toString()); + } + + @Test + public void toUrlParameters() { + final Filter subject = Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo"), "bar"); + final List actual = subject.toUrlParameters(); + assertEquals(Collections.singletonList("filter=foo = \"bar\""), actual); + } +} diff --git a/src/test/java/com/ecwid/consul/v1/QueryParamsTest.java b/src/test/java/com/ecwid/consul/v1/QueryParamsTest.java index 331e50e6..5c825db1 100644 --- a/src/test/java/com/ecwid/consul/v1/QueryParamsTest.java +++ b/src/test/java/com/ecwid/consul/v1/QueryParamsTest.java @@ -1,7 +1,6 @@ package com.ecwid.consul.v1; import com.ecwid.consul.Utils; -import com.ecwid.consul.v1.catalog.CatalogServiceRequest; import nl.jqno.equalsverifier.EqualsVerifier; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; diff --git a/src/test/java/com/ecwid/consul/v1/health/HealthServicesRequestTest.java b/src/test/java/com/ecwid/consul/v1/health/HealthServicesRequestTest.java index f60782c7..8caec2a7 100644 --- a/src/test/java/com/ecwid/consul/v1/health/HealthServicesRequestTest.java +++ b/src/test/java/com/ecwid/consul/v1/health/HealthServicesRequestTest.java @@ -1,5 +1,6 @@ package com.ecwid.consul.v1.health; +import com.ecwid.consul.v1.Filter; import nl.jqno.equalsverifier.EqualsVerifier; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -9,7 +10,13 @@ class HealthServicesRequestTest { class EqualsAndHashCode { @Test void shouldVerify() { - EqualsVerifier.forClass(HealthServicesRequest.class).verify(); + EqualsVerifier.forClass(HealthServicesRequest.class) + .withPrefabValues( + Filter.class, + Filter.of(Filter.MatchingOperator.EQUAL, Filter.Selector.of("foo"), "bar"), + Filter.in("baz", Filter.Selector.of("fang")) + ) + .verify(); } } } From 3296e1b5d2afeb7be3764a536a5d1bb96c980de1 Mon Sep 17 00:00:00 2001 From: Avishai Weissberg Date: Wed, 20 Oct 2021 14:43:54 -0700 Subject: [PATCH 2/3] url-encode filter parameter --- src/main/java/com/ecwid/consul/v1/Filter.java | 26 +++++++- .../v1/health/HealthServicesRequest.java | 8 +-- .../java/com/ecwid/consul/v1/FilterTest.java | 62 ++++++++++--------- 3 files changed, 63 insertions(+), 33 deletions(-) diff --git a/src/main/java/com/ecwid/consul/v1/Filter.java b/src/main/java/com/ecwid/consul/v1/Filter.java index e54763e1..aa44adc8 100644 --- a/src/main/java/com/ecwid/consul/v1/Filter.java +++ b/src/main/java/com/ecwid/consul/v1/Filter.java @@ -27,10 +27,12 @@ public enum MatchingOperator { NOT_MATCHES("not matches", false); private final String representation; + private final String encoded; private final boolean unary; MatchingOperator(final String representation, final boolean unary) { this.representation = representation; + encoded = representation.replaceAll(" ", SPACE); this.unary = unary; } @@ -138,7 +140,7 @@ public Filter not() { @Override public List toUrlParameters() { - return Collections.singletonList("filter=" + toString()); + return Collections.singletonList("filter=" + toEncodedString()); } @Override @@ -181,6 +183,28 @@ public String toString() { return prefix + "(" + result + ")"; } + private static final String SPACE = "%20"; + private static final String DOUBLEQUOTE = "%22"; + + public String toEncodedString() { + final String prefix = positive ? "" : ("not" + SPACE); + if (leaf != null) { + if (leaf.value != null) { + if ((leaf.matchingOperator == MatchingOperator.IN) || (leaf.matchingOperator == MatchingOperator.NOT_IN)) { + return prefix + DOUBLEQUOTE + leaf.value + DOUBLEQUOTE + SPACE + leaf.matchingOperator.encoded + SPACE + leaf.selector; + } + return prefix + leaf.selector + SPACE + leaf.matchingOperator.encoded + SPACE + DOUBLEQUOTE + leaf.value + DOUBLEQUOTE; + } + return prefix + leaf.selector + SPACE + leaf.matchingOperator.encoded; + } + + final String result = children.stream().map(Filter::toEncodedString).collect(Collectors.joining(SPACE + boolOp + SPACE)); + if ((parent == null) && positive) { + return result; + } + return prefix + "(" + result + ")"; + } + private enum BoolOp { OR("or"), AND("and"); diff --git a/src/main/java/com/ecwid/consul/v1/health/HealthServicesRequest.java b/src/main/java/com/ecwid/consul/v1/health/HealthServicesRequest.java index df54d1ce..2fb74b99 100644 --- a/src/main/java/com/ecwid/consul/v1/health/HealthServicesRequest.java +++ b/src/main/java/com/ecwid/consul/v1/health/HealthServicesRequest.java @@ -56,7 +56,7 @@ public String getNear() { /** * @deprecated use {@link HealthServicesRequest.Builder#setFilter(Filter)} to filter by tags - * e.g {@code * setFilter(Filter.in(tag, Filter.Selector.of("Service.tags")))} + * e.g {@code * setFilter(Filter.in(tag, Filter.Selector.of("Service.Tags")))} */ @Deprecated public String getTag() { @@ -65,7 +65,7 @@ public String getTag() { /** * @deprecated use {@link HealthServicesRequest.Builder#setFilter(Filter)} to filter by tags - * e.g {@code * setFilter(Filter.in(tag, Filter.Selector.of("Service.tags")))} + * e.g {@code * setFilter(Filter.in(tag, Filter.Selector.of("Service.Tags")))} */ @Deprecated public String[] getTags() { @@ -117,7 +117,7 @@ public Builder setNear(String near) { /** * @deprecated use {@link #setFilter(Filter)} - * e.g {@code * setFilter(Filter.in(tag, Filter.Selector.of("Service.tags")))} + * e.g {@code * setFilter(Filter.in(tag, Filter.Selector.of("Service.Tags")))} */ @Deprecated public Builder setTag(String tag) { @@ -127,7 +127,7 @@ public Builder setTag(String tag) { /** * @deprecated use {@link #setFilter(Filter)} - * e.g {@code * setFilter(Filter.in(tag, Filter.Selector.of("Service.tags")))} + * e.g {@code * setFilter(Filter.in(tag, Filter.Selector.of("Service.Tags")))} */ @Deprecated public Builder setTags(String[] tags) { diff --git a/src/test/java/com/ecwid/consul/v1/FilterTest.java b/src/test/java/com/ecwid/consul/v1/FilterTest.java index b7cd38e4..1070cbff 100644 --- a/src/test/java/com/ecwid/consul/v1/FilterTest.java +++ b/src/test/java/com/ecwid/consul/v1/FilterTest.java @@ -32,60 +32,60 @@ void shouldVerify() { @Test public void of() { Filter actual = Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo"), "bar"); - assertEquals("foo = \"bar\"", actual.toString()); + assertFilter("foo = \"bar\"", actual); assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo"))); actual = Filter.of(Filter.MatchingOperator.NOT_EQUAL, Selector.of("foo"), "bar"); - assertEquals("foo != \"bar\"", actual.toString()); + assertFilter("foo != \"bar\"", actual); assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.NOT_EQUAL, Selector.of("foo"))); actual = Filter.of(Filter.MatchingOperator.IS_EMPTY, Selector.of("foo"), null); - assertEquals("foo is empty", actual.toString()); + assertFilter("foo is empty", actual); actual = Filter.of(Filter.MatchingOperator.IS_EMPTY, Selector.of("foo")); - assertEquals("foo is empty", actual.toString()); + assertFilter("foo is empty", actual); assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.IS_EMPTY, Selector.of("foo"), "bar")); actual = Filter.of(Filter.MatchingOperator.IS_NOT_EMPTY, Selector.of("foo"), null); - assertEquals("foo is not empty", actual.toString()); + assertFilter("foo is not empty", actual); actual = Filter.of(Filter.MatchingOperator.IS_NOT_EMPTY, Selector.of("foo")); - assertEquals("foo is not empty", actual.toString()); + assertFilter("foo is not empty", actual); assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.IS_NOT_EMPTY, Selector.of("foo"), "bar")); actual = Filter.of(Filter.MatchingOperator.IN, Selector.of("foo"), "bar"); - assertEquals("\"bar\" in foo", actual.toString()); + assertFilter("\"bar\" in foo", actual); assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.IN, Selector.of("foo"))); actual = Filter.of(Filter.MatchingOperator.NOT_IN, Selector.of("foo"), "bar"); - assertEquals("\"bar\" not in foo", actual.toString()); + assertFilter("\"bar\" not in foo", actual); assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.NOT_IN, Selector.of("foo"))); actual = Filter.of(Filter.MatchingOperator.CONTAINS, Selector.of("foo"), "bar"); - assertEquals("foo contains \"bar\"", actual.toString()); + assertFilter("foo contains \"bar\"", actual); assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.CONTAINS, Selector.of("foo"))); actual = Filter.of(Filter.MatchingOperator.NOT_CONTAINS, Selector.of("foo"), "bar"); - assertEquals("foo not contains \"bar\"", actual.toString()); + assertFilter("foo not contains \"bar\"", actual); assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.NOT_CONTAINS, Selector.of("foo"))); actual = Filter.of(Filter.MatchingOperator.MATCHES, Selector.of("foo"), "bar"); - assertEquals("foo matches \"bar\"", actual.toString()); + assertFilter("foo matches \"bar\"", actual); assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.MATCHES, Selector.of("foo"))); actual = Filter.of(Filter.MatchingOperator.NOT_MATCHES, Selector.of("foo"), "bar"); - assertEquals("foo not matches \"bar\"", actual.toString()); + assertFilter("foo not matches \"bar\"", actual); assertThrows(IllegalArgumentException.class, ()-> Filter.of(Filter.MatchingOperator.NOT_MATCHES, Selector.of("foo"))); } @Test public void in() { final Filter actual = Filter.in("bar", Selector.of("foo")); - assertEquals("\"bar\" in foo", actual.toString()); + assertFilter("\"bar\" in foo", actual); } @Test public void notIn() { final Filter actual = Filter.notIn("bar", Selector.of("foo")); - assertEquals("\"bar\" not in foo", actual.toString()); + assertFilter("\"bar\" not in foo", actual); } @Test @@ -96,15 +96,15 @@ public void and() { Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo2"), "bar2") ); - assertEquals("foo = \"bar\" and foo1 = \"bar1\" and foo2 = \"bar2\"", actual.toString()); + assertFilter("foo = \"bar\" and foo1 = \"bar1\" and foo2 = \"bar2\"", actual); final Filter actual2 = f .or(Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo1"), "bar1")) .and(Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo3"), "bar3")); - assertEquals("(foo = \"bar\" or foo1 = \"bar1\") and foo3 = \"bar3\"", actual2.toString()); + assertFilter("(foo = \"bar\" or foo1 = \"bar1\") and foo3 = \"bar3\"", actual2); final Filter actual3 = f.and(); - assertEquals("foo = \"bar\"", actual3.toString()); + assertFilter("foo = \"bar\"", actual3); } @Test @@ -114,7 +114,7 @@ public void addAll() { Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo1"), "bar1") } )); - assertEquals("foo = \"bar\" and foo1 = \"bar1\"", actual.toString()); + assertFilter("foo = \"bar\" and foo1 = \"bar1\"", actual); } @Test @@ -125,15 +125,15 @@ public void or() { Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo2"), "bar2") ); - assertEquals("foo = \"bar\" or foo1 = \"bar1\" or foo2 = \"bar2\"", actual.toString()); + assertFilter("foo = \"bar\" or foo1 = \"bar1\" or foo2 = \"bar2\"", actual); final Filter actual2 = f .and(Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo1"), "bar1")) .or(Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo3"), "bar3")); - assertEquals("(foo = \"bar\" and foo1 = \"bar1\") or foo3 = \"bar3\"", actual2.toString()); + assertFilter("(foo = \"bar\" and foo1 = \"bar1\") or foo3 = \"bar3\"", actual2); final Filter actual3 = f.or(); - assertEquals("foo = \"bar\"", actual3.toString()); + assertFilter("foo = \"bar\"", actual3); } @Test @@ -143,25 +143,31 @@ public void orAll() { Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo1"), "bar1") } )); - assertEquals("foo = \"bar\" or foo1 = \"bar1\"", actual.toString()); + assertFilter("foo = \"bar\" or foo1 = \"bar1\"", actual); } @Test public void not() { final Filter f = Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo"), "bar"); - assertEquals("foo = \"bar\"", f.toString()); - assertEquals("not foo = \"bar\"", f.not().toString()); - assertEquals("foo = \"bar\"", f.not().not().toString()); + assertFilter("foo = \"bar\"", f); + assertFilter("not foo = \"bar\"", f.not()); + assertFilter("foo = \"bar\"", f.not().not()); final Filter f2 = f.and(Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo1"), "bar2")); - assertEquals("foo = \"bar\" and foo1 = \"bar2\"", f2.toString()); - assertEquals("not (foo = \"bar\" and foo1 = \"bar2\")", f2.not().toString()); + assertFilter("foo = \"bar\" and foo1 = \"bar2\"", f2); + assertFilter("not (foo = \"bar\" and foo1 = \"bar2\")", f2.not()); } @Test public void toUrlParameters() { final Filter subject = Filter.of(Filter.MatchingOperator.EQUAL, Selector.of("foo"), "bar"); final List actual = subject.toUrlParameters(); - assertEquals(Collections.singletonList("filter=foo = \"bar\""), actual); + assertEquals(Collections.singletonList("filter=foo%20=%20%22bar%22"), actual); + } + + private void assertFilter(final String expected, final Filter subject) { + assertEquals(expected, subject.toString()); + final String encoded = expected.replaceAll(" ", "%20").replaceAll("\"", "%22"); + assertEquals(encoded, subject.toEncodedString()); } } From 53d7dc2198ab809758649df6e2e98ad069617503 Mon Sep 17 00:00:00 2001 From: Avishai Weissberg Date: Thu, 21 Oct 2021 06:50:47 -0700 Subject: [PATCH 3/3] fewer nested ifs --- src/main/java/com/ecwid/consul/v1/Filter.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/ecwid/consul/v1/Filter.java b/src/main/java/com/ecwid/consul/v1/Filter.java index aa44adc8..60707fde 100644 --- a/src/main/java/com/ecwid/consul/v1/Filter.java +++ b/src/main/java/com/ecwid/consul/v1/Filter.java @@ -188,21 +188,20 @@ public String toString() { public String toEncodedString() { final String prefix = positive ? "" : ("not" + SPACE); - if (leaf != null) { - if (leaf.value != null) { - if ((leaf.matchingOperator == MatchingOperator.IN) || (leaf.matchingOperator == MatchingOperator.NOT_IN)) { - return prefix + DOUBLEQUOTE + leaf.value + DOUBLEQUOTE + SPACE + leaf.matchingOperator.encoded + SPACE + leaf.selector; - } - return prefix + leaf.selector + SPACE + leaf.matchingOperator.encoded + SPACE + DOUBLEQUOTE + leaf.value + DOUBLEQUOTE; + if (leaf == null) { + final String result = children.stream().map(Filter::toEncodedString).collect(Collectors.joining(SPACE + boolOp + SPACE)); + if ((parent == null) && positive) { + return result; } + return prefix + "(" + result + ")"; + } + if (leaf.value == null) { return prefix + leaf.selector + SPACE + leaf.matchingOperator.encoded; } - - final String result = children.stream().map(Filter::toEncodedString).collect(Collectors.joining(SPACE + boolOp + SPACE)); - if ((parent == null) && positive) { - return result; + if ((leaf.matchingOperator == MatchingOperator.IN) || (leaf.matchingOperator == MatchingOperator.NOT_IN)) { + return prefix + DOUBLEQUOTE + leaf.value + DOUBLEQUOTE + SPACE + leaf.matchingOperator.encoded + SPACE + leaf.selector; } - return prefix + "(" + result + ")"; + return prefix + leaf.selector + SPACE + leaf.matchingOperator.encoded + SPACE + DOUBLEQUOTE + leaf.value + DOUBLEQUOTE; } private enum BoolOp {