Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate xContentType in PutWatchRequest. #31088

Merged
merged 4 commits into from
Jun 12, 2018
Merged

Validate xContentType in PutWatchRequest. #31088

merged 4 commits into from
Jun 12, 2018

Conversation

adhulipa
Copy link
Contributor

@adhulipa adhulipa commented Jun 5, 2018

This closes #30057. Trying to post a new watch by
executing POST _xpack/watcher/watch/my_watch
without any body will result in a NullPointerException.

This change fixes that by validating that POST & PUT requests always
have a non-null body.

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS that we support?
  • If you are submitting this code for a class then read our policy for that.

@adhulipa adhulipa mentioned this pull request Jun 5, 2018
Closed
cbuescher
cbuescher reviewed Jun 5, 2018
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks good, but we need a test for it. I found WatchRequestValidationTests which seems to group some existing validation tests (see testPutWatch* tests). I think it would be best to add a case here.
In addition to the unit test, there are integration tests in x-pack/plugin/src/test/resources/rest-api-spec/test/watcher that check basic functionality by going through the rest layer. I think it would be great to also add a test case with an empty body and check the expected exception there. The test use a simplifies yaml format, maybe you can take a look at the watcher/put_watch subdirectory and try adding by modifying an existing test there.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@adhulipa
Copy link
Contributor Author

adhulipa commented Jun 6, 2018

Thanks @cbuescher
I have updated the unit tests in my local build. Still figuring out the integ tests. Will post an update soon!

@adhulipa
Copy link
Contributor Author

adhulipa commented Jun 8, 2018

Thanks! @cbuescher & @spinscale for answering all my questions!
I have been able to write unit & integration tests and successfully validate this change.
You should now be able to see all the relevant commits.

I'd be happy to share more info such as clarification, testing steps or anything else if you'd like.

cbuescher
cbuescher reviewed Jun 8, 2018
View reviewed changes
.../src/test/resources/rest-api-spec/test/watcher/put_watch/90_put_watch_content_validation.yml Outdated
@@ -0,0 +1,12 @@
---
"Test empty body is rejected by put watch":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it makes sense to just add this test case to 10_basic, since it doesn't require any special setup and just tests a general error edge case. But adding another test file like this is also fine I guess, mostly a matter of taste.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok. I'm still learning the layout of the code directories. I'll make the change you suggested.

cbuescher
cbuescher approved these changes Jun 8, 2018
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adhulipa thanks for adding the integration tests, I left a comment which you might or might not want to adress, but I think it looks good in general. I will kick of a whole run of CI tests

@cbuescher
Copy link
Member

@adhulipa there are some test failures that we were having a lot on master a few days ago, probably when you branched of. Would you mind rebasing your commit on top of the current master branch?

@adhulipa
Copy link
Contributor Author

adhulipa commented Jun 8, 2018

Thanks for the updates @cbuescher.
I will update this pull-request shortly.

@adhulipa
Validate xContentType in PutWatchRequest.
67ccbe7 
This closes elastic#30057. Trying to post a new watch by
executing `POST _xpack/watcher/watch/my_watch`
without any body will result in a NullPointerException.

This change fixes that by validating that POST & PUT requests always
have a non-null body.
@adhulipa
Unit tests for xContentType validation in PutWatchRequest.
013252d
@adhulipa
Integration tests for xContentType validation in PutWatchRequest.
610c0d6
@adhulipa
Moved validation integration test into 10_basic suite.
e4d2613
@cbuescher
Copy link
Member

Great, thanks @adhulipa. Lets see how CI likes it now. @elasticmachine test this please

@adhulipa
Copy link
Contributor Author

Hi @cbuescher : Looks like the tests went through fine. I was curious about what the next step would be to get this PR merged? It's my first PR so I'm not sure if there's anything more I need to do.

@cbuescher
Copy link
Member

@adhulipa thanks for the ping, I will merge it to master and the 6.4 branch (meaning this will be released with the 6.4 minor version).
Thanks a lot for picking this up and working through this PR.

