Skip to content

Commit 82fe67b

Browse files
tlrxtlrx
authored
[Rollup] Remove builders from TermsGroupConfig (#32507)
While working on adding the Create Rollup Job API to the high level REST client (#29827), I noticed that the configuration objects like TermsGroupConfig rely on the Builder pattern in order to create or parse instances. These builders are doing some validation but the same validation could be done within the constructor itself or on the server side when appropriate. This commit removes the builder for TermsGroupConfig, removes some other methods that I consider not really usefull once the TermsGroupConfig object will be exposed in the high level REST client. It also simplifies the parsing logic. Related to #29827
1 parent f2f33f3 commit 82fe67b

File tree

9 files changed

+88
-111
lines changed

9 files changed

+88
-111
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/GroupConfig.java

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
import java.util.Objects;
2424
import java.util.Set;
2525

26+
import static java.util.Arrays.asList;
27+
2628
/**
2729
* The configuration object for the groups section in the rollup config.
2830
* Basically just a wrapper for histo/date histo/terms objects
@@ -50,7 +52,7 @@ public class GroupConfig implements Writeable, ToXContentObject {
5052
static {
5153
PARSER.declareObject(GroupConfig.Builder::setDateHisto, (p,c) -> DateHistoGroupConfig.PARSER.apply(p,c).build(), DATE_HISTO);
5254
PARSER.declareObject(GroupConfig.Builder::setHisto, (p,c) -> HistoGroupConfig.PARSER.apply(p,c).build(), HISTO);
53-
PARSER.declareObject(GroupConfig.Builder::setTerms, (p,c) -> TermsGroupConfig.PARSER.apply(p,c).build(), TERMS);
55+
PARSER.declareObject(GroupConfig.Builder::setTerms, (p,c) -> TermsGroupConfig.fromXContent(p), TERMS);
5456
}
5557

5658
private GroupConfig(DateHistoGroupConfig dateHisto, @Nullable HistoGroupConfig histo, @Nullable TermsGroupConfig terms) {
@@ -84,7 +86,7 @@ public Set<String> getAllFields() {
8486
fields.addAll(histo.getAllFields());
8587
}
8688
if (terms != null) {
87-
fields.addAll(terms.getAllFields());
89+
fields.addAll(asList(terms.getFields()));
8890
}
8991
return fields;
9092
}
@@ -100,7 +102,6 @@ public void validateMappings(Map<String, Map<String, FieldCapabilities>> fieldCa
100102
}
101103
}
102104

103-
104105
@Override
105106
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
106107
builder.startObject();
@@ -113,9 +114,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
113114
builder.endObject();
114115
}
115116
if (terms != null) {
116-
builder.startObject(TERMS.getPreferredName());
117-
terms.toXContent(builder, params);
118-
builder.endObject();
117+
builder.field(TERMS.getPreferredName(), terms);
119118
}
120119
builder.endObject();
121120
return builder;
@@ -194,4 +193,4 @@ public GroupConfig build() {
194193
return new GroupConfig(dateHisto, histo, terms);
195194
}
196195
}
197-
}
196+
}

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/TermsGroupConfig.java

Lines changed: 34 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,10 @@
1212
import org.elasticsearch.common.io.stream.StreamInput;
1313
import org.elasticsearch.common.io.stream.StreamOutput;
1414
import org.elasticsearch.common.io.stream.Writeable;
15-
import org.elasticsearch.common.xcontent.ObjectParser;
16-
import org.elasticsearch.common.xcontent.ToXContentFragment;
15+
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
16+
import org.elasticsearch.common.xcontent.ToXContentObject;
1717
import org.elasticsearch.common.xcontent.XContentBuilder;
18+
import org.elasticsearch.common.xcontent.XContentParser;
1819
import org.elasticsearch.index.mapper.KeywordFieldMapper;
1920
import org.elasticsearch.index.mapper.TextFieldMapper;
2021
import org.elasticsearch.search.aggregations.bucket.composite.CompositeValuesSourceBuilder;
@@ -24,13 +25,13 @@
2425

2526
import java.io.IOException;
2627
import java.util.Arrays;
27-
import java.util.Collections;
2828
import java.util.HashMap;
2929
import java.util.List;
3030
import java.util.Map;
31-
import java.util.Set;
3231
import java.util.stream.Collectors;
3332

33+
import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
34+
3435
/**
3536
* The configuration object for the histograms in the rollup config
3637
*
@@ -42,27 +43,39 @@
4243
* ]
4344
* }
4445
*/
45-
public class TermsGroupConfig implements Writeable, ToXContentFragment {
46-
private static final String NAME = "term_group_config";
47-
public static final ObjectParser<TermsGroupConfig.Builder, Void> PARSER = new ObjectParser<>(NAME, TermsGroupConfig.Builder::new);
46+
public class TermsGroupConfig implements Writeable, ToXContentObject {
47+
48+
private static final String NAME = "terms";
49+
private static final String FIELDS = "fields";
4850

49-
private static final ParseField FIELDS = new ParseField("fields");
5051
private static final List<String> FLOAT_TYPES = Arrays.asList("half_float", "float", "double", "scaled_float");
5152
private static final List<String> NATURAL_TYPES = Arrays.asList("byte", "short", "integer", "long");
52-
private final String[] fields;
5353

54+
private static final ConstructingObjectParser<TermsGroupConfig, Void> PARSER;
5455
static {
55-
PARSER.declareStringArray(TermsGroupConfig.Builder::setFields, FIELDS);
56+
PARSER = new ConstructingObjectParser<>(NAME, args -> {
57+
@SuppressWarnings("unchecked") List<String> fields = (List<String>) args[0];
58+
return new TermsGroupConfig(fields != null ? fields.toArray(new String[fields.size()]) : null);
59+
});
60+
PARSER.declareStringArray(constructorArg(), new ParseField(FIELDS));
5661
}
5762

58-
private TermsGroupConfig(String[] fields) {
63+
private final String[] fields;
64+
65+
public TermsGroupConfig(final String... fields) {
66+
if (fields == null || fields.length == 0) {
67+
throw new IllegalArgumentException("Fields must have at least one value");
68+
}
5969
this.fields = fields;
6070
}
6171

6272
TermsGroupConfig(StreamInput in) throws IOException {
6373
fields = in.readStringArray();
6474
}
6575

76+
/**
77+
* @return the names of the fields. Never {@code null}.
78+
*/
6679
public String[] getFields() {
6780
return fields;
6881
}
@@ -72,10 +85,6 @@ public String[] getFields() {
7285
* set of date histograms. Used by the rollup indexer to iterate over historical data
7386
*/
7487
public List<CompositeValuesSourceBuilder<?>> toBuilders() {
75-
if (fields.length == 0) {
76-
return Collections.emptyList();
77-
}
78-
7988
return Arrays.stream(fields).map(f -> {
8089
TermsValuesSourceBuilder vsBuilder
8190
= new TermsValuesSourceBuilder(RollupField.formatIndexerAggName(f, TermsAggregationBuilder.NAME));
@@ -94,14 +103,6 @@ public Map<String, Object> toAggCap() {
94103
return map;
95104
}
96105

97-
public Map<String, Object> getMetadata() {
98-
return Collections.emptyMap();
99-
}
100-
101-
public Set<String> getAllFields() {
102-
return Arrays.stream(fields).collect(Collectors.toSet());
103-
}
104-
105106
public void validateMappings(Map<String, Map<String, FieldCapabilities>> fieldCapsResponse,
106107
ActionRequestValidationException validationException) {
107108

@@ -138,8 +139,11 @@ public void validateMappings(Map<String, Map<String, FieldCapabilities>> fieldCa
138139

139140
@Override
140141
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
141-
builder.field(FIELDS.getPreferredName(), fields);
142-
return builder;
142+
builder.startObject();
143+
{
144+
builder.field(FIELDS, fields);
145+
}
146+
return builder.endObject();
143147
}
144148

145149
@Override
@@ -148,18 +152,15 @@ public void writeTo(StreamOutput out) throws IOException {
148152
}
149153

150154
@Override
151-
public boolean equals(Object other) {
155+
public boolean equals(final Object other) {
152156
if (this == other) {
153157
return true;
154158
}
155-
156159
if (other == null || getClass() != other.getClass()) {
157160
return false;
158161
}
159-
160-
TermsGroupConfig that = (TermsGroupConfig) other;
161-
162-
return Arrays.equals(this.fields, that.fields);
162+
final TermsGroupConfig that = (TermsGroupConfig) other;
163+
return Arrays.equals(fields, that.fields);
163164
}
164165

165166
@Override
@@ -172,23 +173,7 @@ public String toString() {
172173
return Strings.toString(this, true, true);
173174
}
174175

175-
public static class Builder {
176-
private List<String> fields;
177-
178-
public List<String> getFields() {
179-
return fields;
180-
}
181-
182-
public TermsGroupConfig.Builder setFields(List<String> fields) {
183-
this.fields = fields;
184-
return this;
185-
}
186-
187-
public TermsGroupConfig build() {
188-
if (fields == null || fields.isEmpty()) {
189-
throw new IllegalArgumentException("Parameter [" + FIELDS + "] must have at least one value.");
190-
}
191-
return new TermsGroupConfig(fields.toArray(new String[0]));
192-
}
176+
public static TermsGroupConfig fromXContent(final XContentParser parser) throws IOException {
177+
return PARSER.parse(parser, null);
193178
}
194179
}

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/ConfigTestHelpers.java

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,13 @@
1717

1818
import java.util.ArrayList;
1919
import java.util.List;
20+
import java.util.Random;
2021
import java.util.stream.Collectors;
2122
import java.util.stream.IntStream;
2223

24+
import static com.carrotsearch.randomizedtesting.generators.RandomNumbers.randomIntBetween;
25+
import static com.carrotsearch.randomizedtesting.generators.RandomStrings.randomAsciiAlphanumOfLengthBetween;
26+
2327
public class ConfigTestHelpers {
2428

2529
public static RollupJobConfig.Builder getRollupJob(String jobId) {
@@ -49,7 +53,7 @@ public static GroupConfig.Builder getGroupConfig() {
4953
groupBuilder.setHisto(getHisto().build());
5054
}
5155
if (ESTestCase.randomBoolean()) {
52-
groupBuilder.setTerms(getTerms().build());
56+
groupBuilder.setTerms(randomTermsGroupConfig(ESTestCase.random()));
5357
}
5458
return groupBuilder;
5559
}
@@ -105,12 +109,6 @@ public static HistoGroupConfig.Builder getHisto() {
105109
return histoBuilder;
106110
}
107111

108-
public static TermsGroupConfig.Builder getTerms() {
109-
TermsGroupConfig.Builder builder = new TermsGroupConfig.Builder();
110-
builder.setFields(getFields());
111-
return builder;
112-
}
113-
114112
public static List<String> getFields() {
115113
return IntStream.range(0, ESTestCase.randomIntBetween(1, 10))
116114
.mapToObj(n -> ESTestCase.randomAlphaOfLengthBetween(5, 10))
@@ -126,4 +124,21 @@ public static String getCronString() {
126124
" ?" + //day of week
127125
" " + (ESTestCase.randomBoolean() ? "*" : String.valueOf(ESTestCase.randomIntBetween(1970, 2199))); //year
128126
}
127+
128+
public static TermsGroupConfig randomTermsGroupConfig(final Random random) {
129+
return new TermsGroupConfig(randomFields(random));
130+
}
131+
132+
private static String[] randomFields(final Random random) {
133+
final int numFields = randomIntBetween(random, 1, 10);
134+
final String[] fields = new String[numFields];
135+
for (int i = 0; i < numFields; i++) {
136+
fields[i] = randomField(random);
137+
}
138+
return fields;
139+
}
140+
141+
private static String randomField(final Random random) {
142+
return randomAsciiAlphanumOfLengthBetween(random, 5, 10);
143+
}
129144
}

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/job/TermsGroupConfigSerializingTests.java

Lines changed: 13 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -11,27 +11,23 @@
1111
import org.elasticsearch.common.xcontent.XContentParser;
1212
import org.elasticsearch.search.aggregations.bucket.composite.CompositeValuesSourceBuilder;
1313
import org.elasticsearch.test.AbstractSerializingTestCase;
14-
import org.elasticsearch.xpack.core.rollup.ConfigTestHelpers;
1514

1615
import java.io.IOException;
17-
import java.util.Arrays;
1816
import java.util.Collections;
1917
import java.util.HashMap;
2018
import java.util.List;
2119
import java.util.Map;
2220

21+
import static org.elasticsearch.xpack.core.rollup.ConfigTestHelpers.randomTermsGroupConfig;
2322
import static org.hamcrest.Matchers.equalTo;
2423
import static org.mockito.Mockito.mock;
2524
import static org.mockito.Mockito.when;
2625

2726
public class TermsGroupConfigSerializingTests extends AbstractSerializingTestCase<TermsGroupConfig> {
2827

29-
private static final List<String> FLOAT_TYPES = Arrays.asList("half_float", "float", "double", "scaled_float");
30-
private static final List<String> NATURAL_TYPES = Arrays.asList("byte", "short", "integer", "long");
31-
3228
@Override
3329
protected TermsGroupConfig doParseInstance(XContentParser parser) throws IOException {
34-
return TermsGroupConfig.PARSER.apply(parser, null).build();
30+
return TermsGroupConfig.fromXContent(parser);
3531
}
3632

3733
@Override
@@ -41,58 +37,48 @@ protected Writeable.Reader<TermsGroupConfig> instanceReader() {
4137

4238
@Override
4339
protected TermsGroupConfig createTestInstance() {
44-
return ConfigTestHelpers.getTerms().build();
40+
return randomTermsGroupConfig(random());
4541
}
4642

47-
public void testValidateNoMapping() throws IOException {
43+
public void testValidateNoMapping() {
4844
ActionRequestValidationException e = new ActionRequestValidationException();
4945
Map<String, Map<String, FieldCapabilities>> responseMap = new HashMap<>();
5046

51-
TermsGroupConfig config = new TermsGroupConfig.Builder()
52-
.setFields(Collections.singletonList("my_field"))
53-
.build();
47+
TermsGroupConfig config = new TermsGroupConfig("my_field");
5448
config.validateMappings(responseMap, e);
5549
assertThat(e.validationErrors().get(0), equalTo("Could not find a [numeric] or [keyword/text] field with name " +
5650
"[my_field] in any of the indices matching the index pattern."));
5751
}
5852

59-
public void testValidateNomatchingField() throws IOException {
60-
53+
public void testValidateNomatchingField() {
6154
ActionRequestValidationException e = new ActionRequestValidationException();
6255
Map<String, Map<String, FieldCapabilities>> responseMap = new HashMap<>();
6356

6457
// Have to mock fieldcaps because the ctor's aren't public...
6558
FieldCapabilities fieldCaps = mock(FieldCapabilities.class);
6659
responseMap.put("some_other_field", Collections.singletonMap("keyword", fieldCaps));
6760

68-
TermsGroupConfig config = new TermsGroupConfig.Builder()
69-
.setFields(Collections.singletonList("my_field"))
70-
.build();
61+
TermsGroupConfig config = new TermsGroupConfig("my_field");
7162
config.validateMappings(responseMap, e);
7263
assertThat(e.validationErrors().get(0), equalTo("Could not find a [numeric] or [keyword/text] field with name " +
7364
"[my_field] in any of the indices matching the index pattern."));
7465
}
7566

76-
public void testValidateFieldWrongType() throws IOException {
77-
67+
public void testValidateFieldWrongType() {
7868
ActionRequestValidationException e = new ActionRequestValidationException();
7969
Map<String, Map<String, FieldCapabilities>> responseMap = new HashMap<>();
8070

8171
// Have to mock fieldcaps because the ctor's aren't public...
8272
FieldCapabilities fieldCaps = mock(FieldCapabilities.class);
8373
responseMap.put("my_field", Collections.singletonMap("geo_point", fieldCaps));
8474

85-
TermsGroupConfig config = new TermsGroupConfig.Builder()
86-
.setFields(Collections.singletonList("my_field"))
87-
.build();
75+
TermsGroupConfig config = new TermsGroupConfig("my_field");
8876
config.validateMappings(responseMap, e);
8977
assertThat(e.validationErrors().get(0), equalTo("The field referenced by a terms group must be a [numeric] or " +
9078
"[keyword/text] type, but found [geo_point] for field [my_field]"));
9179
}
9280

93-
public void testValidateFieldMatchingNotAggregatable() throws IOException {
94-
95-
81+
public void testValidateFieldMatchingNotAggregatable() {
9682
ActionRequestValidationException e = new ActionRequestValidationException();
9783
Map<String, Map<String, FieldCapabilities>> responseMap = new HashMap<>();
9884

@@ -101,14 +87,12 @@ public void testValidateFieldMatchingNotAggregatable() throws IOException {
10187
when(fieldCaps.isAggregatable()).thenReturn(false);
10288
responseMap.put("my_field", Collections.singletonMap(getRandomType(), fieldCaps));
10389

104-
TermsGroupConfig config = new TermsGroupConfig.Builder()
105-
.setFields(Collections.singletonList("my_field"))
106-
.build();
90+
TermsGroupConfig config = new TermsGroupConfig("my_field");
10791
config.validateMappings(responseMap, e);
10892
assertThat(e.validationErrors().get(0), equalTo("The field [my_field] must be aggregatable across all indices, but is not."));
10993
}
11094

111-
public void testValidateMatchingField() throws IOException {
95+
public void testValidateMatchingField() {
11296
ActionRequestValidationException e = new ActionRequestValidationException();
11397
Map<String, Map<String, FieldCapabilities>> responseMap = new HashMap<>();
11498
String type = getRandomType();
@@ -118,9 +102,7 @@ public void testValidateMatchingField() throws IOException {
118102
when(fieldCaps.isAggregatable()).thenReturn(true);
119103
responseMap.put("my_field", Collections.singletonMap(type, fieldCaps));
120104

121-
TermsGroupConfig config = new TermsGroupConfig.Builder()
122-
.setFields(Collections.singletonList("my_field"))
123-
.build();
105+
TermsGroupConfig config = new TermsGroupConfig("my_field");
124106
config.validateMappings(responseMap, e);
125107
if (e.validationErrors().size() != 0) {
126108
fail(e.getMessage());

0 commit comments

Comments
 (0)