[incubator-zipkin] branch master updated: annotation query parsing improvements (#2572)

[ad...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

adriancole pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-zipkin.git


The following commit(s) were added to refs/heads/master by this push:
     new 7a819ff  annotation query parsing improvements (#2572)
7a819ff is described below

commit 7a819ff31afceedc7d5e7b7e3211bb7f9e888dfe
Author: Jorg Heymans <jo...@gmail.com>
AuthorDate: Thu May 9 12:27:19 2019 +0200

    annotation query parsing improvements (#2572)
    
    trims annotations and properly addresses precedence
---
 .../main/java/zipkin2/storage/QueryRequest.java    | 11 ++++++---
 .../java/zipkin2/storage/QueryRequestTest.java     | 28 ++++++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/zipkin/src/main/java/zipkin2/storage/QueryRequest.java b/zipkin/src/main/java/zipkin2/storage/QueryRequest.java
index d6bdd33..6b7069c 100644
--- a/zipkin/src/main/java/zipkin2/storage/QueryRequest.java
+++ b/zipkin/src/main/java/zipkin2/storage/QueryRequest.java
@@ -186,7 +186,8 @@ public final class QueryRequest {
     }
 
     /**
-     * Corresponds to query parameter "annotationQuery". Ex. "http.method=GET and error"
+     * Corresponds to query parameter "annotationQuery". Ex. "http.method=GET and error". Parameter keys and values are
+     * trimmed.
      *
      * @see QueryRequest#annotationQueryString()
      */
@@ -196,10 +197,14 @@ public final class QueryRequest {
       for (String ann : annotationQuery.split(" and ", 100)) {
         int idx = ann.indexOf('=');
         if (idx == -1) {
-          map.put(ann, "");
+          // put the annotation only if there is no key present already, prevents overriding more specific tags
+          ann = ann.trim();
+          if (!map.containsKey(ann)) map.put(ann, "");
         } else {
+          // tag
           String[] keyValue = ann.split("=", 2);
-          map.put(ann.substring(0, idx), keyValue.length < 2 ? "" : ann.substring(idx + 1));
+          // tags are put regardless, i.e. last tag wins
+          map.put(ann.substring(0, idx).trim(), keyValue.length < 2 ? "" : ann.substring(idx + 1).trim());
         }
       }
       return annotationQuery(map);
diff --git a/zipkin/src/test/java/zipkin2/storage/QueryRequestTest.java b/zipkin/src/test/java/zipkin2/storage/QueryRequestTest.java
index 32b52a7..b1ed800 100644
--- a/zipkin/src/test/java/zipkin2/storage/QueryRequestTest.java
+++ b/zipkin/src/test/java/zipkin2/storage/QueryRequestTest.java
@@ -27,6 +27,7 @@ import zipkin2.TestObjects;
 
 import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.entry;
 
 public class QueryRequestTest {
   @Rule public ExpectedException thrown = ExpectedException.none();
@@ -75,6 +76,33 @@ public class QueryRequestTest {
       .isEmpty();
   }
 
+  @Test public void annotationQueryTrimsSpaces() {
+    // spaces in http.path mixed with 'and'
+    assertThat(queryBuilder.parseAnnotationQuery("fo and o and bar and http.path = /a ").annotationQuery)
+      .containsOnly(entry("fo", ""), entry("o", ""), entry("bar", ""), entry("http.path", "/a"));
+    // http.path in the beginning, more spaces
+    assertThat(queryBuilder.parseAnnotationQuery(" http.path = /a   and fo and o   and bar").annotationQuery)
+      .containsOnly(entry("fo", ""), entry("o", ""), entry("bar", ""), entry("http.path", "/a"));
+    // @adriancole said this would be hard to parse, annotation containing spaces
+    assertThat(queryBuilder.parseAnnotationQuery("L O L").annotationQuery)
+      .containsOnly(entry("L O L", ""));
+    // annotation with spaces combined with tag
+    assertThat(queryBuilder.parseAnnotationQuery("L O L and http.path = /a").annotationQuery)
+      .containsOnly(entry("L O L", ""), entry("http.path", "/a"));
+    assertThat(queryBuilder.parseAnnotationQuery("bar =123 and L O L and http.path = /a and A B C").annotationQuery)
+      .containsOnly(entry("L O L", ""), entry("http.path", "/a"), entry("bar", "123"), entry("A B C", ""));
+  }
+
+  @Test public void annotationQueryParameterSpecificity() {
+    // when a parameter is specified both as a tag and annotation, the tag wins because it's considered to be more
+    // specific
+    assertThat(queryBuilder.parseAnnotationQuery("a=123 and a").annotationQuery).containsOnly(entry("a", "123"));
+    assertThat(queryBuilder.parseAnnotationQuery("a and a=123").annotationQuery).containsOnly(entry("a", "123"));
+    // also last tag wins
+    assertThat(queryBuilder.parseAnnotationQuery("a=123 and a=456").annotationQuery).containsOnly(entry("a", "456"));
+    assertThat(queryBuilder.parseAnnotationQuery("a and a=123 and a=456").annotationQuery).containsOnly(entry("a", "456"));
+  }
+
   @Test public void endTsMustBePositive() {
     thrown.expect(IllegalArgumentException.class);
     thrown.expectMessage("endTs <= 0");