diff --git a/java/com/google/gerrit/server/permissions/SectionSortCache.java b/java/com/google/gerrit/server/permissions/SectionSortCache.java index 6081e9a65a..d800782156 100644 --- a/java/com/google/gerrit/server/permissions/SectionSortCache.java +++ b/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 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 order = val.order(); + List 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 { + private final List sections; + EntryKey key; + + Loader(EntryKey key, List 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 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 sorted = + sections.stream() + .sorted(new MostSpecificComparator(key.ref())) + .collect(toImmutableList()); + ImmutableList.Builder 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 sections) { - return sections.toArray(new AccessSection[sections.size()]); - } - - private static boolean isIdentityTransform( - List sections, IdentityHashMap 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. * *

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 order(); - EntryVal(int[] order) { - this.order = order; + static EntryVal create(ImmutableList order) { + return new AutoValue_SectionSortCache_EntryVal(order); } } } diff --git a/javatests/com/google/gerrit/server/permissions/SectionSortCacheTest.java b/javatests/com/google/gerrit/server/permissions/SectionSortCacheTest.java new file mode 100644 index 0000000000..9ec162501b --- /dev/null +++ b/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 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 input = new ArrayList<>(); + input.add(sectionA); + sectionSortCache.sort(REF_BASE, input); + assertThat(input).containsExactly(sectionA); + } + + @Test + public void sortMultiElements() { + List 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 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 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]); + } + } +}