Merge pull request #255 from jglick/orderOfEntries

Save `flowNodeStore.xml` entries in order

src/main/java/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorage.java

@@ -44,7 +44,10 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
+import java.util.TreeMap;
 
 /**
  * {@link FlowNodeStorage} implementation that stores all the {@link FlowNode}s together in one file for efficient bulk I/O
@@ -64,7 +67,7 @@ public class BulkFlowNodeStorage extends FlowNodeStorage {
     private final FlowExecution exec;
 
     /** Lazy-loaded mapping. */
-    private transient HashMap<String, Tag> nodes = null;
+    private transient Map<String, Tag> nodes = null;
 
     /** If true, we've been modified since last flush. */
     private boolean isModified = false;
@@ -81,20 +84,21 @@ public BulkFlowNodeStorage(FlowExecution exec, File dir) {
     }
 
     /** Loads the nodes listing, lazily - so loading the {@link FlowExecution} doesn't trigger a more complex load. */
-    HashMap<String, Tag> getOrLoadNodes() throws IOException {
+    @SuppressWarnings("unchecked")
+    private Map<String, Tag> getOrLoadNodes() throws IOException {
         if (nodes == null) {
             if (dir.exists()) {
                 File storeFile = getStoreFile();
                 if (storeFile.exists()) {
-                    HashMap<String, Tag> roughNodes = null;
+                    Map<String, Tag> roughNodes = null;
                     try {
-                        roughNodes = (HashMap<String, Tag>) (XSTREAM.fromXML(getStoreFile()));
+                        roughNodes = (Map<String, Tag>) (XSTREAM.fromXML(getStoreFile()));
                     } catch (Exception ex) {
-                       nodes = new HashMap<String, Tag>();
+                       nodes = new HashMap<>();
                        throw new IOException("Failed to read nodes", ex);
                     }
                     if (roughNodes == null) {
-                        nodes = new HashMap<String, Tag>();
+                        nodes = new HashMap<>();
                         throw new IOException("Unable to load nodes, invalid data");
                     }
                     for (Tag t : roughNodes.values()) {
@@ -111,11 +115,11 @@ HashMap<String, Tag> getOrLoadNodes() throws IOException {
                     }
                     nodes = roughNodes;
                 } else {
-                    nodes = new HashMap<String, Tag>();
+                    nodes = new HashMap<>();
                 }
             } else {
                 IOUtils.mkdirs(dir);
-                nodes = new HashMap<String, Tag>();
+                nodes = new HashMap<>();
             }
         }
         return nodes;
@@ -128,6 +132,7 @@ public FlowNode getNode(@NonNull String id) throws IOException {
         return (t != null) ? t.node : null;
     }
 
+    @Override
     public void storeNode(@NonNull FlowNode n, boolean delayWritingActions) throws IOException {
         Tag t = getOrLoadNodes().get(n.getId());
         if (t != null) {
@@ -165,12 +170,24 @@ public void flush() throws IOException {
             if (!dir.exists()) {
                 IOUtils.mkdirs(dir);
             }
-            PipelineIOUtils.writeByXStream(nodes, getStoreFile(), XSTREAM, !this.isAvoidAtomicWrite());
+            Map<String, Tag> sorted = new TreeMap<>(BulkFlowNodeStorage::sort);
+            sorted.putAll(nodes);
+            // Serialize as LinkedHashMap so that XStream does not try to save the comparator:
+            PipelineIOUtils.writeByXStream(new LinkedHashMap<>(sorted), getStoreFile(), XSTREAM, !this.isAvoidAtomicWrite());
 
             isModified = false;
         }
     }
 
+    private static int sort(String k1, String k2) {
+        try {
+            return Integer.parseInt(k1) - Integer.parseInt(k2);
+        } catch (NumberFormatException x) {
+            return k1.compareTo(k2);
+        }
+    }
+
+    @Override
     public List<Action> loadActions(@NonNull FlowNode node) throws IOException {
         Tag t = getOrLoadNodes().get(node.getId());
         return (t != null) ? t.actions() : Collections.<Action>emptyList();
@@ -179,8 +196,9 @@ public List<Action> loadActions(@NonNull FlowNode node) throws IOException {
     /**
      * Just stores this one node
      */
+    @Override
     public void saveActions(@NonNull FlowNode node, @NonNull List<Action> actions) throws IOException {
-        HashMap<String, Tag> map = getOrLoadNodes();
+        Map<String, Tag> map = getOrLoadNodes();
         Tag t = map.get(node.getId());
         if (t != null) {
             t.node = node;
@@ -193,6 +211,7 @@ public void saveActions(@NonNull FlowNode node, @NonNull List<Action> actions) t
     }
 
     /** Have we written everything to disk that we need to, or is there something waiting to be written */
+    @Override
     public boolean isPersistedFully() {
         return !isModified;
     }

src/test/java/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorageTest.java

@@ -0,0 +1,61 @@
+/*
+ * The MIT License
+ *
+ * Copyright 2024 CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package org.jenkinsci.plugins.workflow.support.storage;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import javax.xml.parsers.DocumentBuilderFactory;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
+import org.jenkinsci.plugins.workflow.job.WorkflowJob;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.w3c.dom.Element;
+import org.w3c.dom.Node;
+
+public final class BulkFlowNodeStorageTest {
+
+    @Rule public JenkinsRule r = new JenkinsRule();
+
+    @Test public void orderOfEntries() throws Exception {
+        var p = r.createProject(WorkflowJob.class, "p");
+        p.setDefinition(new CpsFlowDefinition("for (int i = 1; i <= 40; i++) {echo(/step #$i/)}", true));
+        var b = r.buildAndAssertSuccess(p);
+        var entryList = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(new File(b.getRootDir(), "workflow-completed/flowNodeStore.xml")).getDocumentElement().getChildNodes();
+        var ids = new ArrayList<String>();
+        for (int i = 0; i < entryList.getLength(); i++) {
+            var entry = entryList.item(i);
+            if (entry.getNodeType() == Node.ELEMENT_NODE) {
+                ids.add(((Element) entry).getElementsByTagName("*").item(0).getTextContent());
+            }
+        }
+        assertThat(ids, is(IntStream.rangeClosed(2, 43).mapToObj(Integer::toString).collect(Collectors.toList())));
+    }
+
+}