From 4fbe1a8819ba5d6f1e795cbb104ac20fd047a446 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Fri, 14 Oct 2016 14:12:09 -0400 Subject: [PATCH 01/10] CONSOLEify _cat/pending_tasks docs Relates to #18160 --- docs/build.gradle | 1 - docs/reference/cat/pending_tasks.asciidoc | 16 +++++++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/docs/build.gradle b/docs/build.gradle index 3286648da96..69c53f847d0 100644 --- a/docs/build.gradle +++ b/docs/build.gradle @@ -93,7 +93,6 @@ buildRestTests.expectedUnconvertedCandidates = [ 'reference/analysis/tokenfilters/stop-tokenfilter.asciidoc', 'reference/analysis/tokenfilters/synonym-tokenfilter.asciidoc', 'reference/analysis/tokenfilters/word-delimiter-tokenfilter.asciidoc', - 'reference/cat/pending_tasks.asciidoc', 'reference/cat/plugins.asciidoc', 'reference/cat/recovery.asciidoc', 'reference/cat/repositories.asciidoc', diff --git a/docs/reference/cat/pending_tasks.asciidoc b/docs/reference/cat/pending_tasks.asciidoc index 5452052669c..fc0c20d5e13 100644 --- a/docs/reference/cat/pending_tasks.asciidoc +++ b/docs/reference/cat/pending_tasks.asciidoc @@ -3,11 +3,18 @@ `pending_tasks` provides the same information as the <> API in a -convenient tabular format. +convenient tabular format. For example: -[source,sh] +[source,js] +-------------------------------------------------- +GET /_cat/pending_tasks?v +-------------------------------------------------- +// CONSOLE + +Might look like: + +[source,js] -------------------------------------------------- -% curl 'localhost:9200/_cat/pending_tasks?v' insertOrder timeInQueue priority source 1685 855ms HIGH update-mapping [foo][t] 1686 843ms HIGH update-mapping [foo][t] @@ -17,3 +24,6 @@ insertOrder timeInQueue priority source 1690 787ms HIGH update-mapping [foo][t] 1691 773ms HIGH update-mapping [foo][t] -------------------------------------------------- +// TESTRESPONSE[s/(\n.+)+/(\\n.+)*/ _cat] +// We can't assert anything about the tasks in progress here because we don't +// know what might be in progress.... From 1b78618106afe21f94a1293c23ad98e2a45001c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Wed, 12 Oct 2016 19:04:37 +0200 Subject: [PATCH 02/10] Use ObjectParser in ScoreSortBuilder and FieldSortBuilder --- .../search/sort/FieldSortBuilder.java | 81 +++++-------------- .../search/sort/ScoreSortBuilder.java | 30 ++----- .../search/sort/FieldSortBuilderTests.java | 3 +- .../search/sort/ScoreSortBuilderTests.java | 3 +- 4 files changed, 29 insertions(+), 88 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java index d519f740870..46326c5611e 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -24,8 +24,9 @@ import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.xcontent.ObjectParser; +import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource.Nested; import org.elasticsearch.index.fielddata.IndexNumericFieldData; @@ -39,7 +40,6 @@ import org.elasticsearch.search.MultiValueMode; import java.io.IOException; import java.util.Objects; -import java.util.Optional; /** * A sort builder to sort based on a document field. @@ -327,67 +327,24 @@ public class FieldSortBuilder extends SortBuilder { * in '{ "foo": { "order" : "asc"} }'. When parsing the inner object, the field name can be passed in via this argument */ public static FieldSortBuilder fromXContent(QueryParseContext context, String fieldName) throws IOException { - XContentParser parser = context.parser(); + return PARSER.parse(context.parser(), new FieldSortBuilder(fieldName), context); + } - Optional nestedFilter = Optional.empty(); - String nestedPath = null; - Object missing = null; - SortOrder order = null; - SortMode sortMode = null; - String unmappedType = null; + private static ObjectParser PARSER = new ObjectParser<>(NAME); - String currentFieldName = null; - XContentParser.Token token; - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentFieldName = parser.currentName(); - } else if (token == XContentParser.Token.START_OBJECT) { - if (context.getParseFieldMatcher().match(currentFieldName, NESTED_FILTER)) { - nestedFilter = context.parseInnerQueryBuilder(); - } else { - throw new ParsingException(parser.getTokenLocation(), "Expected " + NESTED_FILTER.getPreferredName() + " element."); - } - } else if (token.isValue()) { - if (context.getParseFieldMatcher().match(currentFieldName, NESTED_PATH)) { - nestedPath = parser.text(); - } else if (context.getParseFieldMatcher().match(currentFieldName, MISSING)) { - missing = parser.objectText(); - } else if (context.getParseFieldMatcher().match(currentFieldName, ORDER)) { - String sortOrder = parser.text(); - if ("asc".equals(sortOrder)) { - order = SortOrder.ASC; - } else if ("desc".equals(sortOrder)) { - order = SortOrder.DESC; - } else { - throw new ParsingException(parser.getTokenLocation(), "Sort order [{}] not supported.", sortOrder); - } - } else if (context.getParseFieldMatcher().match(currentFieldName, SORT_MODE)) { - sortMode = SortMode.fromString(parser.text()); - } else if (context.getParseFieldMatcher().match(currentFieldName, UNMAPPED_TYPE)) { - unmappedType = parser.text(); - } else { - throw new ParsingException(parser.getTokenLocation(), "Option [{}] not supported.", currentFieldName); - } - } - } - - FieldSortBuilder builder = new FieldSortBuilder(fieldName); - nestedFilter.ifPresent(builder::setNestedFilter); - if (nestedPath != null) { - builder.setNestedPath(nestedPath); - } - if (missing != null) { - builder.missing(missing); - } - if (order != null) { - builder.order(order); - } - if (sortMode != null) { - builder.sortMode(sortMode); - } - if (unmappedType != null) { - builder.unmappedType(unmappedType); - } - return builder; + static { + PARSER.declareField(FieldSortBuilder::missing, p -> p.objectText(), MISSING, ValueType.VALUE); + PARSER.declareString(FieldSortBuilder::setNestedPath , NESTED_PATH); + PARSER.declareString(FieldSortBuilder::unmappedType , UNMAPPED_TYPE); + PARSER.declareField(FieldSortBuilder::order, p -> SortOrder.fromString(p.text()), ORDER_FIELD, ValueType.STRING); + PARSER.declareField(FieldSortBuilder::sortMode, p -> SortMode.fromString(p.text()), SORT_MODE, ValueType.STRING); + PARSER.declareObject(FieldSortBuilder::setNestedFilter, (p, c) -> { + try { + QueryBuilder builder = c.parseInnerQueryBuilder().orElseThrow( + () -> new ParsingException(p.getTokenLocation(), "Expected " + NESTED_FILTER.getPreferredName() + " element.")); + return builder; + } catch (Exception e) { + throw new ParsingException(p.getTokenLocation(), "Expected " + NESTED_FILTER.getPreferredName() + " element.", e); + }}, NESTED_FILTER); } } diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java index 5b9b139e495..39a0576d7d1 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java @@ -21,12 +21,11 @@ package org.elasticsearch.search.sort; import org.apache.lucene.search.SortField; import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.ParseFieldMatcher; -import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.xcontent.ObjectParser; +import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryParseContext; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.DocValueFormat; @@ -86,26 +85,13 @@ public class ScoreSortBuilder extends SortBuilder { * in '{ "foo": { "order" : "asc"} }'. When parsing the inner object, the field name can be passed in via this argument */ public static ScoreSortBuilder fromXContent(QueryParseContext context, String fieldName) throws IOException { - XContentParser parser = context.parser(); - ParseFieldMatcher matcher = context.getParseFieldMatcher(); + return PARSER.apply(context.parser(), context); + } - XContentParser.Token token; - String currentName = parser.currentName(); - ScoreSortBuilder result = new ScoreSortBuilder(); - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentName = parser.currentName(); - } else if (token.isValue()) { - if (matcher.match(currentName, ORDER_FIELD)) { - result.order(SortOrder.fromString(parser.text())); - } else { - throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] failed to parse field [" + currentName + "]"); - } - } else { - throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] unexpected token [" + token + "]"); - } - } - return result; + private static ObjectParser PARSER = new ObjectParser<>(NAME, ScoreSortBuilder::new); + + static { + PARSER.declareField(ScoreSortBuilder::order, p -> SortOrder.fromString(p.text()), ORDER_FIELD, ValueType.STRING); } @Override diff --git a/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java index baaf3ac5d3c..408a7db2029 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java @@ -21,7 +21,6 @@ package org.elasticsearch.search.sort; import org.apache.lucene.search.SortField; import org.elasticsearch.common.ParseFieldMatcher; -import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryParseContext; @@ -140,7 +139,7 @@ public class FieldSortBuilderTests extends AbstractSortTestCase Date: Thu, 13 Oct 2016 14:45:51 +0200 Subject: [PATCH 03/10] Use ObjectParser in ScriptSortBuilder --- .../java/org/elasticsearch/script/Script.java | 10 +++ .../search/sort/FieldSortBuilder.java | 23 ++---- .../search/sort/GeoDistanceSortBuilder.java | 4 +- .../search/sort/ScoreSortBuilder.java | 5 +- .../search/sort/ScriptSortBuilder.java | 76 +++++-------------- .../search/sort/SortBuilder.java | 15 ++++ .../search/sort/AbstractSortTestCase.java | 6 +- .../search/sort/ScriptSortBuilderTests.java | 37 ++++----- 8 files changed, 66 insertions(+), 110 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/script/Script.java b/core/src/main/java/org/elasticsearch/script/Script.java index 05da7138d19..47ecfd60ca1 100644 --- a/core/src/main/java/org/elasticsearch/script/Script.java +++ b/core/src/main/java/org/elasticsearch/script/Script.java @@ -23,6 +23,7 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParseFieldMatcher; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -32,6 +33,7 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.index.query.QueryParseContext; import org.elasticsearch.script.ScriptService.ScriptType; import java.io.IOException; @@ -189,6 +191,14 @@ public final class Script implements ToXContent, Writeable { return parse(parser, parseFieldMatcher, null); } + public static Script parse(XContentParser parser, QueryParseContext context) { + try { + return parse(parser, context.getParseFieldMatcher(), context.getDefaultScriptLanguage()); + } catch (IOException e) { + throw new ParsingException(parser.getTokenLocation(), "Error parsing [" + ScriptField.SCRIPT.getPreferredName() + "] field", e); + } + } + public static Script parse(XContentParser parser, ParseFieldMatcher parseFieldMatcher, @Nullable String lang) throws IOException { XContentParser.Token token = parser.currentToken(); // If the parser hasn't yet been pushed to the first token, do it now diff --git a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java index 46326c5611e..e1585d708cd 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -21,7 +21,6 @@ package org.elasticsearch.search.sort; import org.apache.lucene.search.SortField; import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ObjectParser; @@ -46,10 +45,7 @@ import java.util.Objects; */ public class FieldSortBuilder extends SortBuilder { public static final String NAME = "field_sort"; - public static final ParseField NESTED_PATH = new ParseField("nested_path"); - public static final ParseField NESTED_FILTER = new ParseField("nested_filter"); public static final ParseField MISSING = new ParseField("missing"); - public static final ParseField ORDER = new ParseField("order"); public static final ParseField SORT_MODE = new ParseField("mode"); public static final ParseField UNMAPPED_TYPE = new ParseField("unmapped_type"); @@ -239,10 +235,10 @@ public class FieldSortBuilder extends SortBuilder { builder.field(SORT_MODE.getPreferredName(), sortMode); } if (nestedFilter != null) { - builder.field(NESTED_FILTER.getPreferredName(), nestedFilter, params); + builder.field(NESTED_FILTER_FIELD.getPreferredName(), nestedFilter, params); } if (nestedPath != null) { - builder.field(NESTED_PATH.getPreferredName(), nestedPath); + builder.field(NESTED_PATH_FIELD.getPreferredName(), nestedPath); } builder.endObject(); builder.endObject(); @@ -334,17 +330,10 @@ public class FieldSortBuilder extends SortBuilder { static { PARSER.declareField(FieldSortBuilder::missing, p -> p.objectText(), MISSING, ValueType.VALUE); - PARSER.declareString(FieldSortBuilder::setNestedPath , NESTED_PATH); + PARSER.declareString(FieldSortBuilder::setNestedPath , NESTED_PATH_FIELD); PARSER.declareString(FieldSortBuilder::unmappedType , UNMAPPED_TYPE); - PARSER.declareField(FieldSortBuilder::order, p -> SortOrder.fromString(p.text()), ORDER_FIELD, ValueType.STRING); - PARSER.declareField(FieldSortBuilder::sortMode, p -> SortMode.fromString(p.text()), SORT_MODE, ValueType.STRING); - PARSER.declareObject(FieldSortBuilder::setNestedFilter, (p, c) -> { - try { - QueryBuilder builder = c.parseInnerQueryBuilder().orElseThrow( - () -> new ParsingException(p.getTokenLocation(), "Expected " + NESTED_FILTER.getPreferredName() + " element.")); - return builder; - } catch (Exception e) { - throw new ParsingException(p.getTokenLocation(), "Expected " + NESTED_FILTER.getPreferredName() + " element.", e); - }}, NESTED_FILTER); + PARSER.declareString((b, v) -> b.order(SortOrder.fromString(v)) , ORDER_FIELD); + PARSER.declareString((b, v) -> b.sortMode(SortMode.fromString(v)), SORT_MODE); + PARSER.declareObject(FieldSortBuilder::setNestedFilter, SortBuilder::parseNestedFilter, NESTED_FILTER_FIELD); } } diff --git a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java index c3511ef0ae1..d7b369ac257 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -80,8 +80,6 @@ public class GeoDistanceSortBuilder extends SortBuilder private static final ParseField COERCE_FIELD = new ParseField("coerce", "normalize") .withAllDeprecated("use validation_method instead"); private static final ParseField SORTMODE_FIELD = new ParseField("mode", "sort_mode"); - private static final ParseField NESTED_PATH_FIELD = new ParseField("nested_path"); - private static final ParseField NESTED_FILTER_FIELD = new ParseField("nested_filter"); private final String fieldName; private final List points = new ArrayList<>(); @@ -511,7 +509,7 @@ public class GeoDistanceSortBuilder extends SortBuilder public SortFieldAndFormat build(QueryShardContext context) throws IOException { final boolean indexCreatedBeforeV2_0 = context.indexVersionCreated().before(Version.V_2_0_0); // validation was not available prior to 2.x, so to support bwc percolation queries we only ignore_malformed on 2.x created indexes - List localPoints = new ArrayList(); + List localPoints = new ArrayList<>(); for (GeoPoint geoPoint : this.points) { localPoints.add(new GeoPoint(geoPoint)); } diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java index 39a0576d7d1..52f25301783 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java @@ -20,11 +20,9 @@ package org.elasticsearch.search.sort; import org.apache.lucene.search.SortField; -import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ObjectParser; -import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.query.QueryParseContext; import org.elasticsearch.index.query.QueryShardContext; @@ -39,7 +37,6 @@ import java.util.Objects; public class ScoreSortBuilder extends SortBuilder { public static final String NAME = "_score"; - public static final ParseField ORDER_FIELD = new ParseField("order"); private static final SortFieldAndFormat SORT_SCORE = new SortFieldAndFormat( new SortField(null, SortField.Type.SCORE), DocValueFormat.RAW); private static final SortFieldAndFormat SORT_SCORE_REVERSE = new SortFieldAndFormat( @@ -91,7 +88,7 @@ public class ScoreSortBuilder extends SortBuilder { private static ObjectParser PARSER = new ObjectParser<>(NAME, ScoreSortBuilder::new); static { - PARSER.declareField(ScoreSortBuilder::order, p -> SortOrder.fromString(p.text()), ORDER_FIELD, ValueType.STRING); + PARSER.declareString((builder, order) -> builder.order(SortOrder.fromString(order)), ORDER_FIELD); } @Override diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java index 2a1ca78f76c..bce30e844b5 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java @@ -26,13 +26,12 @@ import org.apache.lucene.search.SortField; import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.BytesRefBuilder; import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.ParseFieldMatcher; -import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.fielddata.FieldData; import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource.Nested; @@ -57,7 +56,8 @@ import java.io.IOException; import java.util.Collections; import java.util.Locale; import java.util.Objects; -import java.util.Optional; + +import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; /** * Script sort builder allows to sort based on a custom script expression. @@ -68,8 +68,6 @@ public class ScriptSortBuilder extends SortBuilder { public static final ParseField TYPE_FIELD = new ParseField("type"); public static final ParseField SCRIPT_FIELD = new ParseField("script"); public static final ParseField SORTMODE_FIELD = new ParseField("mode"); - public static final ParseField NESTED_PATH_FIELD = new ParseField("nested_path"); - public static final ParseField NESTED_FILTER_FIELD = new ParseField("nested_filter"); private final Script script; @@ -216,6 +214,18 @@ public class ScriptSortBuilder extends SortBuilder { return builder; } + private static ConstructingObjectParser PARSER = new ConstructingObjectParser<>(NAME, + a -> new ScriptSortBuilder((Script) a[0], (ScriptSortType) a[1])); + + static { + PARSER.declareField(constructorArg(), Script::parse, ScriptField.SCRIPT, ValueType.OBJECT_OR_STRING); + PARSER.declareField(constructorArg(), p -> ScriptSortType.fromString(p.text()), TYPE_FIELD, ValueType.STRING); + PARSER.declareString((b, v) -> b.order(SortOrder.fromString(v)), ORDER_FIELD); + PARSER.declareString((b, v) -> b.sortMode(SortMode.fromString(v)), SORTMODE_FIELD); + PARSER.declareString(ScriptSortBuilder::setNestedPath , NESTED_PATH_FIELD); + PARSER.declareObject(ScriptSortBuilder::setNestedFilter, SortBuilder::parseNestedFilter, NESTED_FILTER_FIELD); + } + /** * Creates a new {@link ScriptSortBuilder} from the query held by the {@link QueryParseContext} in * {@link org.elasticsearch.common.xcontent.XContent} format. @@ -226,59 +236,7 @@ public class ScriptSortBuilder extends SortBuilder { * in '{ "foo": { "order" : "asc"} }'. When parsing the inner object, the field name can be passed in via this argument */ public static ScriptSortBuilder fromXContent(QueryParseContext context, String elementName) throws IOException { - XContentParser parser = context.parser(); - ParseFieldMatcher parseField = context.getParseFieldMatcher(); - Script script = null; - ScriptSortType type = null; - SortMode sortMode = null; - SortOrder order = null; - Optional nestedFilter = Optional.empty(); - String nestedPath = null; - - XContentParser.Token token; - String currentName = parser.currentName(); - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentName = parser.currentName(); - } else if (token == XContentParser.Token.START_OBJECT) { - if (parseField.match(currentName, ScriptField.SCRIPT)) { - script = Script.parse(parser, parseField, context.getDefaultScriptLanguage()); - } else if (parseField.match(currentName, NESTED_FILTER_FIELD)) { - nestedFilter = context.parseInnerQueryBuilder(); - } else { - throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] failed to parse field [" + currentName + "]"); - } - } else if (token.isValue()) { - if (parseField.match(currentName, ORDER_FIELD)) { - order = SortOrder.fromString(parser.text()); - } else if (parseField.match(currentName, TYPE_FIELD)) { - type = ScriptSortType.fromString(parser.text()); - } else if (parseField.match(currentName, SORTMODE_FIELD)) { - sortMode = SortMode.fromString(parser.text()); - } else if (parseField.match(currentName, NESTED_PATH_FIELD)) { - nestedPath = parser.text(); - } else if (parseField.match(currentName, ScriptField.SCRIPT)) { - script = Script.parse(parser, parseField, context.getDefaultScriptLanguage()); - } else { - throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] failed to parse field [" + currentName + "]"); - } - } else { - throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] unexpected token [" + token + "]"); - } - } - - ScriptSortBuilder result = new ScriptSortBuilder(script, type); - if (order != null) { - result.order(order); - } - if (sortMode != null) { - result.sortMode(sortMode); - } - nestedFilter.ifPresent(result::setNestedFilter); - if (nestedPath != null) { - result.setNestedPath(nestedPath); - } - return result; + return PARSER.apply(context.parser(), context); } diff --git a/core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java index 6f302626280..365c1f9fe4c 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/SortBuilder.java @@ -25,6 +25,7 @@ import org.apache.lucene.search.SortField; import org.apache.lucene.search.join.BitSetProducer; import org.elasticsearch.action.support.ToXContentToBytes; import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.io.stream.NamedWriteable; import org.elasticsearch.common.lucene.search.Queries; import org.elasticsearch.common.xcontent.XContentParser; @@ -49,7 +50,11 @@ import static java.util.Collections.unmodifiableMap; public abstract class SortBuilder> extends ToXContentToBytes implements NamedWriteable { protected SortOrder order = SortOrder.ASC; + + // parse fields common to more than one SortBuilder public static final ParseField ORDER_FIELD = new ParseField("order"); + public static final ParseField NESTED_FILTER_FIELD = new ParseField("nested_filter"); + public static final ParseField NESTED_PATH_FIELD = new ParseField("nested_path"); private static final Map> PARSERS; static { @@ -196,6 +201,16 @@ public abstract class SortBuilder> extends ToXContentTo return nested; } + protected static QueryBuilder parseNestedFilter(XContentParser parser, QueryParseContext context) { + try { + QueryBuilder builder = context.parseInnerQueryBuilder().orElseThrow(() -> new ParsingException(parser.getTokenLocation(), + "Expected " + NESTED_FILTER_FIELD.getPreferredName() + " element.")); + return builder; + } catch (Exception e) { + throw new ParsingException(parser.getTokenLocation(), "Expected " + NESTED_FILTER_FIELD.getPreferredName() + " element.", e); + } + } + @FunctionalInterface private interface Parser> { T fromXContent(QueryParseContext context, String elementName) throws IOException; diff --git a/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java index d9b3aa376fa..ce5fcc470a3 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java @@ -41,10 +41,10 @@ import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.cache.bitset.BitsetFilterCache; import org.elasticsearch.index.fielddata.IndexFieldDataService; import org.elasticsearch.index.mapper.ContentPath; -import org.elasticsearch.index.mapper.MappedFieldType; -import org.elasticsearch.index.mapper.ObjectMapper; import org.elasticsearch.index.mapper.LegacyDoubleFieldMapper.DoubleFieldType; +import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper.BuilderContext; +import org.elasticsearch.index.mapper.ObjectMapper; import org.elasticsearch.index.mapper.ObjectMapper.Nested; import org.elasticsearch.index.query.IdsQueryBuilder; import org.elasticsearch.index.query.MatchAllQueryBuilder; @@ -245,7 +245,7 @@ public abstract class AbstractSortTestCase> extends EST @Override public ObjectMapper getObjectMapper(String name) { BuilderContext context = new BuilderContext(this.getIndexSettings().getSettings(), new ContentPath()); - return (ObjectMapper) new ObjectMapper.Builder<>(name).nested(Nested.newNested(false, false)).build(context); + return new ObjectMapper.Builder<>(name).nested(Nested.newNested(false, false)).build(context); } }; } diff --git a/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java index e96b02c0e2c..82d0de87801 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/sort/ScriptSortBuilderTests.java @@ -22,7 +22,6 @@ package org.elasticsearch.search.sort; import org.apache.lucene.search.SortField; import org.elasticsearch.common.ParseFieldMatcher; -import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryParseContext; @@ -30,8 +29,6 @@ import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService.ScriptType; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.sort.ScriptSortBuilder.ScriptSortType; -import org.junit.Rule; -import org.junit.rules.ExpectedException; import java.io.IOException; import java.util.HashSet; @@ -146,19 +143,14 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase ScriptSortType.fromString(null)); + assertEquals("input string is null", e.getMessage()); } public void testScriptSortTypeIllegalArgument() { - exceptionRule.expect(IllegalArgumentException.class); - exceptionRule.expectMessage("Unknown ScriptSortType [xyz]"); - ScriptSortType.fromString("xyz"); + Exception e = expectThrows(IllegalArgumentException.class, () -> ScriptSortType.fromString("xyz")); + assertEquals("Unknown ScriptSortType [xyz]", e.getMessage()); } public void testParseJson() throws IOException { @@ -226,9 +218,8 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase ScriptSortBuilder.fromXContent(context, null)); + assertEquals("[_script] unknown field [bad_field], parser not found", e.getMessage()); } public void testParseBadFieldNameExceptionsOnStartObject() throws IOException { @@ -240,9 +231,8 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase ScriptSortBuilder.fromXContent(context, null)); + assertEquals("[_script] unknown field [bad_field], parser not found", e.getMessage()); } public void testParseUnexpectedToken() throws IOException { @@ -253,9 +243,8 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase ScriptSortBuilder.fromXContent(context, null)); + assertEquals("[_script] script doesn't support values of type: START_ARRAY", e.getMessage()); } /** @@ -263,9 +252,9 @@ public class ScriptSortBuilderTests extends AbstractSortTestCase builder.sortMode(SortMode.fromString(sortMode))); + assertEquals("script sort of type [string] doesn't support mode [" + sortMode + "]", e.getMessage()); } @Override From f23ae90d92df2a0efd1f12e92a24a5c13e11f006 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 14 Oct 2016 20:27:10 -0400 Subject: [PATCH 04/10] Fix logging configuration for AwsSdkMetrics logger This commit fixes an issue with the configuration for the AwsSdkMetrics logger; the issue is that the logging configuration had used underscores instead of periods for the settings key (the perils of lenient settings parsing). Relates #20313 --- plugins/repository-s3/config/repository-s3/log4j2.properties | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/repository-s3/config/repository-s3/log4j2.properties b/plugins/repository-s3/config/repository-s3/log4j2.properties index 3fee57ce3e2..aa52f0232e0 100644 --- a/plugins/repository-s3/config/repository-s3/log4j2.properties +++ b/plugins/repository-s3/config/repository-s3/log4j2.properties @@ -4,5 +4,5 @@ logger.com_amazonaws.level = warn logger.com_amazonaws_jmx_SdkMBeanRegistrySupport.name = com.amazonaws.jmx.SdkMBeanRegistrySupport logger.com_amazonaws_jmx_SdkMBeanRegistrySupport.level = error -logger_com_amazonaws_metrics_AwsSdkMetrics.name = com.amazonaws.metrics.AwsSdkMetrics -logger_com_amazonaws_metrics_AwsSdkMetrics.level = error +logger.com_amazonaws_metrics_AwsSdkMetrics.name = com.amazonaws.metrics.AwsSdkMetrics +logger.com_amazonaws_metrics_AwsSdkMetrics.level = error From 5a03eb91e664e64041315c597f320b60b556d9a7 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 14 Oct 2016 23:55:15 -0400 Subject: [PATCH 05/10] Add precise logging on unknown or invalid settings Today when logging an unknown or invalid setting, the log message does not contain the source. This means that if we are archiving such a setting, we do not specify where the setting is from (an index, and which index, or a persistent or transient cluster setting). This commit provides such logging for the end user can better understand the consequences of the unknown or invalid setting. Relates #20951 --- .../metadata/MetaDataIndexUpgradeService.java | 9 +++- .../settings/AbstractScopedSettings.java | 32 ++++++++----- .../org/elasticsearch/gateway/Gateway.java | 28 +++++++++-- .../index/IndexSettingsTests.java | 48 ++++++++++++++----- 4 files changed, 88 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java index 27d28417b75..19bf924910e 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java +++ b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java @@ -19,6 +19,8 @@ package org.elasticsearch.cluster.metadata; import com.carrotsearch.hppc.cursors.ObjectCursor; +import org.apache.logging.log4j.message.ParameterizedMessage; +import org.apache.logging.log4j.util.Supplier; import org.apache.lucene.analysis.Analyzer; import org.elasticsearch.Version; import org.elasticsearch.common.component.AbstractComponent; @@ -26,8 +28,8 @@ import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.IndexScopedSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexSettings; -import org.elasticsearch.index.analysis.IndexAnalyzers; import org.elasticsearch.index.analysis.AnalyzerScope; +import org.elasticsearch.index.analysis.IndexAnalyzers; import org.elasticsearch.index.analysis.NamedAnalyzer; import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.similarity.SimilarityService; @@ -161,7 +163,10 @@ public class MetaDataIndexUpgradeService extends AbstractComponent { IndexMetaData archiveBrokenIndexSettings(IndexMetaData indexMetaData) { final Settings settings = indexMetaData.getSettings(); - final Settings upgrade = indexScopedSettings.archiveUnknownOrBrokenSettings(settings); + final Settings upgrade = indexScopedSettings.archiveUnknownOrInvalidSettings( + settings, + e -> logger.warn("{} ignoring unknown index setting: [{}] with value [{}]; archiving", indexMetaData.getIndex(), e.getKey(), e.getValue()), + (e, ex) -> logger.warn((Supplier) () -> new ParameterizedMessage("{} ignoring invalid index setting: [{}] with value [{}]; archiving", indexMetaData.getIndex(), e.getKey(), e.getValue()), ex)); if (upgrade != settings) { return IndexMetaData.builder(indexMetaData).settings(upgrade).build(); } else { diff --git a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 9a392860096..e72f274fd62 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -498,11 +498,21 @@ public abstract class AbstractScopedSettings extends AbstractComponent { } /** - * Archives broken or unknown settings. Any setting that is not recognized or fails - * validation will be archived. This means the setting is prefixed with {@value ARCHIVED_SETTINGS_PREFIX} - * and remains in the settings object. This can be used to detect broken settings via APIs. + * Archives invalid or unknown settings. Any setting that is not recognized or fails validation + * will be archived. This means the setting is prefixed with {@value ARCHIVED_SETTINGS_PREFIX} + * and remains in the settings object. This can be used to detect invalid settings via APIs. + * + * @param settings the {@link Settings} instance to scan for unknown or invalid settings + * @param unknownConsumer callback on unknown settings (consumer receives unknown key and its + * associated value) + * @param invalidConsumer callback on invalid settings (consumer receives invalid key, its + * associated value and an exception) + * @return a {@link Settings} instance with the unknown or invalid settings archived */ - public Settings archiveUnknownOrBrokenSettings(Settings settings) { + public Settings archiveUnknownOrInvalidSettings( + final Settings settings, + final Consumer> unknownConsumer, + final BiConsumer, IllegalArgumentException> invalidConsumer) { Settings.Builder builder = Settings.builder(); boolean changed = false; for (Map.Entry entry : settings.getAsMap().entrySet()) { @@ -516,10 +526,10 @@ public abstract class AbstractScopedSettings extends AbstractComponent { builder.put(entry.getKey(), entry.getValue()); } else { changed = true; - logger.warn("found unknown setting: {} value: {} - archiving", entry.getKey(), entry.getValue()); + unknownConsumer.accept(entry); /* - * We put them back in here such that tools can check from the outside if there are any indices with broken - * settings. The setting can remain there but we want users to be aware that some of their setting are broken and + * We put them back in here such that tools can check from the outside if there are any indices with invalid + * settings. The setting can remain there but we want users to be aware that some of their setting are invalid and * they can research why and what they need to do to replace them. */ builder.put(ARCHIVED_SETTINGS_PREFIX + entry.getKey(), entry.getValue()); @@ -527,12 +537,10 @@ public abstract class AbstractScopedSettings extends AbstractComponent { } } catch (IllegalArgumentException ex) { changed = true; - logger.warn( - (Supplier) () -> new ParameterizedMessage( - "found invalid setting: {} value: {} - archiving", entry.getKey(), entry.getValue()), ex); + invalidConsumer.accept(entry, ex); /* - * We put them back in here such that tools can check from the outside if there are any indices with broken settings. The - * setting can remain there but we want users to be aware that some of their setting are broken and they can research why + * We put them back in here such that tools can check from the outside if there are any indices with invalid settings. The + * setting can remain there but we want users to be aware that some of their setting are invalid and they can research why * and what they need to do to replace them. */ builder.put(ARCHIVED_SETTINGS_PREFIX + entry.getKey(), entry.getValue()); diff --git a/core/src/main/java/org/elasticsearch/gateway/Gateway.java b/core/src/main/java/org/elasticsearch/gateway/Gateway.java index c0ac2bb56e0..3a6bfa7aec1 100644 --- a/core/src/main/java/org/elasticsearch/gateway/Gateway.java +++ b/core/src/main/java/org/elasticsearch/gateway/Gateway.java @@ -21,7 +21,6 @@ package org.elasticsearch.gateway; import com.carrotsearch.hppc.ObjectFloatHashMap; import com.carrotsearch.hppc.cursors.ObjectCursor; - import org.apache.logging.log4j.message.ParameterizedMessage; import org.elasticsearch.action.FailedNodeException; import org.elasticsearch.cluster.ClusterChangedEvent; @@ -38,6 +37,7 @@ import org.elasticsearch.index.Index; import org.elasticsearch.indices.IndicesService; import java.util.Arrays; +import java.util.Map; import java.util.function.Supplier; public class Gateway extends AbstractComponent implements ClusterStateListener { @@ -146,13 +146,35 @@ public class Gateway extends AbstractComponent implements ClusterStateListener { } } final ClusterSettings clusterSettings = clusterService.getClusterSettings(); - metaDataBuilder.persistentSettings(clusterSettings.archiveUnknownOrBrokenSettings(metaDataBuilder.persistentSettings())); - metaDataBuilder.transientSettings(clusterSettings.archiveUnknownOrBrokenSettings(metaDataBuilder.transientSettings())); + metaDataBuilder.persistentSettings( + clusterSettings.archiveUnknownOrInvalidSettings( + metaDataBuilder.persistentSettings(), + e -> logUnknownSetting("persistent", e), + (e, ex) -> logInvalidSetting("persistent", e, ex))); + metaDataBuilder.transientSettings( + clusterSettings.archiveUnknownOrInvalidSettings( + metaDataBuilder.transientSettings(), + e -> logUnknownSetting("transient", e), + (e, ex) -> logInvalidSetting("transient", e, ex))); ClusterState.Builder builder = ClusterState.builder(clusterService.getClusterName()); builder.metaData(metaDataBuilder); listener.onSuccess(builder.build()); } + private void logUnknownSetting(String settingType, Map.Entry e) { + logger.warn("ignoring unknown {} setting: [{}] with value [{}]; archiving", settingType, e.getKey(), e.getValue()); + } + + private void logInvalidSetting(String settingType, Map.Entry e, IllegalArgumentException ex) { + logger.warn( + (org.apache.logging.log4j.util.Supplier) + () -> new ParameterizedMessage("ignoring invalid {} setting: [{}] with value [{}]; archiving", + settingType, + e.getKey(), + e.getValue()), + ex); + } + @Override public void clusterChanged(final ClusterChangedEvent event) { // order is important, first metaState, and then shardsState diff --git a/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java b/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java index 3909354c989..97a6c6abf70 100644 --- a/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java +++ b/core/src/test/java/org/elasticsearch/index/IndexSettingsTests.java @@ -16,11 +16,11 @@ * specific language governing permissions and limitations * under the License. */ + package org.elasticsearch.index; import org.elasticsearch.Version; import org.elasticsearch.cluster.metadata.IndexMetaData; -import org.elasticsearch.cluster.metadata.MetaDataIndexUpgradeService; import org.elasticsearch.common.regex.Regex; import org.elasticsearch.common.settings.IndexScopedSettings; import org.elasticsearch.common.settings.Setting; @@ -29,18 +29,20 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.index.translog.Translog; -import org.elasticsearch.indices.mapper.MapperRegistry; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.VersionUtils; import java.util.Arrays; -import java.util.Collections; import java.util.HashSet; import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.core.StringContains.containsString; +import static org.hamcrest.object.HasToString.hasToString; + public class IndexSettingsTests extends ESTestCase { public void testRunListener() { @@ -348,26 +350,48 @@ public class IndexSettingsTests extends ESTestCase { assertEquals(actualNewTranslogFlushThresholdSize, settings.getFlushThresholdSize()); } - public void testArchiveBrokenIndexSettings() { - Settings settings = IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrBrokenSettings(Settings.EMPTY); + Settings settings = + IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrInvalidSettings( + Settings.EMPTY, + e -> { assert false : "should not have been invoked, no unknown settings"; }, + (e, ex) -> { assert false : "should not have been invoked, no invalid settings"; }); assertSame(settings, Settings.EMPTY); - settings = IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrBrokenSettings(Settings.builder() - .put("index.refresh_interval", "-200").build()); + settings = + IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrInvalidSettings( + Settings.builder().put("index.refresh_interval", "-200").build(), + e -> { assert false : "should not have been invoked, no invalid settings"; }, + (e, ex) -> { + assertThat(e.getKey(), equalTo("index.refresh_interval")); + assertThat(e.getValue(), equalTo("-200")); + assertThat(ex, hasToString(containsString("failed to parse setting [index.refresh_interval] with value [-200]"))); + }); assertEquals("-200", settings.get("archived.index.refresh_interval")); assertNull(settings.get("index.refresh_interval")); Settings prevSettings = settings; // no double archive - settings = IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrBrokenSettings(prevSettings); + settings = + IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrInvalidSettings( + prevSettings, + e -> { assert false : "should not have been invoked, no unknown settings"; }, + (e, ex) -> { assert false : "should not have been invoked, no invalid settings"; }); assertSame(prevSettings, settings); - settings = IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrBrokenSettings(Settings.builder() - .put("index.version.created", Version.CURRENT.id) // private setting - .put("index.unknown", "foo") - .put("index.refresh_interval", "2s").build()); + settings = + IndexScopedSettings.DEFAULT_SCOPED_SETTINGS.archiveUnknownOrInvalidSettings( + Settings.builder() + .put("index.version.created", Version.CURRENT.id) // private setting + .put("index.unknown", "foo") + .put("index.refresh_interval", "2s").build(), + e -> { + assertThat(e.getKey(), equalTo("index.unknown")); + assertThat(e.getValue(), equalTo("foo")); + }, + (e, ex) -> { assert false : "should not have been invoked, no invalid settings"; }); assertEquals("foo", settings.get("archived.index.unknown")); assertEquals(Integer.toString(Version.CURRENT.id), settings.get("index.version.created")); assertEquals("2s", settings.get("index.refresh_interval")); } + } From 5137f44bd66bdd92b8e99310eefa6b3fd935cc9b Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Sat, 15 Oct 2016 14:45:24 +0200 Subject: [PATCH 06/10] [TEST] return empty array if AbstractQueryTestCase#currentTypes is null This is important to allow any test to use RandomQueryBuilder#createQuery() since some of the query builders that are used in this test test the length of the types array and otherwise will thow NPE if the test is not a subclass of AbstractQueryTestCase. --- .../main/java/org/elasticsearch/test/AbstractQueryTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java index 9cb8af9f74b..0b152fcf341 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java @@ -159,7 +159,7 @@ public abstract class AbstractQueryTestCase> } protected static String[] getCurrentTypes() { - return currentTypes; + return currentTypes == null ? Strings.EMPTY_ARRAY : currentTypes; } protected Collection> getPlugins() { From 8d602c6692ebedf12294025e3dc6cfba243798d9 Mon Sep 17 00:00:00 2001 From: Clinton Gormley Date: Sun, 16 Oct 2016 13:25:03 +0200 Subject: [PATCH 07/10] Updated REST test README to include required params when running tests --- .../src/main/resources/rest-api-spec/test/README.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc b/rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc index 612831575e1..db33513962f 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc @@ -10,7 +10,7 @@ Elasticsearch as follows: [source,sh] --------------------- -bin/elasticsearch -E script.inline true -E node.attr.testattr test -E path.repo /tmp -E repositories.url.allowed_urls 'http://snapshot.*' +bin/elasticsearch -Enode.attr.testattr=test -Epath.repo=/tmp -Erepositories.url.allowed_urls='http://snapshot.*' --------------------- ======================================= From cd5777593abf4709b658fd031d4538afc7756424 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Sun, 16 Oct 2016 13:18:09 -0400 Subject: [PATCH 08/10] Fix connection close header handling This commit fixes an issue with the handling of the value "close" on the Connection header in the Netty 4 HTTP implementation. The issue was using the wrong equals method to compare an AsciiString instance and a String instance (they could never be equal). This commit fixes this to use the correct equals method to compare for content equality. Relates #20956 --- .../http/netty4/Netty4HttpChannel.java | 2 +- .../http/netty4/Netty4HttpChannelTests.java | 22 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java index 62443dc541e..5539cc86986 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java @@ -185,7 +185,7 @@ final class Netty4HttpChannel extends AbstractRestChannel { // Determine if the request connection should be closed on completion. private boolean isCloseConnection() { final boolean http10 = isHttp10(); - return HttpHeaderValues.CLOSE.equals(nettyRequest.headers().get(HttpHeaderNames.CONNECTION)) || + return HttpHeaderValues.CLOSE.contentEqualsIgnoreCase(nettyRequest.headers().get(HttpHeaderNames.CONNECTION)) || (http10 && HttpHeaderValues.KEEP_ALIVE.equals(nettyRequest.headers().get(HttpHeaderNames.CONNECTION)) == false); } diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java index 1185419d0dd..d17970fa179 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java @@ -30,10 +30,12 @@ import io.netty.channel.ChannelPipeline; import io.netty.channel.ChannelProgressivePromise; import io.netty.channel.ChannelPromise; import io.netty.channel.EventLoop; +import io.netty.channel.embedded.EmbeddedChannel; import io.netty.handler.codec.http.DefaultFullHttpRequest; import io.netty.handler.codec.http.FullHttpRequest; import io.netty.handler.codec.http.FullHttpResponse; import io.netty.handler.codec.http.HttpHeaderNames; +import io.netty.handler.codec.http.HttpHeaderValues; import io.netty.handler.codec.http.HttpMethod; import io.netty.handler.codec.http.HttpResponse; import io.netty.handler.codec.http.HttpVersion; @@ -212,6 +214,26 @@ public class Netty4HttpChannelTests extends ESTestCase { } } + public void testConnectionClose() throws Exception { + final Settings settings = Settings.builder().build(); + try (Netty4HttpServerTransport httpServerTransport = + new Netty4HttpServerTransport(settings, networkService, bigArrays, threadPool)) { + httpServerTransport.start(); + final FullHttpRequest httpRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); + httpRequest.headers().add(HttpHeaderNames.CONNECTION, HttpHeaderValues.CLOSE); + final EmbeddedChannel embeddedChannel = new EmbeddedChannel(); + final Netty4HttpRequest request = new Netty4HttpRequest(httpRequest, embeddedChannel); + + // send a response, the channel should close + assertTrue(embeddedChannel.isOpen()); + final Netty4HttpChannel channel = + new Netty4HttpChannel(httpServerTransport, request, null, randomBoolean(), threadPool.getThreadContext()); + final TestResponse resp = new TestResponse(); + channel.sendResponse(resp); + assertFalse(embeddedChannel.isOpen()); + } + } + private FullHttpResponse executeRequest(final Settings settings, final String host) { return executeRequest(settings, null, host); } From c1bdaaf80f90a9a14fd90e13de5f5e49a82dc41d Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Sun, 16 Oct 2016 19:49:03 -0400 Subject: [PATCH 09/10] Fix connection keep-alive header handling This commit fixes an issue with the handling of the value "keep-alive" on the Connection header in the Netty 4 HTTP implementation while handling an HTTP 1.0 request. The issue was using the wrong equals method to compare an AsciiString instance and a String instance (they could never be equal). This commit fixes this to use the correct equals method to compare for content equality. --- .../http/netty4/Netty4HttpChannel.java | 2 +- .../http/netty4/Netty4HttpChannelTests.java | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java index 5539cc86986..e9bb129cf6d 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java @@ -186,7 +186,7 @@ final class Netty4HttpChannel extends AbstractRestChannel { private boolean isCloseConnection() { final boolean http10 = isHttp10(); return HttpHeaderValues.CLOSE.contentEqualsIgnoreCase(nettyRequest.headers().get(HttpHeaderNames.CONNECTION)) || - (http10 && HttpHeaderValues.KEEP_ALIVE.equals(nettyRequest.headers().get(HttpHeaderNames.CONNECTION)) == false); + (http10 && !HttpHeaderValues.KEEP_ALIVE.contentEqualsIgnoreCase(nettyRequest.headers().get(HttpHeaderNames.CONNECTION))); } // Create a new {@link HttpResponse} to transmit the response for the netty request. diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java index d17970fa179..a5e0381b3fd 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpChannelTests.java @@ -219,18 +219,29 @@ public class Netty4HttpChannelTests extends ESTestCase { try (Netty4HttpServerTransport httpServerTransport = new Netty4HttpServerTransport(settings, networkService, bigArrays, threadPool)) { httpServerTransport.start(); - final FullHttpRequest httpRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); - httpRequest.headers().add(HttpHeaderNames.CONNECTION, HttpHeaderValues.CLOSE); + final FullHttpRequest httpRequest; + final boolean close = randomBoolean(); + if (randomBoolean()) { + httpRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); + if (close) { + httpRequest.headers().add(HttpHeaderNames.CONNECTION, HttpHeaderValues.CLOSE); + } + } else { + httpRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_0, HttpMethod.GET, "/"); + if (!close) { + httpRequest.headers().add(HttpHeaderNames.CONNECTION, HttpHeaderValues.KEEP_ALIVE); + } + } final EmbeddedChannel embeddedChannel = new EmbeddedChannel(); final Netty4HttpRequest request = new Netty4HttpRequest(httpRequest, embeddedChannel); - // send a response, the channel should close + // send a response, the channel close status should match assertTrue(embeddedChannel.isOpen()); final Netty4HttpChannel channel = new Netty4HttpChannel(httpServerTransport, request, null, randomBoolean(), threadPool.getThreadContext()); final TestResponse resp = new TestResponse(); channel.sendResponse(resp); - assertFalse(embeddedChannel.isOpen()); + assertThat(embeddedChannel.isOpen(), equalTo(!close)); } } From 1755cc08f30c3e313bd907d25446b6bee6d9bc37 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Mon, 17 Oct 2016 09:19:07 +0200 Subject: [PATCH 10/10] REST API parser should fail on duplicate params/paths/methods/parts (#20940) This commit changes the current REST API parser to make it fail and throw an exception when a REST specification file contains a duplicated parameters, or path, or method, or path part. --- .../ClientYamlSuiteRestApiParser.java | 26 +++- ...entYamlSuiteRestApiParserFailingTests.java | 113 +++++++++++++++++- 2 files changed, 128 insertions(+), 11 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParser.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParser.java index 178fb9c5f34..8abcfc35f27 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParser.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParser.java @@ -42,7 +42,11 @@ public class ClientYamlSuiteRestApiParser { if ("methods".equals(parser.currentName())) { parser.nextToken(); while (parser.nextToken() == XContentParser.Token.VALUE_STRING) { - restApi.addMethod(parser.text()); + String method = parser.text(); + if (restApi.getMethods().contains(method)) { + throw new IllegalArgumentException("Found duplicate method [" + method + "]"); + } + restApi.addMethod(method); } } @@ -56,16 +60,24 @@ public class ClientYamlSuiteRestApiParser { if (parser.currentToken() == XContentParser.Token.START_ARRAY && "paths".equals(currentFieldName)) { while (parser.nextToken() == XContentParser.Token.VALUE_STRING) { - restApi.addPath(parser.text()); + String path = parser.text(); + if (restApi.getPaths().contains(path)) { + throw new IllegalArgumentException("Found duplicate path [" + path + "]"); + } + restApi.addPath(path); } } if (parser.currentToken() == XContentParser.Token.START_OBJECT && "parts".equals(currentFieldName)) { while (parser.nextToken() == XContentParser.Token.FIELD_NAME) { - restApi.addPathPart(parser.currentName()); + String part = parser.currentName(); + if (restApi.getPathParts().contains(part)) { + throw new IllegalArgumentException("Found duplicate part [" + part + "]"); + } + restApi.addPathPart(part); parser.nextToken(); if (parser.currentToken() != XContentParser.Token.START_OBJECT) { - throw new IOException("Expected parts field in rest api definition to contain an object"); + throw new IllegalArgumentException("Expected parts field in rest api definition to contain an object"); } parser.skipChildren(); } @@ -73,10 +85,14 @@ public class ClientYamlSuiteRestApiParser { if (parser.currentToken() == XContentParser.Token.START_OBJECT && "params".equals(currentFieldName)) { while (parser.nextToken() == XContentParser.Token.FIELD_NAME) { + String param = parser.currentName(); + if (restApi.getParams().contains(param)) { + throw new IllegalArgumentException("Found duplicate param [" + param + "]"); + } restApi.addParam(parser.currentName()); parser.nextToken(); if (parser.currentToken() != XContentParser.Token.START_OBJECT) { - throw new IOException("Expected params field in rest api definition to contain an object"); + throw new IllegalArgumentException("Expected params field in rest api definition to contain an object"); } parser.skipChildren(); } diff --git a/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParserFailingTests.java b/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParserFailingTests.java index f2219816462..02995e84bd9 100644 --- a/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParserFailingTests.java +++ b/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParserFailingTests.java @@ -32,6 +32,109 @@ import static org.hamcrest.Matchers.containsString; * stream */ public class ClientYamlSuiteRestApiParserFailingTests extends ESTestCase { + + public void testDuplicateMethods() throws Exception { + parseAndExpectFailure("{\n" + + " \"ping\": {" + + " \"documentation\": \"http://www.elasticsearch.org/guide/\"," + + " \"methods\": [\"PUT\", \"PUT\"]," + + " \"url\": {" + + " \"path\": \"/\"," + + " \"paths\": [\"/\"]," + + " \"parts\": {" + + " }," + + " \"params\": {" + + " \"type\" : \"boolean\",\n" + + " \"description\" : \"Whether specified concrete indices should be ignored when unavailable (missing or closed)\"" + + " }" + + " }," + + " \"body\": null" + + " }" + + "}", "Found duplicate method [PUT]"); + } + + public void testDuplicatePaths() throws Exception { + parseAndExpectFailure("{\n" + + " \"ping\": {" + + " \"documentation\": \"http://www.elasticsearch.org/guide/\"," + + " \"methods\": [\"PUT\"]," + + " \"url\": {" + + " \"path\": \"/pingone\"," + + " \"paths\": [\"/pingone\", \"/pingtwo\", \"/pingtwo\"]," + + " \"parts\": {" + + " }," + + " \"params\": {" + + " \"type\" : \"boolean\",\n" + + " \"description\" : \"Whether specified concrete indices should be ignored when unavailable (missing or closed)\"" + + " }" + + " }," + + " \"body\": null" + + " }" + + "}", "Found duplicate path [/pingtwo]"); + } + + public void testDuplicateParts() throws Exception { + parseAndExpectFailure("{\n" + + " \"ping\": {" + + " \"documentation\": \"http://www.elasticsearch.org/guide/\"," + + " \"methods\": [\"PUT\"]," + + " \"url\": {" + + " \"path\": \"/\"," + + " \"paths\": [\"/\"]," + + " \"parts\": {" + + " \"index\": {" + + " \"type\" : \"string\",\n" + + " \"description\" : \"index part\"\n" + + " }," + + " \"type\": {" + + " \"type\" : \"string\",\n" + + " \"description\" : \"type part\"\n" + + " }," + + " \"index\": {" + + " \"type\" : \"string\",\n" + + " \"description\" : \"index parameter part\"\n" + + " }" + + " }," + + " \"params\": {" + + " \"type\" : \"boolean\",\n" + + " \"description\" : \"Whether specified concrete indices should be ignored when unavailable (missing or closed)\"" + + " }" + + " }," + + " \"body\": null" + + " }" + + "}", "Found duplicate part [index]"); + } + + public void testDuplicateParams() throws Exception { + parseAndExpectFailure("{\n" + + " \"ping\": {" + + " \"documentation\": \"http://www.elasticsearch.org/guide/\"," + + " \"methods\": [\"PUT\"]," + + " \"url\": {" + + " \"path\": \"/\"," + + " \"paths\": [\"/\"]," + + " \"parts\": {" + + " }," + + " \"params\": {" + + " \"timeout\": {" + + " \"type\" : \"string\",\n" + + " \"description\" : \"timeout parameter\"\n" + + " }," + + " \"refresh\": {" + + " \"type\" : \"string\",\n" + + " \"description\" : \"refresh parameter\"\n" + + " }," + + " \"timeout\": {" + + " \"type\" : \"string\",\n" + + " \"description\" : \"timeout parameter again\"\n" + + " }" + + " }" + + " }," + + " \"body\": null" + + " }" + + "}", "Found duplicate param [timeout]"); + } + public void testBrokenSpecShouldThrowUsefulExceptionWhenParsingFailsOnParams() throws Exception { parseAndExpectFailure(BROKEN_SPEC_PARAMS, "Expected params field in rest api definition to contain an object"); } @@ -42,12 +145,10 @@ public class ClientYamlSuiteRestApiParserFailingTests extends ESTestCase { private void parseAndExpectFailure(String brokenJson, String expectedErrorMessage) throws Exception { XContentParser parser = JsonXContent.jsonXContent.createParser(brokenJson); - try { - new ClientYamlSuiteRestApiParser().parse("location", parser); - fail("Expected to fail parsing but did not happen"); - } catch (IOException e) { - assertThat(e.getMessage(), containsString(expectedErrorMessage)); - } + ClientYamlSuiteRestApiParser restApiParser = new ClientYamlSuiteRestApiParser(); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> restApiParser.parse("location", parser)); + assertThat(e.getMessage(), containsString(expectedErrorMessage)); } // see params section is broken, an inside param is missing