Merge "Convert SectionSortCache to use get/loader instead of getIfPresent/put"

java/com/google/gerrit/server/permissions/SectionSortCache.java

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.server.permissions;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.cache.Cache;
 import com.google.common.collect.ImmutableList;
@@ -28,6 +30,8 @@ import com.google.inject.name.Named;
 import java.util.ArrayList;
 import java.util.IdentityHashMap;
 import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
 
 /**
  * Caches the order AccessSections should be sorted for evaluation.
@@ -60,67 +64,62 @@ public class SectionSortCache {
     this.cache = cache;
   }
 
-  // Sorts the given sections, but does not disturb ordering between equally exact sections.
+  /**
+   * Sorts the given sections in-place, but does not disturb ordering between equally exact
+   * sections.
+   */
   void sort(String ref, List<AccessSection> sections) {
     final int cnt = sections.size();
     if (cnt <= 1) {
       return;
     }
-
     EntryKey key = EntryKey.create(ref, sections);
-    EntryVal val = cache.getIfPresent(key);
-    if (val != null) {
-      int[] srcIdx = val.order;
-      if (srcIdx != null) {
-        AccessSection[] srcList = copy(sections);
-        for (int i = 0; i < cnt; i++) {
-          sections.set(i, srcList[srcIdx[i]]);
-        }
-      } else {
-        // Identity transform. No sorting is required.
-      }
+    EntryVal val;
+    try {
+      val = cache.get(key, new Loader(key, sections));
+    } catch (ExecutionException e) {
+      logger.atWarning().withCause(e).log("Error happened while sorting access sections.");
+      return;
+    }
+    ImmutableList<Integer> order = val.order();
+    List<AccessSection> sorted = new ArrayList<>();
+    for (int i = 0; i < cnt; i++) {
+      sorted.add(sections.get(order.get(i)));
+    }
+    for (int i = 0; i < cnt; i++) {
+      sections.set(i, sorted.get(i));
+    }
+  }
 
-    } else {
-      boolean poison = false;
+  private static class Loader implements Callable<EntryVal> {
+    private final List<AccessSection> sections;
+    EntryKey key;
+
+    Loader(EntryKey key, List<AccessSection> sections) {
+      this.key = key;
+      this.sections = sections;
+    }
+
+    @Override
+    public EntryVal call() throws Exception {
+      // We use IdentityHashMap (which uses reference equality for keys/values) to preserve distinct
+      // entries in the map for identical AccessSection keys
       IdentityHashMap<AccessSection, Integer> srcMap = new IdentityHashMap<>();
-      for (int i = 0; i < cnt; i++) {
-        poison |= srcMap.put(sections.get(i), i) != null;
+      for (int i = 0; i < sections.size(); i++) {
+        srcMap.put(sections.get(i), i);
       }
-
-      sections.sort(new MostSpecificComparator(ref));
-
-      int[] srcIdx;
-      if (isIdentityTransform(sections, srcMap)) {
-        srcIdx = null;
-      } else {
-        srcIdx = new int[cnt];
-        for (int i = 0; i < cnt; i++) {
-          srcIdx[i] = srcMap.get(sections.get(i));
-        }
-      }
-
-      if (poison) {
-        logger.atSevere().log("Received duplicate AccessSection instances, not caching sort");
-      } else {
-        cache.put(key, new EntryVal(srcIdx));
+      ImmutableList<AccessSection> sorted =
+          sections.stream()
+              .sorted(new MostSpecificComparator(key.ref()))
+              .collect(toImmutableList());
+      ImmutableList.Builder<Integer> order = ImmutableList.builderWithExpectedSize(sections.size());
+      for (int i = 0; i < sorted.size(); i++) {
+        order.add(srcMap.get(sorted.get(i)));
       }
+      return EntryVal.create(order.build());
     }
   }
 
-  private static AccessSection[] copy(List<AccessSection> sections) {
-    return sections.toArray(new AccessSection[sections.size()]);
-  }
-
-  private static boolean isIdentityTransform(
-      List<AccessSection> sections, IdentityHashMap<AccessSection, Integer> srcMap) {
-    for (int i = 0; i < sections.size(); i++) {
-      if (i != srcMap.get(sections.get(i))) {
-        return false;
-      }
-    }
-    return true;
-  }
-
   @AutoValue
   abstract static class EntryKey {
     public abstract String ref();
@@ -146,17 +145,18 @@ public class SectionSortCache {
     }
   }
 
-  static final class EntryVal {
+  @AutoValue
+  abstract static class EntryVal {
     /**
      * Maps the input index to the output index.
      *
      * <p>For {@code x == order[y]} the expression means move the item at source position {@code x}
      * to the output position {@code y}.
      */
-    final int[] order;
+    abstract ImmutableList<Integer> order();
 
-    EntryVal(int[] order) {
-      this.order = order;
+    static EntryVal create(ImmutableList<Integer> order) {
+      return new AutoValue_SectionSortCache_EntryVal(order);
     }
   }
 }

javatests/com/google/gerrit/server/permissions/SectionSortCacheTest.java

@@ -0,0 +1,84 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.permissions;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.gerrit.entities.AccessSection;
+import com.google.gerrit.server.permissions.SectionSortCache.EntryKey;
+import com.google.gerrit.server.permissions.SectionSortCache.EntryVal;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Test for {@link SectionSortCache} */
+public class SectionSortCacheTest {
+  private SectionSortCache sectionSortCache;
+  private Cache<EntryKey, EntryVal> cache;
+
+  private static final AccessSection sectionA = AccessSection.create("refs/heads/branch_1");
+  private static final AccessSection sectionB = AccessSection.create("refs/base/branch_2");
+  private static final String REF_BASE = "refs/base";
+
+  @Before
+  public void setup() {
+    cache = CacheBuilder.newBuilder().build();
+    sectionSortCache = new SectionSortCache(cache);
+  }
+
+  @Test
+  public void sortSingleElement() {
+    List<AccessSection> input = new ArrayList<>();
+    input.add(sectionA);
+    sectionSortCache.sort(REF_BASE, input);
+    assertThat(input).containsExactly(sectionA);
+  }
+
+  @Test
+  public void sortMultiElements() {
+    List<AccessSection> input = new ArrayList<>();
+    input.add(sectionA);
+    input.add(sectionB);
+    sectionSortCache.sort(REF_BASE, input);
+    assertThat(input).containsExactly(sectionB, sectionA).inOrder();
+  }
+
+  @Test
+  public void sortMultiElementsWhenAlreadyOrdered() {
+    List<AccessSection> input = new ArrayList<>();
+    input.add(sectionB);
+    input.add(sectionA);
+    sectionSortCache.sort(REF_BASE, input);
+    assertThat(input).containsExactly(sectionB, sectionA).inOrder();
+  }
+
+  @Test
+  public void sortMultiElementsWithDuplicates() {
+    AccessSection sectionAClone = sectionA.toBuilder().build();
+    AccessSection sectionBClone = sectionB.toBuilder().build();
+    AccessSection[] input = {sectionBClone, sectionA, sectionAClone, sectionA, sectionB};
+    List<AccessSection> sorted = Arrays.asList(input);
+    sectionSortCache.sort(REF_BASE, sorted);
+    // Cache preserves relative order (reference equality) for identical elements
+    AccessSection[] expected = {sectionBClone, sectionB, sectionA, sectionAClone, sectionA};
+    for (int i = 0; i < sorted.size(); i++) {
+      assert (sorted.get(i) == expected[i]);
+    }
+  }
+}