[JENKINS-41255] Propagate SCMSource id changes when the new SCMSource…

… is identical to the old
jenkinsci · Jan 20, 2017 · 95bd3e9 · 95bd3e9
1 parent a06c22d
commit 95bd3e9
Show file tree

Hide file tree

Showing 3 changed files with 234 additions and 4 deletions.
diff --git a/src/main/java/jenkins/branch/MultiBranchProject.java b/src/main/java/jenkins/branch/MultiBranchProject.java
@@ -58,12 +58,15 @@
 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;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.io.UnsupportedEncodingException;
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -72,6 +75,7 @@
 import java.util.IdentityHashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.ListIterator;
 import java.util.Map;
 import java.util.Set;
 import java.util.TreeSet;
@@ -357,6 +361,146 @@ 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
+     * @throws IOException
+     */
+    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);
+
+        // we will be manipulating the sources, so make sure we own the backing list impl
+        sources = new ArrayList<>(sources);
+        for (ListIterator<BranchSource> i = sources.listIterator(); i.hasNext(); ) {
+            BranchSource addition = i.next();
+            if (!additions.contains(addition.getSource().getId())) {
+                continue;
+            }
+            for (BranchSource removal: this.sources) {
+                if (!removals.contains(removal.getSource().getId())) {
+                    continue;
+                }
+                if (!equalButForId(removal.getSource(), addition.getSource())) {
+                    continue;
+                }
+                changedIds.put(removal.getSource().getId(), addition.getSource().getId());
+                // now take this one out of consideration
+                removals.remove(removal.getSource().getId());
+                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;
+        }
+        boolean fallBack = false;
+        Class<?> clazz = a.getClass();
+        REFLECTION:
+        while (clazz != null && SCMSource.class.isAssignableFrom(clazz) && clazz != SCMSource.class) {
+            for (Field field : clazz.getDeclaredFields()) {
+                int modifiers = field.getModifiers();
+                if (Modifier.isStatic(modifiers)) {
+                    continue;
+                }
+                if (Modifier.isTransient(modifiers)) {
+                    continue;
+                }
+                // TODO Will Java 9 modules lock setAccessible permissions? Likely would break XStream too!
+                if (!field.isAccessible()) {
+                    field.setAccessible(true);
+                }
+                if (!field.isAccessible()) {
+                    // err on the side of caution
+                    fallBack = true;
+                    break REFLECTION;
+                }
+                Object aField;
+                Object bField;
+                try {
+                    aField = field.get(a);
+                    bField = field.get(b);
+                } catch (IllegalAccessException e) {
+                    // err on the side of caution
+                    fallBack = true;
+                    break REFLECTION;
+                }
+                if (aField == null && bField == null) {
+                    // next field
+                    continue;
+                }
+                if (aField == null || bField == null) {
+                    // we know they are different
+                    return false;
+                }
+                if (aField.equals(bField)) {
+                    // next field
+                    continue;
+                }
+                if (aField instanceof Boolean || aField instanceof Number || aField instanceof String) {
+                    // no need to compare primatives by XML, we know they are different
+                    return false;
+                }
+                if (SOURCE_ID_OMITTED_XSTREAM.toXML(aField).equals(SOURCE_ID_OMITTED_XSTREAM.toXML(bField))) {
+                    // next field
+                    continue;
+                }
+                // different enough
+                return false;
+            }
+
+            clazz = clazz.getSuperclass();
+        }
+        return !fallBack || 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}
      */
@@ -756,7 +900,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 {