[JENKINS-44087] Only track project credentials if last build exists #496

MarkEWaite · 2017-05-06T12:35:55Z

Git parameters plugin etected a null pointer exception when it was used before the first run of a job.

JENKINS-44087 describes the testing that has been performed. I've checked it interactively, Boguslaw has checked it interactively.

@reviewbybees

[JENKINS-44087] detected a null pointer exception in the git parameters plugin when it was used before the first run of a job.

ghost · 2017-05-06T12:46:49Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

jglick

Amends #490 IIUC.

jglick · 2017-05-09T14:03:57Z

src/main/java/hudson/plugins/git/GitSCM.java

@@ -764,7 +764,9 @@ public GitClient createClient(TaskListener listener, EnvVars environment, Run<?,
                StandardUsernameCredentials credentials = CredentialsMatchers.firstOrNull(urlCredentials, idMatcher);
                if (credentials != null) {
                    c.addCredentials(url, credentials);
-                    CredentialsProvider.track(project.getLastBuild(), credentials);
+                    if (project != null && project.getLastBuild() != null) {


If project can really be null, should the parameter not be marked @CheckForNull?

jglick · 2017-05-09T14:04:36Z

src/main/java/hudson/plugins/git/GitSCM.java

@@ -764,7 +764,9 @@ public GitClient createClient(TaskListener listener, EnvVars environment, Run<?,
                StandardUsernameCredentials credentials = CredentialsMatchers.firstOrNull(urlCredentials, idMatcher);
                if (credentials != null) {
                    c.addCredentials(url, credentials);
-                    CredentialsProvider.track(project.getLastBuild(), credentials);
+                    if (project != null && project.getLastBuild() != null) {
+                        CredentialsProvider.track(project.getLastBuild(), credentials);


Tiny race condition risk (the one remaining build could be deleted between these two lines). Safer to assign the lastBuild to a local variable and check that for null.

jglick · 2017-05-09T14:08:19Z

Seems like this should have been easily reproduced in a functional test, no?

MarkEWaite · 2017-05-09T16:08:38Z

@jglick I was definitely able to reproduce it in an interactive test, and have included that test in my jenkins-bugs-private repository so that it is regularly executed.

I should also be able to reproduce it with a test in Java, but didn't want to take the time to write the Java test. Do you feel strongly that it needs a test in code, not just the verification test in my high level test harness?

jglick · 2017-05-09T17:54:25Z

Just general advice. At least my own practice is to start with trying to reproduce such bugs with a JenkinsRule test. Once you get a little practice and some infrastructure in place, it is as fast or faster than reproducing manually. And certainly it is better to have regular JUnit tests in the repository like every other plugin (run on PR builds, by plugin-compat-tester, etc. etc.) than a separate testing system. (Though there are always going to be some weird cases which cannot be reproduced properly even using DockerFixture.)

MarkEWaite · 2017-05-09T18:34:36Z

I like your advice very much. My general pattern has been

Duplicate the bug interactively so that I can see what the user sees, and confirm that I've understood the problem in the user context
Investigate the code to understand what is happening in that area
Write tests (unit tests first preference, JenkinsRule second) which show the problem
Fix the problem, confirm the tests pass
Interactively confirm the fixed code works as expected and doesn't surprise in other areas

In this case, I skipped step 3 due to the simplicity of the code change. I'll make my best effort to create a test that shows the problem, then can confirm that the code change fixes it.

When I've skipped step 1 in the past, I've found that I wasted time in areas of the code that weren't relevant to the bug report.

Only track project credentials if last build exists

6641628

[JENKINS-44087] detected a null pointer exception in the git parameters plugin when it was used before the first run of a job.

MarkEWaite requested a review from a user May 6, 2017 12:35

MarkEWaite changed the title ~~Only track project credentials if last build exists~~ [JENKINS-44087] Only track project credentials if last build exists May 6, 2017

jglick approved these changes May 9, 2017

View reviewed changes

MarkEWaite merged commit 9daa4dd into jenkinsci:master May 20, 2017

MarkEWaite deleted the master-JENKINS-44087-NPE-before-first-build branch May 20, 2017 00:45

MarkEWaite added the bugfix Fixes a bug - used by Release Drafter label Jul 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-44087] Only track project credentials if last build exists #496

[JENKINS-44087] Only track project credentials if last build exists #496

MarkEWaite commented May 6, 2017 •

edited

Loading

ghost commented May 6, 2017

jglick left a comment

jglick May 9, 2017

jglick May 9, 2017

jglick commented May 9, 2017

MarkEWaite commented May 9, 2017

jglick commented May 9, 2017

MarkEWaite commented May 9, 2017 •

edited

Loading

[JENKINS-44087] Only track project credentials if last build exists #496

[JENKINS-44087] Only track project credentials if last build exists #496

Conversation

MarkEWaite commented May 6, 2017 • edited Loading

ghost commented May 6, 2017

jglick left a comment

Choose a reason for hiding this comment

jglick May 9, 2017

Choose a reason for hiding this comment

jglick May 9, 2017

Choose a reason for hiding this comment

jglick commented May 9, 2017

MarkEWaite commented May 9, 2017

jglick commented May 9, 2017

MarkEWaite commented May 9, 2017 • edited Loading

MarkEWaite commented May 6, 2017 •

edited

Loading

MarkEWaite commented May 9, 2017 •

edited

Loading