Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIXED JENKINS-26452] Prevent NPE if no publisher in conditional step. #7

Merged
merged 7 commits into from Jan 17, 2015

Conversation

christ66
Copy link
Member

JENKINS-26452

If there are no publishers added to the flexible publish plugin, then the plugin can throw a NPE when saving, executing a build, and restarting Jenkins will fail to load the job.

@reviewbybees

…e conditional step. This also prevents a NPE when the job loads.
@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

// This can happen if the projects publisher list was removed, returning
// a list with one null element.
if (publisherList.contains(null)) {
publisherList.remove(null);
Copy link
Member

Choose a reason for hiding this comment

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

Better done in readResolve.

@jglick jglick changed the title [JENKINS-19985] Prevent NPE if no publisher in conditional step. [FIXED JENKINS-19985] Prevent NPE if no publisher in conditional step. Jan 15, 2015
@jglick
Copy link
Member

jglick commented Jan 15, 2015

While the changes here seem fine as a robustness improvement, I feel like something is missing. How could the UI form be submitting a list with null elements to begin with? The reproduction step

  1. Save the project without any conditional publishers.

should just result in an empty list if standard form binding is being used, and that should be harmless.

@christ66
Copy link
Member Author

The problem was the ConditionalPublisher.newInstance method could have added a null value to the list. I also made modifications to not throw any exception, and just have an empty list instead.

@ikedam
Copy link
Member

ikedam commented Jan 16, 2015

Looks almost perfect to me. Thanks.

@jglick jglick changed the title [FIXED JENKINS-19985] Prevent NPE if no publisher in conditional step. [FIXED JENKINS-26452] Prevent NPE if no publisher in conditional step. Jan 16, 2015
@jglick
Copy link
Member

jglick commented Jan 16, 2015

In the long term it would be better to get rid of the newInstance override entirely, but in the meantime 👍 other than the gratuitous whitespace changes.

@christ66
Copy link
Member Author

Sorry about the whitespace changes. I thought I had fixed my IDE to not make these changes.

FlexiblePublisher fp = new FlexiblePublisher(Arrays.asList(conditionalPublisher));
p.getPublishersList().add(fp);
p.save();
jenkins.reload();
Copy link
Member

Choose a reason for hiding this comment

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

BTW another way of doing this is to use @LocalData with a prepared example of historically broken configuration.

@jglick
Copy link
Member

jglick commented Jan 16, 2015

👍

ikedam added a commit that referenced this pull request Jan 17, 2015
[FIXED JENKINS-26452] Prevent NPE if no publisher in conditional step.
@ikedam ikedam merged commit 932c255 into jenkinsci:master Jan 17, 2015
@ikedam
Copy link
Member

ikedam commented Jan 17, 2015

Thanks for the pull request!
I released 0.14.1, which will be available in a day.

@ikedam
Copy link
Member

ikedam commented Jan 17, 2015

Oh... I had to release as 0.13.1 (that means fix for 0.13)...
I mistook!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants