Support sending email message via Notifications passthrough API #158
Conversation
Signed-off-by: Mohammad Qureshi <[email protected]>
Signed-off-by: Mohammad Qureshi <[email protected]>
| return try { | ||
| XContentHelper.toXContent(this, XContentType.JSON, EMPTY_PARAMS, true).utf8ToString() | ||
| } catch (e: IOException) { | ||
| super.toString() + " threw " + e.toString() |
There was a problem hiding this comment.
Are we publishing this information outside of the system?
There was a problem hiding this comment.
Alerting is using the response after publishing a message and setting that as the actionResponseContent in runAction.
Since this is the actionResult and we store it in the Alert it can be published externally via ctx.alert in messages.
I had to catch the IOException since detekt was complaining that toString() should not throw exceptions. If there are concerns here, should I log super.toString() + " threw " + e.toString() instead and make the returned string something generic?
There was a problem hiding this comment.
but this would suppress the exception? which might be misleading for its consumers.
There was a problem hiding this comment.
From what I've read, it seems it's not that unusual to suppress the exception here. Conventionally, toString() should not be throwing exceptions.
Here is an example of a thread that I found when looking into it: https://stackoverflow.com/a/42011744
This is probably why the linter also failed on it. I can add a log entry as a follow-up change so the exception is logged at least for debugging purposes. If you disagree with this though, feel free to create an issue for it to discuss further.
There was a problem hiding this comment.
+1 on debug logger here instead
|
Can you serialize and deserialize logic test for |
There was a problem hiding this comment.
LGTM overall. Few minor comments to address. Thanks @qreshi
| return try { | ||
| XContentHelper.toXContent(this, XContentType.JSON, EMPTY_PARAMS, true).utf8ToString() | ||
| } catch (e: IOException) { | ||
| super.toString() + " threw " + e.toString() |
There was a problem hiding this comment.
+1 on debug logger here instead
* Add legacy email support as a legacy Destination type Signed-off-by: Mohammad Qureshi <[email protected]> * Add accountName to LegacyEmailMessage Signed-off-by: Mohammad Qureshi <[email protected]> Signed-off-by: Zelin Hao <[email protected]>
…search-project#158) * Add legacy email support as a legacy Destination type Signed-off-by: Mohammad Qureshi <[email protected]> * Add accountName to LegacyEmailMessage Signed-off-by: Mohammad Qureshi <[email protected]>
Description
The Notifications passthrough API is used for sending legacy messages for consumers of the old notification subproject in Alerting. Alerting has support for Email Destinations that need to use the passthrough API as a fallback mechanism for Destinations that have not migrated.
This PR adds support for email via passthrough.
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.