[JENKINS-36748] Not process null Triggers #2463

fbelzunc · 2016-07-18T06:45:22Z

https://issues.jenkins-ci.org/browse/JENKINS-36748

@reviewbybees CC @jtnord

ghost · 2016-07-18T06:59:31Z

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.

jtnord · 2016-07-18T08:15:48Z

core/src/main/java/hudson/triggers/Trigger.java

                    }
+                } else {
+                    LOGGER.log(Level.WARNING, "The job {0} has a syntactically incorrect config and is missing the cron spec for a trigger", p.getFullName());


the spec is optional for a Trigger IIUC.
Hence the check should be inside if (!(t instanceof SCMTrigger && scmd.synchronousPolling)) rather than outside it, or the log level and message should be adjusted.

fbelzunc · 2016-07-19T05:37:21Z

@jtnord I think I correctly addressed your feedback.

jtnord · 2016-07-19T08:36:57Z

🐝 - a unit test would be great to prevent a regression...

oleg-nenashev · 2016-07-19T09:55:55Z

the fox is a bit overloaded by "improvement" changes, but LGTM

oleg-nenashev · 2016-07-19T21:57:35Z

Oh, 🐝
@reviewbybees done

KostyaSha · 2016-07-21T10:47:43Z

👎 for merging it without test.

KostyaSha · 2016-07-21T10:49:35Z

Issue wasn't closed https://issues.jenkins-ci.org/browse/JENKINS-36748

KostyaSha · 2016-07-21T10:51:58Z

core/src/main/java/hudson/triggers/Trigger.java

-                            // don't let that cancel the polling activity. report and move on.
-                            LOGGER.log(Level.WARNING, t.getClass().getName() + ".run() failed for " + p, e);
+                if (!(t instanceof SCMTrigger && scmd.synchronousPolling)) {
+                    if (t !=null && t.spec != null && t.tabs != null) {


tabs can't be null

Please describe the case when getTriggers().values() may return null.

tabs can be null even when they should not, see the JIRA issue. basically an XML pushed with some missing fields, or JobDSL configuration without a cron...
getTriggers() should not return null - but hey this is about robustness and we have a similar patterns elsewhere in Jenkins for this.

So where is the fix for real problem then?

@jtnord if you get getTriggers(), then values() will be non null always. So you need check for getTriggers() nullness firstly.

So where is the fix for real problem then

t.tabs != null

We can not check and validate everything on disk, or created from XML as we have no schema to validate against, unless you have some suggestions here ?

getTriggers() should not return null - but hey this is about robustness and we have a similar patterns elsewhere in Jenkins for this.

I should be more careful when I type.

getTriggers() could possibly contain a null value, rather than itself be null, don;t ask me how or where, it was just deemed safe given we had to check tabs to check more than the initially observered case.

if getTriggers can return null then this needs a separate fix (and a separate JIRA as the way to reproduce this would be different).

KostyaSha · 2016-07-21T10:59:31Z

Trigger API requires right trigger initialisation and usage, i annotated them long time ago. If ghprb (and i sure that is) is using APIs in wrong ways, then core shouldn't be hacked for it.

jtnord · 2016-07-21T11:38:41Z

Annotations can be violated in so many ways, they are compile time for code not run time.
This issue was form a job with some invalid syntax, as JobDSL allows you to create stuff like that :-(

I think whilst we can argue about creating garbage until the cows come home (there are many ways to do it) I think we can agree that a single garbage job should not impact other jobs from working correctly - and this was actually the case here.

KostyaSha · 2016-07-21T14:07:40Z

and this was actually the case here.

waiting for test case then.

jtnord · 2016-07-21T14:29:14Z

and this was actually the case here.

waiting for test case then.

It's coming.

fbelzunc · 2016-07-27T07:16:08Z

@jtnord @KostyaSha Per your request #2482 was created.

Would you mind to validate it, please?

jglick · 2016-07-27T12:46:51Z

👎, this looks like the wrong fix. Basic validation of the content of fields like triggers should be done during deserialization, not at runtime.

RobustCollectionConvertor simply drops elements from a collection which cannot be deserialized. It will not insert nulls. So I doubt the issue is that the whole Trigger was dropped.
RobustReflectionConverter allows a reference-typed field to be nulled out if it cannot be deserialized. If a given field is required in a given class, readResolve must enforce that, for example by throwing IllegalStateException. Or more cleanly, XStream2.addCriticalField could be made a public API, or replaced by an annotation.

KostyaSha · 2016-07-27T12:49:31Z

I think you are trying to fix nulls where they shouldn't be at all. With such assumption every few lines in jenkins code will have null checks.

jglick · 2016-07-27T13:04:15Z

Right, that is my point: it is better to fix malformed settings at the point of origin (configuration or deserialization).

daniel-beck · 2016-07-28T09:58:46Z

@jglick

If a given field is required in a given class, readResolve must enforce that

Which it does, in Trigger#readResolve(). But GhprbTrigger overrides it. Pretty much a bug in that plugin. Nonetheless, this is a robustness improvement. Far from the first one in Jenkins.

fbelzunc · 2016-07-28T11:52:06Z

The main problem that I see for the right fix is that it needs to be done in XStream and even if I try to do there, we will not pick up the fix until some years happens and we update the pom. So either, we can revert this change or we merge the test case #2482

[JENKINS-36748] Not process null Triggers

2e8ffe3

jtnord reviewed Jul 18, 2016
View reviewed changes

fbelzunc added 2 commits July 18, 2016 14:08

[JENKINS-36748] Specify job

4bc34db

[JENKINS-36748] Correctly check NullPointerException

f82bac4

fbelzunc closed this Jul 19, 2016

fbelzunc reopened this Jul 19, 2016

oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jul 20, 2016

oleg-nenashev merged commit 40687bd into jenkinsci:master Jul 20, 2016

KostyaSha reviewed Jul 21, 2016
View reviewed changes

daniel-beck added a commit that referenced this pull request Jul 25, 2016

Noting #2446 #2453 #2455 #2463 #2464 #2465 #2466

ac1ff2b

fbelzunc mentioned this pull request Jul 27, 2016

[JENKINS-36748] Add a test case and not process null specs #2482

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-36748] Not process null Triggers #2463

[JENKINS-36748] Not process null Triggers #2463

fbelzunc commented Jul 18, 2016

ghost commented Jul 18, 2016

jtnord Jul 18, 2016 •

edited

Loading

fbelzunc commented Jul 19, 2016

jtnord commented Jul 19, 2016

oleg-nenashev commented Jul 19, 2016

oleg-nenashev commented Jul 19, 2016

KostyaSha commented Jul 21, 2016

KostyaSha commented Jul 21, 2016

KostyaSha Jul 21, 2016

KostyaSha Jul 21, 2016

jtnord Jul 21, 2016

KostyaSha Jul 21, 2016

KostyaSha Jul 21, 2016

jtnord Jul 21, 2016

KostyaSha commented Jul 21, 2016

jtnord commented Jul 21, 2016

KostyaSha commented Jul 21, 2016

jtnord commented Jul 21, 2016 •

edited

Loading

fbelzunc commented Jul 27, 2016

jglick commented Jul 27, 2016

KostyaSha commented Jul 27, 2016

jglick commented Jul 27, 2016

daniel-beck commented Jul 28, 2016

fbelzunc commented Jul 28, 2016

[JENKINS-36748] Not process null Triggers #2463

[JENKINS-36748] Not process null Triggers #2463

Conversation

fbelzunc commented Jul 18, 2016

ghost commented Jul 18, 2016

jtnord Jul 18, 2016 • edited Loading

Choose a reason for hiding this comment

fbelzunc commented Jul 19, 2016

jtnord commented Jul 19, 2016

oleg-nenashev commented Jul 19, 2016

oleg-nenashev commented Jul 19, 2016

KostyaSha commented Jul 21, 2016

KostyaSha commented Jul 21, 2016

KostyaSha Jul 21, 2016

Choose a reason for hiding this comment

KostyaSha Jul 21, 2016

Choose a reason for hiding this comment

jtnord Jul 21, 2016

Choose a reason for hiding this comment

KostyaSha Jul 21, 2016

Choose a reason for hiding this comment

KostyaSha Jul 21, 2016

Choose a reason for hiding this comment

jtnord Jul 21, 2016

Choose a reason for hiding this comment

KostyaSha commented Jul 21, 2016

jtnord commented Jul 21, 2016

KostyaSha commented Jul 21, 2016

jtnord commented Jul 21, 2016 • edited Loading

fbelzunc commented Jul 27, 2016

jglick commented Jul 27, 2016

KostyaSha commented Jul 27, 2016

jglick commented Jul 27, 2016

daniel-beck commented Jul 28, 2016

fbelzunc commented Jul 28, 2016

jtnord Jul 18, 2016 •

edited

Loading

jtnord commented Jul 21, 2016 •

edited

Loading