timja / jenkins-gh-issues-poc-06-18

0 stars 0 forks source link

[JENKINS-24994] Poor error reporting when an anonymous Cause is used #5527

Open timja opened 10 years ago

timja commented 10 years ago

EnvInject throws a StringIndexOutOfBoundsException in BuildCauseRetriever.getTriggeredCause if the class that triggered it was anonymous. This is because getTriggerName uses cause.getClass().getSimpleName(), which returns an empty string for anonymous classes, empty strings are skipped when getTriggeredCause iterates over build causes, but it always assumes that there is at least one non-empty build cause.

To reproduce, create a trigger that uses an anonymous Cause like this:

private static void scheduleBuild(BuildableItem job) {
    job.scheduleBuild(new Cause() {
@Override
public String getShortDescription() {
    return "Triggered by XYZ";
}
    });
}

To work around this, the above snippet can be rewritten to avoid the anonymous class:

private static void scheduleBuild(BuildableItem job) {
    job.scheduleBuild(new XyzCause());
}

private static class XyzCause extends Cause {
    @Override
    public String getShortDescription() {
return "Triggered by XYZ";
    }
}

I'm not sure which fix would be most consistent with the intent of the ENV_CAUSE variable for custom triggers. I see these options:


Originally reported by dvdckl, imported from: Poor error reporting when an anonymous Cause is used
  • status: Open
  • priority: Minor
  • resolution: Unresolved
  • imported: 2022/01/10
timja commented 10 years ago

danielbeck:

How can this be reproduced? What cause is set up like this? Doesn't it lead to problems with XML serialization?

timja commented 10 years ago

dvdckl:

Daniel, I'm not sure what you mean about XML serialization. That exception does cause my builds to fail if that's what you mean.
I've added an example snippet to reproduce the error in the description.

timja commented 10 years ago

danielbeck:

Builds are stored as XML on disk in files called build.xml. If you create causes like that, they serialize a lot of garbage to disk, and are brittle in the face of code changes. I replaced the cause of ParameterizedJobMixin.scheduleBuild() (the overload without arguments) by something similar to your sample code, and this is what it looks like on disk:

      

  
    
      
      
      false
      
      
      true
      false
      false
      false
      
      false
      
      
      
    
  

      

Note that the demo job I used is empty, and this would contain the entire XML configuration at the time the cause was serialized, because it's defined inside ParameterizedJobMixIn and inherits the outer classes' environment. It deserializes a separate instance of the outer class as well that is different from the regularly loaded instance.

I think it's pointless to fix this issue, as any implementations of anonymous inner classes suffer from similar problems, making them into bugs waiting to happen that should be avoided. You should probably check to see which of these issues affect the plugin you're using and report issues against it.

Resolve as Won't Fix?

timja commented 10 years ago

dvdckl:

Understood. Another option to resolve is to add a better exception. Out of the 70+ plugins we're using, the only one to error out was envinject. A one-liner error test and exception about an unsupported trigger might save someone some debugging time in the future.

If you feel that Jenkins shouldn't allow that kind of trigger at all, do you think that should be filed as a enhancement against the appropriate core functionality?

timja commented 10 years ago

jglick:

It might make sense for Jenkins to define an annotation indicating abstract types whose implementations are intended to be serialized with XStream (Cause, RunAction2, etc.). An annotation processor could then report an error if an implementation were an anonymous inner class or a non-static nested class, or had fields of various types known to be nonserializable or known to themselves be serialized at top level (User, Run, etc.).

timja commented 2 years ago

[Originally related to: JENKINS-22665]