Skip to content

fix:handle edge case for empty recipients to prevent crash#1537

Open
Rashmi1613 wants to merge 10 commits intojenkinsci:mainfrom
Rashmi1613:fix-empty-recipients
Open

fix:handle edge case for empty recipients to prevent crash#1537
Rashmi1613 wants to merge 10 commits intojenkinsci:mainfrom
Rashmi1613:fix-empty-recipients

Conversation

@Rashmi1613
Copy link
Copy Markdown

Previously, if msg.getAllRecipients() returned null or an empty array,
the code attempted to access allRecipients[0], which could lead to
NullPointerException or ArrayIndexOutOfBoundsException.

This change adds a safety check to prevent crashes and logs a clear message instead.


Changes Made

  • Added null and empty check for allRecipients
  • Prevented unsafe access to allRecipients[0]
  • Added log message for better debugging

Testing

  • Verified that when recipients are empty, email is not sent
  • Confirmed no crash occurs in this scenario

@Rashmi1613 Rashmi1613 requested a review from a team as a code owner March 27, 2026 18:16
@ArpanC6
Copy link
Copy Markdown

ArpanC6 commented Mar 28, 2026

I went through the diff and noticed a few things

  1. Compile error missing parentheses
    This is probably why CI is failing. getListener and getLogger are methods not fields so they need parentheses:

context.getListener().getLogger().println(...)

  1. Logging approach
    getLogger().println() writes to the build console, not the plugin logs. The rest of the codebase seems to be using java.util.logging.Logger so it might be better to stay consistent with that.

  2. Check is happening too early
    Since executePresendScript() can modify the recipient list at runtime, checking getAllRecipients() before it runs might not give the final result. It would be safer to move the null/empty check after executePresendScript().

  3. Log message formatting
    Small thing there’s a missing space after the comma in "No recipients found,email couldn't be sent."

  4. Stray debug comment
    Looks like there’s a debug comment left in the diff probably worth removing before pushing.

Overall the fix definitely makes sense.
Once the compile issue and the presend script ordering are sorted, this should be in good shape..

@Rashmi1613
Copy link
Copy Markdown
Author

Rashmi1613 commented Mar 28, 2026

@ArpanC6 Thanks for the feedback! I have moved the recipient check after executePresendScript(). I have kept the current logging approach for consistency ,but happy to update if preferred.

import java.net.MalformedURLException;
import java.net.SocketException;
import java.net.URL;
import java.util.*;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please don't use * imports.

import java.util.Date;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Arrays;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't mean to harp on this, or seem like I am nitpicking, but please follow the existing format for imports, which is alphabetical order. You can see that most (if not all) java.util imports are listed in alphabetical order.

return false;
}

if (StringUtils.isNotBlank(getDescriptor().getEmergencyReroute())) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the ordering here is incorrect. Even if the set of TO/CC/BCC is empty, if the emergency reroute is setup, then that should be sent still.

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