Watcher: Validate email adresses when storing a watch #34042
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Right now, watches fail on runtime, when invalid email addresses are used. All those fields can be checked on parsing, if no mustache is used in any email address template. In that case we can return immediate feedback, that invalid email addresses should not be specified when trying to store a watch.
|
Pinging @elastic/es-core-infra |
hub-cap
approved these changes
Sep 25, 2018
|
|
||
| public TextTemplate(String template) { | ||
| this.script = null; | ||
| this.inlineTemplate = template; | ||
| this.isUsingMustache = template.contains("{{") && template.indexOf("{{") < template.indexOf("}}"); |
There was a problem hiding this comment.
sneak attack bring back the validation
| this.inlineTemplate = null; | ||
| } | ||
|
|
||
| public TextTemplate(Script script) { | ||
| this.script = script; | ||
| this.inlineTemplate = null; | ||
| this.isUsingMustache = script.getIdOrCode().contains("{{") && |
There was a problem hiding this comment.
can we lazy load this in the getter? maybe remove the word get so we know it "mutates state" and load it once only if we decide to actually validate if we care or not. At least it will appease the contains'ing every string is bad gods.
There was a problem hiding this comment.
I changed this only to check for {{, and it #33978 will introduce a check for every text template, so that this will be called anyway...
There was a problem hiding this comment.
oof, how did i miss that? I guess going backwards thru my review-requested emails is not such a good idea!!
|
still lgtm |
kcm
pushed a commit
that referenced
this pull request
Oct 30, 2018
Right now, watches fail on runtime, when invalid email addresses are used. All those fields can be checked on parsing, if no mustache is used in any email address template. In that case we can return immediate feedback, that invalid email addresses should not be specified when trying to store a watch.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now, watches fail on runtime, when invalid email addresses are
used.
All those fields can be checked on parsing, if no mustache is used in
any email address template. In that case we can return immediate
feedback, that invalid email addresses should not be specified when
trying to store a watch.
Reviewers note: This is rather for discussion than for review. Do you think this makes sense despite the additional check if mustache templates are used or not? When mustache is used, than this can still fail on runtime - even though the majority of the cases that I have seen are indeed using static email addresses when defining a watch.
Closes #33980