Slack message empty text #31596

albendz · 2018-06-27T03:46:48Z

Adding constructors to the slack message class to support more flexibility in null parameters. Additionally provided setters for use-cases where this can be added before message template is created.

#30071

Replacing old pull request: #31288

…force that one of text and attachments is present;fix test verification methods

…slack_message_empty_text

tvernum · 2018-06-27T03:54:08Z

...r/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessage.java

+    // Slack with fail to send a message if it has no attachments AND no text.
+    // Slack will succeed with attachments OR text.
+    public SlackMessage(String from, String[] to, String icon, Attachment[] attachments) {
+        this(from, to, icon, null, null); // Empty text


Shouldn't attachments be used here?

++, I think this is a copy-paste from the last commit just with that missing.

elasticmachine · 2018-06-27T03:54:41Z

Pinging @elastic/es-core-infra

…slack_message_empty_text

hub-cap · 2018-06-27T18:37:09Z

@elasticmachine test this please

hub-cap · 2018-06-27T19:04:47Z

This looks good other than that one change requested. I manually tested it against our slack, just to double check that it works. Once youve fixed that up, ill do the needful and get this merged in to master and 6.x (assuming the tests pass, which ill keep an eye on too)!

albendz · 2018-06-27T22:43:34Z

Thank you for reviewing; you are great with responsiveness. I'll work on getting this fixed up this week.

hub-cap · 2018-06-28T13:53:36Z

@albendz there was a failure in your test as well. You can reproduce it locally via the command in the wall of text thats in the build log. Note the REPRODUCE line in the message below. I typically just search for REPRODUCE if i see a build failed, in hopes that I can narrow it down to just one test instead of all the tests :) Thx again for the submission!!

14:59:20 Suite: org.elasticsearch.xpack.watcher.notification.slack.message.SlackMessageTests
14:59:20   1> [2018-06-27T16:59:20,230][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testTemplateParse]: before test
14:59:20   1> [2018-06-27T16:59:20,231][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testTemplateParse]: after test
14:59:20   1> [2018-06-27T16:59:20,234][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testUrlPathIsFiltered]: before test
14:59:20   1> [2018-06-27T16:59:20,236][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testUrlPathIsFiltered]: after test
14:59:20   1> [2018-06-27T16:59:20,239][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testTemplateParseSelfGenerated]: before test
14:59:20   1> [2018-06-27T16:59:20,239][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testTemplateParseSelfGenerated]: after test
14:59:20   1> [2018-06-27T16:59:20,243][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testToXContent]: before test
14:59:20   1> [2018-06-27T16:59:20,243][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testToXContent]: after test
14:59:20   1> [2018-06-27T16:59:20,247][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testTemplateRender]: before test
14:59:20   1> [2018-06-27T16:59:20,247][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testTemplateRender]: after test
14:59:20   1> [2018-06-27T16:59:20,249][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testCanHaveNullText]: before test
14:59:20   1> [2018-06-27T16:59:20,249][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testCanHaveNullText]: after test
14:59:20   2> REPRODUCE WITH: ./gradlew :x-pack:plugin:watcher:test -Dtests.seed=F00754B260566339 -Dtests.class=org.elasticsearch.xpack.watcher.notification.slack.message.SlackMessageTests -Dtests.method="testCanHaveNullText" -Dtests.security.manager=true -Dtests.locale=es-NI -Dtests.timezone=America/Goose_Bay
14:59:20 FAILURE 0.01s J2 | SlackMessageTests.testCanHaveNullText <<< FAILURES!
14:59:20    > Throwable #1: java.lang.AssertionError
14:59:20    > 	at __randomizedtesting.SeedInfo.seed([F00754B260566339:6906370860684020]:0)
14:59:20    > 	at org.elasticsearch.xpack.watcher.notification.slack.message.SlackMessageTests.testCanHaveNullText(SlackMessageTests.java:606)
14:59:20    > 	at java.lang.Thread.run(Thread.java:748)
14:59:20   1> [2018-06-27T16:59:20,260][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testCanHaveNullAttachments]: before test
14:59:20   1> [2018-06-27T16:59:20,260][INFO ][o.e.x.w.n.s.m.SlackMessageTests] [testCanHaveNullAttachments]: after test

albendz · 2018-07-03T23:44:25Z

I have fixed the error but clearly I didn't run the right command before I posted this. I have updated this after running targets ':x-pack:plugin:watcher:clean :x-pack:plugin:watcher:build precommit'. Let me know if I should be also running others.

