[JENKINS-40807] Fix Findbugs errors #162

varyvol · 2017-05-16T13:29:19Z

After JENKINS-40806, Findbugs is run as part of the build. However, such builds does not fail if any error arise.

This change revert that behaviour and fixes/suppresses already present errors.

…them.

ghost · 2017-05-16T13:33:24Z

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.

varyvol · 2017-06-07T08:48:27Z

@yacaovsnc did you have the opportunity to review these changes?

yacaovsnc · 2017-06-07T19:16:08Z

+@davidstaheli

jpricket · 2017-06-23T19:38:40Z

Hi @varyvol, I am taking over from yacaovsnc. All of the changes seem reasonable. Unfortunately, I pushed some of my own changes before see this. So, I probably introduced some additional errors. I plan on merging your changes and then fixes my errors so we can get down to zero again.
Thanks for this contribution!

varyvol · 2017-06-26T06:29:36Z

@jpricketMSFT Thank you!

oleg-nenashev

Belated 🐛 for making fields transient. From what I see it impacts persistency of actions on the disk.

oleg-nenashev · 2017-06-26T07:16:20Z

tfs/src/main/java/hudson/plugins/tfs/ChangeSetReader.java

@@ -25,6 +26,7 @@
 public class ChangeSetReader extends ChangeLogParser {

    @Override
+    @SuppressFBWarnings(value = "DM_DEFAULT_ENCODING", justification = "Better mot modify charset in case it might raise errors")


Would be better to explicitly use the System default then

oleg-nenashev · 2017-06-26T07:17:10Z

tfs/src/main/java/hudson/plugins/tfs/TeamCollectResultsPostBuildAction.java

@@ -51,6 +52,7 @@ public void setRequestedResults(final List<TeamRequestedResult> requestedResults
        this.requestedResults = requestedResults;
    }

+    @SuppressFBWarnings(value = "RV_RETURN_VALUE_IGNORED_BAD_PRACTICE", justification = "No matter the result of mkdirs")


Why? This is a correct issue. Ideally a Java 7 Files API should be used instead

oleg-nenashev · 2017-06-26T07:19:19Z

tfs/src/main/java/hudson/plugins/tfs/TeamFoundationServerScm.java

@@ -391,7 +391,7 @@ public boolean pollChanges(AbstractProject hudsonProject, Launcher launcher, Fil
    @Override
    public boolean processWorkspaceBeforeDeletion(AbstractProject<?, ?> project, FilePath workspace, Node node) throws IOException, InterruptedException {
        Run<?,?> lastRun = project.getLastBuild();


You can explicitly use AbstractBuild here, there is no need to cast the type later

oleg-nenashev · 2017-06-26T07:22:10Z

tfs/src/main/java/hudson/plugins/tfs/TeamPullRequestMergedDetailsAction.java

@@ -24,7 +24,7 @@
    private static final long serialVersionUID = 1L;
    private static final String URL_NAME = "team-pullRequestMergedDetails";

-    public GitPullRequestEx gitPullRequest;
+    public transient GitPullRequestEx gitPullRequest;


🐛 It is going to break the event persistence on the disk, because the field won't be stored anymore.
And this action is persisted from what I see

GitPullRequestEx is not serializable so in case someone was serializing it, it would have already broken.

"Serializable" is not required for persisting the data on the disk. My concern is that my making the class formally serializable you may break the data persistency

oleg-nenashev · 2017-06-26T07:23:52Z

tfs/src/main/java/hudson/plugins/tfs/model/GitCodePushedEventArgs.java

 import java.net.URI;
 import java.net.URISyntaxException;

-public class GitCodePushedEventArgs {
+public class GitCodePushedEventArgs implements Serializable {


It breaks the compatibility, because the class is not final.
If you make it serializable, also define the serialVersionID

Why does adding an interface break the compatibility? My understanding is that you can add them, being the removal the problematic thing.

https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html

Serializable is a special case. If you add this interface, all child classes must become serializable as well. And there is no guarantee they really can support it.

Fromt the binary compatibility standpoint it is compatible, of course

Well you have to consider whether this is a plausible situation where someone might have subclassed it to begin with.

oleg-nenashev · 2017-06-26T07:25:11Z

tfs/src/main/java/hudson/plugins/tfs/model/ManualCredentialsConfigurer.java

 import hudson.Extension;
 import hudson.util.Secret;
 import org.kohsuke.stapler.DataBoundConstructor;

+@SuppressFBWarnings(value = "SE_TRANSIENT_FIELD_NOT_RESTORED", justification = "Maintain compatibility")


There should have been readResolve() then

Resolving to what? It's not something that can be predicted I think.

Also there are tests that are specifically checking the serialization generates an empty CredentialsConfigurer (that's why I did not removed the transient).

oleg-nenashev · 2017-06-26T07:26:59Z

tfs/src/main/java/hudson/plugins/tfs/util/BuildWorkspaceConfigurationRetriever.java

@@ -45,17 +45,23 @@ public BuildWorkspaceConfiguration getLatestForNode(Node needleNode, Run<?,?> la

    public static class BuildWorkspaceConfiguration extends WorkspaceConfiguration {
        private static final long serialVersionUID = 1L;
-        private final AbstractBuild<?, ?> build;
+        private final transient AbstractBuild<?, ?> build;


I am not sure what is the use-case for this class, but it may also break persistency on the disk 🐜

AbstractBuild is not serializable so this could have never been serialized.

Serialization is not required for persistency via XStream. That engine is different.
Persisting builds likely ends up with a relative link in XML, e.g. "../..". By making the field transient you are just removing it from XML at all

Actually due to lazy loading issues there are cases where including a nontransient field of type Run will cause serious errors, so it is mandatory to make this transient.

I agree the persisted Build instance is a bad thing in any case. But the one who solves that has to ensure the plugin's logic does not suffer. Rejuice methods or whatever. It is not enough to just make the field transient

varyvol · 2017-06-27T09:48:13Z

@oleg-nenashev I have answered some of your comments. I will create another PR with some fixes based on others.

oleg-nenashev · 2017-06-28T07:04:25Z

tfs/src/main/java/hudson/plugins/tfs/util/BuildWorkspaceConfigurationRetriever.java

@@ -45,17 +45,23 @@ public BuildWorkspaceConfiguration getLatestForNode(Node needleNode, Run<?,?> la

    public static class BuildWorkspaceConfiguration extends WorkspaceConfiguration {
        private static final long serialVersionUID = 1L;
-        private final AbstractBuild<?, ?> build;
+        private final transient AbstractBuild<?, ?> build;


Serialization is not required for persistency via XStream. That engine is different.
Persisting builds likely ends up with a relative link in XML, e.g. "../..". By making the field transient you are just removing it from XML at all

oleg-nenashev · 2017-06-28T07:04:51Z

tfs/src/main/java/hudson/plugins/tfs/util/BuildWorkspaceConfigurationRetriever.java


        public BuildWorkspaceConfiguration(WorkspaceConfiguration configuration, AbstractBuild<?, ?> build) {
            super(configuration);
            this.build = build;
        }
+
        public void save() throws IOException {
            if (!workspaceExists()) {
                build.getAction(WorkspaceConfiguration.class).setWorkspaceWasRemoved();
            }
            build.save();


risk of NPE after the change?

Evaristo Gutiérrez added 2 commits May 16, 2017 13:19

[JENKINS-40807] Make build fail if findbugs errors and solve some of …

0d948fb

…them.

[JENKINS-40807] Fix more findbugs errors.

110f0e7

jpricket merged commit f763aad into jenkinsci:master Jun 23, 2017

oleg-nenashev reviewed Jun 26, 2017

View reviewed changes

varyvol mentioned this pull request Jun 27, 2017

[JENKINS-40807] Fix and improve removal of Findbugs errors #165

Merged

oleg-nenashev reviewed Jun 28, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-40807] Fix Findbugs errors #162

[JENKINS-40807] Fix Findbugs errors #162

varyvol commented May 16, 2017

ghost commented May 16, 2017

varyvol commented Jun 7, 2017

yacaovsnc commented Jun 7, 2017

jpricket commented Jun 23, 2017

varyvol commented Jun 26, 2017

oleg-nenashev left a comment

oleg-nenashev Jun 26, 2017

oleg-nenashev Jun 26, 2017

oleg-nenashev Jun 26, 2017

oleg-nenashev Jun 26, 2017

varyvol Jun 27, 2017

oleg-nenashev Jun 28, 2017

oleg-nenashev Jun 26, 2017

varyvol Jun 27, 2017

oleg-nenashev Jun 28, 2017

jglick Jun 28, 2017

oleg-nenashev Jun 26, 2017

varyvol Jun 27, 2017 •

edited

Loading

oleg-nenashev Jun 26, 2017

varyvol Jun 27, 2017

oleg-nenashev Jun 28, 2017

jglick Jun 28, 2017

oleg-nenashev Jun 28, 2017

varyvol commented Jun 27, 2017

oleg-nenashev Jun 28, 2017

oleg-nenashev Jun 28, 2017

[JENKINS-40807] Fix Findbugs errors #162

[JENKINS-40807] Fix Findbugs errors #162

Conversation

varyvol commented May 16, 2017

ghost commented May 16, 2017

varyvol commented Jun 7, 2017

yacaovsnc commented Jun 7, 2017

jpricket commented Jun 23, 2017

varyvol commented Jun 26, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varyvol Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varyvol commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varyvol Jun 27, 2017 •

edited

Loading