Fixed NPEs caused by requests without content. #23497

qwerty4030 · 2017-03-07T06:12:05Z

Fixed NPEs caused by requests without content.
REST handlers that require a body will throw an an ElasticsearchParseException "request body required".
REST handlers that require a body OR source param will throw an ElasticsearchParseException "request body or source parameter is required".
Replaced asserts in BulkRequest parsing code with a more descriptive IllegalArgumentException if the line contains an empty object.

Before this fix all the following requests threw confusing exceptions:

POST _bulk
POST _search/template/test
POST _scripts/test
POST _mapping/test
POST test-index/test-type
response:
{
  "error": {
    "root_cause": [
      {
        "type": "null_pointer_exception",
        "reason": null
      }
    ],
    "type": "null_pointer_exception",
    "reason": null
  },
  "status": 500
}

POST _template/test
response:
{

  "error": {
    "root_cause": [
      {
        "type": "not_x_content_exception",
        "reason": "Compressor detection can only be called on some xcontent bytes or compressed xcontent bytes"
      }
    ],
    "type": "not_x_content_exception",
    "reason": "Compressor detection can only be called on some xcontent bytes or compressed xcontent bytes"
  },
  "status": 500
}

POST _bulk
{}
response (note strange null in reason):
{
  "error": {
    "root_cause": [
      {
        "type": "illegal_argument_exception",
        "reason": "Malformed action/metadata line [1], expected START_OBJECT or END_OBJECT but found [null]"
      }
    ],
    "type": "illegal_argument_exception",
    "reason": "Malformed action/metadata line [1], expected START_OBJECT or END_OBJECT but found [null]"
  },
  "status": 400
}

After this fix, requests that require a body will throw this exception:

{
  "error": {
    "root_cause": [
      {
        "type": "parse_exception",
        "reason": "request body required"
      }
    ],
    "type": "parse_exception",
    "reason": "request body required"
  },
  "status": 400
}

After this fix, requests require a body or source param:

{
  "error": {
    "root_cause": [
      {
        "type": "parse_exception",
        "reason": "request body or source parameter is required"
      }
    ],
    "type": "parse_exception",
    "reason": "request body or source parameter is required"
  },
  "status": 400
}

Fixed confusing _bulk response:

POST _bulk
{}
response:
{
  "error": {
    "root_cause": [
      {
        "type": "illegal_argument_exception",
        "reason": "Malformed action/metadata line [1], expected FIELD_NAME but found [END_OBJECT]"
      }
    ],
    "type": "illegal_argument_exception",
    "reason": "Malformed action/metadata line [1], expected FIELD_NAME but found [END_OBJECT]"
  },
  "status": 400
}

Closes #24701

elasticmachine · 2017-03-07T06:12:08Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

qwerty4030 · 2017-03-07T06:18:59Z

@jaymode Do you mind taking a look at this? I noticed you made some changes recently related to content type in #23146. I'm not sure this is the best solution; seems like there should be better way to reject requests at a higher level when there is no content (but it's required). Thanks!

Also I did not add tests for the missing content scenarios. Should this be tested with the REST tests or Java integration tests?

jasontedor

I left one comment, but it applies throughout the change. Additionally, I think that REST tests need to be added for all of these changes.

jasontedor · 2017-03-07T07:24:37Z

core/src/main/java/org/elasticsearch/rest/action/document/RestBulkAction.java

+        if(request.hasContent()) {
+            bulkRequest.add(request.content(), defaultIndex, defaultType, defaultRouting, defaultFields,
+                defaultFetchSourceContext, defaultPipeline, null, allowExplicitIndex, request.getXContentType());
+        }



I don't understand this change? I think that we should still blow up here, just not with an NPE? This comment applies elsewhere in this pull request.

I followed the pattern used in a few other places. e.g. RestCreateIndexAction#L44-L46.
Instead of failing in RestHandler.prepareRequest a validation exception will be thrown from ActionRequest.validate() if request content is missing but required.
Would an approach similar to RestDeleteByQueryAction.java#L51-L53 be better for cases when request content is required?

jaymode · 2017-03-07T15:20:28Z

I think that we should still blow up here, just not with an NPE?

I agree. This change may get larger but it would be great to have a general purpose way to do this. Thinking out loud:

Make request#content throw an exception when there is no content
Add new method to the RestHandler interface, something like requiresContentOrSource. This one is tricky because some rest handlers allow the content to be specified as the source parameter when it is a GET request with a body, so a simple boolean is not sufficient as not everything will support the source parameter.
Remove direct uses of RestRequest#content and always use RestRequest#contentOrSourceParamParser and similar methods, which would need to be updated to ensure we have methods that throw when a body/source param is required etc. I do not think we'll be able to remove all of these methods as sometimes we just pass on the bytes directly.

I think all of these options have flaws, but hopefully this helps generate some other ideas about how to handle this.

qwerty4030 · 2017-03-17T04:35:25Z

@jaymode I like your first suggestion:

Make request#content throw an exception when there is no content

If content is required this method will be called like it was before this PR and throw an exception if content is missing.
If content is optional the call can be placed inside if(request.hasContent()) {}

Let me know if that sounds good to you and I'll revert the changes in this PR and update the RestRequest#content method to throw new ElasticsearchParseException("Body required"); similar to RestRequest#contentOrSourceParamParser.

While I was looking at the code I noticed RestRequest#contentOrSourceParam does not throw an exception if content and source are missing. I traced one usage to RestMultiSearchAction#parseMultiLineRequest and it results in an validation exception:

POST _msearch
response:
{
  "error": {
    "root_cause": [
      {
        "type": "action_request_validation_exception",
        "reason": "Validation Failed: 1: no requests added;"
      }
    ],
    "type": "action_request_validation_exception",
    "reason": "Validation Failed: 1: no requests added;"
  },
  "status": 400
}

Should the requests that use this method fail earlier (while parsing) as well?

jaymode · 2017-04-06T20:04:42Z

@qwerty4030 I apologize for the delay here.

If content is required this method will be called like it was before this PR and throw an exception if content is missing.
If content is optional the call can be placed inside if(request.hasContent()) {}

This sounds good to me.

While I was looking at the code I noticed RestRequest#contentOrSourceParam does not throw an exception if content and source are missing. I traced one usage to RestMultiSearchAction#parseMultiLineRequest and it results in an validation exception:

I think we should probably try to get rid of this behavior and just rely on the behavior you described above (throw exception if content is missing).

qwerty4030 · 2017-04-08T03:42:46Z

@jaymode Pushed new changes. I had to create a new RestRequest#requiredContent() method since content() was abstract and I didn't want to mess with the implementations. Also this way we can just call content() if its optional or requiredContent() if its not (IMO better than having to wrap it in an if(request.hasContent()).

I was unable to add REST tests because the internal test runner validates the request using the api spec, which requires a body for these requests. Should I look into adding some kind of override or try adding integration/unit tests instread? Thanks!

jasontedor · 2017-04-08T03:50:16Z

This sounds like a good scenario for an ESRestTestCase.

qwerty4030 · 2017-04-08T04:40:20Z

@jasontedor Looks like I spoke too soon. I discovered that you can do a raw REST call:

---
"missing body":
  - do:
      catch: /request body required/
      raw:
        method: POST
        path: _bulk

Would you prefer that?

Another idea I had was to somehow use the REST API spec to automatically test all requests that require a body.

jasontedor · 2017-04-09T20:29:49Z

Would you prefer that?

The REST tests (the YAML-based ones) are preferred because then the other language clients benefit from them too.

Another idea I had was to somehow use the REST API spec to automatically test all requests that require a body.

I'd rather that we not get fancy here, there are other consumers of the REST tests.

qwerty4030 · 2017-04-11T04:42:47Z

@jasontedor Pushed updates to the REST tests and cleaned up some RestHandlers to use the changes in RestRequest to throw an exception when content is missing.

jaymode · 2017-04-17T18:35:08Z

core/src/main/java/org/elasticsearch/action/bulk/BulkRequest.java