hub-cap · 2018-07-05T14:43:02Z

So I chatted with @rjernst and he and I agreed that we probably do not need the extra constructors here. I then tested it out in our real slack integration tests and these fields are in fact already nullable fields. If you do not include these fields in the payload you submit, then they do not get sent to slack, which is what we want.

What we should do instead is just use the existing constructor (no reason to add another one for this), make both text and attachments @Nullable. This is from org.elasticsearch.common and there are other places in the codebase its used, so you can look at them. After that, you should do a check in the constructor that verifies that they are not both null, and if they are, throw an IllegalArgumentException stating they cannot both be null. I also checked this case and the slack API bombs out, which is what we are trying to prevent :)

Then of course have a test that nulls them both out and ensures the exception is thrown via expectThrows.

Sorry for the churn on this, but I think this is the best solution, and it involves adding less things, so less surface area to have to test!

albendz · 2018-07-05T17:01:59Z

No worries! Better to vet the code before it's checked in. Summarizing what I will do:

Remove additional constructors
Add @nullable to existing constructor fields for attachments and text
Add verification that (text == null) XOR (attachments == null)
Add test to verify both attachments and text cannot be null
Keep added tests that verify text can be null and separately attachments can be null

hub-cap · 2018-07-05T18:33:51Z

sounds like a great plan!

… text and attachments not null

…ty_text

hub-cap

One small change and then we can call this done!

hub-cap · 2018-07-06T14:35:50Z

.../test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java

@@ -49,7 +49,7 @@ public void testToXContent() throws Exception {
        }
        String icon = randomBoolean() ? null : randomAlphaOfLength(10);
        String text = randomBoolean() ? null : randomAlphaOfLength(50);
-        Attachment[] attachments = randomBoolean() ? null : new Attachment[randomIntBetween(0, 2)];
+        Attachment[] attachments = !(randomBoolean() && text == null) ? null : new Attachment[randomIntBetween(0, 2)];


we have a convention to not use the ! operator in statements like this. Please use == false instead.

(text != null && randomBoolean()) ? null : new .....

This should also work

Out of curiosity, why?

I'll make this change.

hub-cap · 2018-07-06T14:36:57Z

@elasticmachine test this please.

hub-cap · 2018-07-07T15:35:58Z

@elasticmachine test this please.

…sage_empty_text

hub-cap · 2018-07-09T14:54:20Z

@elasticmachine test this please.

I pushed some new refs to this to see if we can get CI passing!

hub-cap · 2018-07-10T14:47:52Z

I have merged it. Ill begin the backport process now and thank you for your contribution!

ywelsch · 2018-07-11T09:15:54Z

This failed in #31948
@hub-cap Hold off on backporting until fixing this please

Slack accepts an empty text or attachments, but not both. This commit ensures that both are not empty when creating a watch. Closes #30071 Replacing old pull request: #31288

hub-cap · 2018-07-11T15:01:08Z

Bug fixed and backported. Ty again @albendz !!