@cbuescher cbuescher merged commit 72f8199 into elastic:master Jun 12, 2018
cbuescher pushed a commit that referenced this pull request Jun 12, 2018
@adhulipa
Validate xContentType in PutWatchRequest. (#31088)
b9944aa 
Trying to post a new watch without any body currently results in a 
NullPointerException. This change fixes that by validating that 
Post and Put requests always have a body.

Closes #30057
dnhatn added a commit that referenced this pull request Jun 14, 2018
@dnhatn
Merge branch 'master' into ccr
2cffd85 
* master:
  Remove RestGetAllAliasesAction (#31308)
  Temporary fix for broken build
  Reenable Checkstyle's unused import rule (#31270)
  Remove remaining unused imports before merging #31270
  Fix non-REST doc snippet
  [DOC] Extend SQL docs
  Immediately flush channel after writing to buffer (#31301)
  [DOCS] Shortens ML API intros
  Use quotes in the call invocation (#31249)
  move security ingest processors to a sub ingest directory (#31306)
  Add 5.6.11 version constant.
  Fix version detection.
  SQL: Whitelist SQL utility class for better scripting (#30681)
  [Docs] All Rollup docs experimental, agg limitations, clarify DeleteJob (#31299)
  CCS: don't proxy requests for already connected node (#31273)
  Mute ScriptedMetricAggregatorTests testSelfReferencingAggStateAfterMap
  [test] opensuse packaging turn up debug logging
  Add unreleased version 6.3.1
  Removes experimental tag from scripted_metric aggregation (#31298)
  [Rollup] Metric config parser must use builder so validation runs (#31159)
  [ML] Check licence when datafeeds use cross cluster search  (#31247)
  Add notion of internal index settings (#31286)
  Test: Remove broken yml test feature (#31255)
  REST hl client: cluster health to default to cluster level (#31268)
  [ML] Update test thresholds to account for changes to memory control (#31289)
  Log warnings when cluster state publication failed to some nodes (#31233)
  Fix AntFixture waiting condition (#31272)
  Ignore numeric shard count if waiting for ALL (#31265)
  [ML] Implement new rules design (#31110)
  index_prefixes back-compat should test 6.3 (#30951)
  Core: Remove plain execute method on TransportAction (#30998)
  Update checkstyle to 8.10.1 (#31269)
  Set analyzer version in PreBuiltAnalyzerProviderFactory (#31202)
  Modify pipelining handlers to require full requests (#31280)
  Revert upgrade to Netty 4.1.25.Final (#31282)
  Use armored input stream for reading public key (#31229)
  Fix Netty 4 Server Transport tests. Again.
  REST hl client: adjust wait_for_active_shards param in cluster health (#31266)
  REST high-level Client: remove deprecated API methods (#31200)
  [DOCS] Mark SQL feature as experimental
  [DOCS] Updates machine learning custom URL screenshots (#31222)
  Fix naming conventions check for XPackTestCase
  Fix security Netty 4 transport tests
  Fix race in clear scroll (#31259)
  [DOCS] Clarify audit index settings when remote indexing (#30923)
  Delete typos in SAML docs (#31199)
  REST high-level client: add Cluster Health API (#29331)
  [ML][TEST] Mute tests using rules (#31204)
  Support RequestedAuthnContext (#31238)
  SyncedFlushResponse to implement ToXContentObject (#31155)
  Add Get Aliases API to the high-level REST client (#28799)
  Remove some line length supressions (#31209)
  Validate xContentType in PutWatchRequest. (#31088)
  [INGEST] Interrupt the current thread if evaluation grok expressions take too long (#31024)
  Suppress extras FS on caching directory tests
  Revert "[DOCS] Added 6.3 info & updated the upgrade table. (#30940)"
  Revert "Fix snippets in upgrade docs"
  Fix snippets in upgrade docs
  [DOCS] Added 6.3 info & updated the upgrade table. (#30940)
  LLClient: Support host selection (#30523)
  Upgrade to Netty 4.1.25.Final (#31232)
  Enable custom credentials for core REST tests (#31235)
  Move ESIndexLevelReplicationTestCase to test framework (#31243)
  Encapsulate Translog in Engine (#31220)
  HLRest: Add get index templates API (#31161)
  Remove all unused imports and fix CRLF (#31207)
  [Tests] Fix self-referencing tests
  [TEST] Fix testRecoveryAfterPrimaryPromotion
  [Docs] Remove mention pattern files in Grok processor (#31170)
  Use stronger write-once semantics for Azure repository (#30437)
  Don't swallow exceptions on replication (#31179)
  Limit the number of concurrent requests per node (#31206)
  Call ensureNoSelfReferences() on _agg state variable after scripted metric agg script executions (#31044)
  Move java version checker back to its own jar (#30708)
  [test] add fix for rare virtualbox error (#31212)
dnhatn added a commit that referenced this pull request Jun 14, 2018
@dnhatn
Merge branch '6.x' into ccr-6.x
51a8f86 
* 6.x:
  SQL: Fix build on Java 10
  [Tests] Mutualize fixtures code in BaseHttpFixture (#31210)
  [TEST] Fix RemoteClusterClientTests#testEnsureWeReconnect
  [ML] Update test thresholds to account for changes to memory control (#31289)
  Reenable Checkstyle's unused import rule (#31270)
  [ML] Check licence when datafeeds use cross cluster search  (#31247)
  Fix non-REST doc snippet
  [DOC] Extend SQL docs
  [DOCS] Shortens ML API intros
  Use quotes in the call invocation (#31249)
  move security ingest processors to a sub ingest directory (#31306)
  SQL: Whitelist SQL utility class for better scripting (#30681)
  Add 5.6.11 version constant.
  Fix version detection.
  [Docs] All Rollup docs experimental, agg limitations, clarify DeleteJob (#31299)
  Add missing release notes.
  Security: fix token bwc with pre 6.0.0-beta2 (#31254)
  Fix compilation error in UpdateSettingsIT (#31304)
  Test: Remove broken yml test feature (#31255)
  Add unreleased version 6.3.1
  [Rollup] Metric config parser must use builder so validation runs (#31159)
  Removes experimental tag from scripted_metric aggregation (#31298)
  [DOCS] Removes coming tag from 6.3.0 release notes
  6.3 release notes.
  Add notion of internal index settings (#31286)
  REST high-level client: add Cluster Health API (#29331)
  Remove leftover usage of deprecated client API
  SyncedFlushResponse to implement ToXContentObject (#31155)
  Add Get Aliases API to the high-level REST client (#28799)
  HLRest: Add get index templates API (#31161)
  Log warnings when cluster state publication failed to some nodes (#31233)
  Fix AntFixture waiting condition (#31272)
  [TEST] Mute RecoveryIT.testHistoryUUIDIsGenerated
  Ignore numeric shard count if waiting for ALL (#31265)
  Update checkstyle to 8.10.1 (#31269)
  Set analyzer version in PreBuiltAnalyzerProviderFactory (#31202)
  Revert upgrade to Netty 4.1.25.Final (#31282)
  Use armored input stream for reading public key (#31229)
  [DOCS] Added 'fail_on_unsupported_field' param to MLT. Closes #28008 (#31160)
  Fix Netty 4 Server Transport tests. Again.
  [DOCS] Fixed typo.
  [DOCS] Added release highlights for 6.3 (#31256)
  [DOCS] Mark SQL feature as experimental
  [DOCS] Updates machine learning custom URL screenshots (#31222)
  Fix naming conventions check for XPackTestCase
  Fix security Netty 4 transport tests
  Fix race in clear scroll (#31259)
  [DOCS] Clarify audit index settings when remote indexing (#30923)
  [ML][TEST] Mute tests using rules (#31204)
  Support RequestedAuthnContext (#31238)
  Validate xContentType in PutWatchRequest. (#31088)
  [INGEST] Interrupt the current thread if evaluation grok expressions take too long (#31024)
  Upgrade to Netty 4.1.25.Final (#31232)
  Suppress extras FS on caching directory tests
  Revert "[DOCS] Added 6.3 info & updated the upgrade table. (#30940)"
  Revert "Fix snippets in upgrade docs"
  Fix snippets in upgrade docs
  [DOCS] Added 6.3 info & updated the upgrade table. (#30940)
  Enable custom credentials for core REST tests (#31235)
  Move ESIndexLevelReplicationTestCase to test framework (#31243)
  Encapsulate Translog in Engine (#31220)
  [DOCS] Adds machine learning 6.3.0 release notes (#31217)
  Remove all unused imports and fix CRLF (#31207)
  [TEST] Fix testRecoveryAfterPrimaryPromotion
  [Docs] Remove mention pattern files in Grok processor (#31170)
  Use stronger write-once semantics for Azure repository (#30437)
  Don't swallow exceptions on replication (#31179)
  Compliant SAML Response destination check (#31175)
  Move java version checker back to its own jar (#30708)
  TEST:  Retry synced-flush if ongoing ops on primary (#30978)
  [test] add fix for rare virtualbox error (#31212)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbuescher cbuescher cbuescher approved these changes

Assignees
No one assigned
Labels
>bug :Data Management/Watcher v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Watcher: POST _xpack/watcher/watch/any_watch with any returns an NPE
6 participants
@adhulipa @elasticmachine @cbuescher @colings86 @mayya-sharipova @lcawl