From 3312df0031b5b96950914acf58c77bb20501bbd0 Mon Sep 17 00:00:00 2001 From: jimczi Date: Wed, 16 Oct 2019 17:55:51 +0200 Subject: [PATCH 1/3] Do not throw errors on unknown types in SearchAfterBuilder The support for BigInteger and BigDecimal was added for XContent in https://github.com/elastic/elasticsearch/pull/32888. However the SearchAfterBuilder xcontent parser doesn't expect them to be present so it throws an AssertionError. This change fixes this discrepancy by changing the AssertionError into an IllegalArgumentException that will not cause the node to die when thrown. Closes #48074 --- .../searchafter/SearchAfterBuilder.java | 2 +- .../searchafter/SearchAfterBuilderTests.java | 34 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java b/server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java index bdd44c3438dd6..0fdbfbe8c40cf 100644 --- a/server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java @@ -243,7 +243,7 @@ public static SearchAfterBuilder fromXContent(XContentParser parser) throws IOEx break; default: - throw new AssertionError("Unknown number type []" + parser.numberType()); + throw new IllegalArgumentException("Unknown number type: [" + parser.numberType() + "]"); } } else if (token == XContentParser.Token.VALUE_STRING) { values.add(parser.text()); diff --git a/server/src/test/java/org/elasticsearch/search/searchafter/SearchAfterBuilderTests.java b/server/src/test/java/org/elasticsearch/search/searchafter/SearchAfterBuilderTests.java index f7457d965744a..f67301bdecf90 100644 --- a/server/src/test/java/org/elasticsearch/search/searchafter/SearchAfterBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/searchafter/SearchAfterBuilderTests.java @@ -28,6 +28,7 @@ import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.text.Text; +import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; @@ -38,10 +39,13 @@ import org.elasticsearch.test.ESTestCase; import java.io.IOException; +import java.math.BigDecimal; +import java.math.BigInteger; import java.util.Collections; import static org.elasticsearch.search.searchafter.SearchAfterBuilder.extractSortType; import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; public class SearchAfterBuilderTests extends ESTestCase { @@ -187,6 +191,36 @@ public void testFromXContent() throws Exception { } } + public void testFromXContentIllegalType() throws Exception { + XContentBuilder xContent = XContentFactory.contentBuilder(randomFrom(XContentType.values())); + xContent.startObject() + .startArray("search_after") + .value(new BigInteger("9223372036854776000")) + .endArray() + .endObject(); + try (XContentParser parser = createParser(shuffleXContent(xContent))) { + parser.nextToken(); + parser.nextToken(); + parser.nextToken(); + IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> SearchAfterBuilder.fromXContent(parser)); + assertThat(exc.getMessage(), containsString("BIG_INTEGER")); + } + + xContent = XContentFactory.contentBuilder(randomFrom(XContentType.values())); + xContent.startObject() + .startArray("search_after") + .value(new BigDecimal("9223372036854776000.35")) + .endArray() + .endObject(); + try (XContentParser parser = createParser(shuffleXContent(xContent))) { + parser.nextToken(); + parser.nextToken(); + parser.nextToken(); + IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> SearchAfterBuilder.fromXContent(parser)); + assertThat(exc.getMessage(), containsString("BIG_DECIMAL")); + } + } + public void testWithNullArray() throws Exception { SearchAfterBuilder builder = new SearchAfterBuilder(); try { From e807dd82e13a06659c0f17660027f5288d3313e4 Mon Sep 17 00:00:00 2001 From: jimczi Date: Wed, 16 Oct 2019 17:58:05 +0200 Subject: [PATCH 2/3] improve error message --- .../elasticsearch/search/searchafter/SearchAfterBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java b/server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java index 0fdbfbe8c40cf..032f41073f5cd 100644 --- a/server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java @@ -243,7 +243,7 @@ public static SearchAfterBuilder fromXContent(XContentParser parser) throws IOEx break; default: - throw new IllegalArgumentException("Unknown number type: [" + parser.numberType() + "]"); + throw new IllegalArgumentException("Illegal number type, got [" + parser.numberType() + "] for " + parser.text()); } } else if (token == XContentParser.Token.VALUE_STRING) { values.add(parser.text()); From 7aa2e3d086456375bcfc7d34bfc6f7fc7aa4be38 Mon Sep 17 00:00:00 2001 From: jimczi Date: Tue, 22 Oct 2019 11:57:55 +0200 Subject: [PATCH 3/3] address review --- .../searchafter/SearchAfterBuilder.java | 3 +- .../searchafter/SearchAfterBuilderTests.java | 57 +++++++++++-------- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java b/server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java index 032f41073f5cd..304a639a8981a 100644 --- a/server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/searchafter/SearchAfterBuilder.java @@ -243,7 +243,8 @@ public static SearchAfterBuilder fromXContent(XContentParser parser) throws IOEx break; default: - throw new IllegalArgumentException("Illegal number type, got [" + parser.numberType() + "] for " + parser.text()); + throw new IllegalArgumentException("[search_after] does not accept numbers of type [" + + parser.numberType() + "], got " + parser.text()); } } else if (token == XContentParser.Token.VALUE_STRING) { values.add(parser.text()); diff --git a/server/src/test/java/org/elasticsearch/search/searchafter/SearchAfterBuilderTests.java b/server/src/test/java/org/elasticsearch/search/searchafter/SearchAfterBuilderTests.java index f67301bdecf90..a0e4c6bbbea6e 100644 --- a/server/src/test/java/org/elasticsearch/search/searchafter/SearchAfterBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/searchafter/SearchAfterBuilderTests.java @@ -28,7 +28,6 @@ import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.text.Text; -import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; @@ -192,32 +191,40 @@ public void testFromXContent() throws Exception { } public void testFromXContentIllegalType() throws Exception { - XContentBuilder xContent = XContentFactory.contentBuilder(randomFrom(XContentType.values())); - xContent.startObject() - .startArray("search_after") + for (XContentType type : XContentType.values()) { + // BIG_INTEGER + XContentBuilder xContent = XContentFactory.contentBuilder(type); + xContent.startObject() + .startArray("search_after") .value(new BigInteger("9223372036854776000")) - .endArray() - .endObject(); - try (XContentParser parser = createParser(shuffleXContent(xContent))) { - parser.nextToken(); - parser.nextToken(); - parser.nextToken(); - IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> SearchAfterBuilder.fromXContent(parser)); - assertThat(exc.getMessage(), containsString("BIG_INTEGER")); - } + .endArray() + .endObject(); + try (XContentParser parser = createParser(xContent)) { + parser.nextToken(); + parser.nextToken(); + parser.nextToken(); + IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> SearchAfterBuilder.fromXContent(parser)); + assertThat(exc.getMessage(), containsString("BIG_INTEGER")); + } - xContent = XContentFactory.contentBuilder(randomFrom(XContentType.values())); - xContent.startObject() - .startArray("search_after") - .value(new BigDecimal("9223372036854776000.35")) - .endArray() - .endObject(); - try (XContentParser parser = createParser(shuffleXContent(xContent))) { - parser.nextToken(); - parser.nextToken(); - parser.nextToken(); - IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> SearchAfterBuilder.fromXContent(parser)); - assertThat(exc.getMessage(), containsString("BIG_DECIMAL")); + // BIG_DECIMAL + // ignore json and yaml, they parse floating point numbers as floats/doubles + if (type == XContentType.JSON || type == XContentType.YAML) { + continue; + } + xContent = XContentFactory.contentBuilder(type); + xContent.startObject() + .startArray("search_after") + .value(new BigDecimal("9223372036854776003.3")) + .endArray() + .endObject(); + try (XContentParser parser = createParser(xContent)) { + parser.nextToken(); + parser.nextToken(); + parser.nextToken(); + IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> SearchAfterBuilder.fromXContent(parser)); + assertThat(exc.getMessage(), containsString("BIG_DECIMAL")); + } } }