From 638a719370eba388c1c2c901639c4c454faa3346 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Thu, 24 May 2018 08:55:14 -0700 Subject: [PATCH] Ensure that ip_range aggregations always return bucket keys. (#30701) --- .../bucket/iprange-aggregation.asciidoc | 2 + .../test/search.aggregation/40_range.yml | 28 +++++---- .../bucket/range/InternalBinaryRange.java | 59 +++++++++---------- .../bucket/range/ParsedBinaryRange.java | 25 ++------ .../search/aggregations/bucket/IpRangeIT.java | 26 +++++--- .../range/InternalBinaryRangeTests.java | 11 ++++ 6 files changed, 80 insertions(+), 71 deletions(-) diff --git a/docs/reference/aggregations/bucket/iprange-aggregation.asciidoc b/docs/reference/aggregations/bucket/iprange-aggregation.asciidoc index c8bd896b037..0aabd3a71ed 100644 --- 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 index 9a07e6f8ad5..bc845928a04 100644 --- 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/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java index afa3be702cc..c647a38f7e0 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java +++ b/server/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 final class InternalBinaryRange 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 final class InternalBinaryRange 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 final class InternalBinaryRange 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/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java index ccfe3f3670f..79b1cd6cc0d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java @@ -98,18 +98,16 @@ public class ParsedBinaryRange extends ParsedMultiBucketAggregation