Skip to content

Check for optional.isPresent() before calling any public mailer methods#1003

Closed
Calvinp wants to merge 2 commits intomasterfrom
fix-no-smtp-config-singularity-death
Closed

Check for optional.isPresent() before calling any public mailer methods#1003
Calvinp wants to merge 2 commits intomasterfrom
fix-no-smtp-config-singularity-death

Conversation

@Calvinp
Copy link
Copy Markdown
Contributor

@Calvinp Calvinp commented Apr 19, 2016

Fixes #1001 by checking if smtpConfig is absent before allowing any public smtp methods to be called.

@wsorenson
Copy link
Copy Markdown
Contributor

Should we remove other checks to compensate for this check?

@Calvinp
Copy link
Copy Markdown
Contributor Author

Calvinp commented Apr 19, 2016

I was thinking that - it could be redundant to check in the private methods now that the public methods all check.

}

public void sendRequestRemovedMail(SingularityRequest request, Optional<String> user, Optional<String> message) {
if (!maybeSmtpConfiguration.isPresent()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this one isn't needed, we check in the first line of sendRequestMail

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, I was going to remove the check in sendRequestMail and leave those in.
The justification being that I'd rather have it be standard that the public methods check and then can safely call the private methods than split between the two.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, fair enough. yeah, can put it at the beginning of the public methods for readability

}

public void sendRequestUnpausedMail(SingularityRequest request, Optional<String> user, Optional<String> message) {
if (!maybeSmtpConfiguration.isPresent()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

similar for check in first line of sendRequestMail

@ssalinas
Copy link
Copy Markdown
Contributor

Closing in favor of #1004

@ssalinas ssalinas closed this Apr 20, 2016
@ssalinas ssalinas deleted the fix-no-smtp-config-singularity-death branch November 1, 2016 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants