Skip to content

Commit 49c9c76

Browse files
benwtrentbenwtrent
committed
ML refactor DatafeedsConfig(Update) so defaults are not populated in queries or aggs (elastic#38822)
* ML refactor DatafeedsConfig(Update) so defaults are not populated in queries or aggs * Addressing pr feedback
1 parent 12f6963 commit 49c9c76

File tree

8 files changed

+372
-157
lines changed

8 files changed

+372
-157
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java

Lines changed: 69 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
*/
66
package org.elasticsearch.xpack.core.ml.datafeed;
77

8+
import org.apache.logging.log4j.LogManager;
9+
import org.apache.logging.log4j.Logger;
810
import org.elasticsearch.ElasticsearchException;
911
import org.elasticsearch.Version;
1012
import org.elasticsearch.cluster.AbstractDiffable;
@@ -18,11 +20,9 @@
1820
import org.elasticsearch.common.xcontent.ObjectParser;
1921
import org.elasticsearch.common.xcontent.ToXContentObject;
2022
import org.elasticsearch.common.xcontent.XContentBuilder;
21-
import org.elasticsearch.common.xcontent.XContentParseException;
2223
import org.elasticsearch.common.xcontent.XContentParser;
23-
import org.elasticsearch.index.query.AbstractQueryBuilder;
24+
import org.elasticsearch.index.query.MatchAllQueryBuilder;
2425
import org.elasticsearch.index.query.QueryBuilder;
25-
import org.elasticsearch.index.query.QueryBuilders;
2626
import org.elasticsearch.search.aggregations.AggregationBuilder;
2727
import org.elasticsearch.search.aggregations.AggregatorFactories;
2828
import org.elasticsearch.search.aggregations.metrics.MaxAggregationBuilder;
@@ -71,19 +71,12 @@ public class DatafeedConfig extends AbstractDiffable<DatafeedConfig> implements
7171
(objectMap, id, warnings) -> {
7272
try {
7373
return QUERY_TRANSFORMER.fromMap(objectMap, warnings);
74-
} catch (IOException | XContentParseException exception) {
74+
} catch (Exception exception) {
7575
// Certain thrown exceptions wrap up the real Illegal argument making it hard to determine cause for the user
7676
if (exception.getCause() instanceof IllegalArgumentException) {
77-
throw ExceptionsHelper.badRequestException(
78-
Messages.getMessage(Messages.DATAFEED_CONFIG_QUERY_BAD_FORMAT,
79-
id,
80-
exception.getCause().getMessage()),
81-
exception.getCause());
82-
} else {
83-
throw ExceptionsHelper.badRequestException(
84-
Messages.getMessage(Messages.DATAFEED_CONFIG_QUERY_BAD_FORMAT, exception, id),
85-
exception);
77+
exception = (Exception)exception.getCause();
8678
}
79+
throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.DATAFEED_CONFIG_QUERY_BAD_FORMAT, id), exception);
8780
}
8881
};
8982

@@ -92,22 +85,17 @@ public class DatafeedConfig extends AbstractDiffable<DatafeedConfig> implements
9285
(objectMap, id, warnings) -> {
9386
try {
9487
return AGG_TRANSFORMER.fromMap(objectMap, warnings);
95-
} catch (IOException | XContentParseException exception) {
88+
} catch (Exception exception) {
9689
// Certain thrown exceptions wrap up the real Illegal argument making it hard to determine cause for the user
9790
if (exception.getCause() instanceof IllegalArgumentException) {
98-
throw ExceptionsHelper.badRequestException(
99-
Messages.getMessage(Messages.DATAFEED_CONFIG_AGG_BAD_FORMAT,
100-
id,
101-
exception.getCause().getMessage()),
102-
exception.getCause());
103-
} else {
104-
throw ExceptionsHelper.badRequestException(
105-
Messages.getMessage(Messages.DATAFEED_CONFIG_AGG_BAD_FORMAT, exception.getMessage(), id),
106-
exception);
91+
exception = (Exception)exception.getCause();
10792
}
93+
throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.DATAFEED_CONFIG_AGG_BAD_FORMAT, id), exception);
10894
}
10995
};
11096

