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

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

    Details

    • Similar Issues:

      Description

      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);
      }

        Attachments

          Issue Links

            Activity

            Hide
            jglick Jesse Glick added a comment -

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

            Show
            jglick Jesse Glick added a comment - The “simple test” is illegitimate since the property must be assigned to the job if it is used.
            Hide
            jglick 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.

            Show
            jglick 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.
            Hide
            oleg_nenashev Oleg Nenashev added a comment -

            Jesse Glick

            > 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.

             

             

             

            Show
            oleg_nenashev Oleg Nenashev added a comment - Jesse Glick > 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.      
            Hide
            jglick Jesse Glick added a comment -

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

            Show
            jglick Jesse Glick added a comment - JobProperty.setOwner was not being called properly. This is the responsibility of the code adding the property.
            Hide
            oleg_nenashev Oleg Nenashev added a comment -

            Jesse GlickI 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?

            Show
            oleg_nenashev Oleg Nenashev added a comment - Jesse Glick 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?
            Hide
            jglick 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.

            Show
            jglick 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.

              People

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

                Dates

                • Created:
                  Updated: