From efd7b1bc02cce39037e8ad253669e59ae8698aef Mon Sep 17 00:00:00 2001 From: Avishai Weissberg Date: Wed, 20 Oct 2021 14:43:54 -0700 Subject: [PATCH 1/5] url-encode filter parameter --- build.gradle | 2 +- src/main/java/com/ecwid/consul/v1/Filter.java | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/build.gradle b/build.gradle index 4e8a6bd..92b68eb 100644 --- a/build.gradle +++ b/build.gradle @@ -61,7 +61,7 @@ artifacts { group = "com.indeed" archivesBaseName = "consul-api" -version = "1.4.8" +version = "1.4.9" def doUploadArchives = project.hasProperty('sonatypeUsername') && project.hasProperty('sonatypePassword') if (doUploadArchives) { diff --git a/src/main/java/com/ecwid/consul/v1/Filter.java b/src/main/java/com/ecwid/consul/v1/Filter.java index e54763e..e791c8b 100644 --- a/src/main/java/com/ecwid/consul/v1/Filter.java +++ b/src/main/java/com/ecwid/consul/v1/Filter.java @@ -161,17 +161,20 @@ public int hashCode() { return Objects.hash(children, boolOp, leaf, positive); } + private static final String SPACE = "%20"; + private static final String DOUBLEQUOTE = "%22"; + @Override public String toString() { - final String prefix = positive ? "" : "not "; + final String prefix = positive ? "" : ("not" + SPACE); 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 prefix + DOUBLEQUOTE + leaf.value + DOUBLEQUOTE + SPACE + leaf.matchingOperator + SPACE + leaf.selector; } - return String.format(prefix + "%s %s \"%s\"", leaf.selector, leaf.matchingOperator, leaf.value); + return prefix + leaf.selector + SPACE + leaf.matchingOperator + SPACE + DOUBLEQUOTE + leaf.value + DOUBLEQUOTE; } - return String.format(prefix + "%s %s", leaf.selector, leaf.matchingOperator); + return prefix + leaf.selector + SPACE + leaf.matchingOperator; } final String result = children.stream().map(Filter::toString).collect(Collectors.joining(" " + boolOp + " ")); From 515b7afa8fc4312bc18df132ea89211e01fb9adc Mon Sep 17 00:00:00 2001 From: Avishai Weissberg Date: Wed, 20 Oct 2021 14:51:25 -0700 Subject: [PATCH 2/5] fix usage in javadocs --- .../com/ecwid/consul/v1/health/HealthServicesRequest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 8f6d13a..48f4b4a 100644 --- a/src/main/java/com/ecwid/consul/v1/health/HealthServicesRequest.java +++ b/src/main/java/com/ecwid/consul/v1/health/HealthServicesRequest.java @@ -80,7 +80,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() { @@ -89,7 +89,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() { @@ -146,7 +146,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) { @@ -156,7 +156,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) { From b7067dbce77bd06eca22f5f0531288bcacb219fe Mon Sep 17 00:00:00 2001 From: Avishai Weissberg Date: Wed, 20 Oct 2021 15:43:55 -0700 Subject: [PATCH 3/5] fix tests, use new toEncodedString() method --- src/main/java/com/ecwid/consul/v1/Filter.java | 35 ++++++++--- .../java/com/ecwid/consul/v1/FilterTest.java | 62 ++++++++++--------- 2 files changed, 62 insertions(+), 35 deletions(-) diff --git a/src/main/java/com/ecwid/consul/v1/Filter.java b/src/main/java/com/ecwid/consul/v1/Filter.java index e791c8b..e0a9957 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(" ", "%20"); 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 @@ -161,23 +163,42 @@ 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 static final String SPACE = "%20"; private static final String DOUBLEQUOTE = "%22"; - @Override - 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 + SPACE + leaf.selector; + return prefix + DOUBLEQUOTE + leaf.value + DOUBLEQUOTE + SPACE + leaf.matchingOperator.encoded + SPACE + leaf.selector; } - return prefix + leaf.selector + SPACE + leaf.matchingOperator + SPACE + DOUBLEQUOTE + leaf.value + DOUBLEQUOTE; + return prefix + leaf.selector + SPACE + leaf.matchingOperator.encoded + SPACE + DOUBLEQUOTE + leaf.value + DOUBLEQUOTE; } - return prefix + leaf.selector + SPACE + leaf.matchingOperator; + return prefix + leaf.selector + SPACE + leaf.matchingOperator.encoded; } - final String result = children.stream().map(Filter::toString).collect(Collectors.joining(" " + boolOp + " ")); + final String result = children.stream().map(Filter::toEncodedString).collect(Collectors.joining(SPACE + boolOp + SPACE)); if ((parent == null) && positive) { return result; } diff --git a/src/test/java/com/ecwid/consul/v1/FilterTest.java b/src/test/java/com/ecwid/consul/v1/FilterTest.java index b7cd38e..1070cbf 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 84505665fb351822312eb1e14da5c0aa1e0714c9 Mon Sep 17 00:00:00 2001 From: Avishai Weissberg Date: Wed, 20 Oct 2021 16:38:26 -0700 Subject: [PATCH 4/5] use SPACE everywhere --- src/main/java/com/ecwid/consul/v1/Filter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/ecwid/consul/v1/Filter.java b/src/main/java/com/ecwid/consul/v1/Filter.java index e0a9957..aa44adc 100644 --- a/src/main/java/com/ecwid/consul/v1/Filter.java +++ b/src/main/java/com/ecwid/consul/v1/Filter.java @@ -32,7 +32,7 @@ public enum MatchingOperator { MatchingOperator(final String representation, final boolean unary) { this.representation = representation; - encoded = representation.replaceAll(" ", "%20"); + encoded = representation.replaceAll(" ", SPACE); this.unary = unary; } From c0d69a2169274ee1e46e974d0da28a91d1cda8e1 Mon Sep 17 00:00:00 2001 From: Avishai Weissberg Date: Thu, 21 Oct 2021 06:50:47 -0700 Subject: [PATCH 5/5] 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 aa44adc..60707fd 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 {