Merge pull request #74 from stephenc/jenkins-41255

[JENKINS-41255] Propagate SCMSource id changes when the new SCMSource is identical to the old
jenkinsci · Jan 21, 2017 · f867609 · f867609
2 parents c8d9b63 + 9394a7e
commit f867609
Show file tree

Hide file tree

Showing 3 changed files with 175 additions and 4 deletions.
diff --git a/src/main/java/jenkins/branch/MultiBranchProject.java b/src/main/java/jenkins/branch/MultiBranchProject.java
@@ -58,6 +58,7 @@
 import hudson.security.Permission;
 import hudson.util.LogTaskListener;
 import hudson.util.PersistedList;
+import hudson.util.XStream2;
 import hudson.util.io.ReopenableRotatingFileOutputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
@@ -401,6 +402,90 @@ public PersistedList<BranchSource> getSourcesList() {
         return sources;
     }
 
+    /**
+     * Offers direct access to set the configurable list of branch sources <strong>while</strong> preserving
+     * branch source id associations for sources that are otherwise unmodified
+     *
+     * @param sources the new sources.
+     * @throws IOException if the sources could not be persisted to disk.
+     */
+    public void setSourcesList(List<BranchSource> sources) throws IOException {
+        if (this.sources.isEmpty() || sources.isEmpty()) {
+            // easy
+            this.sources.replaceBy(sources);
+            return;
+        }
+        Set<String> oldIds = sourceIds(this.sources);
+        Set<String> newIds = sourceIds(sources);
+        if (oldIds.containsAll(newIds) || newIds.containsAll(oldIds)) {
+            // either adding, removing, or updating without an id change
+            this.sources.replaceBy(sources);
+            return;
+        }
+        // Now we need to check if any of the new entries are effectively the same as an old entry that is being removed
+        // we will store the ID changes in a map and process all the affected branches to update their sourceIds
+        Map<String,String> changedIds = new HashMap<>();
+        Set<String> additions = new HashSet<>(newIds);
+        additions.removeAll(oldIds);
+        Set<String> removals = new HashSet<>(oldIds);
+        removals.removeAll(newIds);
+
+        for (BranchSource addition : sources) {
+            String additionId = addition.getSource().getId();
+            if (!additions.contains(additionId)) {
+                continue;
+            }
+            for (BranchSource removal : this.sources) {
+                String removalId = removal.getSource().getId();
+                if (!removals.contains(removalId)) {
+                    continue;
+                }
+                if (!equalButForId(removal.getSource(), addition.getSource())) {
+                    continue;
+                }
+                changedIds.put(removalId, additionId);
+                // now take these two out of consideration
+                removals.remove(removalId);
+                additions.remove(additionId);
+                break;
+            }
+        }
+        this.sources.replaceBy(sources);
+        BranchProjectFactory<P,R> factory = getProjectFactory();
+        for (P item: getItems()) {
+            if (!factory.isProject(item)) {
+                continue;
+            }
+            Branch oldBranch = factory.getBranch(item);
+            if (changedIds.containsKey(oldBranch.getSourceId())) {
+                Branch newBranch = new Branch(changedIds.get(oldBranch.getSourceId()), oldBranch.getHead(), oldBranch.getScm(), oldBranch.getProperties());
+                newBranch.setActions(oldBranch.getActions());
+                factory.setBranch(item, newBranch);
+            }
+        }
+    }
+
+    private Set<String> sourceIds(List<BranchSource> sources) {
+        Set<String> result = new HashSet<>();
+        for (BranchSource s: sources) {
+            result.add(s.getSource().getId());
+        }
+        return result;
+    }
+
+    private boolean equalButForId(SCMSource a, SCMSource b) {
+        if (!a.getClass().equals(b.getClass())) {
+            return false;
+        }
+        return SOURCE_ID_OMITTED_XSTREAM.toXML(a).equals(SOURCE_ID_OMITTED_XSTREAM.toXML(b));
+    }
+
+    private final static XStream2 SOURCE_ID_OMITTED_XSTREAM = new XStream2();
+
+    static {
+        SOURCE_ID_OMITTED_XSTREAM.omitField(SCMSource.class, "id");
+    }
+
     /**
      * {@inheritDoc}
      */
