Change ObjectParser exception (#31030)

ObjectParser should throw XContentParseExceptions, not IAE. A dedicated parsing exception can includes the place where the error occurred. Closes #30605
elastic · Jun 4, 2018 · 3f87c79 · 3f87c79
1 parent abe6115
commit 3f87c79
Show file tree

Hide file tree

Showing 18 changed files with 87 additions and 88 deletions.
diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java
@@ -155,7 +155,7 @@ public Value parse(XContentParser parser, Value value, Context context) throws I
         while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
             if (token == XContentParser.Token.FIELD_NAME) {
                 currentFieldName = parser.currentName();
-                fieldParser = getParser(currentFieldName);
+                fieldParser = getParser(currentFieldName, parser);
             } else {
                 if (currentFieldName == null) {
                     throw new XContentParseException(parser.getTokenLocation(), "[" + name  + "] no field found");
@@ -341,10 +341,11 @@ private void parseSub(XContentParser parser, FieldParser fieldParser, String cur
         }
     }
 
-    private FieldParser getParser(String fieldName) {
+    private FieldParser getParser(String fieldName, XContentParser xContentParser) {
         FieldParser parser = fieldParserMap.get(fieldName);
         if (parser == null && false == ignoreUnknownFields) {
-            throw new IllegalArgumentException("[" + name  + "] unknown field [" + fieldName + "], parser not found");
+            throw new XContentParseException(xContentParser.getTokenLocation(),
+                    "[" + name + "] unknown field [" + fieldName + "], parser not found");
         }
         return parser;
     }

diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java
@@ -35,7 +35,6 @@
 import java.util.Collections;
 import java.util.List;
 
-import static org.hamcrest.CoreMatchers.startsWith;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.hasSize;
 
@@ -186,7 +185,6 @@ public URI parseURI(XContentParser parser) {
     }
 
     public void testExceptions() throws IOException {
-        XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"test\" : \"foo\"}");
         class TestStruct {
             public void setTest(int test) {
             }
@@ -195,20 +193,16 @@ public void setTest(int test) {
         TestStruct s = new TestStruct();
         objectParser.declareInt(TestStruct::setTest, new ParseField("test"));
 
-        try {
-            objectParser.parse(parser, s, null);
-            fail("numeric value expected");
-        } catch (XContentParseException ex) {
+        {
+            XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"test\" : \"foo\"}");
+            XContentParseException ex = expectThrows(XContentParseException.class, () -> objectParser.parse(parser, s, null));
             assertThat(ex.getMessage(), containsString("[the_parser] failed to parse field [test]"));
             assertTrue(ex.getCause() instanceof NumberFormatException);
         }
-
-        parser = createParser(JsonXContent.jsonXContent, "{\"not_supported_field\" : \"foo\"}");
-        try {
-            objectParser.parse(parser, s, null);
-            fail("field not supported");
-        } catch (IllegalArgumentException ex) {
-            assertEquals(ex.getMessage(), "[the_parser] unknown field [not_supported_field], parser not found");
+        {
+            XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"not_supported_field\" : \"foo\"}");
+            XContentParseException ex = expectThrows(XContentParseException.class, () -> objectParser.parse(parser, s, null));
+            assertEquals(ex.getMessage(), "[1:2] [the_parser] unknown field [not_supported_field], parser not found");
         }
     }
 

diff --git a/...s/mapper-extras/src/test/java/org/elasticsearch/index/query/FeatureQueryBuilderTests.java b/...s/mapper-extras/src/test/java/org/elasticsearch/index/query/FeatureQueryBuilderTests.java
@@ -84,12 +84,6 @@ protected void doAssertLuceneQuery(FeatureQueryBuilder queryBuilder, Query query
         assertThat(query, either(instanceOf(MatchNoDocsQuery.class)).or(instanceOf(expectedClass)));
     }
 
-    @Override
-    @AwaitsFix(bugUrl="https://github.com/elastic/elasticsearch/issues/30605")
-    public void testUnknownField() {
-        super.testUnknownField();
-    }
-
     public void testDefaultScoreFunction() throws IOException {
         assumeTrue("test runs only when at least a type is registered", getCurrentTypes().length > 0);
         String query = "{\n" +

diff --git a/...nk-eval/src/test/java/org/elasticsearch/index/rankeval/DiscountedCumulativeGainTests.java b/...nk-eval/src/test/java/org/elasticsearch/index/rankeval/DiscountedCumulativeGainTests.java
@@ -25,6 +25,7 @@
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.common.xcontent.XContentParseException;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
@@ -41,7 +42,7 @@
 import static org.elasticsearch.index.rankeval.EvaluationMetric.filterUnknownDocuments;
 import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode;
 import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
-import static org.hamcrest.Matchers.startsWith;
+import static org.hamcrest.CoreMatchers.containsString;
 
 public class DiscountedCumulativeGainTests extends ESTestCase {
 
@@ -280,9 +281,9 @@ public void testXContentParsingIsNotLenient() throws IOException {
         try (XContentParser parser = createParser(xContentType.xContent(), withRandomFields)) {
             parser.nextToken();
             parser.nextToken();
-            IllegalArgumentException exception = expectThrows(IllegalArgumentException.class,
+            XContentParseException exception = expectThrows(XContentParseException.class,
                     () -> DiscountedCumulativeGain.fromXContent(parser));
-            assertThat(exception.getMessage(), startsWith("[dcg_at] unknown field"));
+            assertThat(exception.getMessage(), containsString("[dcg_at] unknown field"));
         }
     }
 

diff --git a/...les/rank-eval/src/test/java/org/elasticsearch/index/rankeval/MeanReciprocalRankTests.java b/...les/rank-eval/src/test/java/org/elasticsearch/index/rankeval/MeanReciprocalRankTests.java
@@ -25,6 +25,7 @@
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.common.xcontent.XContentParseException;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
@@ -41,7 +42,7 @@
 
 import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode;
 import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
-import static org.hamcrest.Matchers.startsWith;
+import static org.hamcrest.CoreMatchers.containsString;
 
 public class MeanReciprocalRankTests extends ESTestCase {
 
@@ -189,9 +190,9 @@ public void testXContentParsingIsNotLenient() throws IOException {
         try (XContentParser parser = createParser(xContentType.xContent(), withRandomFields)) {
             parser.nextToken();
             parser.nextToken();
-            IllegalArgumentException exception = expectThrows(IllegalArgumentException.class,
+            XContentParseException exception = expectThrows(XContentParseException.class,
                     () -> MeanReciprocalRank.fromXContent(parser));
-            assertThat(exception.getMessage(), startsWith("[reciprocal_rank] unknown field"));
+            assertThat(exception.getMessage(), containsString("[reciprocal_rank] unknown field"));
         }
     }
 

diff --git a/modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/PrecisionAtKTests.java b/modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/PrecisionAtKTests.java
@@ -25,6 +25,7 @@
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.common.xcontent.XContentParseException;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
@@ -41,7 +42,7 @@
 
 import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode;
 import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
-import static org.hamcrest.Matchers.startsWith;
+import static org.hamcrest.CoreMatchers.containsString;
 
 public class PrecisionAtKTests extends ESTestCase {
 
@@ -203,8 +204,8 @@ public void testXContentParsingIsNotLenient() throws IOException {
         try (XContentParser parser = createParser(xContentType.xContent(), withRandomFields)) {
             parser.nextToken();
             parser.nextToken();
-            IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> PrecisionAtK.fromXContent(parser));
-            assertThat(exception.getMessage(), startsWith("[precision] unknown field"));
+            XContentParseException exception = expectThrows(XContentParseException.class, () -> PrecisionAtK.fromXContent(parser));
+            assertThat(exception.getMessage(), containsString("[precision] unknown field"));
         }
     }
 

diff --git a/modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/RatedDocumentTests.java b/modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/RatedDocumentTests.java
@@ -24,6 +24,7 @@
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.common.xcontent.XContentParseException;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.test.ESTestCase;
@@ -33,7 +34,7 @@
 
 import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode;
 import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
-import static org.hamcrest.Matchers.startsWith;
+import static org.hamcrest.CoreMatchers.containsString;
 
 public class RatedDocumentTests extends ESTestCase {
 
@@ -59,8 +60,8 @@ public void testXContentParsingIsNotLenient() throws IOException {
         BytesReference originalBytes = toShuffledXContent(testItem, xContentType, ToXContent.EMPTY_PARAMS, randomBoolean());
         BytesReference withRandomFields = insertRandomFields(xContentType, originalBytes, null, random());
         try (XContentParser parser = createParser(xContentType.xContent(), withRandomFields)) {
-            Exception exception = expectThrows(IllegalArgumentException.class, () -> RatedDocument.fromXContent(parser));
-            assertThat(exception.getMessage(), startsWith("[rated_document] unknown field"));
+            XContentParseException exception = expectThrows(XContentParseException.class, () -> RatedDocument.fromXContent(parser));
+            assertThat(exception.getMessage(), containsString("[rated_document] unknown field"));
         }
     }
 

diff --git a/...va/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequestTests.java b/...va/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequestTests.java
@@ -21,6 +21,7 @@
 
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.XContentParseException;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.test.ESTestCase;
@@ -29,7 +30,8 @@
 import java.io.IOException;
 import java.util.Collections;
 
-import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.equalTo;;
 
 public class ClusterUpdateSettingsRequestTests extends ESTestCase {
 
@@ -51,10 +53,10 @@ private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws
             String unsupportedField = "unsupported_field";
             BytesReference mutated = BytesReference.bytes(XContentTestUtils.insertIntoXContent(xContentType.xContent(), originalBytes,
                     Collections.singletonList(""), () -> unsupportedField, () -> randomAlphaOfLengthBetween(3, 10)));
-            IllegalArgumentException iae = expectThrows(IllegalArgumentException.class,
+            XContentParseException iae = expectThrows(XContentParseException.class,
                     () -> ClusterUpdateSettingsRequest.fromXContent(createParser(xContentType.xContent(), mutated)));
             assertThat(iae.getMessage(),
-                    equalTo("[cluster_update_settings_request] unknown field [" + unsupportedField + "], parser not found"));
+                    containsString("[cluster_update_settings_request] unknown field [" + unsupportedField + "], parser not found"));
         } else {
             XContentParser parser = createParser(xContentType.xContent(), originalBytes);
             ClusterUpdateSettingsRequest parsedRequest = ClusterUpdateSettingsRequest.fromXContent(parser);

diff --git a/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java b/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java
@@ -33,6 +33,7 @@
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentHelper;
+import org.elasticsearch.common.xcontent.XContentParseException;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
@@ -70,6 +71,7 @@ public class UpdateRequestTests extends ESTestCase {
 
     private UpdateHelper updateHelper;
 
+    @Override
     @Before
     public void setUp() throws Exception {
         super.setUp();
@@ -283,8 +285,8 @@ public void testUnknownFieldParsing() throws Exception {
                     .field("unknown_field", "test")
                 .endObject());
 
-        IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> request.fromXContent(contentParser));
-        assertEquals("[UpdateRequest] unknown field [unknown_field], parser not found", ex.getMessage());
+        XContentParseException ex = expectThrows(XContentParseException.class, () -> request.fromXContent(contentParser));
+        assertEquals("[1:2] [UpdateRequest] unknown field [unknown_field], parser not found", ex.getMessage());
 
         UpdateRequest request2 = new UpdateRequest("test", "type", "1");
         XContentParser unknownObject = createParser(XContentFactory.jsonBuilder()
@@ -294,8 +296,8 @@ public void testUnknownFieldParsing() throws Exception {
                         .field("count", 1)
                     .endObject()
                 .endObject());
-        ex = expectThrows(IllegalArgumentException.class, () -> request2.fromXContent(unknownObject));
-        assertEquals("[UpdateRequest] unknown field [params], parser not found", ex.getMessage());
+        ex = expectThrows(XContentParseException.class, () -> request2.fromXContent(unknownObject));
+        assertEquals("[1:76] [UpdateRequest] unknown field [params], parser not found", ex.getMessage());
     }
 
     public void testFetchSourceParsing() throws Exception {

diff --git a/...rc/test/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightBuilderTests.java b/...rc/test/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightBuilderTests.java
@@ -158,10 +158,10 @@ public void testFromXContent() throws IOException {
      */
     public void testUnknownArrayNameExpection() throws IOException {
         {
-            IllegalArgumentException e = expectParseThrows(IllegalArgumentException.class, "{\n" +
+            XContentParseException e = expectParseThrows(XContentParseException.class, "{\n" +
                     "    \"bad_fieldname\" : [ \"field1\" 1 \"field2\" ]\n" +
                     "}\n");
-            assertEquals("[highlight] unknown field [bad_fieldname], parser not found", e.getMessage());
+            assertEquals("[2:5] [highlight] unknown field [bad_fieldname], parser not found", e.getMessage());
         }
 
         {
@@ -174,7 +174,7 @@ public void testUnknownArrayNameExpection() throws IOException {
                     "}\n");
             assertThat(e.getMessage(), containsString("[highlight] failed to parse field [fields]"));
             assertThat(e.getCause().getMessage(), containsString("[fields] failed to parse field [body]"));
-            assertEquals("[highlight_field] unknown field [bad_fieldname], parser not found", e.getCause().getCause().getMessage());
+            assertEquals("[4:9] [highlight_field] unknown field [bad_fieldname], parser not found", e.getCause().getCause().getMessage());
         }
     }
 
@@ -188,10 +188,10 @@ private <T extends Throwable> T expectParseThrows(Class<T> exceptionClass, Strin
      */
     public void testUnknownFieldnameExpection() throws IOException {
         {
-            IllegalArgumentException e = expectParseThrows(IllegalArgumentException.class, "{\n" +
+            XContentParseException e = expectParseThrows(XContentParseException.class, "{\n" +
                     "    \"bad_fieldname\" : \"value\"\n" +
                     "}\n");
-            assertEquals("[highlight] unknown field [bad_fieldname], parser not found", e.getMessage());
+            assertEquals("[2:5] [highlight] unknown field [bad_fieldname], parser not found", e.getMessage());
         }
 
         {
@@ -204,7 +204,7 @@ public void testUnknownFieldnameExpection() throws IOException {
                     "}\n");
             assertThat(e.getMessage(), containsString("[highlight] failed to parse field [fields]"));
             assertThat(e.getCause().getMessage(), containsString("[fields] failed to parse field [body]"));
-            assertEquals("[highlight_field] unknown field [bad_fieldname], parser not found", e.getCause().getCause().getMessage());
+            assertEquals("[4:9] [highlight_field] unknown field [bad_fieldname], parser not found", e.getCause().getCause().getMessage());
         }
     }
 
@@ -213,10 +213,10 @@ public void testUnknownFieldnameExpection() throws IOException {
      */
     public void testUnknownObjectFieldnameExpection() throws IOException {
         {
-            IllegalArgumentException e = expectParseThrows(IllegalArgumentException.class, "{\n" +
+            XContentParseException e = expectParseThrows(XContentParseException.class, "{\n" +
                     "    \"bad_fieldname\" :  { \"field\" : \"value\" }\n \n" +
                     "}\n");
-            assertEquals("[highlight] unknown field [bad_fieldname], parser not found", e.getMessage());
+            assertEquals("[2:5] [highlight] unknown field [bad_fieldname], parser not found", e.getMessage());
         }
 
         {
@@ -229,7 +229,7 @@ public void testUnknownObjectFieldnameExpection() throws IOException {
                     "}\n");
             assertThat(e.getMessage(), containsString("[highlight] failed to parse field [fields]"));
             assertThat(e.getCause().getMessage(), containsString("[fields] failed to parse field [body]"));
-            assertEquals("[highlight_field] unknown field [bad_fieldname], parser not found", e.getCause().getCause().getMessage());
+            assertEquals("[4:9] [highlight_field] unknown field [bad_fieldname], parser not found", e.getCause().getCause().getMessage());
         }
     }
 

diff --git a/server/src/test/java/org/elasticsearch/search/rescore/QueryRescorerBuilderTests.java b/server/src/test/java/org/elasticsearch/search/rescore/QueryRescorerBuilderTests.java
@@ -170,6 +170,7 @@ public void testRescoreQueryNull() throws IOException {
 
     class AlwaysRewriteQueryBuilder extends MatchAllQueryBuilder {
 
+        @Override
         protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOException {
             return new MatchAllQueryBuilder();
         }
@@ -254,8 +255,8 @@ public void testUnknownFieldsExpection() throws IOException {
                 "}\n";
         {
             XContentParser parser = createParser(rescoreElement);
-            Exception e = expectThrows(IllegalArgumentException.class, () -> RescorerBuilder.parseFromXContent(parser));
-            assertEquals("[query] unknown field [bad_fieldname], parser not found", e.getMessage());
+            XContentParseException e = expectThrows(XContentParseException.class, () -> RescorerBuilder.parseFromXContent(parser));
+            assertEquals("[3:17] [query] unknown field [bad_fieldname], parser not found", e.getMessage());
         }
 
         rescoreElement = "{\n" +