Skip to content

Commit 638a719

Browse files
jtibshiranijtibshirani
authored
Ensure that ip_range aggregations always return bucket keys. (#30701)
1 parent 8bbfdf1 commit 638a719

File tree

6 files changed

+80
-71
lines changed

6 files changed

+80
-71
lines changed

‎docs/reference/aggregations/bucket/iprange-aggregation.asciidoc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,12 @@ Response:
3737
"ip_ranges": {
3838
"buckets" : [
3939
{
40+
"key": "*-10.0.0.5",
4041
"to": "10.0.0.5",
4142
"doc_count": 10
4243
},
4344
{
45+
"key": "10.0.0.5-*",
4446
"from": "10.0.0.5",
4547
"doc_count": 260
4648
}

‎rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -144,25 +144,18 @@ setup:
144144

145145
- length: { aggregations.ip_range.buckets: 3 }
146146

147-
# ip_range does not automatically add keys to buckets, see #21045
148-
# - match: { aggregations.ip_range.buckets.0.key: "*-192.168.0.0" }
149-
150147
- is_false: aggregations.ip_range.buckets.0.from
151148

152149
- match: { aggregations.ip_range.buckets.0.to: "192.168.0.0" }
153150

154151
- match: { aggregations.ip_range.buckets.0.doc_count: 1 }
155152

156-
# - match: { aggregations.ip_range.buckets.1.key: "192.168.0.0-192.169.0.0" }
157-
158153
- match: { aggregations.ip_range.buckets.1.from: "192.168.0.0" }
159154

160155
- match: { aggregations.ip_range.buckets.1.to: "192.169.0.0" }
161156

162157
- match: { aggregations.ip_range.buckets.1.doc_count: 2 }
163158

164-
# - match: { aggregations.ip_range.buckets.2.key: "192.169.0.0-*" }
165-
166159
- match: { aggregations.ip_range.buckets.2.from: "192.169.0.0" }
167160

168161
- is_false: aggregations.ip_range.buckets.2.to
@@ -177,24 +170,18 @@ setup:
177170

178171
- length: { aggregations.ip_range.buckets: 3 }
179172

180-
# - match: { aggregations.ip_range.buckets.0.key: "*-192.168.0.0" }
181-
182173
- is_false: aggregations.ip_range.buckets.0.from
183174

184175
- match: { aggregations.ip_range.buckets.0.to: "192.168.0.0" }
185176

186177
- match: { aggregations.ip_range.buckets.0.doc_count: 1 }
187178

188-
# - match: { aggregations.ip_range.buckets.1.key: "192.168.0.0-192.169.0.0" }
189-
190179
- match: { aggregations.ip_range.buckets.1.from: "192.168.0.0" }
191180

192181
- match: { aggregations.ip_range.buckets.1.to: "192.169.0.0" }
193182

194183
- match: { aggregations.ip_range.buckets.1.doc_count: 2 }
195184

196-
# - match: { aggregations.ip_range.buckets.2.key: "192.169.0.0-*" }
197-
198185
- match: { aggregations.ip_range.buckets.2.from: "192.169.0.0" }
199186

200187
- is_false: aggregations.ip_range.buckets.2.to
@@ -223,6 +210,21 @@ setup:
223210

224211
- match: { aggregations.ip_range.buckets.1.doc_count: 2 }
225212

213+
---
214+
"IP Range Key Generation":
215+
- skip:
216+
version: " - 6.99.99"
217+
reason: "Before 7.0.0, ip_range did not always generate bucket keys (see #21045)."
218+
219+
- do:
220+
search:
221+
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" } ] } } } }
222+
223+
- length: { aggregations.ip_range.buckets: 3 }
224+
- match: { aggregations.ip_range.buckets.0.key: "*-192.168.0.0" }
225+
- match: { aggregations.ip_range.buckets.1.key: "192.168.0.0-192.169.0.0" }
226+
- match: { aggregations.ip_range.buckets.2.key: "192.169.0.0-*" }
227+
226228
---
227229
"Date range":
228230
- do:

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java

Lines changed: 28 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
package org.elasticsearch.search.aggregations.bucket.range;
2121

2222
import org.apache.lucene.util.BytesRef;
23+
import org.elasticsearch.Version;
2324
import org.elasticsearch.common.io.stream.StreamInput;
2425
import org.elasticsearch.common.io.stream.StreamOutput;
2526
import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -57,35 +58,41 @@ public Bucket(DocValueFormat format, boolean keyed, String key, BytesRef from, B
5758
long docCount, InternalAggregations aggregations) {
5859
this.format = format;
5960
this.keyed = keyed;
60-
this.key = key;
61+
this.key = key != null ? key : generateKey(from, to, format);
6162
this.from = from;
6263
this.to = to;
6364
this.docCount = docCount;
6465
this.aggregations = aggregations;
6566
}
6667

67-
// for serialization
68-
private Bucket(StreamInput in, DocValueFormat format, boolean keyed) throws IOException {
69-
this.format = format;
70-
this.keyed = keyed;
71-
key = in.readOptionalString();
72-
if (in.readBoolean()) {
73-
from = in.readBytesRef();
74-
} else {
75-
from = null;
76-
}
77-
if (in.readBoolean()) {
78-
to = in.readBytesRef();
79-
} else {
80-
to = null;
81-
}
82-
docCount = in.readLong();
83-
aggregations = InternalAggregations.readAggregations(in);
68+
private static String generateKey(BytesRef from, BytesRef to, DocValueFormat format) {
69+
StringBuilder builder = new StringBuilder()
70+
.append(from == null ? "*" : format.format(from))
71+
.append("-")
72+
.append(to == null ? "*" : format.format(to));
73+
return builder.toString();
74+
}
75+
76+
private static Bucket createFromStream(StreamInput in, DocValueFormat format, boolean keyed) throws IOException {
77+
String key = in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)
78+
? in.readString()
79+
: in.readOptionalString();
80+
81+
BytesRef from = in.readBoolean() ? in.readBytesRef() : null;
82+
BytesRef to = in.readBoolean() ? in.readBytesRef() : null;
83+
long docCount = in.readLong();
84+
InternalAggregations aggregations = InternalAggregations.readAggregations(in);
85+
86+
return new Bucket(format, keyed, key, from, to, docCount, aggregations);
8487
}
8588

8689
@Override
8790
public void writeTo(StreamOutput out) throws IOException {
88-
out.writeOptionalString(key);
91+
if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
92+
out.writeString(key);
93+
} else {
94+
out.writeOptionalString(key);
95+
}
8996
out.writeBoolean(from != null);
9097
if (from != null) {
9198
out.writeBytesRef(from);
@@ -122,19 +129,10 @@ public Aggregations getAggregations() {
122129
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
123130
String key = this.key;
124131
if (keyed) {
125-
if (key == null) {
126-
StringBuilder keyBuilder = new StringBuilder();
127-
keyBuilder.append(from == null ? "*" : format.format(from));
128-
keyBuilder.append("-");
129-
keyBuilder.append(to == null ? "*" : format.format(to));
130-
key = keyBuilder.toString();
131-
}
132132
builder.startObject(key);
133133
} else {
134134
builder.startObject();
135-
if (key != null) {
136-
builder.field(CommonFields.KEY.getPreferredName(), key);
137-
}
135+
builder.field(CommonFields.KEY.getPreferredName(), key);
138136
}
139137
if (from != null) {
140138
builder.field(CommonFields.FROM.getPreferredName(), getFrom());
@@ -208,10 +206,9 @@ public InternalBinaryRange(StreamInput in) throws IOException {
208206
super(in);
209207
format = in.readNamedWriteable(DocValueFormat.class);
210208
keyed = in.readBoolean();
211-
buckets = in.readList(stream -> new Bucket(stream, format, keyed));
209+
buckets = in.readList(stream -> Bucket.createFromStream(stream, format, keyed));
212210
}
213211

214-
215212
@Override
216213
protected void doWriteTo(StreamOutput out) throws IOException {
217214
out.writeNamedWriteable(format);

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -98,18 +98,16 @@ public String getToAsString() {
9898
@Override
9999
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
100100
if (isKeyed()) {
101-
builder.startObject(key != null ? key : rangeKey(from, to));
101+
builder.startObject(key);
102102
} else {
103103
builder.startObject();
104-
if (key != null) {
105-
builder.field(CommonFields.KEY.getPreferredName(), key);
106-
}
104+
builder.field(CommonFields.KEY.getPreferredName(), key);
107105
}
108106
if (from != null) {
109-
builder.field(CommonFields.FROM.getPreferredName(), getFrom());
107+
builder.field(CommonFields.FROM.getPreferredName(), from);
110108
}
111109
if (to != null) {
112-
builder.field(CommonFields.TO.getPreferredName(), getTo());
110+
builder.field(CommonFields.TO.getPreferredName(), to);
113111
}
114112
builder.field(CommonFields.DOC_COUNT.getPreferredName(), getDocCount());
115113
getAggregations().toXContentInternal(builder, params);
@@ -123,10 +121,9 @@ static ParsedBucket fromXContent(final XContentParser parser, final boolean keye
123121
XContentParser.Token token = parser.currentToken();
124122
String currentFieldName = parser.currentName();
125123

126-
String rangeKey = null;
127124
if (keyed) {
128125
ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation);
129-
rangeKey = currentFieldName;
126+
bucket.key = currentFieldName;
130127
ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
131128
}
132129

@@ -150,19 +147,7 @@ static ParsedBucket fromXContent(final XContentParser parser, final boolean keye
150147
}
151148
}
152149
bucket.setAggregations(new Aggregations(aggregations));
153-
154-
if (keyed) {
155-
if (rangeKey(bucket.from, bucket.to).equals(rangeKey)) {
156-
bucket.key = null;
157-
} else {
158-
bucket.key = rangeKey;
159-
}
160-
}
161150
return bucket;
162151
}
163-
164-
private static String rangeKey(String from, String to) {
165-
return (from == null ? "*" : from) + '-' + (to == null ? "*" : to);
166-
}
167152
}
168153
}

‎server/src/test/java/org/elasticsearch/search/aggregations/bucket/IpRangeIT.java

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,8 @@
1818
*/
1919
package org.elasticsearch.search.aggregations.bucket;
2020

21-
import java.util.Arrays;
22-
import java.util.Collection;
23-
import java.util.Collections;
24-
import java.util.Map;
25-
import java.util.function.Function;
26-
27-
import org.elasticsearch.action.search.SearchResponse;
2821
import org.elasticsearch.action.search.SearchPhaseExecutionException;
22+
import org.elasticsearch.action.search.SearchResponse;
2923
import org.elasticsearch.cluster.health.ClusterHealthStatus;
3024
import org.elasticsearch.plugins.Plugin;
3125
import org.elasticsearch.script.MockScriptPlugin;
@@ -35,6 +29,12 @@
3529
import org.elasticsearch.search.aggregations.bucket.range.Range;
3630
import org.elasticsearch.test.ESIntegTestCase;
3731

32+
import java.util.Arrays;
33+
import java.util.Collection;
34+
import java.util.Collections;
35+
import java.util.Map;
36+
import java.util.function.Function;
37+
3838
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
3939
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse;
4040
import static org.hamcrest.Matchers.containsString;
@@ -91,16 +91,19 @@ public void testSingleValuedField() {
9191
Range.Bucket bucket1 = range.getBuckets().get(0);
9292
assertNull(bucket1.getFrom());
9393
assertEquals("192.168.1.0", bucket1.getTo());
94+
assertEquals("*-192.168.1.0", bucket1.getKey());
9495
assertEquals(0, bucket1.getDocCount());
9596

9697
Range.Bucket bucket2 = range.getBuckets().get(1);
9798
assertEquals("192.168.1.0", bucket2.getFrom());
9899
assertEquals("192.168.1.10", bucket2.getTo());
100+
assertEquals("192.168.1.0-192.168.1.10", bucket2.getKey());
99101
assertEquals(1, bucket2.getDocCount());
100102

101103
Range.Bucket bucket3 = range.getBuckets().get(2);
102104
assertEquals("192.168.1.10", bucket3.getFrom());
103105
assertNull(bucket3.getTo());
106+
assertEquals("192.168.1.10-*", bucket3.getKey());
104107
assertEquals(2, bucket3.getDocCount());
105108
}
106109

@@ -118,16 +121,19 @@ public void testMultiValuedField() {
118121
Range.Bucket bucket1 = range.getBuckets().get(0);
119122
assertNull(bucket1.getFrom());
120123
assertEquals("192.168.1.0", bucket1.getTo());
124+
assertEquals("*-192.168.1.0", bucket1.getKey());
121125
assertEquals(1, bucket1.getDocCount());
122126

123127
Range.Bucket bucket2 = range.getBuckets().get(1);
124128
assertEquals("192.168.1.0", bucket2.getFrom());
125129
assertEquals("192.168.1.10", bucket2.getTo());
130+
assertEquals("192.168.1.0-192.168.1.10", bucket2.getKey());
126131
assertEquals(1, bucket2.getDocCount());
127132

128133
Range.Bucket bucket3 = range.getBuckets().get(2);
129134
assertEquals("192.168.1.10", bucket3.getFrom());
130135
assertNull(bucket3.getTo());
136+
assertEquals("192.168.1.10-*", bucket3.getKey());
131137
assertEquals(2, bucket3.getDocCount());
132138
}
133139

@@ -169,16 +175,19 @@ public void testPartiallyUnmapped() {
169175
Range.Bucket bucket1 = range.getBuckets().get(0);
170176
assertNull(bucket1.getFrom());
171177
assertEquals("192.168.1.0", bucket1.getTo());
178+
assertEquals("*-192.168.1.0", bucket1.getKey());
172179
assertEquals(0, bucket1.getDocCount());
173180

174181
Range.Bucket bucket2 = range.getBuckets().get(1);
175182
assertEquals("192.168.1.0", bucket2.getFrom());
176183
assertEquals("192.168.1.10", bucket2.getTo());
184+
assertEquals("192.168.1.0-192.168.1.10", bucket2.getKey());
177185
assertEquals(1, bucket2.getDocCount());
178186

179187
Range.Bucket bucket3 = range.getBuckets().get(2);
180188
assertEquals("192.168.1.10", bucket3.getFrom());
181189
assertNull(bucket3.getTo());
190+
assertEquals("192.168.1.10-*", bucket3.getKey());
182191
assertEquals(2, bucket3.getDocCount());
183192
}
184193

@@ -196,16 +205,19 @@ public void testUnmapped() {
196205
Range.Bucket bucket1 = range.getBuckets().get(0);
197206
assertNull(bucket1.getFrom());
198207
assertEquals("192.168.1.0", bucket1.getTo());
208+
assertEquals("*-192.168.1.0", bucket1.getKey());
199209
assertEquals(0, bucket1.getDocCount());
200210

201211
Range.Bucket bucket2 = range.getBuckets().get(1);
202212
assertEquals("192.168.1.0", bucket2.getFrom());
203213
assertEquals("192.168.1.10", bucket2.getTo());
214+
assertEquals("192.168.1.0-192.168.1.10", bucket2.getKey());
204215
assertEquals(0, bucket2.getDocCount());
205216

206217
Range.Bucket bucket3 = range.getBuckets().get(2);
207218
assertEquals("192.168.1.10", bucket3.getFrom());
208219
assertNull(bucket3.getTo());
220+
assertEquals("192.168.1.10-*", bucket3.getKey());
209221
assertEquals(0, bucket3.getDocCount());
210222
}
211223

‎server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,4 +157,15 @@ protected InternalBinaryRange mutateInstance(InternalBinaryRange instance) {
157157
}
158158
return new InternalBinaryRange(name, format, keyed, buckets, pipelineAggregators, metaData);
159159
}
160+
161+
/**
162+
* Checks the invariant that bucket keys are always non-null, even if null keys
163+
* were originally provided.
164+
*/
165+
public void testKeyGeneration() {
166+
InternalBinaryRange range = createTestInstance();
167+
for (InternalBinaryRange.Bucket bucket : range.getBuckets()) {
168+
assertNotNull(bucket.getKey());
169+
}
170+
}
160171
}

0 commit comments

Comments
 (0)