@@ -800,7 +885,7 @@ protected void submit(StaplerRequest req, StaplerResponse rsp)
         List<SCMSource> _sources = new ArrayList<>();
         synchronized (this) {
             JSONObject json = req.getSubmittedForm();
-            sources.replaceBy(req.bindJSONToList(BranchSource.class, json.opt("sources")));
+            setSourcesList(req.bindJSONToList(BranchSource.class, json.opt("sources")));
             for (SCMSource scmSource : getSCMSources()) {
                 scmSource.setOwner(this);
                 _sources.add(scmSource);

diff --git a/src/main/java/jenkins/branch/OrganizationFolder.java b/src/main/java/jenkins/branch/OrganizationFolder.java
@@ -1126,9 +1126,7 @@ public void complete() throws IllegalStateException, InterruptedException {
                         if (existing != null) {
                             BulkChange bc = new BulkChange(existing);
                             try {
-                                PersistedList<BranchSource> sourcesList = existing.getSourcesList();
-                                sourcesList.clear();
-                                sourcesList.addAll(createBranchSources());
+                                existing.setSourcesList(createBranchSources());
                                 existing.setOrphanedItemStrategy(getOrphanedItemStrategy());
                                 factory.updateExistingProject(existing, attributes, listener);
                                 ProjectNameProperty property =

diff --git a/src/test/java/integration/EventsTest.java b/src/test/java/integration/EventsTest.java
@@ -61,6 +61,7 @@
 import org.junit.Before;
 import org.junit.ClassRule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.TestExtension;
 
@@ -188,6 +189,93 @@ public void given_multibranchWithSources_when_indexing_then_branchesAreFoundAndB
         }
     }
 
+    @Test
+    @Issue("JENKINS-41255")
+    public void given_multibranchWithSource_when_replacingSource_then_noBuildStorm() throws Exception {
+        try (MockSCMController c = MockSCMController.create()) {
+            c.createRepository("foo");
+            BasicMultiBranchProject prj = r.jenkins.createProject(BasicMultiBranchProject.class, "foo");
+            prj.setCriteria(null);
+            // set the first list
+            prj.setSourcesList(Collections.singletonList(new BranchSource(new MockSCMSource("firstId", c, "foo", true, false, false))));
+            prj.scheduleBuild2(0).getFuture().get();
+            r.waitUntilNoActivity();
+            assertThat("We now have branches",
+                    prj.getItems(), not(is((Collection<FreeStyleProject>) Collections.<FreeStyleProject>emptyList())));
+            FreeStyleProject master = prj.getItem("master");
+            assertThat("We now have the master branch", master, notNullValue());
+            r.waitUntilNoActivity();
+            assertThat("The master branch was built", master.getLastBuild(), notNullValue());
+            assertThat("The master branch was built", master.getLastBuild().getNumber(), is(1));
+            Branch branch = prj.getProjectFactory().getBranch(master);
+            assertThat("The branch source is the configured source", branch.getSourceId(), is("firstId"));
+            prj.setSourcesList(
+                    Collections.singletonList(new BranchSource(new MockSCMSource("secondId", c, "foo", true, false, false))));
+            Branch updated = prj.getProjectFactory().getBranch(master);
+            assertThat("The branch source is new configured source", updated.getSourceId(), is("secondId"));
+            prj.scheduleBuild2(0).getFuture().get();
+            assertThat("The master branch was not rebuilt", master.getLastBuild().getNumber(), is(1));
+        }
+    }
+
+    @Test
+    @Issue("JENKINS-41255")
+    public void given_multibranchWithSources_when_replacingSources_then_noBuildStorm() throws Exception {
+        try (MockSCMController c = MockSCMController.create()) {
+            c.createRepository("foo");
+            Integer id = c.openChangeRequest("foo", "master");
+            c.createRepository("bar");
+            c.createBranch("bar", "fun");
+            BasicMultiBranchProject prj = r.jenkins.createProject(BasicMultiBranchProject.class, "foo");
+            prj.setCriteria(null);
+            // set the first list
+            prj.setSourcesList(Arrays.asList(
+                    new BranchSource(new MockSCMSource("firstId", c, "foo", true, false, false)),
+                    new BranchSource(new MockSCMSource("secondId", c, "foo", false, false, true)),
+                    new BranchSource(new MockSCMSource("thirdId", c, "bar", true, false, false))
+            ));
+            prj.scheduleBuild2(0).getFuture().get();
+            r.waitUntilNoActivity();
+            assertThat("We now have branches",
+                    prj.getItems(), not(is((Collection<FreeStyleProject>) Collections.<FreeStyleProject>emptyList())));
+            FreeStyleProject master = prj.getItem("master");
+            assertThat("We now have the master branch", master, notNullValue());
+            FreeStyleProject change = prj.getItem("CR-" + id);
+            assertThat("We now have the change request", change, notNullValue());
+            FreeStyleProject fun = prj.getItem("fun");
+            assertThat("We now have the fun branch", fun, notNullValue());
+            r.waitUntilNoActivity();
+            assertThat("The master branch was built", master.getLastBuild(), notNullValue());
+            assertThat("The master branch was built", master.getLastBuild().getNumber(), is(1));
+            assertThat("The change request was built", change.getLastBuild(), notNullValue());
+            assertThat("The change request was built", change.getLastBuild().getNumber(), is(1));
+            assertThat("The fun branch was built", fun.getLastBuild(), notNullValue());
+            assertThat("The fun branch was built", fun.getLastBuild().getNumber(), is(1));
+            assertThat("The master branch source is the configured source",
+                    prj.getProjectFactory().getBranch(master).getSourceId(), is("firstId"));
+            assertThat("The change request source is the configured source",
+                    prj.getProjectFactory().getBranch(change).getSourceId(), is("secondId"));
+            assertThat("The fun branch source is the configured source",
+                    prj.getProjectFactory().getBranch(fun).getSourceId(), is("thirdId"));
+            prj.setSourcesList(Arrays.asList(
+                    new BranchSource(new MockSCMSource("idFirst", c, "foo", true, false, false)),
+                    new BranchSource(new MockSCMSource("idThird", c, "bar", true, false, false)),
+                    new BranchSource(new MockSCMSource("idSecond", c, "foo", false, false, true))
+            ));
+            Branch updated = prj.getProjectFactory().getBranch(master);
+            assertThat("The master branch source is new configured source",
+                    prj.getProjectFactory().getBranch(master).getSourceId(), is("idFirst"));
+            assertThat("The change request source is new configured source",
+                    prj.getProjectFactory().getBranch(change).getSourceId(), is("idSecond"));
+            assertThat("The fun branch source is new configured source",
+                    prj.getProjectFactory().getBranch(fun).getSourceId(), is("idThird"));
+            prj.scheduleBuild2(0).getFuture().get();
+            assertThat("The master branch was not rebuilt", master.getLastBuild().getNumber(), is(1));
+            assertThat("The change request was not rebuilt", change.getLastBuild().getNumber(), is(1));
+            assertThat("The fun branch was not rebuilt", master.getLastBuild().getNumber(), is(1));
+        }
+    }
+
     @Test
     public void given_multibranchWithSourcesWantingBranchesOnly_when_indexing_then_onlyBranchesAreFoundAndBuilt()
             throws Exception {