Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-48950] [JEP-200] Stop trying to serialize github-api types #616

Merged
merged 1 commit into from Jan 21, 2018

Conversation

Projects
None yet
6 participants
@jglick
Copy link
Member

commented Jan 16, 2018

JENKINS-48950

Testing:

  • can build a PR without error (formerly failed with a JEP-200 exception)
  • after restart, build index page shows cause
  • after restart, build api/xml shows relevant information
  • after restart, build Parameters shows relevant information

@reviewbybees esp. @oleg-nenashev

@reviewbybees

This comment has been minimized.

Copy link

commented Jan 16, 2018

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.

@oleg-nenashev oleg-nenashev self-requested a review Jan 16, 2018

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jan 16, 2018

🐝 for a hotfix. NPEs are better than loading issues even if getters are not checked somewhere. I will dive into the code a bit

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jan 16, 2018

@jglick

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2018

I already reviewed the code and tested scenarios I could think of, and there seems to be no reason for these fields to have ever been serialized to begin with. I took the extra step of running the plugin (in the #615 branch) against 2.101, prior to JEP-200. (BTW I have been using cloudbeers/yolo#4 as a test case.) build.xml indeed contains all kinds of grotesque junk.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jan 16, 2018

Getter review results:

  • getTriggerSender() - "fine"
  • getCommitAuthor() - fine
  • getPullRequestAuthor() - fine - schedule build only
  • UI - no usages from what I see

The first one has such code in scheduleBuild(). It should be fine since there is no restart robustness in this type (no new issues).

try {
            triggerAuthor = getString(cause.getTriggerSender().getName(), "");
        } catch (Exception e) {
        }
        try {
            triggerAuthorEmail = getString(cause.getTriggerSender().getEmail(), "");
        } catch (Exception e) {
        }
        try {
            triggerAuthorLogin = getString(cause.getTriggerSender().getLogin(), "");
        } catch (Exception e) {
        }
@jglick

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2018

@oleg-nenashev oleg-nenashev requested a review from samrocketman Jan 16, 2018

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jan 16, 2018

Fort the record, alternative approaches are here:

We will consider them if and only if there are major fallouts in other plugins using GitHub API. No evidence of that so far.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jan 17, 2018

@bjoernhaeuser @samrocketman would it be possible to release it? Although there is a workaround available (whitelisting of github-api classes), we do not consider this workaround as feasible. A plugin release would help a lot

@@ -30,13 +30,13 @@

private final URL url;

private final GHUser triggerSender;
private final transient GHUser triggerSender;

This comment has been minimized.

Copy link
@jglick

jglick Jan 17, 2018

Author Member

Note: as mentioned in JIRA, this will still cause problems for loading historical builds (predating this fix and Jenkins 2.102). Probably solvable simply by renaming the fields—but untested.

@sgleske-ias

This comment has been minimized.

Copy link

commented Jan 17, 2018

I'll prioritize review and release ASAP.

@bjoernhaeuser

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2018

Currently testing the changes here according to #604

@bjoernhaeuser

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2018

@jglick @oleg-nenashev

I am currently seeing this exception when starting a new build:

Can't update build descriptionjava.io.IOException: java.lang.RuntimeException: Failed to serialize hudson.model.Actionable#actions for class hudson.model.FreeStyleBuild
	at hudson.XmlFile.write(XmlFile.java:201)
	at hudson.model.Run.save(Run.java:1923)
	at hudson.model.Run.setDescription(Run.java:2228)
	at org.jenkinsci.plugins.ghprb.GhprbBuilds.onStarted(GhprbBuilds.java:159)
	at org.jenkinsci.plugins.ghprb.GhprbBuildListener.onStarted(GhprbBuildListener.java:20)
	at hudson.model.listeners.RunListener.fireStarted(RunListener.java:240)
	at hudson.model.Run.execute(Run.java:1723)
	at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
	at hudson.model.ResourceController.execute(ResourceController.java:97)
	at hudson.model.Executor.run(Executor.java:429)
Caused by: java.lang.RuntimeException: Failed to serialize hudson.model.Actionable#actions for class hudson.model.FreeStyleBuild
	at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:256)
	at hudson.util.RobustReflectionConverter$2.visit(RobustReflectionConverter.java:224)
	at com.thoughtworks.xstream.converters.reflection.PureJavaReflectionProvider.visitSerializableFields(PureJavaReflectionProvider.java:138)
	at hudson.util.RobustReflectionConverter.doMarshal(RobustReflectionConverter.java:209)
	at hudson.util.RobustReflectionConverter.marshal(RobustReflectionConverter.java:150)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:69)
	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:58)
	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:43)
	at com.thoughtworks.xstream.core.TreeMarshaller.start(TreeMarshaller.java:82)
	at com.thoughtworks.xstream.core.AbstractTreeMarshallingStrategy.marshal(AbstractTreeMarshallingStrategy.java:37)
	at com.thoughtworks.xstream.XStream.marshal(XStream.java:1026)
	at com.thoughtworks.xstream.XStream.marshal(XStream.java:1015)
	at com.thoughtworks.xstream.XStream.toXML(XStream.java:988)
	at hudson.XmlFile.write(XmlFile.java:194)
	... 9 more
Caused by: java.lang.RuntimeException: Failed to serialize hudson.model.CauseAction#causeBag for class hudson.model.CauseAction
	at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:256)
	at hudson.util.RobustReflectionConverter$2.visit(RobustReflectionConverter.java:224)
	at com.thoughtworks.xstream.converters.reflection.PureJavaReflectionProvider.visitSerializableFields(PureJavaReflectionProvider.java:138)
	at hudson.util.RobustReflectionConverter.doMarshal(RobustReflectionConverter.java:209)
	at hudson.util.RobustReflectionConverter.marshal(RobustReflectionConverter.java:150)
	at hudson.util.XStream2$PassthruConverter.marshal(XStream2.java:478)
	at hudson.util.XStream2$AssociatedConverterImpl.marshal(XStream2.java:448)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:69)
	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:58)
	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:43)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller$1.convertAnother(AbstractReferenceMarshaller.java:88)
	at com.thoughtworks.xstream.converters.collections.AbstractCollectionConverter.writeItem(AbstractCollectionConverter.java:64)
	at com.thoughtworks.xstream.converters.collections.CollectionConverter.marshal(CollectionConverter.java:74)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:69)
	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:58)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller$1.convertAnother(AbstractReferenceMarshaller.java:84)
	at hudson.util.RobustReflectionConverter.marshallField(RobustReflectionConverter.java:265)
	at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:252)
	... 22 more
Caused by: java.lang.RuntimeException: Failed to serialize org.jenkinsci.plugins.ghprb.GhprbCause#triggerSender for class org.jenkinsci.plugins.ghprb.GhprbCause
	at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:256)
	at hudson.util.RobustReflectionConverter$2.visit(RobustReflectionConverter.java:224)
	at com.thoughtworks.xstream.converters.reflection.PureJavaReflectionProvider.visitSerializableFields(PureJavaReflectionProvider.java:138)
	at hudson.util.RobustReflectionConverter.doMarshal(RobustReflectionConverter.java:209)
	at hudson.util.RobustReflectionConverter.marshal(RobustReflectionConverter.java:150)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:69)
	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:58)
	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:43)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller$1.convertAnother(AbstractReferenceMarshaller.java:88)
	at com.thoughtworks.xstream.converters.collections.AbstractCollectionConverter.writeItem(AbstractCollectionConverter.java:64)
	at com.thoughtworks.xstream.converters.collections.MapConverter.marshal(MapConverter.java:78)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:69)
	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:58)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller$1.convertAnother(AbstractReferenceMarshaller.java:84)
	at hudson.util.RobustReflectionConverter.marshallField(RobustReflectionConverter.java:265)
	at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:252)
	... 39 more
Caused by: java.lang.UnsupportedOperationException: Refusing to marshal org.kohsuke.github.GHUser for security reasons; see https://jenkins.io/redirect/class-filter/
	at hudson.util.XStream2$BlacklistedTypesConverter.marshal(XStream2.java:530)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:69)
	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:58)
	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller$1.convertAnother(AbstractReferenceMarshaller.java:84)
	at hudson.util.RobustReflectionConverter.marshallField(RobustReflectionConverter.java:265)
	at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:252)
	... 54 more

Do you also have these exceptions in the log? Though, contrary to the message the description of the build get's actually set.

@bjoernhaeuser

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2018

Okay, false alarm. Everything seems to work!

@samrocketman samrocketman merged commit e381590 into jenkinsci:master Jan 21, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jan 21, 2018

@samrocketman ping me once it's released. I will update Wiki pages, etc.

@samrocketman

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

Released 1.40.0 via #621.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

@samrocketman Thanks a lot! The Wiki has been updated. Let's see how the fix flies

@jglick jglick deleted the jglick:JENKINS-48950 branch Jan 22, 2018

nosmo pushed a commit to nosmo/ghprb-plugin that referenced this pull request Dec 12, 2018

Merge pull request jenkinsci#616 from jglick/JENKINS-48950
[JENKINS-48950] [JEP-200] Stop trying to serialize github-api types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.