@@ -300,10 +300,16 @@ public BulkRequest add(BytesReference data, @Nullable String defaultIndex, @Null
                if (token == null) {
                    continue;
                }
-                assert token == XContentParser.Token.START_OBJECT;
+                if(token != XContentParser.Token.START_OBJECT) {


Please use a space between the if and (. Also, I think it would be good to use XContentParserUtils#ensureExpectedToken or XContentParserUtils#ensureFieldName

jaymode · 2017-04-17T18:35:35Z

core/src/main/java/org/elasticsearch/action/bulk/BulkRequest.java

                // Move to FIELD_NAME, that's the action
                token = parser.nextToken();
-                assert token == XContentParser.Token.FIELD_NAME;
+                if(token != XContentParser.Token.FIELD_NAME) {


I think it would be good to use XContentParserUtils#ensureExpectedToken or XContentParserUtils#ensureFieldName

I tried to use these methods but the ParsingException they throw has the wrong line number due to BulkRequest#add creating a new XContentParser for each action.

On potential fix it to catch the ParsingException and rethrow a new ParsingException with the correct line number.

jaymode · 2017-04-17T18:36:21Z

core/src/main/java/org/elasticsearch/rest/RestRequest.java

+     * @return content of the request body or throw an exception if the body or content type is missing
+     */
+    public final BytesReference requiredContent() {
+        if(hasContent() == false) {


space between if and (

jaymode · 2017-04-17T18:38:40Z

core/src/main/java/org/elasticsearch/rest/RestRequest.java

-            }
-            return new Tuple<>(xContentType, bytes);
+        if (source == null || typeParam == null) {
+            throw new IllegalStateException("source and source_content_type param required");


Change "param required" to "parameters are required"

jaymode · 2017-04-17T18:41:05Z

...es/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java

-                IOUtils.close(searchRequestParser);
-            }
-        });
+        XContentParser parser = restRequest.contentOrSourceParamParser();


use a try-with resources for the parser value

jaymode · 2017-04-17T18:41:44Z

...es/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java

-            }
-        });
+        XContentParser parser = restRequest.contentOrSourceParamParser();
+        parser = extractRequestSpecificFieldsAndReturnSearchCompatibleParser(parser, bodyConsumers);


use a different variable

qwerty4030 · 2017-04-18T02:58:35Z

@jaymode Pushed changes to address all your comments except the one about using XContentParserUtils#ensureExpectedToken (see my comment above). Thanks!

qwerty4030 · 2017-05-03T22:28:47Z

@jasontedor @jaymode Hey guys, are there any other changes you would like me to make? Thanks.

jasontedor · 2017-05-03T22:36:25Z

This branch is not up to date with master, can you merge it in and resolve conflicts? Also @jaymode is on vacation this week, and we are going to need his eyes on this next week before it can be pulled.

qwerty4030 · 2017-05-03T23:33:16Z

Sounds good. I'll merge master a bit later this week to catch any future conflicts.

qwerty4030 · 2017-05-08T05:00:49Z

...es/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java

@@ -83,6 +83,9 @@ protected void parseInternalRequest(Request internal, RestRequest restRequest,
     */
    private XContentParser extractRequestSpecificFields(RestRequest restRequest,
                                                        Map<String, Consumer<Object>> bodyConsumers) throws IOException {
+        if(restRequest.hasContentOrSourceParam() == false) {


Jumped the gun on cleaning up this class: turns out that update_by_query allows an empty body so we need to make sure this method does not throw an exception if the request body or source param is missing.

qwerty4030 · 2017-05-09T06:28:59Z

Merged master and disabled the new REST tests for BWC tests.

qwerty4030 · 2017-05-16T05:25:54Z

@jaymode Let me know when you'll have time to take another look. Thanks.

qwerty4030 · 2017-05-28T00:34:42Z

Merged latest master to resolve conflicts and ran gradle check successfully.
@jaymode Please take a look when you have a chance.

jaymode

left two style comments, but otherwise I think this is ready to get in after fixing those and merging master again. @jasontedor do you want to take another look as well?

jaymode · 2017-06-01T13:58:14Z

...es/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java

-            return parser;
+    private XContentParser extractRequestSpecificFields(RestRequest restRequest,
+                                                        Map<String, Consumer<Object>> bodyConsumers) throws IOException {
+        if(restRequest.hasContentOrSourceParam() == false) {


can you add a space between the if and (

done in 08d1749

jaymode · 2017-06-01T13:59:24Z

core/src/test/java/org/elasticsearch/action/bulk/BulkRequestTests.java

+        String emptyObject = "{}\r\n";
+        StringBuilder bulk = new StringBuilder();
+        int emptyLine;
+        if(randomBoolean()) {


space between if and (

done in 08d1749

jasontedor

LGTM.

jaymode · 2017-06-05T17:43:07Z

Pushed 4ad47d2 for 5.x

qwerty4030 · 2017-06-05T19:17:47Z

@jaymode Thanks for the review. Question: do we need to do something different for backport REST tests because they should pass for latest 5.X, but not for versions <= 5.4?

jaymode · 2017-06-05T19:39:50Z

@qwerty4030 I took care of it during the backport by changing the skip version to 5.4.99. I'll also update the tests on master shortly to use that for the skip version

This updates the missing body tests to run against versions >= 5.5.0 after backporting the change to the 5.x branch. See #23497

* master: Add support for clear scroll to high level REST client (elastic#25038) Tiny correction in inner-hits.asciidoc (elastic#25066) Added release notes for 6.0.0-alpha2 Expand index expressions against indices only when managing aliases (elastic#23997) Collapse inner hits rest test should not skip 5.x Settings: Fix secure settings by prefix (elastic#25064) add `exclude_keys` option to KeyValueProcessor (elastic#24876) Test: update missing body tests to run against versions >= 5.5.0 Track EWMA[1] of task execution time in search threadpool executor Removes an invalid assert in resizing big arrays which does not always hold (resizing can result in a smaller size than the current size, while the assert attempted to verify the new size is always greater than the current). Fixed NPEs caused by requests without content. (elastic#23497) Plugins can register pre-configured char filters (elastic#25000) Build: Allow preserving shared dir (elastic#24962) Tests: Make secure settings available from settings builder for tests (elastic#25037)

* master: (1210 commits) Add support for clear scroll to high level REST client (elastic#25038) Tiny correction in inner-hits.asciidoc (elastic#25066) Added release notes for 6.0.0-alpha2 Expand index expressions against indices only when managing aliases (elastic#23997) Collapse inner hits rest test should not skip 5.x Settings: Fix secure settings by prefix (elastic#25064) add `exclude_keys` option to KeyValueProcessor (elastic#24876) Test: update missing body tests to run against versions >= 5.5.0 Track EWMA[1] of task execution time in search threadpool executor Removes an invalid assert in resizing big arrays which does not always hold (resizing can result in a smaller size than the current size, while the assert attempted to verify the new size is always greater than the current). Fixed NPEs caused by requests without content. (elastic#23497) Plugins can register pre-configured char filters (elastic#25000) Build: Allow preserving shared dir (elastic#24962) Tests: Make secure settings available from settings builder for tests (elastic#25037) [TEST] Skip wildcard expansion test due to breaking change Test that gradle and Java version types match (elastic#24943) Include duplicate jar when jarhell check fails Change ScriptContexts to use needs instead of uses$. (elastic#25036) Change `has_child`, `has_parent` queries and `childen` aggregation to work with the new join field type and at the same time maintaining support for the `_parent` meta field type. Remove comma-separated feature parsing for GetIndicesAction ...

* master: (619 commits) Add support for clear scroll to high level REST client (elastic#25038) Tiny correction in inner-hits.asciidoc (elastic#25066) Added release notes for 6.0.0-alpha2 Expand index expressions against indices only when managing aliases (elastic#23997) Collapse inner hits rest test should not skip 5.x Settings: Fix secure settings by prefix (elastic#25064) add `exclude_keys` option to KeyValueProcessor (elastic#24876) Test: update missing body tests to run against versions >= 5.5.0 Track EWMA[1] of task execution time in search threadpool executor Removes an invalid assert in resizing big arrays which does not always hold (resizing can result in a smaller size than the current size, while the assert attempted to verify the new size is always greater than the current). Fixed NPEs caused by requests without content. (elastic#23497) Plugins can register pre-configured char filters (elastic#25000) Build: Allow preserving shared dir (elastic#24962) Tests: Make secure settings available from settings builder for tests (elastic#25037) [TEST] Skip wildcard expansion test due to breaking change Test that gradle and Java version types match (elastic#24943) Include duplicate jar when jarhell check fails Change ScriptContexts to use needs instead of uses$. (elastic#25036) Change `has_child`, `has_parent` queries and `childen` aggregation to work with the new join field type and at the same time maintaining support for the `_parent` meta field type. Remove comma-separated feature parsing for GetIndicesAction ...

Raw requests are supported only by the java yaml test runner and were introduced to test docs snippets. Some yaml tests ended up using them (see elastic#23497) which causes failures for other language clients. This commit migrates those yaml tests to Java tests that send requests through the Java low-level REST client, and also moves the ability to send raw requests to a special client that's only available when testing docs snippets. Closes elastic#25694

Raw requests are supported only by the java yaml test runner and were introduced to test docs snippets. Some yaml tests ended up using them (see #23497) which causes failures for other language clients. This commit migrates those yaml tests to Java tests that send requests through the Java low-level REST client, and also moves the ability to send raw requests to a special client that's only available when testing docs snippets. Closes #25694

jasontedor requested changes Mar 7, 2017

View reviewed changes

qwerty4030 force-pushed the fix/no_content_npe branch from 1ceb605 to 9ba0fcc Compare April 8, 2017 03:32

jasontedor mentioned this pull request Apr 11, 2017

NullPointerException when trying a url with a typo #24047

Closed

jaymode requested changes Apr 17, 2017

View reviewed changes

jasontedor mentioned this pull request May 6, 2017

Improve REST error handling when endpoint does not support HTTP verb, add OPTIONS support #24437

Merged

qwerty4030 commented May 8, 2017

View reviewed changes

dadoonet mentioned this pull request May 16, 2017

Bulk index operation w/o body throws NPE #24701

Closed

clintongormley added :Core/Infra/REST API REST infrastructure and utilities >bug labels May 26, 2017

jasontedor mentioned this pull request May 30, 2017

Index API: NullPointerException when document isn't provided #24968

Closed

jaymode requested changes Jun 1, 2017

View reviewed changes

jasontedor mentioned this pull request Jun 1, 2017

Cannot optimize index due to NPE #25004

Closed

jasontedor approved these changes Jun 1, 2017

View reviewed changes

qwerty4030 deleted the fix/no_content_npe branch June 5, 2017 19:17

jaymode added a commit that referenced this pull request Jun 5, 2017

Test: update missing body tests to run against versions >= 5.5.0

e98d567

This updates the missing body tests to run against versions >= 5.5.0 after backporting the change to the 5.x branch. See #23497

clintongormley added v6.0.0-alpha2 v6.0.0 and removed v6.0.0 v6.0.0-alpha2 labels Jun 6, 2017

tlrx mentioned this pull request Jun 28, 2017

Null pointer when using bulk inserts #25398

Closed

colings86 added v6.0.0-beta1 and removed v6.0.0 labels Jul 31, 2017

javanna mentioned this pull request Aug 3, 2017

[TEST] prevent yaml tests from using raw requests #26044

Merged

javanna mentioned this pull request Sep 25, 2017

Failed to derive xcontent from org.elasticsearch.common.bytes.ChannelBufferBytesReference #8595

Closed

jasontedor mentioned this pull request Nov 14, 2017

logs: [WARN ][r.suppressed ] #27379

Closed

Fixed NPEs caused by requests without content. #23497

Fixed NPEs caused by requests without content. #23497

Conversation

qwerty4030 commented Mar 7, 2017 • edited by danielmitterdorfer Loading

elasticmachine commented Mar 7, 2017

qwerty4030 commented Mar 7, 2017

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymode commented Mar 7, 2017

qwerty4030 commented Mar 17, 2017

jaymode commented Apr 6, 2017

qwerty4030 commented Apr 8, 2017

jasontedor commented Apr 8, 2017

qwerty4030 commented Apr 8, 2017

jasontedor commented Apr 9, 2017

qwerty4030 commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qwerty4030 commented Apr 18, 2017

qwerty4030 commented May 3, 2017

jasontedor commented May 3, 2017 • edited Loading

qwerty4030 commented May 3, 2017

qwerty4030 May 8, 2017 • edited Loading

Choose a reason for hiding this comment

qwerty4030 commented May 9, 2017

qwerty4030 commented May 16, 2017

qwerty4030 commented May 28, 2017

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

jaymode commented Jun 5, 2017

qwerty4030 commented Jun 5, 2017

jaymode commented Jun 5, 2017

qwerty4030 commented Mar 7, 2017 •

edited by danielmitterdorfer

Loading

jasontedor commented May 3, 2017 •

edited

Loading

qwerty4030 May 8, 2017 •

edited

Loading