Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-45067

PipelineTriggersJobProperty#startTriggers() throws NPE when the property is not assigned to job

    • Icon: Bug Bug
    • Resolution: Incomplete
    • Icon: Minor Minor
    • workflow-job-plugin
    • None

      When somebody creates a property in API and occasionally triggers the startTriggers() method, Trigger#start(owner, bool) gets invoked with null argument, which violates the contract and causes NPE.

      java.lang.NullPointerException
       at hudson.triggers.Trigger.start(Trigger.java:94)
       at org.jenkinsci.plugins.workflow.job.properties.PipelineTriggersJobProperty.startTriggers(PipelineTriggersJobProperty.java:103)
       at .... (whatever code)

      I think this NPE should be prevented, because such behavior is not documented in the class Javadoc, and hence API users may expect a robust behavior.

       

      I have hit it in one of the proprietary Job templating engines, but actually it is a valid case reproducible via a simple test (see below). 

      public void triggerMethodsShouldNotThrowNPEWhenNotAssigned() {
        MockTrigger t = new MockTrigger();
        PipelineTriggersJobProperty prop = new PipelineTriggersJobProperty(Arrays.asList(t));
      
        prop.startTriggers(true);
      }

          [JENKINS-45067] PipelineTriggersJobProperty#startTriggers() throws NPE when the property is not assigned to job

          Jesse Glick added a comment -

          The “simple test” is illegitimate since the property must be assigned to the job if it is used.

          Jesse Glick added a comment - The “simple test” is illegitimate since the property must be assigned to the job if it is used.

          Jesse Glick added a comment -

          Possibly just a symptom of JENKINS-42446. Hard to tell without knowing how to reproduce the problem in a realistic situation.

          Jesse Glick added a comment - Possibly just a symptom of  JENKINS-42446 . Hard to tell without knowing how to reproduce the problem in a realistic situation.

          Oleg Nenashev added a comment -

          jglick

          > The “simple test” is illegitimate since the property must be assigned to the job if it is used.

          I agree, but I am not sure if it is actually documented anywhere (e.g. methods have no Javadoc). Please provide a link. If it is not documented, I assume the snippet is something possibly valid.

          Anyway, the pull request adds diagnostics and Javadoc, which explicitly notify about invalid state, and hence Jenkins admins will get notified about a bug in whatever calling logic. The proposed change is better than the original NPE, because we fail fast and prevent whatever undefined behavior in triggers.

           

           

           

          Oleg Nenashev added a comment - jglick > The “simple test” is illegitimate since the property must be assigned to the job if it is used. I agree, but I am not sure if it is actually documented anywhere (e.g. methods have no Javadoc). Please provide a link. If it is not documented, I assume the snippet is something possibly valid. Anyway, the pull request adds diagnostics and Javadoc, which explicitly notify about invalid state, and hence Jenkins admins will get notified about a bug in whatever calling logic. The proposed change is better than the original NPE, because we fail fast and prevent whatever undefined behavior in triggers.      

          Jesse Glick added a comment -

          JobProperty.setOwner was not being called properly. This is the responsibility of the code adding the property.

          Jesse Glick added a comment - JobProperty.setOwner was not being called properly. This is the responsibility of the code adding the property.

          Oleg Nenashev added a comment -

          jglickI do not feel you have answered to my last comment. And seems you refer to whatever sacred knowledge again. Where is it explicitly documented for API users?

          Oleg Nenashev added a comment - jglick I do not feel you have answered to my last comment. And seems you refer to whatever sacred knowledge again. Where is it explicitly documented for API users?

          Jesse Glick added a comment -

          you have answered to my last comment

          What are you looking for an answer to specifically? Note that I did not reject the PR so long as it is treated purely as a robustness / diagnosability improvement.

          seems you refer to whatever sacred knowledge again. Where is it explicitly documented for API users?

          XProperty.owner fields in Jenkins core (and also cloudbees-folder) are expected to be set before the property is used in any way. This is just a basic prerequisite for that pattern. (An antipattern IMO, but not one we can change now.) Whether this fact is mentioned in Javadoc or just left to API users to figure out for themselves, I do not know offhand.

          Jesse Glick added a comment - you have answered to my last comment What are you looking for an answer to specifically? Note that I did not reject the PR so long as it is treated purely as a robustness / diagnosability improvement. seems you refer to whatever sacred knowledge again. Where is it explicitly documented for API users? XProperty.owner fields in Jenkins core (and also cloudbees-folder ) are expected to be set before the property is used in any way. This is just a basic prerequisite for that pattern. (An antipattern IMO, but not one we can change now.) Whether this fact is mentioned in Javadoc or just left to API users to figure out for themselves, I do not know offhand.

          Jesse Glick added a comment -

          Unclear what would lead to this condition.

          Jesse Glick added a comment - Unclear what would lead to this condition.

            oleg_nenashev Oleg Nenashev
            oleg_nenashev Oleg Nenashev
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated:
              Resolved: