diff --git a/docs/reference/aggregations/bucket/iprange-aggregation.asciidoc b/docs/reference/aggregations/bucket/iprange-aggregation.asciidoc index c8bd896b037fa..0aabd3a71ed30 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 9a07e6f8ad580..bc845928a0460 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 afa3be702cc33..c647a38f7e06e 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 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/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 ccfe3f3670f91..79b1cd6cc0d09 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 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 index 17450b31450d5..ffa7f97015105 100644 --- 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/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java index 68785fc387661..00d0c7e050908 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java +++ b/server/src/test/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()); + } + } }