[JENKINS-75986] Refactor BuildReferenceMapAdapter (#10946)

Co-authored-by: Jesse Glick <jglick@cloudbees.com>
Co-authored-by: Dmytro Ukhlov <dukhlov@csod.com>

Author: 3 people

core/src/main/java/hudson/model/RunMap.java

@@ -95,7 +95,7 @@ public RunMap(@NonNull Job<?, ?> job) {
      *      Used to create new instance of {@link Run}.
      * @since 2.451
      */
-    public RunMap(@NonNull Job<?, ?> job, Constructor cons) {
+    public RunMap(@NonNull Job<?, ?> job, Constructor<R> cons) {
         this.job = Objects.requireNonNull(job);
         this.cons = cons;
         initBaseDir(job.getBuildDir());
@@ -105,7 +105,7 @@ public RunMap(@NonNull Job<?, ?> job, Constructor cons) {
      * @deprecated Use {@link #RunMap(Job, Constructor)}.
      */
     @Deprecated
-    public RunMap(File baseDir, Constructor cons) {
+    public RunMap(File baseDir, Constructor<R> cons) {
         job = null;
         this.cons = cons;
         initBaseDir(baseDir);
@@ -187,6 +187,8 @@ public R oldestValue() {
      */
     public interface Constructor<R extends Run<?, R>> {
         R create(File dir) throws IOException;
+
+        Class<R> getBuildClass();
     }
 
     @Override
@@ -201,7 +203,7 @@ protected String getIdOf(R r) {
 
     /**
      * Add a <em>new</em> build to the map.
-     * Do not use when loading existing builds (use {@link #put(Integer, Object)}).
+     * Do not use when loading existing builds (use {@link #putAll(Map)}).
      */
     @Override
     public R put(R r) {
@@ -298,6 +300,11 @@ protected R retrieve(File d) throws IOException {
         return null;
     }
 
+    @Override
+    protected Class<R> getBuildClass() {
+        return cons.getBuildClass();
+    }
+
     /**
      * Backward compatibility method that notifies {@link RunMap} of who the owner is.
      *

core/src/main/java/hudson/util/Iterators.java

@@ -35,6 +35,7 @@
 import java.util.NoSuchElementException;
 import java.util.Objects;
 import java.util.Set;
+import java.util.function.Function;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
@@ -326,6 +327,18 @@ public static <T> Iterator<T> removeNull(final Iterator<T> itr) {
         return com.google.common.collect.Iterators.filter(itr, Objects::nonNull);
     }
 
+    /**
+     * Wraps another iterator and map iterable objects.
+     */
+    public static <T, U> Iterator<U> map(final Iterator<T> itr, Function<T, U> mapper) {
+        return new AdaptedIterator<>(itr) {
+            @Override
+            protected U adapt(T item) {
+                return mapper.apply(item);
+            }
+        };
+    }
+
     /**
      * Returns an {@link Iterable} that iterates over all the given {@link Iterable}s.
      *

core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java

@@ -35,9 +35,7 @@
 import hudson.util.CopyOnWriteMap;
 import java.io.File;
 import java.io.IOException;
-import java.util.AbstractCollection;
 import java.util.AbstractMap;
-import java.util.AbstractSet;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
@@ -46,10 +44,7 @@
 import java.util.NoSuchElementException;
 import java.util.Set;
 import java.util.SortedMap;
-import java.util.Spliterator;
-import java.util.Spliterators;
 import java.util.TreeMap;
-import java.util.function.IntConsumer;
 import java.util.function.IntPredicate;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -101,146 +96,28 @@
 public abstract class AbstractLazyLoadRunMap<R> extends AbstractMap<Integer, R> implements SortedMap<Integer, R> {
     private final CopyOnWriteMap.Tree<Integer, BuildReference<R>> core = new CopyOnWriteMap.Tree<>(
             Collections.reverseOrder());
-
-    private LazyLoadRunMapEntrySet<R> entrySet = new LazyLoadRunMapEntrySet<>(this);
-
-    private transient volatile Set<Integer> keySet;
-    private transient volatile Collection<R> values;
+    private final BuildReferenceMapAdapter<R> adapter = new BuildReferenceMapAdapter<>(core,
+            this::resolveBuildRef, this::getNumberOf, this::getBuildClass) {
+        @Override
+        protected boolean removeValue(R value) {
+            return AbstractLazyLoadRunMap.this.removeValue(value);
+        }
+    };
 
     @Override
     public Set<Integer> keySet() {
-        Set<Integer> ks = keySet;
-        if (ks == null) {
-            ks = new AbstractSet<>() {
-                @Override
-                public Iterator<Integer> iterator() {
-                    return new Iterator() {
-                        private final Iterator<Entry<Integer, R>> it = entrySet().iterator();
-
-                        @Override
-                        public boolean hasNext() {
-                            return it.hasNext();
-                        }
-
-                        @Override
-                        public Integer next() {
-                            return it.next().getKey();
-                        }
-
-                        @Override
-                        public void remove() {
-                            it.remove();
-                        }
-                    };
-                }
-
-                @Override
-                public Spliterator<Integer> spliterator() {
-                    return new Spliterators.AbstractIntSpliterator(
-                            Long.MAX_VALUE,
-                            Spliterator.DISTINCT | Spliterator.ORDERED | Spliterator.SORTED) {
-                        private final Iterator<Integer> it = iterator();
-
-                        @Override
-                        public boolean tryAdvance(IntConsumer action) {
-                            if (action == null) {
-                                throw new NullPointerException();
-                            }
-                            if (it.hasNext()) {
-                                action.accept(it.next());
-                                return true;
-                            }
-                            return false;
-                        }
-
-                        @Override
-                        public Comparator<Integer> getComparator() {
-                            return Collections.reverseOrder();
-                        }
-                    };
-                }
-
-                @Override
-                public int size() {
-                    return AbstractLazyLoadRunMap.this.size();
-                }
-
-                @Override
-                public boolean isEmpty() {
-                    return AbstractLazyLoadRunMap.this.isEmpty();
-                }
-
-                @Override
-                public void clear() {
-                    AbstractLazyLoadRunMap.this.clear();
-                }
-
-                @Override
-                public boolean contains(Object k) {
-                    return AbstractLazyLoadRunMap.this.containsKey(k);
-                }
-            };
-            keySet = ks;
-        }
-        return ks;
+        return adapter.keySet();
     }
 
     @Override
     public Collection<R> values() {
-        Collection<R> vals = values;
-        if (vals == null) {
-            vals = new AbstractCollection<>() {
-                @Override
-                public Iterator<R> iterator() {
-                    return new Iterator<>() {
-                        private final Iterator<Entry<Integer, R>> it = entrySet().iterator();
-
-                        @Override
-                        public boolean hasNext() {
-                            return it.hasNext();
-                        }
-
-                        @Override
-                        public R next() {
-                            return it.next().getValue();
-                        }
-
-                        @Override
-                        public void remove() {
-                            it.remove();
-                        }
-                    };
-                }
-
-                @Override
-                public Spliterator<R> spliterator() {
-                    return Spliterators.spliteratorUnknownSize(
-                            iterator(), Spliterator.DISTINCT | Spliterator.ORDERED);
-                }
-
-                @Override
-                public int size() {
-                    return AbstractLazyLoadRunMap.this.size();
-                }
-
-                @Override
-                public boolean isEmpty() {
-                    return AbstractLazyLoadRunMap.this.isEmpty();
-                }
-
-                @Override
-                public void clear() {
-                    AbstractLazyLoadRunMap.this.clear();
-                }
+        return adapter.values();
+    }
 
-                @Override
-                public boolean contains(Object v) {
-                    return AbstractLazyLoadRunMap.this.containsValue(v);
-                }
-            };
-            values = vals;
-        }
-        return vals;
+    @Override
+    public Set<Entry<Integer, R>> entrySet() {
+        assert baseDirInitialized();
+        return adapter.entrySet();
     }
 
     /**
@@ -354,18 +231,22 @@ public final void recognizeNumber(int buildNumber) {
 
     @Override
     public Comparator<? super Integer> comparator() {
-        return Collections.reverseOrder();
+        return core.comparator();
     }
 
     @Override
     public boolean isEmpty() {
-        return search(Integer.MAX_VALUE, DESC) == null;
+        return adapter.isEmpty();
     }
 
     @Override
-    public Set<Entry<Integer, R>> entrySet() {
-        assert baseDirInitialized();
-        return entrySet;
+    public boolean containsKey(Object value) {
+        return adapter.containsKey(value);
+    }
+
+    @Override
+    public boolean containsValue(Object value) {
+        return adapter.containsValue(value);
     }
 
     /**
@@ -379,7 +260,7 @@ public SortedMap<Integer, R> getLoadedBuilds() {
                 res.put(entry.getKey(), buildRef);
             }
         }
-        return Collections.unmodifiableSortedMap(new BuildReferenceMapAdapter<>(this, res));
+        return new BuildReferenceMapAdapter<>(res, this::resolveBuildRef, this::getNumberOf, this::getBuildClass);
     }
 
     /**
@@ -390,33 +271,17 @@ public SortedMap<Integer, R> getLoadedBuilds() {
      */
     @Override
     public SortedMap<Integer, R> subMap(Integer fromKey, Integer toKey) {
-        // TODO: if this method can produce a lazy map, that'd be wonderful
-        // because due to the lack of floor/ceil/higher/lower kind of methods
-        // to look up keys in SortedMap, various places of Jenkins rely on
-        // subMap+firstKey/lastKey combo.
-
-        R start = search(fromKey, DESC);
-        if (start == null)    return EMPTY_SORTED_MAP;
-
-        R end = search(toKey, ASC);
-        if (end == null)      return EMPTY_SORTED_MAP;
-
-        for (R i = start; i != end; ) {
-            i = search(getNumberOf(i) - 1, DESC);
-            assert i != null;
-        }
-
-        return Collections.unmodifiableSortedMap(new BuildReferenceMapAdapter<>(this, core.subMap(fromKey, toKey)));
+        return adapter.subMap(fromKey, toKey);
     }
 
     @Override
     public SortedMap<Integer, R> headMap(Integer toKey) {
-        return subMap(Integer.MAX_VALUE, toKey);
+        return adapter.headMap(toKey);
     }
 
     @Override
     public SortedMap<Integer, R> tailMap(Integer fromKey) {
-        return subMap(fromKey, Integer.MIN_VALUE);
+        return adapter.tailMap(fromKey);
     }
 
     @Override
@@ -443,11 +308,7 @@ public R oldestBuild() {
 
     @Override
     public R get(Object key) {
-        if (key instanceof Integer) {
-            int n = (Integer) key;
-            return get(n);
-        }
-        return super.get(key);
+        return adapter.get(key);
     }
 
     public R get(int n) {
@@ -552,7 +413,7 @@ private R resolveBuildRef(BuildReference<R> ref) {
     }
 
     public R getByNumber(int n) {
-        return resolveBuildRef(core.get(n));
+        return adapter.get(n);
     }
 
     /**
@@ -597,14 +458,9 @@ public synchronized void putAll(Map<? extends Integer, ? extends R> newData) {
         core.putAll(newWrapperData);
     }
 
-    /**
-     * Return underlining {@link BuildReference} core map.
-     *
-     * @return
-     *      full build reference map.
-     */
-    /*package*/ SortedMap<Integer, BuildReference<R>> all() {
-        return core;
+    @Override
+    public R remove(Object key) {
+        return adapter.remove(key);
     }
 
     /**
@@ -652,7 +508,6 @@ protected BuildReference<R> createReference(R r) {
         return new BuildReference<>(getIdOf(r), r);
     }
 
-
     /**
      * Parses {@code R} instance from data in the specified directory.
      *
@@ -664,6 +519,8 @@ protected BuildReference<R> createReference(R r) {
      */
     protected abstract R retrieve(File dir) throws IOException;
 
+    protected abstract Class<R> getBuildClass();
+
     public synchronized boolean removeValue(R run) {
         return core.remove(getNumberOf(run)) != null;
     }
@@ -694,7 +551,5 @@ public enum Direction {
         ASC, DESC, EXACT
     }
 
-    private static final SortedMap EMPTY_SORTED_MAP = Collections.unmodifiableSortedMap(new TreeMap());
-
     static final Logger LOGGER = Logger.getLogger(AbstractLazyLoadRunMap.class.getName());
 }