97+
private static final Logger logger = LogManager.getLogger(DatafeedConfig.class);
98+
11199
// Used for QueryPage
112100
public static final ParseField RESULTS_FIELD = new ParseField("datafeeds");
113101
public static String TYPE = "datafeed";
@@ -164,15 +152,11 @@ private static ObjectParser<Builder, Void> createParser(boolean ignoreUnknownFie
164152
builder.setQueryDelay(TimeValue.parseTimeValue(val, QUERY_DELAY.getPreferredName())), QUERY_DELAY);
165153
parser.declareString((builder, val) ->
166154
builder.setFrequency(TimeValue.parseTimeValue(val, FREQUENCY.getPreferredName())), FREQUENCY);
167-
if (ignoreUnknownFields) {
168-
parser.declareObject(Builder::setQuery, (p, c) -> p.mapOrdered(), QUERY);
169-
parser.declareObject(Builder::setAggregations, (p, c) -> p.mapOrdered(), AGGREGATIONS);
170-
parser.declareObject(Builder::setAggregations, (p, c) -> p.mapOrdered(), AGGS);
171-
} else {
172-
parser.declareObject(Builder::setParsedQuery, (p, c) -> AbstractQueryBuilder.parseInnerQueryBuilder(p), QUERY);
173-
parser.declareObject(Builder::setParsedAggregations, (p, c) -> AggregatorFactories.parseAggregators(p), AGGREGATIONS);
174-
parser.declareObject(Builder::setParsedAggregations, (p, c) -> AggregatorFactories.parseAggregators(p), AGGS);
175-
}
155+
parser.declareObject((builder, val) -> builder.setQuery(val, ignoreUnknownFields), (p, c) -> p.mapOrdered(), QUERY);
156+
parser.declareObject((builder, val) -> builder.setAggregationsSafe(val, ignoreUnknownFields), (p, c) -> p.mapOrdered(),
157+
AGGREGATIONS);
158+
parser.declareObject((builder, val) -> builder.setAggregationsSafe(val, ignoreUnknownFields), (p, c) -> p.mapOrdered(),
159+
AGGS);
176160
parser.declareObject(Builder::setScriptFields, (p, c) -> {
177161
List<SearchSourceBuilder.ScriptField> parsedScriptFields = new ArrayList<>();
178162
while (p.nextToken() != XContentParser.Token.END_OBJECT) {
@@ -582,19 +566,15 @@ public static class Builder {
582566
private TimeValue queryDelay;
583567
private TimeValue frequency;
584568
private List<String> indices = Collections.emptyList();
585-
private Map<String, Object> query;
569+
private Map<String, Object> query = Collections.singletonMap(MatchAllQueryBuilder.NAME, Collections.emptyMap());
586570
private Map<String, Object> aggregations;
587571
private List<SearchSourceBuilder.ScriptField> scriptFields;
588572
private Integer scrollSize = DEFAULT_SCROLL_SIZE;
589573
private ChunkingConfig chunkingConfig;
590574
private Map<String, String> headers = Collections.emptyMap();
591575
private DelayedDataCheckConfig delayedDataCheckConfig = DelayedDataCheckConfig.defaultDelayedDataCheckConfig();
592576

593-
public Builder() {
594-
try {
595-
this.query = QUERY_TRANSFORMER.toMap(QueryBuilders.matchAllQuery());
596-
} catch (IOException ex) { /*Should never happen*/ }
597-
}
577+
public Builder() { }
598578

599579
public Builder(String id, String jobId) {
600580
this();
@@ -647,48 +627,74 @@ public void setFrequency(TimeValue frequency) {
647627
this.frequency = frequency;
648628
}
649629

650-
public void setParsedQuery(QueryBuilder query) {
630+
public void setQuery(Map<String, Object> query) {
631+
setQuery(query, true);
632+
}
633+
634+
public void setQuery(Map<String, Object> query, boolean lenient) {
635+
this.query = ExceptionsHelper.requireNonNull(query, QUERY.getPreferredName());
651636
try {
652-
setQuery(QUERY_TRANSFORMER.toMap(ExceptionsHelper.requireNonNull(query, QUERY.getPreferredName())));
653-
} catch (IOException | XContentParseException exception) {
654-
if (exception.getCause() instanceof IllegalArgumentException) {
655-
// Certain thrown exceptions wrap up the real Illegal argument making it hard to determine cause for the user
656-
throw ExceptionsHelper.badRequestException(
657-
Messages.getMessage(Messages.DATAFEED_CONFIG_QUERY_BAD_FORMAT,
658-
id,
659-
exception.getCause().getMessage()),
660-
exception.getCause());
637+
QUERY_TRANSFORMER.fromMap(query);
638+
} catch(Exception ex) {
639+
String msg = Messages.getMessage(Messages.DATAFEED_CONFIG_QUERY_BAD_FORMAT, id);
640+
641+
if (ex.getCause() instanceof IllegalArgumentException) {
642+
ex = (Exception)ex.getCause();
643+
}
644+
645+
if (lenient) {
646+
logger.warn(msg, ex);
661647
} else {
662-
throw ExceptionsHelper.badRequestException(
663-
Messages.getMessage(Messages.DATAFEED_CONFIG_QUERY_BAD_FORMAT, id, exception.getMessage()), exception);
648+
throw ExceptionsHelper.badRequestException(msg, ex);
664649
}
665650
}
666651
}
667652

668-
public void setQuery(Map<String, Object> query) {
669-
this.query = ExceptionsHelper.requireNonNull(query, QUERY.getPreferredName());
670-
}
671-
653+
// Kept for easier testing
672654
public void setParsedAggregations(AggregatorFactories.Builder aggregations) {
673655
try {
674656
setAggregations(AGG_TRANSFORMER.toMap(aggregations));
675-
} catch (IOException | XContentParseException exception) {
657+
} catch (Exception exception) {
676658
// Certain thrown exceptions wrap up the real Illegal argument making it hard to determine cause for the user
677659
if (exception.getCause() instanceof IllegalArgumentException) {
678-
throw ExceptionsHelper.badRequestException(
679-
Messages.getMessage(Messages.DATAFEED_CONFIG_AGG_BAD_FORMAT,
680-
id,
681-
exception.getCause().getMessage()),
682-
exception.getCause());
683-
} else {
684-
throw ExceptionsHelper.badRequestException(
685-
Messages.getMessage(Messages.DATAFEED_CONFIG_AGG_BAD_FORMAT, id, exception.getMessage()), exception);
660+
exception = (Exception)exception.getCause();
686661
}
662+
throw ExceptionsHelper.badRequestException(
663+
Messages.getMessage(Messages.DATAFEED_CONFIG_AGG_BAD_FORMAT, id), exception);
687664
}
688665
}
689666

667+
private void setAggregationsSafe(Map<String, Object> aggregations, boolean lenient) {
668+
if (this.aggregations != null) {
669+
throw ExceptionsHelper.badRequestException("Found two aggregation definitions: [aggs] and [aggregations]");
670+
}
671+
setAggregations(aggregations, lenient);
672+
}
673+
690674
void setAggregations(Map<String, Object> aggregations) {
675+
setAggregations(aggregations, true);
676+
}
677+
678+
void setAggregations(Map<String, Object> aggregations, boolean lenient) {
691679
this.aggregations = aggregations;
680+
try {
681+
if (aggregations != null && aggregations.isEmpty()) {
682+
throw new Exception("[aggregations] are empty");
683+
}
684+
AGG_TRANSFORMER.fromMap(aggregations);
685+
} catch (Exception ex) {
686+
String msg = Messages.getMessage(Messages.DATAFEED_CONFIG_AGG_BAD_FORMAT, id);
687+
688+
if (ex.getCause() instanceof IllegalArgumentException) {
689+
ex = (Exception)ex.getCause();
690+
}
691+
692+
if (lenient) {
693+
logger.warn(msg, ex);
694+
} else {
695+
throw ExceptionsHelper.badRequestException(msg, ex);
696+
}
697+
}
692698
}
693699

694700
public void setScriptFields(List<SearchSourceBuilder.ScriptField> scriptFields) {

0 commit comments

Comments
 (0)