Watcher: POST _xpack/watcher/watch/any_watch with any returns an NPE

[elasticmachine]

Original comment by @spinscale:

Trying to post a new watch without any body results in an NPE.

Happens in 6.1.3

POST _xpack/watcher/watch/my_watch

returns

{
  "error": {
    "root_cause": [
      {
        "type": "null_pointer_exception",
        "reason": null
      }
    ],
    "type": "null_pointer_exception",
    "reason": null
  },
  "status": 500
}

stack trace in the logs

[2018-02-07T17:27:44,835][WARN ][r.suppressed             ] path: /_xpack/watcher/watch/my_watch, params: {id=pagerduty_watch}
java.lang.NullPointerException: null
	at org.elasticsearch.xpack.watcher.watch.Watch$Parser.parse(Watch.java:263) ~[x-pack-6.1.3.jar:6.1.3]
	at org.elasticsearch.xpack.watcher.watch.Watch$Parser.parseWithSecrets(Watch.java:252) ~[x-pack-6.1.3.jar:6.1.3]
	at org.elasticsearch.xpack.watcher.transport.actions.put.TransportPutWatchAction.masterOperation(TransportPutWatchAction.java:80) [x-pack-6.1.3.jar:6.1.3]
	at org.elasticsearch.xpack.watcher.transport.actions.put.TransportPutWatchAction.masterOperation(TransportPutWatchAction.java:55) [x-pack-6.1.3.jar:6.1.3]
	at org.elasticsearch.action.support.master.TransportMasterNodeAction.masterOperation(TransportMasterNodeAction.java:88) [elasticsearch-6.1.3.jar:6.1.3]
	at org.elasticsearch.action.support.master.TransportMasterNodeAction$AsyncSingleAction$2.doRun(TransportMasterNodeAction.java:167) [elasticsearch-6.1.3.jar:6.1.3]
	at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:637) [elasticsearch-6.1.3.jar:6.1.3]
	at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) [elasticsearch-6.1.3.jar:6.1.3]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) [?:?]
	at java.lang.Thread.run(Thread.java:844) [?:?]

[adhulipa]

@dakrone, @spinscale I'm pinging you directly because I noticed that you edited this file most recently (in march, and february) & I believe you may have the most context about the issue. Also, I'd like to contribute a patch here and wanted to make sure I'm reaching out to the relevant people to give me the go-ahead 😄

Root cause

The root cause is a NullPointerException when xContentType is null in the parse() method in WatchParser.java:113.

Should the fix just be a matter of a null check within the method definition or should we handle XContentType being null everywhere else too?

XContentType is null within the request when the "Content-Type" header parameter is not present in the request.

Repro steps

I reproduced this by running a couple of curl requests on my local build of es:

#f03c15 Input without Content-Type:

curl -XPOST 'elastic-admin:elastic-password@localhost:9200/_xpack/watcher/watch/my_watch' | jq

#f03c15 Output with NPE:

# Aditya at Adityas-MacBook-Pro.local in ~/Projects/elastic/downloads/kibana-7.0.0-alpha1-SNAPSHOT-darwin-x86_64  [22:17:17]
λ curl -XPOST 'elastic-admin:elastic-password@localhost:9200/_xpack/watcher/watch/my_watch' | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   133  100   133    0     0     31      0  0:00:04  0:00:04 --:--:--    31
{
  "error": {
    "root_cause": [
      {
        "type": "null_pointer_exception",
        "reason": null
      }
    ],
    "type": "null_pointer_exception",
    "reason": null
  },
  "status": 500
}

#c5f015 Input with Content-Type:

curl -XPOST -H "Content-Type: application/json" 'elastic-admin:elastic-password@localhost:9200/_xpack/watcher/watch/my_watch' | jq

#c5f015 Output with no NPE:

# Aditya at Adityas-MacBook-Pro.local in ~/Projects/elastic/downloads/kibana-7.0.0-alpha1-SNAPSHOT-darwin-x86_64  [22:16:04]
λ curl -XPOST -H "Content-Type: application/json" 'elastic-admin:elastic-password@localhost:9200/_xpack/watcher/watch/my_watch' | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   203  100   203    0     0      3      0  0:01:07  0:00:57  0:00:10    41
{
  "error": {
    "root_cause": [
      {
        "type": "parse_exception",
        "reason": "could not parse watch [my_watch]. null token"
      }
    ],
    "type": "parse_exception",
    "reason": "could not parse watch [my_watch]. null token"
  },
  "status": 400
}

[spinscale]

I think we should check in the PutWatchRequest.validate() method for the existence of this, so we catch this issue in the transport and in the REST layer. This would catch this issue early on, and we can rely in the watch parser that this value is not null.

If you want to give it a try, feel free and go ahead!

[adhulipa]

Hi @spinscale
I made a fix and tested it out locally:

λ curl -XPOST 'elastic-admin:elastic-password@localhost:9200/_xpack/watcher/watch/my_watch' | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   247  100   247    0     0  29429      0 --:--:-- --:--:-- --:--:-- 30875
{
  "error": {
    "root_cause": [
      {
        "type": "action_request_validation_exception",
        "reason": "Validation Failed: 1: Content-Type is missing;"
      }
    ],
    "type": "action_request_validation_exception",
    "reason": "Validation Failed: 1: Content-Type is missing;"
  },
  "status": 400
}

But the tests ./gradlew check are taking a long time. It's been running for 30mins and has completed only 12%. Is that expected?

I didn't see any specific tests for PutWatchRequest. Should I create a new test file for this file or am I missing something?

Also, I opened a pull request of what I have so far just so that I can get a sense of if I'm doing it correctly or not.

Looking forward to your feedback. Thanks for all your help! :)

[cbuescher]

But the tests ./gradlew check are taking a long time. It's been running for 30mins and has completed only 12%. Is that expected?

Unfortunaltely running the whole test suite takes quiet long now. It is possible to run the tests for only part of the project though, in your case the changes are in the "x-pack" module, so running ./gradlew gradle :x-pack:plugin:check or maybe even only ./gradlew gradle :x-pack:plugin:watcher:check should be a good start in this case. We run full CI tests on PRs later anyway that might catch other issues.

I didn't see any specific tests for PutWatchRequest. Should I create a new test file for this file or am I missing something?

I think you are correct, I couldn't find any tests yet. Would be great to start with one then, I will comment on the PR.

[adhulipa]

Hey @cbuescher , I'm having trouble with the integ tests.
Is this correct? -> running ./gradlew :x-pack:plugin:watcher:integTest will only run the integTests for watcher plugin which are defined in elasticsearch/x-pack/plugin/src/test/resources/rest-api-spec/test/watcher/

How can I intentionally break an integ test to understand how the integration tests work?
For example, I'm changing the line 50 from id: "my_watch" to id: "not_any_at_all_my_watch" in watcher/put_watch/70_put_watch_with_index_action_using_id.yml and calling ./gradlew :x-pack:plugin:watcher:integTest. Then I should expect a FAILURE, right?

[cbuescher]

running ./gradlew :x-pack:plugin:watcher:integTest will only run the integTests for watcher plugin which are defined in elasticsearch/x-pack/plugin/src/test/resources/rest-api-spec/test/watcher/

I would have thought so, but the change that you mentioned doesn't fail, so I guess you either need to run all x-pack tests (" ./gradlew :x-pack:plugin:integTest"), but I think you can also filter out only the watcher tests, but then the incantation gets slightly more verbose. Try:

./gradlew :x-pack:plugin:integTestRunner -Dtests.class=org.elasticsearch.xpack.test.rest.XPackRestIT -Dtests.method="test {p0=watcher/*}"

That seems to work on my machine to run only the watcher tests and will fail with the modification you described above. You can find a little bit more information about the structure of the test files in a somewhat hidden README in /rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc
In your special case I think you expect exceptions, so you should take a look at the catch clauses in some of the tests. Hope this helps, otherwise don't hesitate to ping me again.