* master: [TEST] Mute SlackMessageTests.testTemplateRender Docs: Explain closing the high level client [ML] Re-enable memory limit integration tests (#31328) [test] disable packaging tests for suse boxes Add nio transport to security plugin (#31942) XContentTests : Insert random fields at random positions (#30867) Force execution of fetch tasks (#31974) Fix unreachable error condition in AmazonS3Fixture (#32005) Tests: Fix SearchFieldsIT.testDocValueFields (#31995) Add Expected Reciprocal Rank metric (#31891) [ML] Get ForecastRequestStats doc in RestoreModelSnapshotIT (#31973) SQL: Add support for single parameter text manipulating functions (#31874) [ML] Ensure immutability of MlMetadata (#31957) Tests: Mute SearchFieldsIT.testDocValueFields() muted tests due to #31940 Work around reported problem in eclipse (#31960) Move build integration tests out of :buildSrc project (#31961) Tests: Remove use of joda time in some tests (#31922) [Test] Reactive 3rd party tests on CI (#31919) SQL: Support for escape sequences (#31884) SQL: HAVING clause should accept only aggregates (#31872) Docs: fix typo in datehistogram (#31972) Switch url repository rest tests to new style requests (#31944) Switch reindex tests to new style requests (#31941) Docs: Added note about cloud service to installation and getting started [DOCS] Removes alternative docker pull example (#31934) Add Snapshots Status API to High Level Rest Client (#31515) ingest: date_index_name processor template resolution (#31841) Test: fix null failure in watcher test (#31968) Switch test framework to new style requests (#31939) Switch low level rest tests to new style Requests (#31938) Switch high level rest tests to new style requests (#31937) [ML] Mute test failing due to Java 11 date time format parsing bug (#31899) [TEST] Mute SlackMessageTests.testTemplateRender Fix assertIngestDocument wrongfully passing (#31913) Remove unused reference to filePermissionsCache (#31923) rolling upgrade should use a replica to prevent relocations while running a scroll HLREST: Bundle the x-pack protocol project (#31904) Increase logging level for testStressMaybeFlush Added lenient flag for synonym token filter (#31484) [X-Pack] Beats centralized management: security role + licensing (#30520) HLRest: Move xPackInfo() to xPack().info() (#31905) Docs: add security delete role to api call table (#31907) [test] port archive distribution packaging tests (#31314) Watcher: Slack message empty text (#31596) [ML] Mute failing DetectionRulesIT.testCondition() test Fix broken NaN check in MovingFunctions#stdDev() (#31888) Date: Add DateFormatters class that uses java.time (#31856) [ML] Switch native QA tests to a 3 node cluster (#31757) Change trappy float comparison (#31889) Fix building AD URL from domain name (#31849) Add opaque_id to audit logging (#31878) re-enable backcompat tests add support for is_write_index in put-alias body parsing (#31674) Improve release notes script (#31833) [DOCS] Fix broken link in painless example Handle missing values in painless (#30975) Remove the ability to index or query context suggestions without context (#31007) Ingest: Enable Templated Fieldnames in Rename (#31690) [Docs] Fix typo in the Rollup API Quick Reference (#31855) Ingest: Add ignore_missing option to RemoveProc (#31693) Add template config for Beat state to X-Pack Monitoring (#31809) Watcher: Add ssl.trust email account setting (#31684) Remove link to oss-MSI (#31844) Painless: Restructure Definition/Whitelist (#31879) HLREST: Add x-pack-info API (#31870)

* 6.x: Force execution of fetch tasks (#31974) [TEST] Mute SlackMessageTests.testTemplateRender Docs: Explain closing the high level client [test] disable java packaging tests for suse XContentTests : Insert random fields at random positions (#30867) Add Get Snapshots High Level REST API (#31980) Fix unreachable error condition in AmazonS3Fixture (#32005) [6.x][ML] Ensure immutability of MlMetadata (#31994) Add Expected Reciprocal Rank metric (#31891) SQL: Add support for single parameter text manipulating functions (#31874) muted tests due to #31940 Work around reported problem in eclipse (#31960) Move build integration tests out of :buildSrc project (#31961) [Test] Reactive 3rd party tests on CI (#31919) Fix assertIngestDocument wrongfully passing (#31913) (#31951) SQL: Support for escape sequences (#31884) SQL: HAVING clause should accept only aggregates (#31872) Docs: fix typo in datehistogram (#31972) Switch url repository rest tests to new style requests (#31944) Switch reindex tests to new style requests (#31941) Switch test framework to new style requests (#31939) Docs: Added note about cloud service to installation and getting started [DOCS] Removes alternative docker pull example (#31934) ingest: date_index_name processor template resolution (#31841) Test: fix null failure in watcher test (#31968) Watcher: Slack message empty text (#31596) Switch low level rest tests to new style Requests (#31938) Switch high level rest tests to new style requests (#31937) HLREST: Bundle the x-pack protocol project (#31904) [ML] Mute test failing due to Java 11 date time format parsing bug (#31899) Increase logging level for testStressMaybeFlush rolling upgrade should use a replica to prevent relocations while running a scroll [test] port archive distribution packaging tests (#31314) HLRest: Move xPackInfo() to xPack().info() (#31905) Increase logging level for :qa:rolling-upgrade Backport: Add template config for Beat state to X-Pack Monitoring (#31809) (#31893) Fix building AD URL from domain name (#31849) Fix broken NaN check in MovingFunctions#stdDev() (#31888) Change trappy float comparison (#31889) Add opaque_id to audit logging (#31878) add support for is_write_index in put-alias body parsing (#31674) Ingest: Enable Templated Fieldnames in Rename (#31690) (#31896) Ingest: Add ignore_missing option to RemoveProc (#31693) (#31892) [Docs] Fix typo in the Rollup API Quick Reference (#31855) Watcher: Add ssl.trust email account setting (#31684) [PkiRealm] Invalidate cache on role mappings change (#31510) [Security] Check auth scheme case insensitively (#31490) HLREST: Add x-pack-info API (#31870) Remove link to oss-MSI (#31844) Painless: Restructure Definition/Whitelist (#31879)

albendz added 5 commits June 12, 2018 19:49

Make text message not required in constructor for slack

9c8e69c

Remove unnecessary comments in test file

b6876b4

Bring back final in all message attributes; change constructors to en…

226300b

…force that one of text and attachments is present;fix test verification methods

Merge branch 'master' of git://github.com/elastic/elasticsearch into …

9e7a501

…slack_message_empty_text

Correct test method names so that they run

b677906

albendz mentioned this pull request Jun 27, 2018

Allow null message in SlackMessage #31288

Closed

jasontedor requested a review from hub-cap June 27, 2018 03:54

tvernum reviewed Jun 27, 2018

View reviewed changes

jasontedor added the :Data Management/Watcher label Jun 27, 2018

Merge branch 'master' of git://github.com/elastic/elasticsearch into …

f46dd0e

…slack_message_empty_text

hub-cap added >enhancement v7.0.0 v6.4.0 labels Jun 27, 2018

Fix incorrect constructor

3b3f782

albendz added 3 commits July 5, 2018 21:47

Remove additional constructors; make arguments nullable; add test for…

b260d02

… text and attachments not null

Merge remote-tracking branch 'upstream/master' into slack_message_emp…

c9b3e92

…ty_text

Remove some randomness from SlackMessage tests

190b19c

hub-cap suggested changes Jul 6, 2018

View reviewed changes

Expand the statement wide ! into the individual terms

c126182

Merge remote-tracking branch 'upstream/master' into albendz/slack_mes…

3a68cd7

…sage_empty_text

hub-cap approved these changes Jul 9, 2018

View reviewed changes

hub-cap merged commit 8ec33b7 into elastic:master Jul 10, 2018

hub-cap self-assigned this Jul 10, 2018

hub-cap added the backport pending label Jul 10, 2018

albendz deleted the slack_message_empty_text branch July 11, 2018 04:33

spinscale mentioned this pull request Jul 11, 2018

[test failure] :x-pack:plugin:watcher:test can fail with java.lang.IllegalArgumentException: Both text and attachments cannot be null. #31948

Closed

hub-cap removed the backport pending label Jul 11, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slack message empty text #31596

Slack message empty text #31596

albendz commented Jun 27, 2018

tvernum Jun 27, 2018 •

edited

Loading

hub-cap Jun 27, 2018

elasticmachine commented Jun 27, 2018

hub-cap commented Jun 27, 2018

hub-cap commented Jun 27, 2018

albendz commented Jun 27, 2018

hub-cap commented Jun 28, 2018

albendz commented Jul 3, 2018

hub-cap commented Jul 5, 2018

albendz commented Jul 5, 2018

hub-cap commented Jul 5, 2018

hub-cap left a comment

hub-cap Jul 6, 2018

albendz Jul 7, 2018

hub-cap commented Jul 6, 2018

hub-cap commented Jul 7, 2018

hub-cap commented Jul 9, 2018

hub-cap commented Jul 10, 2018

ywelsch commented Jul 11, 2018

hub-cap commented Jul 11, 2018

Slack message empty text #31596

Slack message empty text #31596

Conversation

albendz commented Jun 27, 2018

tvernum Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

hub-cap Jun 27, 2018

Choose a reason for hiding this comment

elasticmachine commented Jun 27, 2018

hub-cap commented Jun 27, 2018

hub-cap commented Jun 27, 2018

albendz commented Jun 27, 2018

hub-cap commented Jun 28, 2018

albendz commented Jul 3, 2018

hub-cap commented Jul 5, 2018

albendz commented Jul 5, 2018

hub-cap commented Jul 5, 2018

hub-cap left a comment

Choose a reason for hiding this comment

hub-cap Jul 6, 2018

Choose a reason for hiding this comment

albendz Jul 7, 2018

Choose a reason for hiding this comment

hub-cap commented Jul 6, 2018

hub-cap commented Jul 7, 2018

hub-cap commented Jul 9, 2018

hub-cap commented Jul 10, 2018

ywelsch commented Jul 11, 2018

hub-cap commented Jul 11, 2018

tvernum Jun 27, 2018 •

edited

Loading