Ensure that ip_range aggregations always return bucket keys. (#30701)

elastic · May 24, 2018 · 638a719 · 638a719
1 parent 8bbfdf1
commit 638a719
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 71 deletions.
diff --git a/docs/reference/aggregations/bucket/iprange-aggregation.asciidoc b/docs/reference/aggregations/bucket/iprange-aggregation.asciidoc
@@ -37,10 +37,12 @@ Response:
         "ip_ranges": {
             "buckets" : [
                 {
+                    "key": "*-10.0.0.5",
                     "to": "10.0.0.5",
                     "doc_count": 10
                 },
                 {
+                    "key": "10.0.0.5-*",
                     "from": "10.0.0.5",
                     "doc_count": 260
                 }

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml
@@ -144,25 +144,18 @@ setup:
 
   - length: { aggregations.ip_range.buckets: 3 }
 
-# ip_range does not automatically add keys to buckets, see #21045
-#  - match: { aggregations.ip_range.buckets.0.key: "*-192.168.0.0" }
-
   - is_false: aggregations.ip_range.buckets.0.from
 
   - match: { aggregations.ip_range.buckets.0.to: "192.168.0.0" }
 
   - match: { aggregations.ip_range.buckets.0.doc_count: 1 }
 
-#  - match: { aggregations.ip_range.buckets.1.key: "192.168.0.0-192.169.0.0" }
-
   - match: { aggregations.ip_range.buckets.1.from: "192.168.0.0" }
 
   - match: { aggregations.ip_range.buckets.1.to: "192.169.0.0" }
 
   - match: { aggregations.ip_range.buckets.1.doc_count: 2 }
 
-#  - match: { aggregations.ip_range.buckets.2.key: "192.169.0.0-*" }
-
   - match: { aggregations.ip_range.buckets.2.from: "192.169.0.0" }
 
   - is_false:  aggregations.ip_range.buckets.2.to
@@ -177,24 +170,18 @@ setup:
 
   - length: { aggregations.ip_range.buckets: 3 }
 
-#  - match: { aggregations.ip_range.buckets.0.key: "*-192.168.0.0" }
-
   - is_false: aggregations.ip_range.buckets.0.from
 
   - match: { aggregations.ip_range.buckets.0.to: "192.168.0.0" }
 
   - match: { aggregations.ip_range.buckets.0.doc_count: 1 }
 
-#  - match: { aggregations.ip_range.buckets.1.key: "192.168.0.0-192.169.0.0" }
-
   - match: { aggregations.ip_range.buckets.1.from: "192.168.0.0" }
 
   - match: { aggregations.ip_range.buckets.1.to: "192.169.0.0" }
 
   - match: { aggregations.ip_range.buckets.1.doc_count: 2 }
 
-#  - match: { aggregations.ip_range.buckets.2.key: "192.169.0.0-*" }
-
   - match: { aggregations.ip_range.buckets.2.from: "192.169.0.0" }
 
   - is_false:  aggregations.ip_range.buckets.2.to
@@ -223,6 +210,21 @@ setup:
 
   - match: { aggregations.ip_range.buckets.1.doc_count: 2 } 
 
+---
+"IP Range Key Generation":
+  - skip:
+     version: " - 6.99.99"
+     reason: "Before 7.0.0, ip_range did not always generate bucket keys (see #21045)."
+
+  - do:
+      search:
+        body: { "size" : 0, "aggs" : { "ip_range" : { "ip_range" : { "field" : "ip", "ranges": [ { "to": "192.168.0.0" }, { "from": "192.168.0.0", "to": "192.169.0.0" }, { "from": "192.169.0.0" } ] } } } }
+
+  - length: { aggregations.ip_range.buckets: 3 }
+  - match: { aggregations.ip_range.buckets.0.key: "*-192.168.0.0" }
+  - match: { aggregations.ip_range.buckets.1.key: "192.168.0.0-192.169.0.0" }
+  - match: { aggregations.ip_range.buckets.2.key: "192.169.0.0-*" }
+
 ---
 "Date range":
   - do:

diff --git a/...src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java b/...src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java
@@ -20,6 +20,7 @@
 package org.elasticsearch.search.aggregations.bucket.range;
 
 import org.apache.lucene.util.BytesRef;
+import org.elasticsearch.Version;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -57,35 +58,41 @@ public Bucket(DocValueFormat format, boolean keyed, String key, BytesRef from, B
                 long docCount, InternalAggregations aggregations) {
             this.format = format;
             this.keyed = keyed;
-            this.key = key;
+            this.key = key != null ? key : generateKey(from, to, format);
             this.from = from;
             this.to = to;
             this.docCount = docCount;
             this.aggregations = aggregations;
         }
 
-        // for serialization
-        private Bucket(StreamInput in, DocValueFormat format, boolean keyed) throws IOException {
-            this.format = format;
-            this.keyed = keyed;
-            key = in.readOptionalString();
-            if (in.readBoolean()) {
-                from = in.readBytesRef();
-            } else {
-                from = null;
-            }
-            if (in.readBoolean()) {
-                to = in.readBytesRef();
-            } else {
-                to = null;
-            }
-            docCount = in.readLong();
-            aggregations = InternalAggregations.readAggregations(in);
+        private static String generateKey(BytesRef from, BytesRef to, DocValueFormat format) {
+            StringBuilder builder = new StringBuilder()
+                .append(from == null ? "*" : format.format(from))
+                .append("-")
+                .append(to == null ? "*" : format.format(to));
+            return builder.toString();
+        }
+
+        private static Bucket createFromStream(StreamInput in, DocValueFormat format, boolean keyed) throws IOException {
+            String key = in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)
+                ? in.readString()
+                : in.readOptionalString();
+
+            BytesRef from = in.readBoolean() ? in.readBytesRef() : null;
+            BytesRef to = in.readBoolean() ? in.readBytesRef() : null;
+            long docCount = in.readLong();
+            InternalAggregations aggregations = InternalAggregations.readAggregations(in);
+
+            return new Bucket(format, keyed, key, from, to, docCount, aggregations);
         }
 
         @Override
         public void writeTo(StreamOutput out) throws IOException {
-            out.writeOptionalString(key);
+            if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
+                out.writeString(key);
+            } else {
+                out.writeOptionalString(key);
+            }
             out.writeBoolean(from != null);
             if (from != null) {
                 out.writeBytesRef(from);
@@ -122,19 +129,10 @@ public Aggregations getAggregations() {
         public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
             String key = this.key;
             if (keyed) {
-                if (key == null) {
-                    StringBuilder keyBuilder = new StringBuilder();
-                    keyBuilder.append(from == null ? "*" : format.format(from));
-                    keyBuilder.append("-");
-                    keyBuilder.append(to == null ? "*" : format.format(to));
-                    key = keyBuilder.toString();
-                }
                 builder.startObject(key);
             } else {
                 builder.startObject();
-                if (key != null) {
-                    builder.field(CommonFields.KEY.getPreferredName(), key);
-                }
+                builder.field(CommonFields.KEY.getPreferredName(), key);
             }
             if (from != null) {
                 builder.field(CommonFields.FROM.getPreferredName(), getFrom());
@@ -208,10 +206,9 @@ public InternalBinaryRange(StreamInput in) throws IOException {
         super(in);
         format = in.readNamedWriteable(DocValueFormat.class);
         keyed = in.readBoolean();
-        buckets = in.readList(stream -> new Bucket(stream, format, keyed));
+        buckets = in.readList(stream -> Bucket.createFromStream(stream, format, keyed));
     }
 
-
     @Override
     protected void doWriteTo(StreamOutput out) throws IOException {
         out.writeNamedWriteable(format);

diff --git a/...r/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java b/...r/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java
@@ -98,18 +98,16 @@ public String getToAsString() {
         @Override
         public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
             if (isKeyed()) {
-                builder.startObject(key != null ? key : rangeKey(from, to));
+                builder.startObject(key);
             } else {
                 builder.startObject();
-                if (key != null) {
-                    builder.field(CommonFields.KEY.getPreferredName(), key);
-                }
+                builder.field(CommonFields.KEY.getPreferredName(), key);
             }
             if (from != null) {
-                builder.field(CommonFields.FROM.getPreferredName(), getFrom());
+                builder.field(CommonFields.FROM.getPreferredName(), from);
             }
             if (to != null) {
-                builder.field(CommonFields.TO.getPreferredName(), getTo());
+                builder.field(CommonFields.TO.getPreferredName(), to);
             }
             builder.field(CommonFields.DOC_COUNT.getPreferredName(), getDocCount());
             getAggregations().toXContentInternal(builder, params);
@@ -123,10 +121,9 @@ static ParsedBucket fromXContent(final XContentParser parser, final boolean keye
             XContentParser.Token token = parser.currentToken();
             String currentFieldName = parser.currentName();
 
-            String rangeKey = null;
             if (keyed) {
                 ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation);
-                rangeKey = currentFieldName;
+                bucket.key = currentFieldName;
                 ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
             }
 
@@ -150,19 +147,7 @@ static ParsedBucket fromXContent(final XContentParser parser, final boolean keye
                 }
             }
             bucket.setAggregations(new Aggregations(aggregations));
-
-            if (keyed) {
-                if (rangeKey(bucket.from, bucket.to).equals(rangeKey)) {
-                    bucket.key = null;
-                } else {
-                    bucket.key = rangeKey;
-                }
-            }
             return bucket;
         }
-
-        private static String rangeKey(String from, String to) {
-            return (from == null ? "*" : from) + '-' + (to == null ? "*" : to);
-        }
     }
 }
diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/IpRangeIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/IpRangeIT.java
@@ -18,14 +18,8 @@
  */
 package org.elasticsearch.search.aggregations.bucket;
 
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Map;
-import java.util.function.Function;
-
-import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.action.search.SearchPhaseExecutionException;
+import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.cluster.health.ClusterHealthStatus;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.script.MockScriptPlugin;
@@ -35,6 +29,12 @@
 import org.elasticsearch.search.aggregations.bucket.range.Range;
 import org.elasticsearch.test.ESIntegTestCase;
 
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Map;
+import java.util.function.Function;
+
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse;
 import static org.hamcrest.Matchers.containsString;
@@ -91,16 +91,19 @@ public void testSingleValuedField() {
         Range.Bucket bucket1 = range.getBuckets().get(0);
         assertNull(bucket1.getFrom());
         assertEquals("192.168.1.0", bucket1.getTo());
+        assertEquals("*-192.168.1.0", bucket1.getKey());
         assertEquals(0, bucket1.getDocCount());
 
         Range.Bucket bucket2 = range.getBuckets().get(1);
         assertEquals("192.168.1.0", bucket2.getFrom());
         assertEquals("192.168.1.10", bucket2.getTo());
+        assertEquals("192.168.1.0-192.168.1.10", bucket2.getKey());
         assertEquals(1, bucket2.getDocCount());
 
         Range.Bucket bucket3 = range.getBuckets().get(2);
         assertEquals("192.168.1.10", bucket3.getFrom());
         assertNull(bucket3.getTo());
+        assertEquals("192.168.1.10-*", bucket3.getKey());
         assertEquals(2, bucket3.getDocCount());
     }
 
@@ -118,16 +121,19 @@ public void testMultiValuedField() {
         Range.Bucket bucket1 = range.getBuckets().get(0);
         assertNull(bucket1.getFrom());
         assertEquals("192.168.1.0", bucket1.getTo());
+        assertEquals("*-192.168.1.0", bucket1.getKey());
         assertEquals(1, bucket1.getDocCount());
 
         Range.Bucket bucket2 = range.getBuckets().get(1);
         assertEquals("192.168.1.0", bucket2.getFrom());
         assertEquals("192.168.1.10", bucket2.getTo());
+        assertEquals("192.168.1.0-192.168.1.10", bucket2.getKey());
         assertEquals(1, bucket2.getDocCount());
 
         Range.Bucket bucket3 = range.getBuckets().get(2);
         assertEquals("192.168.1.10", bucket3.getFrom());
         assertNull(bucket3.getTo());
+        assertEquals("192.168.1.10-*", bucket3.getKey());
         assertEquals(2, bucket3.getDocCount());
     }
 
@@ -169,16 +175,19 @@ public void testPartiallyUnmapped() {
         Range.Bucket bucket1 = range.getBuckets().get(0);
         assertNull(bucket1.getFrom());
         assertEquals("192.168.1.0", bucket1.getTo());
+        assertEquals("*-192.168.1.0", bucket1.getKey());
         assertEquals(0, bucket1.getDocCount());
 
         Range.Bucket bucket2 = range.getBuckets().get(1);
         assertEquals("192.168.1.0", bucket2.getFrom());
         assertEquals("192.168.1.10", bucket2.getTo());
+        assertEquals("192.168.1.0-192.168.1.10", bucket2.getKey());
         assertEquals(1, bucket2.getDocCount());
 
         Range.Bucket bucket3 = range.getBuckets().get(2);
         assertEquals("192.168.1.10", bucket3.getFrom());
         assertNull(bucket3.getTo());
+        assertEquals("192.168.1.10-*", bucket3.getKey());
         assertEquals(2, bucket3.getDocCount());
     }
 
@@ -196,16 +205,19 @@ public void testUnmapped() {
         Range.Bucket bucket1 = range.getBuckets().get(0);
         assertNull(bucket1.getFrom());
         assertEquals("192.168.1.0", bucket1.getTo());
+        assertEquals("*-192.168.1.0", bucket1.getKey());
         assertEquals(0, bucket1.getDocCount());
 
         Range.Bucket bucket2 = range.getBuckets().get(1);
         assertEquals("192.168.1.0", bucket2.getFrom());
         assertEquals("192.168.1.10", bucket2.getTo());
+        assertEquals("192.168.1.0-192.168.1.10", bucket2.getKey());
         assertEquals(0, bucket2.getDocCount());
 
         Range.Bucket bucket3 = range.getBuckets().get(2);
         assertEquals("192.168.1.10", bucket3.getFrom());
         assertNull(bucket3.getTo());
+        assertEquals("192.168.1.10-*", bucket3.getKey());
         assertEquals(0, bucket3.getDocCount());
     }
 

diff --git a/...est/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java b/...est/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java
@@ -157,4 +157,15 @@ protected InternalBinaryRange mutateInstance(InternalBinaryRange instance) {
         }
         return new InternalBinaryRange(name, format, keyed, buckets, pipelineAggregators, metaData);
     }
+
+    /**
+     * Checks the invariant that bucket keys are always non-null, even if null keys
+     * were originally provided.
+     */
+    public void testKeyGeneration() {
+        InternalBinaryRange range = createTestInstance();
+        for (InternalBinaryRange.Bucket bucket : range.getBuckets()) {
+            assertNotNull(bucket.getKey());
+        }
+    }
 }