From 45f928e0d162090060c84a468fd4cff373391364 Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Thu, 15 Nov 2018 11:16:07 -0500 Subject: [PATCH 1/3] GEO: More robust handling of ignore_malformed in geoshape parsing Adds a method to XContent parser to skip all children of a current element in case of the parsing failure and applies this method to be able to ignore the rest of the GeoJson shape if the parsing fails and we need to ignore the geoshape due to the ignore malformed flag. Supersedes #34498 Closes #34047 --- .../common/xcontent/XContentParser.java | 26 +++ .../xcontent/json/JsonXContentParser.java | 48 +++++- .../common/xcontent/XContentParserTests.java | 162 ++++++++++++++++++ .../common/geo/parsers/GeoJsonParser.java | 6 +- .../common/geo/GeoJsonShapeParserTests.java | 27 +++ .../xcontent/WatcherXContentParser.java | 5 + 6 files changed, 268 insertions(+), 6 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentParser.java index 81cc39c5793cf..4ef06b6058bb1 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentParser.java @@ -253,4 +253,30 @@ enum NumberType { * The callback to notify when parsing encounters a deprecated field. */ DeprecationHandler getDeprecationHandler(); + + /** + * Marks the current token to be used with skipMarkedChildren. + * + * Can be used to gracefully handle parsing errors within large nested structures. Usage: + * + * Mark mark = parser.mark(); + * try { + * while (parser.nextToken() != Token.END_OBJECT) { + * // parse + * if (isSomethingWrong) { + * throw ElasticSearchParsingException("some thing is wrong here"); + * } + * } + * } catch(ElasticSearchParsingException ex) { + * mark.skipChildren(); // skip children and continue parsing + * } + * + * which would be equvivalent to parser.skipChildren() in case of an error + * + */ + Mark markParent(); + + interface Mark { + void skipChildren() throws IOException; + } } diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentParser.java index 14bb21e8243bf..7c8db538296bd 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentParser.java @@ -21,14 +21,14 @@ import com.fasterxml.jackson.core.JsonLocation; import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.JsonStreamContext; import com.fasterxml.jackson.core.JsonToken; - -import org.elasticsearch.core.internal.io.IOUtils; import org.elasticsearch.common.xcontent.DeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentLocation; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.support.AbstractXContentParser; +import org.elasticsearch.core.internal.io.IOUtils; import java.io.IOException; import java.nio.CharBuffer; @@ -245,4 +245,48 @@ private Token convertToken(JsonToken token) { public boolean isClosed() { return parser.isClosed(); } + + @Override + public Mark markParent() { + if (currentToken() != Token.START_OBJECT && currentToken() != Token.START_ARRAY) { + throw new IllegalStateException("markParent() has to be called on the start of an object or an array"); + } + JsonStreamContext context = parser.getParsingContext(); + JsonStreamContext parentContext = context.getParent(); + if (parentContext == null) { + // we cannot be here because we already checked for the first token, but just in case we double check + throw new IllegalStateException("Cannot mark root"); + } + return () -> { + JsonStreamContext currentContext; + Token token = currentToken(); + // First we skip all tokens until we are back to the parsing context that we started with + for (currentContext = parser.getParsingContext(); + currentContext != null && parentContext.equals(parser.getParsingContext()) == false; + currentContext = parser.getParsingContext() + ) { + token = nextToken(); + } + if (currentContext == null) { + throw new IllegalStateException("Didn't find parent context"); + } + // Now we are going to skip all children of the current structure + int level = 1; + while (true) { + if (token == null) { + throw new IllegalStateException("Unexpected end of the stream"); + } + if (token == Token.START_OBJECT || token == Token.START_ARRAY) { + level++; + } else if (token == Token.END_OBJECT || token == Token.END_ARRAY) { + if (--level == 0) { + return; + } + } + token = nextToken(); + } + + }; + } + } diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java index 113c21bacd10e..019dec6d28269 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java @@ -20,6 +20,7 @@ package org.elasticsearch.common.xcontent; import com.fasterxml.jackson.core.JsonParseException; +import org.elasticsearch.common.CheckedSupplier; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.xcontent.json.JsonXContent; @@ -327,4 +328,165 @@ public void testNestedMapInList() throws IOException { parser.list()); } } + + public void testMarkParent() throws IOException { + XContentBuilder builder = XContentFactory.jsonBuilder(); + boolean testObject = randomBoolean(); + int numberOfTokens; + if (testObject) { + numberOfTokens = generateRandomObjectForMarking(builder); + } else { + numberOfTokens = generateRandomArrayForMarking(builder); + } + String content = Strings.toString(builder); + + try (XContentParser parser = createParser(JsonXContent.jsonXContent, content)) { + assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); + assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // first field + assertEquals("first_field", parser.currentName()); + assertEquals(XContentParser.Token.VALUE_STRING, parser.nextToken()); // foo + assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // marked field + assertEquals("marked_field", parser.currentName()); + if (testObject) { + assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); // { + } else { + assertEquals(XContentParser.Token.START_ARRAY, parser.nextToken()); // [ + } + XContentParser.Mark mark = parser.markParent(); + int tokensToSkip = randomInt(numberOfTokens - 1); + for (int i = 0; i < tokensToSkip; i++) { + // Simulate incomplete parsing + assertNotNull(parser.nextToken()); + } + // now skip to the end of the marked_field + mark.skipChildren(); + assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // last field + assertEquals("last_field", parser.currentName()); + assertEquals(XContentParser.Token.VALUE_STRING, parser.nextToken()); + assertEquals(XContentParser.Token.END_OBJECT, parser.nextToken()); + assertNull(parser.nextToken()); + } + } + + public void testMarkAtWrongPlace() throws IOException { + XContentBuilder builder = XContentFactory.jsonBuilder(); + generateRandomObjectForMarking(builder); + String content = Strings.toString(builder); + + try (XContentParser parser = createParser(JsonXContent.jsonXContent, content)) { + assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); + assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // first field + assertEquals("first_field", parser.currentName()); + IllegalStateException exception = expectThrows(IllegalStateException.class, parser::markParent); + assertEquals("markParent() has to be called on the start of an object or an array", exception.getMessage()); + } + } + + + public void testMarkRoot() throws IOException { + XContentBuilder builder = XContentFactory.jsonBuilder(); + int numberOfTokens = generateRandomObjectForMarking(builder); + String content = Strings.toString(builder); + + try (XContentParser parser = createParser(JsonXContent.jsonXContent, content)) { + assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); + XContentParser.Mark mark = parser.markParent(); + int tokensToSkip = randomInt(numberOfTokens + 3); + for (int i = 0; i < tokensToSkip; i++) { + // Simulate incomplete parsing + assertNotNull(parser.nextToken()); + } + mark.skipChildren(); + assertNull(parser.nextToken()); + } + + } + + /** + * Generates a random object {"first_field": "foo", "marked_field": {...random...}, "last_field": "bar} + * + * Returns the number of tokens in the marked field + */ + private int generateRandomObjectForMarking(XContentBuilder builder) throws IOException { + builder.startObject() + .field("first_field", "foo") + .field("marked_field"); + int numberOfTokens = generateRandomObject(builder, 0); + builder.field("last_field", "bar").endObject(); + return numberOfTokens; + } + + + /** + * Generates a random object {"first_field": "foo", "marked_field": [...random...], "last_field": "bar} + * + * Returns the number of tokens in the marked field + */ + private int generateRandomArrayForMarking(XContentBuilder builder) throws IOException { + builder.startObject() + .field("first_field", "foo") + .field("marked_field"); + int numberOfTokens = generateRandomArray(builder, 0); + builder.field("last_field", "bar").endObject(); + return numberOfTokens; + } + + private int generateRandomObject(XContentBuilder builder, int level) throws IOException { + int tokens = 2; + builder.startObject(); + int numberOfElements = randomInt(5); + for (int i = 0; i < numberOfElements; i++) { + builder.field(randomAlphaOfLength(10) + "_" + i); + tokens += generateRandomValue(builder, level + 1); + } + builder.endObject(); + return tokens; + } + + private int generateRandomValue(XContentBuilder builder, int level) throws IOException { + @SuppressWarnings("unchecked") CheckedSupplier fieldGenerator = randomFrom( + () -> { + builder.value(randomInt()); + return 1; + }, + () -> { + builder.value(randomAlphaOfLength(10)); + return 1; + }, + () -> { + builder.value(randomDouble()); + return 1; + }, + () -> { + if (level < 3) { + // don't need to go too deep + return generateRandomObject(builder, level + 1); + } else { + builder.value(0); + return 1; + } + }, + () -> { + if (level < 5) { // don't need to go too deep + return generateRandomArray(builder, level); + } else { + builder.value(0); + return 1; + } + } + ); + return fieldGenerator.get(); + } + + private int generateRandomArray(XContentBuilder builder, int level) throws IOException { + int tokens = 2; + int arraySize = randomInt(3); + builder.startArray(); + for (int i = 0; i < arraySize; i++) { + tokens += generateRandomValue(builder, level + 1); + } + builder.endArray(); + return tokens; + } + } diff --git a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java index 45ce2b610ca0c..f690f996b7861 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java +++ b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java @@ -55,6 +55,7 @@ protected static ShapeBuilder parse(XContentParser parser, GeoShapeFieldMapper s String malformedException = null; XContentParser.Token token; + XContentParser.Mark mark = parser.markParent(); try { while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { @@ -110,10 +111,7 @@ protected static ShapeBuilder parse(XContentParser parser, GeoShapeFieldMapper s } } catch (Exception ex) { // Skip all other fields until the end of the object - while (parser.currentToken() != XContentParser.Token.END_OBJECT && parser.currentToken() != null) { - parser.nextToken(); - parser.skipChildren(); - } + mark.skipChildren(); throw ex; } diff --git a/server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java b/server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java index 57cb6b6262384..f13d645a1414b 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java @@ -1213,4 +1213,31 @@ public void testParseInvalidShapes() throws IOException { assertNull(parser.nextToken()); } } + + public void testParseInvalidGeometryCollectionShapes() throws IOException { + // single dimensions point + XContentBuilder invalidPoints = XContentFactory.jsonBuilder() + .startObject() + .startObject("foo") + .field("type", "geometrycollection") + .startArray("geometries") + .startObject() + .field("type", "polygon") + .startArray("coordinates") + .startArray().value("46.6022226498514").value("24.7237442867977").endArray() + .startArray().value("46.6031857243798").value("24.722968774929").endArray() + .endArray() // coordinates + .endObject() + .endArray() // geometries + .endObject() + .endObject(); + try (XContentParser parser = createParser(invalidPoints)) { + parser.nextToken(); // foo + parser.nextToken(); // start object + parser.nextToken(); // start object + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertEquals(XContentParser.Token.END_OBJECT, parser.nextToken()); // end of the document + assertNull(parser.nextToken()); // no more elements afterwards + } + } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/support/xcontent/WatcherXContentParser.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/support/xcontent/WatcherXContentParser.java index be422d4c38da7..7b19de6939a19 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/support/xcontent/WatcherXContentParser.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/support/xcontent/WatcherXContentParser.java @@ -287,4 +287,9 @@ public void close() throws IOException { public DeprecationHandler getDeprecationHandler() { return parser.getDeprecationHandler(); } + + @Override + public Mark markParent() { + return parser.markParent(); + } } From 8a83a7dbc0e31a1afe4544dcff4b6c1b1bbc978e Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Mon, 19 Nov 2018 16:12:29 -0500 Subject: [PATCH 2/3] Replace mark and skip children methods with a wrapper --- .../common/xcontent/XContentParser.java | 26 -- .../common/xcontent/XContentSubParser.java | 280 ++++++++++++++++++ .../xcontent/json/JsonXContentParser.java | 45 --- .../common/xcontent/XContentParserTests.java | 67 ++--- .../common/geo/parsers/GeoJsonParser.java | 46 ++- .../xcontent/WatcherXContentParser.java | 5 - 6 files changed, 324 insertions(+), 145 deletions(-) create mode 100644 libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentParser.java index 4ef06b6058bb1..81cc39c5793cf 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentParser.java @@ -253,30 +253,4 @@ enum NumberType { * The callback to notify when parsing encounters a deprecated field. */ DeprecationHandler getDeprecationHandler(); - - /** - * Marks the current token to be used with skipMarkedChildren. - * - * Can be used to gracefully handle parsing errors within large nested structures. Usage: - * - * Mark mark = parser.mark(); - * try { - * while (parser.nextToken() != Token.END_OBJECT) { - * // parse - * if (isSomethingWrong) { - * throw ElasticSearchParsingException("some thing is wrong here"); - * } - * } - * } catch(ElasticSearchParsingException ex) { - * mark.skipChildren(); // skip children and continue parsing - * } - * - * which would be equvivalent to parser.skipChildren() in case of an error - * - */ - Mark markParent(); - - interface Mark { - void skipChildren() throws IOException; - } } diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java new file mode 100644 index 0000000000000..eaa910fac42c8 --- /dev/null +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java @@ -0,0 +1,280 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.xcontent; + +import java.io.IOException; +import java.nio.CharBuffer; +import java.util.List; +import java.util.Map; + +/** + * Wrapper for a XContentParser that makes a single object to look like a complete document. + * + * The wrapper prevents the parsing logic to consume tokens outside of the wrapped object as well + * as skipping to the end of the object in case of a parsing error. The wrapper is intended to be + * used for parsing objects that should be ignored if they are malformed. + */ +public class XContentSubParser implements XContentParser { + + private final XContentParser parser; + private int level; + + public XContentSubParser(XContentParser parser) { + this.parser = parser; + if (parser.currentToken() != Token.START_OBJECT) { + throw new IllegalStateException("The sub parser has to be created on the start of an object"); + } + level = 1; + } + + @Override + public XContentType contentType() { + return parser.contentType(); + } + + @Override + public Token nextToken() throws IOException { + if (level > 0) { + Token token = parser.nextToken(); + if (token == Token.START_OBJECT || token == Token.START_ARRAY) { + level++; + } else if (token == Token.END_OBJECT || token == Token.END_ARRAY) { + level--; + } + return token; + } else { + return null; // we have reached the end of the wrapped object + } + } + + @Override + public void skipChildren() throws IOException { + Token token = parser.currentToken(); + if (token != Token.START_OBJECT && token != Token.START_ARRAY) { + // skip if not starting on an object or an array + return; + } + int backToLevel = level - 1; + while (nextToken() != null) { + if (level <= backToLevel) { + return; + } + } + } + + @Override + public Token currentToken() { + return parser.currentToken(); + } + + @Override + public String currentName() throws IOException { + return parser.currentName(); + } + + @Override + public Map map() throws IOException { + return parser.map(); + } + + @Override + public Map mapOrdered() throws IOException { + return parser.mapOrdered(); + } + + @Override + public Map mapStrings() throws IOException { + return parser.mapStrings(); + } + + @Override + public Map mapStringsOrdered() throws IOException { + return parser.mapStringsOrdered(); + } + + @Override + public List list() throws IOException { + return parser.list(); + } + + @Override + public List listOrderedMap() throws IOException { + return parser.listOrderedMap(); + } + + @Override + public String text() throws IOException { + return parser.text(); + } + + @Override + public String textOrNull() throws IOException { + return parser.textOrNull(); + } + + @Override + public CharBuffer charBufferOrNull() throws IOException { + return parser.charBufferOrNull(); + } + + @Override + public CharBuffer charBuffer() throws IOException { + return parser.charBuffer(); + } + + @Override + public Object objectText() throws IOException { + return parser.objectText(); + } + + @Override + public Object objectBytes() throws IOException { + return parser.objectBytes(); + } + + @Override + public boolean hasTextCharacters() { + return parser.hasTextCharacters(); + } + + @Override + public char[] textCharacters() throws IOException { + return parser.textCharacters(); + } + + @Override + public int textLength() throws IOException { + return parser.textLength(); + } + + @Override + public int textOffset() throws IOException { + return parser.textOffset(); + } + + @Override + public Number numberValue() throws IOException { + return parser.numberValue(); + } + + @Override + public NumberType numberType() throws IOException { + return parser.numberType(); + } + + @Override + public short shortValue(boolean coerce) throws IOException { + return parser.shortValue(coerce); + } + + @Override + public int intValue(boolean coerce) throws IOException { + return parser.intValue(coerce); + } + + @Override + public long longValue(boolean coerce) throws IOException { + return parser.longValue(coerce); + } + + @Override + public float floatValue(boolean coerce) throws IOException { + return parser.floatValue(coerce); + } + + @Override + public double doubleValue(boolean coerce) throws IOException { + return parser.doubleValue(); + } + + @Override + public short shortValue() throws IOException { + return parser.shortValue(); + } + + @Override + public int intValue() throws IOException { + return parser.intValue(); + } + + @Override + public long longValue() throws IOException { + return parser.longValue(); + } + + @Override + public float floatValue() throws IOException { + return parser.floatValue(); + } + + @Override + public double doubleValue() throws IOException { + return parser.doubleValue(); + } + + @Override + public boolean isBooleanValue() throws IOException { + return parser.isBooleanValue(); + } + + @Override + public boolean booleanValue() throws IOException { + return parser.booleanValue(); + } + + @Override + public byte[] binaryValue() throws IOException { + return parser.binaryValue(); + } + + @Override + public XContentLocation getTokenLocation() { + return parser.getTokenLocation(); + } + + @Override + public T namedObject(Class categoryClass, String name, Object context) throws IOException { + return parser.namedObject(categoryClass, name, context); + } + + @Override + public NamedXContentRegistry getXContentRegistry() { + return parser.getXContentRegistry(); + } + + @Override + public boolean isClosed() { + return parser.isClosed(); + } + + @Override + public DeprecationHandler getDeprecationHandler() { + return parser.getDeprecationHandler(); + } + + @Override + public void close() throws IOException { + while (true) { + if (nextToken() == null) { + return; + } + } + } +} diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentParser.java index 7c8db538296bd..be0d01763b893 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentParser.java @@ -21,7 +21,6 @@ import com.fasterxml.jackson.core.JsonLocation; import com.fasterxml.jackson.core.JsonParser; -import com.fasterxml.jackson.core.JsonStreamContext; import com.fasterxml.jackson.core.JsonToken; import org.elasticsearch.common.xcontent.DeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; @@ -245,48 +244,4 @@ private Token convertToken(JsonToken token) { public boolean isClosed() { return parser.isClosed(); } - - @Override - public Mark markParent() { - if (currentToken() != Token.START_OBJECT && currentToken() != Token.START_ARRAY) { - throw new IllegalStateException("markParent() has to be called on the start of an object or an array"); - } - JsonStreamContext context = parser.getParsingContext(); - JsonStreamContext parentContext = context.getParent(); - if (parentContext == null) { - // we cannot be here because we already checked for the first token, but just in case we double check - throw new IllegalStateException("Cannot mark root"); - } - return () -> { - JsonStreamContext currentContext; - Token token = currentToken(); - // First we skip all tokens until we are back to the parsing context that we started with - for (currentContext = parser.getParsingContext(); - currentContext != null && parentContext.equals(parser.getParsingContext()) == false; - currentContext = parser.getParsingContext() - ) { - token = nextToken(); - } - if (currentContext == null) { - throw new IllegalStateException("Didn't find parent context"); - } - // Now we are going to skip all children of the current structure - int level = 1; - while (true) { - if (token == null) { - throw new IllegalStateException("Unexpected end of the stream"); - } - if (token == Token.START_OBJECT || token == Token.START_ARRAY) { - level++; - } else if (token == Token.END_OBJECT || token == Token.END_ARRAY) { - if (--level == 0) { - return; - } - } - token = nextToken(); - } - - }; - } - } diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java index 019dec6d28269..ca387d28ffa86 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java @@ -329,15 +329,10 @@ public void testNestedMapInList() throws IOException { } } - public void testMarkParent() throws IOException { + public void testSubParser() throws IOException { XContentBuilder builder = XContentFactory.jsonBuilder(); - boolean testObject = randomBoolean(); int numberOfTokens; - if (testObject) { - numberOfTokens = generateRandomObjectForMarking(builder); - } else { - numberOfTokens = generateRandomArrayForMarking(builder); - } + numberOfTokens = generateRandomObjectForMarking(builder); String content = Strings.toString(builder); try (XContentParser parser = createParser(JsonXContent.jsonXContent, content)) { @@ -347,19 +342,18 @@ public void testMarkParent() throws IOException { assertEquals(XContentParser.Token.VALUE_STRING, parser.nextToken()); // foo assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // marked field assertEquals("marked_field", parser.currentName()); - if (testObject) { - assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); // { - } else { - assertEquals(XContentParser.Token.START_ARRAY, parser.nextToken()); // [ - } - XContentParser.Mark mark = parser.markParent(); - int tokensToSkip = randomInt(numberOfTokens - 1); - for (int i = 0; i < tokensToSkip; i++) { - // Simulate incomplete parsing - assertNotNull(parser.nextToken()); + assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); // { + try (XContentParser subParser = new XContentSubParser(parser)) { + int tokensToSkip = randomInt(numberOfTokens - 1); + for (int i = 0; i < tokensToSkip; i++) { + // Simulate incomplete parsing + assertNotNull(subParser.nextToken()); + } + if (randomBoolean()) { + // And sometimes skipping children + subParser.skipChildren(); + } } - // now skip to the end of the marked_field - mark.skipChildren(); assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // last field assertEquals("last_field", parser.currentName()); assertEquals(XContentParser.Token.VALUE_STRING, parser.nextToken()); @@ -368,7 +362,7 @@ public void testMarkParent() throws IOException { } } - public void testMarkAtWrongPlace() throws IOException { + public void testCreateSubParserAtAWrongPlace() throws IOException { XContentBuilder builder = XContentFactory.jsonBuilder(); generateRandomObjectForMarking(builder); String content = Strings.toString(builder); @@ -377,26 +371,26 @@ public void testMarkAtWrongPlace() throws IOException { assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // first field assertEquals("first_field", parser.currentName()); - IllegalStateException exception = expectThrows(IllegalStateException.class, parser::markParent); - assertEquals("markParent() has to be called on the start of an object or an array", exception.getMessage()); + IllegalStateException exception = expectThrows(IllegalStateException.class, () -> new XContentSubParser(parser)); + assertEquals("The sub parser has to be created on the start of an object", exception.getMessage()); } } - public void testMarkRoot() throws IOException { + public void testCreateRootSubParser() throws IOException { XContentBuilder builder = XContentFactory.jsonBuilder(); int numberOfTokens = generateRandomObjectForMarking(builder); String content = Strings.toString(builder); try (XContentParser parser = createParser(JsonXContent.jsonXContent, content)) { assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); - XContentParser.Mark mark = parser.markParent(); - int tokensToSkip = randomInt(numberOfTokens + 3); - for (int i = 0; i < tokensToSkip; i++) { - // Simulate incomplete parsing - assertNotNull(parser.nextToken()); + try (XContentParser subParser = new XContentSubParser(parser)) { + int tokensToSkip = randomInt(numberOfTokens + 3); + for (int i = 0; i < tokensToSkip; i++) { + // Simulate incomplete parsing + assertNotNull(subParser.nextToken()); + } } - mark.skipChildren(); assertNull(parser.nextToken()); } @@ -416,21 +410,6 @@ private int generateRandomObjectForMarking(XContentBuilder builder) throws IOExc return numberOfTokens; } - - /** - * Generates a random object {"first_field": "foo", "marked_field": [...random...], "last_field": "bar} - * - * Returns the number of tokens in the marked field - */ - private int generateRandomArrayForMarking(XContentBuilder builder) throws IOException { - builder.startObject() - .field("first_field", "foo") - .field("marked_field"); - int numberOfTokens = generateRandomArray(builder, 0); - builder.field("last_field", "bar").endObject(); - return numberOfTokens; - } - private int generateRandomObject(XContentBuilder builder, int level) throws IOException { int tokens = 2; builder.startObject(); diff --git a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java index f690f996b7861..4f0586711e439 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java +++ b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java @@ -27,6 +27,7 @@ import org.elasticsearch.common.geo.builders.ShapeBuilder; import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentSubParser; import org.elasticsearch.index.mapper.GeoShapeFieldMapper; import org.locationtech.jts.geom.Coordinate; @@ -55,64 +56,59 @@ protected static ShapeBuilder parse(XContentParser parser, GeoShapeFieldMapper s String malformedException = null; XContentParser.Token token; - XContentParser.Mark mark = parser.markParent(); - try { - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + try (XContentParser subParser = new XContentSubParser(parser)) { + while ((token = subParser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { - String fieldName = parser.currentName(); + String fieldName = subParser.currentName(); - if (ShapeParser.FIELD_TYPE.match(fieldName, parser.getDeprecationHandler())) { - parser.nextToken(); - final GeoShapeType type = GeoShapeType.forName(parser.text()); + if (ShapeParser.FIELD_TYPE.match(fieldName, subParser.getDeprecationHandler())) { + subParser.nextToken(); + final GeoShapeType type = GeoShapeType.forName(subParser.text()); if (shapeType != null && shapeType.equals(type) == false) { malformedException = ShapeParser.FIELD_TYPE + " already parsed as [" + shapeType + "] cannot redefine as [" + type + "]"; } else { shapeType = type; } - } else if (ShapeParser.FIELD_COORDINATES.match(fieldName, parser.getDeprecationHandler())) { - parser.nextToken(); - CoordinateNode tempNode = parseCoordinates(parser, ignoreZValue.value()); + } else if (ShapeParser.FIELD_COORDINATES.match(fieldName, subParser.getDeprecationHandler())) { + subParser.nextToken(); + CoordinateNode tempNode = parseCoordinates(subParser, ignoreZValue.value()); if (coordinateNode != null && tempNode.numDimensions() != coordinateNode.numDimensions()) { throw new ElasticsearchParseException("Exception parsing coordinates: " + "number of dimensions do not match"); } coordinateNode = tempNode; - } else if (ShapeParser.FIELD_GEOMETRIES.match(fieldName, parser.getDeprecationHandler())) { + } else if (ShapeParser.FIELD_GEOMETRIES.match(fieldName, subParser.getDeprecationHandler())) { if (shapeType == null) { shapeType = GeoShapeType.GEOMETRYCOLLECTION; } else if (shapeType.equals(GeoShapeType.GEOMETRYCOLLECTION) == false) { malformedException = "cannot have [" + ShapeParser.FIELD_GEOMETRIES + "] with type set to [" + shapeType + "]"; } - parser.nextToken(); - geometryCollections = parseGeometries(parser, shapeMapper); - } else if (CircleBuilder.FIELD_RADIUS.match(fieldName, parser.getDeprecationHandler())) { + subParser.nextToken(); + geometryCollections = parseGeometries(subParser, shapeMapper); + } else if (CircleBuilder.FIELD_RADIUS.match(fieldName, subParser.getDeprecationHandler())) { if (shapeType == null) { shapeType = GeoShapeType.CIRCLE; } else if (shapeType != null && shapeType.equals(GeoShapeType.CIRCLE) == false) { malformedException = "cannot have [" + CircleBuilder.FIELD_RADIUS + "] with type set to [" + shapeType + "]"; } - parser.nextToken(); - radius = DistanceUnit.Distance.parseDistance(parser.text()); - } else if (ShapeParser.FIELD_ORIENTATION.match(fieldName, parser.getDeprecationHandler())) { + subParser.nextToken(); + radius = DistanceUnit.Distance.parseDistance(subParser.text()); + } else if (ShapeParser.FIELD_ORIENTATION.match(fieldName, subParser.getDeprecationHandler())) { if (shapeType != null && (shapeType.equals(GeoShapeType.POLYGON) || shapeType.equals(GeoShapeType.MULTIPOLYGON)) == false) { malformedException = "cannot have [" + ShapeParser.FIELD_ORIENTATION + "] with type set to [" + shapeType + "]"; } - parser.nextToken(); - requestedOrientation = ShapeBuilder.Orientation.fromString(parser.text()); + subParser.nextToken(); + requestedOrientation = ShapeBuilder.Orientation.fromString(subParser.text()); } else { - parser.nextToken(); - parser.skipChildren(); + subParser.nextToken(); + subParser.skipChildren(); } } } - } catch (Exception ex) { - // Skip all other fields until the end of the object - mark.skipChildren(); - throw ex; } if (malformedException != null) { diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/support/xcontent/WatcherXContentParser.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/support/xcontent/WatcherXContentParser.java index 7b19de6939a19..be422d4c38da7 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/support/xcontent/WatcherXContentParser.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/support/xcontent/WatcherXContentParser.java @@ -287,9 +287,4 @@ public void close() throws IOException { public DeprecationHandler getDeprecationHandler() { return parser.getDeprecationHandler(); } - - @Override - public Mark markParent() { - return parser.markParent(); - } } From 6eed22ddfb551d393e2a8cf92b03c5b8c0484d3c Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Tue, 20 Nov 2018 14:35:52 -0500 Subject: [PATCH 3/3] Add proper support for isClosed() to sub parser --- .../common/xcontent/XContentSubParser.java | 12 ++++++++---- .../common/xcontent/XContentParserTests.java | 7 ++++++- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java index eaa910fac42c8..e02f9f176246e 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java @@ -35,6 +35,7 @@ public class XContentSubParser implements XContentParser { private final XContentParser parser; private int level; + private boolean closed; public XContentSubParser(XContentParser parser) { this.parser = parser; @@ -261,7 +262,7 @@ public NamedXContentRegistry getXContentRegistry() { @Override public boolean isClosed() { - return parser.isClosed(); + return closed; } @Override @@ -271,9 +272,12 @@ public DeprecationHandler getDeprecationHandler() { @Override public void close() throws IOException { - while (true) { - if (nextToken() == null) { - return; + if (closed == false) { + closed = true; + while (true) { + if (nextToken() == null) { + return; + } } } } diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java index ca387d28ffa86..5dbe7be40f312 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java @@ -343,7 +343,8 @@ public void testSubParser() throws IOException { assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // marked field assertEquals("marked_field", parser.currentName()); assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); // { - try (XContentParser subParser = new XContentSubParser(parser)) { + XContentParser subParser = new XContentSubParser(parser); + try { int tokensToSkip = randomInt(numberOfTokens - 1); for (int i = 0; i < tokensToSkip; i++) { // Simulate incomplete parsing @@ -353,6 +354,10 @@ public void testSubParser() throws IOException { // And sometimes skipping children subParser.skipChildren(); } + } finally { + assertFalse(subParser.isClosed()); + subParser.close(); + assertTrue(subParser.isClosed()); } assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // last field assertEquals("last_field", parser.currentName());