diff --git a/libs/x-content/src/main/java/org/elasticsearch/xcontent/AbstractObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/xcontent/AbstractObjectParser.java index 8ec9e135acad1..b72f14613d28b 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/xcontent/AbstractObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/xcontent/AbstractObjectParser.java @@ -156,7 +156,7 @@ public void declareField(BiConsumer consumer, CheckedFunction void declareObject(BiConsumer consumer, ContextParser objectParser, ParseField field) { - declareField(consumer, (p, c) -> objectParser.parse(p, c), field, ValueType.OBJECT); + declareField(consumer, objectParser, field, ValueType.OBJECT); } /** @@ -240,7 +240,7 @@ public void declareBoolean(BiConsumer consumer, ParseField field } public void declareObjectArray(BiConsumer> consumer, ContextParser objectParser, ParseField field) { - declareFieldArray(consumer, (p, c) -> objectParser.parse(p, c), field, ValueType.OBJECT_ARRAY); + declareFieldArray(consumer, objectParser, field, ValueType.OBJECT_ARRAY); } /** diff --git a/libs/x-content/src/main/java/org/elasticsearch/xcontent/ConstructingObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/xcontent/ConstructingObjectParser.java index 3a0f3b7056b4a..e8ed7cd1faf4b 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/xcontent/ConstructingObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/xcontent/ConstructingObjectParser.java @@ -426,7 +426,7 @@ private class Target { /** * The parse context that is used for this invocation. Stored here so that it can be passed to the {@link #builder}. */ - private Context context; + private final Context context; /** * How many of the constructor parameters have we collected? We keep track of this so we don't have to count the diff --git a/libs/x-content/src/main/java/org/elasticsearch/xcontent/ObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/xcontent/ObjectParser.java index f9aafcfb51f5a..6eb8bef86ebe7 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/xcontent/ObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/xcontent/ObjectParser.java @@ -170,7 +170,7 @@ public ObjectParser(String name) { * @param valueSupplier A supplier that creates a new Value instance. Used when the parser is used as an inner object parser. */ public ObjectParser(String name, @Nullable Supplier valueSupplier) { - this(name, errorOnUnknown(), c -> valueSupplier.get()); + this(name, errorOnUnknown(), wrapValueSupplier(valueSupplier)); } /** @@ -192,7 +192,13 @@ public static ObjectParser fromBuilder(String n * @param valueSupplier a supplier that creates a new Value instance used when the parser is used as an inner object parser. */ public ObjectParser(String name, boolean ignoreUnknownFields, @Nullable Supplier valueSupplier) { - this(name, ignoreUnknownFields ? ignoreUnknown() : errorOnUnknown(), c -> valueSupplier.get()); + this(name, ignoreUnknownFields ? ignoreUnknown() : errorOnUnknown(), wrapValueSupplier(valueSupplier)); + } + + private static Function wrapValueSupplier(@Nullable Supplier valueSupplier) { + return valueSupplier == null ? c -> { + throw new NullPointerException(); + } : c -> valueSupplier.get(); } /** @@ -202,7 +208,7 @@ public ObjectParser(String name, boolean ignoreUnknownFields, @Nullable Supplier * @param valueSupplier a supplier that creates a new Value instance used when the parser is used as an inner object parser. */ public ObjectParser(String name, UnknownFieldConsumer unknownFieldConsumer, @Nullable Supplier valueSupplier) { - this(name, consumeUnknownField(unknownFieldConsumer), c -> valueSupplier.get()); + this(name, consumeUnknownField(unknownFieldConsumer), wrapValueSupplier(valueSupplier)); } /** @@ -219,7 +225,7 @@ public ObjectParser( BiConsumer unknownFieldConsumer, @Nullable Supplier valueSupplier ) { - this(name, unknownIsNamedXContent(categoryClass, unknownFieldConsumer), c -> valueSupplier.get()); + this(name, unknownIsNamedXContent(categoryClass, unknownFieldConsumer), wrapValueSupplier(valueSupplier)); } /** @@ -232,7 +238,9 @@ private ObjectParser(String name, UnknownFieldParser unknownFiel @Nullable Function valueBuilder) { this.name = name; this.unknownFieldParser = unknownFieldParser; - this.valueBuilder = valueBuilder; + this.valueBuilder = valueBuilder == null ? c -> { + throw new NullPointerException("valueBuilder is not set"); + } : valueBuilder; } /** @@ -244,9 +252,6 @@ private ObjectParser(String name, UnknownFieldParser unknownFiel */ @Override public Value parse(XContentParser parser, Context context) throws IOException { - if (valueBuilder == null) { - throw new NullPointerException("valueBuilder is not set"); - } return parse(parser, valueBuilder.apply(context), context); } @@ -260,60 +265,52 @@ public Value parse(XContentParser parser, Context context) throws IOException { */ public Value parse(XContentParser parser, Value value, Context context) throws IOException { XContentParser.Token token; - if (parser.currentToken() == XContentParser.Token.START_OBJECT) { - token = parser.currentToken(); - } else { + if (parser.currentToken() != XContentParser.Token.START_OBJECT) { token = parser.nextToken(); if (token != XContentParser.Token.START_OBJECT) { - throw new XContentParseException(parser.getTokenLocation(), "[" + name + "] Expected START_OBJECT but was: " + token); + throwExpectedStartObject(parser, token); } } FieldParser fieldParser = null; String currentFieldName = null; XContentLocation currentPosition = null; - List requiredFields = new ArrayList<>(this.requiredFieldSets); - List> exclusiveFields = new ArrayList<>(); - for (int i = 0; i < this.exclusiveFieldSets.size(); i++) { - exclusiveFields.add(new ArrayList<>()); + final List requiredFields = this.requiredFieldSets.isEmpty() ? null : new ArrayList<>(this.requiredFieldSets); + final List> exclusiveFields; + if (exclusiveFieldSets.isEmpty()) { + exclusiveFields = null; + } else { + exclusiveFields = new ArrayList<>(); + for (int i = 0; i < this.exclusiveFieldSets.size(); i++) { + exclusiveFields.add(new ArrayList<>()); + } } + final Map parsers = fieldParserMap.getOrDefault(parser.getRestApiVersion(), Collections.emptyMap()); while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); currentPosition = parser.getTokenLocation(); - fieldParser = fieldParserMap.getOrDefault(parser.getRestApiVersion(), Collections.emptyMap()) - .get(currentFieldName); + fieldParser = parsers.get(currentFieldName); } else { if (currentFieldName == null) { - throw new XContentParseException(parser.getTokenLocation(), "[" + name + "] no field found"); + throwNoFieldFound(parser); } if (fieldParser == null) { unknownFieldParser.acceptUnknownField(this, currentFieldName, currentPosition, parser, value, context); } else { fieldParser.assertSupports(name, parser, currentFieldName); - // Check to see if this field is a required field, if it is we can - // remove the entry as the requirement is satisfied - Iterator iter = requiredFields.iterator(); - while (iter.hasNext()) { - String[] requriedFields = iter.next(); - for (String field : requriedFields) { - if (field.equals(currentFieldName)) { - iter.remove(); - break; - } - } + if (requiredFields != null) { + // Check to see if this field is a required field, if it is we can + // remove the entry as the requirement is satisfied + maybeMarkRequiredField(currentFieldName, requiredFields); } - // Check if this field is in an exclusive set, if it is then mark - // it as seen. - for (int i = 0; i < this.exclusiveFieldSets.size(); i++) { - for (String field : this.exclusiveFieldSets.get(i)) { - if (field.equals(currentFieldName)) { - exclusiveFields.get(i).add(currentFieldName); - } - } + if (exclusiveFields != null) { + // Check if this field is in an exclusive set, if it is then mark + // it as seen. + maybeMarkExclusiveField(currentFieldName, exclusiveFields); } parseSub(parser, fieldParser, currentFieldName, value, context); @@ -322,26 +319,68 @@ public Value parse(XContentParser parser, Value value, Context context) throws I } } - // Check for a) multiple entries appearing in exclusive field sets and b) empty - // required field entries - StringBuilder message = new StringBuilder(); + // Check for a) multiple entries appearing in exclusive field sets and b) empty required field entries + if (exclusiveFields != null) { + ensureExclusiveFields(exclusiveFields); + } + if (requiredFields != null && requiredFields.isEmpty() == false) { + throwMissingRequiredFields(requiredFields); + } + return value; + } + + private void throwExpectedStartObject(XContentParser parser, XContentParser.Token token) { + throw new XContentParseException(parser.getTokenLocation(), "[" + name + "] Expected START_OBJECT but was: " + token); + } + + private void throwNoFieldFound(XContentParser parser) { + throw new XContentParseException(parser.getTokenLocation(), "[" + name + "] no field found"); + } + + private void throwMissingRequiredFields(List requiredFields) { + final StringBuilder message = new StringBuilder(); + for (String[] fields : requiredFields) { + message.append("Required one of fields ").append(Arrays.toString(fields)).append(", but none were specified. "); + } + throw new IllegalArgumentException(message.toString()); + } + + private void ensureExclusiveFields(List> exclusiveFields) { + StringBuilder message = null; for (List fieldset : exclusiveFields) { if (fieldset.size() > 1) { - message.append("The following fields are not allowed together: ").append(fieldset.toString()).append(" "); + if (message == null) { + message = new StringBuilder(); + } + message.append("The following fields are not allowed together: ").append(fieldset).append(" "); } } - if (message.length() > 0) { + if (message != null && message.length() > 0) { throw new IllegalArgumentException(message.toString()); } + } - if (requiredFields.isEmpty() == false) { - for (String[] fields : requiredFields) { - message.append("Required one of fields ").append(Arrays.toString(fields)).append(", but none were specified. "); + private void maybeMarkExclusiveField(String currentFieldName, List> exclusiveFields) { + for (int i = 0; i < this.exclusiveFieldSets.size(); i++) { + for (String field : this.exclusiveFieldSets.get(i)) { + if (field.equals(currentFieldName)) { + exclusiveFields.get(i).add(currentFieldName); + } } - throw new IllegalArgumentException(message.toString()); } + } - return value; + private void maybeMarkRequiredField(String currentFieldName, List requiredFields) { + Iterator iter = requiredFields.iterator(); + while (iter.hasNext()) { + String[] requiredFieldNames = iter.next(); + for (String field : requiredFieldNames) { + if (field.equals(currentFieldName)) { + iter.remove(); + break; + } + } + } } @Override @@ -427,14 +466,10 @@ public void declareNamedObject(BiConsumer consumer, NamedObjectPar assert token == XContentParser.Token.END_OBJECT; return namedObject; } catch (Exception e) { - throw new XContentParseException( - p.getTokenLocation(), - "[" + field + "] failed to parse field [" + currentName + "]", - e - ); + throw rethrowFieldParseFailure(field, p, currentName, e); } } catch (IOException e) { - throw new XContentParseException(p.getTokenLocation(), "[" + field + "] error while parsing named object", e); + throw wrapParseError(field, p, e, "error while parsing named object"); } }; @@ -447,8 +482,7 @@ public void declareNamedObjects(BiConsumer> consumer, NamedOb // This creates and parses the named object BiFunction objectParser = (XContentParser p, Context c) -> { if (p.currentToken() != XContentParser.Token.FIELD_NAME) { - throw new XContentParseException(p.getTokenLocation(), "[" + field + "] can be a single object with any number of " - + "fields or an array where each entry is an object with a single field"); + throw wrapCanBeObjectOrArrayOfObjects(field, p); } // This messy exception nesting has the nice side effect of telling the user which field failed to parse try { @@ -456,45 +490,59 @@ public void declareNamedObjects(BiConsumer> consumer, NamedOb try { return namedObjectParser.parse(p, c, currentName); } catch (Exception e) { - throw new XContentParseException( - p.getTokenLocation(), - "[" + field + "] failed to parse field [" + currentName + "]", - e - ); + throw rethrowFieldParseFailure(field, p, currentName, e); } } catch (IOException e) { - throw new XContentParseException(p.getTokenLocation(), "[" + field + "] error while parsing", e); + throw wrapParseError(field, p, e, "error while parsing"); } }; declareField((XContentParser p, Value v, Context c) -> { List fields = new ArrayList<>(); - XContentParser.Token token; if (p.currentToken() == XContentParser.Token.START_OBJECT) { // Fields are just named entries in a single object - while ((token = p.nextToken()) != XContentParser.Token.END_OBJECT) { + while (p.nextToken() != XContentParser.Token.END_OBJECT) { fields.add(objectParser.apply(p, c)); } } else if (p.currentToken() == XContentParser.Token.START_ARRAY) { // Fields are objects in an array. Each object contains a named field. - orderedModeCallback.accept(v); - while ((token = p.nextToken()) != XContentParser.Token.END_ARRAY) { - if (token != XContentParser.Token.START_OBJECT) { - throw new XContentParseException(p.getTokenLocation(), "[" + field + "] can be a single object with any number of " - + "fields or an array where each entry is an object with a single field"); - } - p.nextToken(); // Move to the first field in the object - fields.add(objectParser.apply(p, c)); - p.nextToken(); // Move past the object, should be back to into the array - if (p.currentToken() != XContentParser.Token.END_OBJECT) { - throw new XContentParseException(p.getTokenLocation(), "[" + field + "] can be a single object with any number of " - + "fields or an array where each entry is an object with a single field"); - } - } + parseObjectsInArray(orderedModeCallback, field, objectParser, p, v, c, fields); } consumer.accept(v, fields); }, field, ValueType.OBJECT_ARRAY); } + private void parseObjectsInArray(Consumer orderedModeCallback, + ParseField field, BiFunction objectParser, + XContentParser p, Value v, Context c, + List fields) throws IOException { + orderedModeCallback.accept(v); + XContentParser.Token token; + while ((token = p.nextToken()) != XContentParser.Token.END_ARRAY) { + if (token != XContentParser.Token.START_OBJECT) { + throw wrapCanBeObjectOrArrayOfObjects(field, p); + } + p.nextToken(); // Move to the first field in the object + fields.add(objectParser.apply(p, c)); + p.nextToken(); // Move past the object, should be back to into the array + if (p.currentToken() != XContentParser.Token.END_OBJECT) { + throw wrapCanBeObjectOrArrayOfObjects(field, p); + } + } + } + + private XContentParseException wrapCanBeObjectOrArrayOfObjects(ParseField field, XContentParser p) { + return new XContentParseException(p.getTokenLocation(), "[" + field + "] can be a single object with any number of " + + "fields or an array where each entry is an object with a single field"); + } + + private XContentParseException wrapParseError(ParseField field, XContentParser p, IOException e, String s) { + return new XContentParseException(p.getTokenLocation(), "[" + field + "] " + s, e); + } + + private XContentParseException rethrowFieldParseFailure(ParseField field, XContentParser p, String currentName, Exception e) { + return new XContentParseException(p.getTokenLocation(), "[" + field + "] failed to parse field [" + currentName + "]", e); + } + @Override public void declareNamedObjects(BiConsumer> consumer, NamedObjectParser namedObjectParser, ParseField field) { @@ -537,24 +585,24 @@ public void declareExclusiveFieldSet(String... exclusiveSet) { this.exclusiveFieldSets.add(exclusiveSet); } - private void parseArray(XContentParser parser, FieldParser fieldParser, String currentFieldName, Value value, Context context) - throws IOException { + private void parseArray(XContentParser parser, FieldParser fieldParser, String currentFieldName, Value value, Context context) { assert parser.currentToken() == XContentParser.Token.START_ARRAY : "Token was: " + parser.currentToken(); parseValue(parser, fieldParser, currentFieldName, value, context); } - private void parseValue(XContentParser parser, FieldParser fieldParser, String currentFieldName, Value value, Context context) - throws IOException { + private void parseValue(XContentParser parser, FieldParser fieldParser, String currentFieldName, Value value, Context context) { try { fieldParser.parser.parse(parser, value, context); } catch (Exception ex) { - throw new XContentParseException(parser.getTokenLocation(), - "[" + name + "] failed to parse field [" + currentFieldName + "]", ex); + throwFailedToParse(parser, currentFieldName, ex); } } - private void parseSub(XContentParser parser, FieldParser fieldParser, String currentFieldName, Value value, Context context) - throws IOException { + private void throwFailedToParse(XContentParser parser, String currentFieldName, Exception ex) { + throw new XContentParseException(parser.getTokenLocation(), "[" + name + "] failed to parse field [" + currentFieldName + "]", ex); + } + + private void parseSub(XContentParser parser, FieldParser fieldParser, String currentFieldName, Value value, Context context) { final XContentParser.Token token = parser.currentToken(); switch (token) { case START_OBJECT: @@ -568,7 +616,7 @@ private void parseSub(XContentParser parser, FieldParser fieldParser, String cur * for having a cheap test. */ if (parser.currentToken() != XContentParser.Token.END_OBJECT) { - throw new IllegalStateException("parser for [" + currentFieldName + "] did not end on END_OBJECT"); + throwMustEndOn(currentFieldName, XContentParser.Token.END_OBJECT); } break; case START_ARRAY: @@ -582,13 +630,13 @@ private void parseSub(XContentParser parser, FieldParser fieldParser, String cur * for having a cheap test. */ if (parser.currentToken() != XContentParser.Token.END_ARRAY) { - throw new IllegalStateException("parser for [" + currentFieldName + "] did not end on END_ARRAY"); + throwMustEndOn(currentFieldName, XContentParser.Token.END_ARRAY); } break; case END_OBJECT: case END_ARRAY: case FIELD_NAME: - throw new XContentParseException(parser.getTokenLocation(), "[" + name + "]" + token + " is unexpected"); + throw throwUnexpectedToken(parser, token); case VALUE_STRING: case VALUE_NUMBER: case VALUE_BOOLEAN: @@ -598,6 +646,14 @@ private void parseSub(XContentParser parser, FieldParser fieldParser, String cur } } + private void throwMustEndOn(String currentFieldName, XContentParser.Token token) { + throw new IllegalStateException("parser for [" + currentFieldName + "] did not end on " + token); + } + + private XContentParseException throwUnexpectedToken(XContentParser parser, XContentParser.Token token) { + return new XContentParseException(parser.getTokenLocation(), "[" + name + "]" + token + " is unexpected"); + } + private class FieldParser { private final Parser parser; private final EnumSet supportedTokens; diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java index 57af96005ac9a..dace3f8d0c13a 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java @@ -23,12 +23,11 @@ import org.elasticsearch.xcontent.XContentParser; import java.io.IOException; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.TreeMap; -import java.util.function.Function; -import java.util.stream.Collectors; /** * Represents set of {@link LifecycleAction}s which should be executed at a @@ -42,8 +41,16 @@ public class Phase implements ToXContentObject, Writeable { @SuppressWarnings("unchecked") private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>("phase", false, - (a, name) -> new Phase(name, (TimeValue) a[0], ((List) a[1]).stream() - .collect(Collectors.toMap(LifecycleAction::getWriteableName, Function.identity())))); + (a, name) -> { + final List lifecycleActions = (List) a[1]; + Map map = new HashMap<>(lifecycleActions.size()); + for (LifecycleAction lifecycleAction : lifecycleActions) { + if (map.put(lifecycleAction.getWriteableName(), lifecycleAction) != null) { + throw new IllegalStateException("Duplicate key"); + } + } + return new Phase(name, (TimeValue) a[0], map); + }); static { PARSER.declareField(ConstructingObjectParser.optionalConstructorArg(), (ContextParser) (p, c) -> { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/PhaseCacheManagementTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/PhaseCacheManagementTests.java index fcd320cad8a9e..df0b6ee75b67c 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/PhaseCacheManagementTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/PhaseCacheManagementTests.java @@ -31,6 +31,7 @@ import static org.elasticsearch.xpack.core.ilm.PhaseCacheManagement.refreshPhaseDefinition; import static org.elasticsearch.xpack.core.ilm.PhaseCacheManagement.updateIndicesForPolicy; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.equalTo; import static org.mockito.Mockito.mock; @@ -228,7 +229,7 @@ public void testReadStepKeys() { " \"version\" : 1,\n" + " \"modified_date_in_millis\" : 1578521007076\n" + " }", "phase", null), - contains(new Step.StepKey("phase", "rollover", WaitForRolloverReadyStep.NAME), + containsInAnyOrder(new Step.StepKey("phase", "rollover", WaitForRolloverReadyStep.NAME), new Step.StepKey("phase", "rollover", RolloverStep.NAME), new Step.StepKey("phase", "rollover", WaitForActiveShardsStep.NAME), new Step.StepKey("phase", "rollover", UpdateRolloverLifecycleDateStep.NAME),