opendev/gerrit
Code Issues Proposed changes

Add a RefFilterOption to always return all most recent ref changes

Browse Source 
This is useful only for installations that use reftables, for
example, googlesource.com, which can directly use
PermissionBackend.RefFilterOptions.

Currently, DefaultRefFilter filters the input and specifically for
refs/changes it filters it by returning only the available changes
in the change index and cache (all the most recent changes).
However, if no refs/changes are sent to this method, no refs/changes
are returned.

This change adds an option that when the option is set to true,
DefaultRefFilter ignores refs/changes sent as input, and always anyway
returns all changes available in the change index and cache (which are
the most recent changes). For Elastic and googlesource.com, the limit is
10000 changes, but this could be different.

This is useful since in some instances, there are O(millions) of
refs/changes. This change allows us not to send those millions of
refs/changes as input in the first place.

Tests include sending as input 0,1, and many refs (since those are all
separate code paths) and also with or without refs/* permission, which
is another code path that allows all refs to be returned.

Change-Id: I9a264b381ed0c94e8f2a662d120c741987481022
This commit is contained in:
Gal Paikin
2021-02-02 18:58:17 +01:00
parent 8b1c98a126
commit a0037092b5
3 changed files with 235 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

68
java/com/google/gerrit/server/permissions/DefaultRefFilter.java
View File

@@ -27,7 +27,6 @@ import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
@@ -57,9 +56,12 @@ import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectIdRef;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Ref.Storage;
import org.eclipse.jgit.lib.Repository;
class DefaultRefFilter {
@@ -82,7 +84,7 @@ class DefaultRefFilter {
private final Counter0 skipFilterCount;
private final boolean skipFullRefEvaluationIfAllRefsAreVisible;
private Map<Change.Id, BranchNameKey> visibleChanges;
private Map<Change.Id, ChangeNotes> visibleChanges;
@Inject
DefaultRefFilter(
@@ -135,6 +137,15 @@ class DefaultRefFilter {
"Project state %s permits read = %s",
projectState.getProject().getState(), projectState.statePermitsRead());
// If we anyway always return all available (most recent) changes in the change index and cache,
// we shouldn't care about refs/changes.
if (opts.returnMostRecentRefChanges()) {
refs =
refs.stream()
.filter(r -> !RefNames.isRefsChanges(r.getName()))
.collect(Collectors.toList());
}
// See if we can get away with a single, cheap ref evaluation.
if (refs.size() == 1) {
String refName = Iterables.getOnlyElement(refs).getName();
@@ -142,7 +153,7 @@ class DefaultRefFilter {
logger.atFinest().log("Filter out metadata ref %s", refName);
return ImmutableList.of();
}
if (RefNames.isRefsChanges(refName)) {
if (RefNames.isRefsChanges(refName) && !opts.returnMostRecentRefChanges()) {
boolean isChangeRefVisisble = canSeeSingleChangeRef(refName);
if (isChangeRefVisisble) {
logger.atFinest().log("Change ref %s is visible", refName);
@@ -185,10 +196,31 @@ class DefaultRefFilter {
}
}
if (opts.returnMostRecentRefChanges()) {
visibleChanges(repo).values().stream()
.forEach(n -> addAllChangeAndPatchsetRefs(visibleRefs, n));
}
logger.atFinest().log("visible refs = %s", visibleRefs);
return visibleRefs;
}
private void addAllChangeAndPatchsetRefs(Collection<Ref> refs, ChangeNotes changeNotes) {
refs.add(
new ObjectIdRef.PeeledNonTag(
Storage.PACKED,
RefNames.changeMetaRef(changeNotes.getChangeId()),
changeNotes.getMetaId()));
changeNotes
.getPatchSets()
.values()
.forEach(
p ->
refs.add(
new ObjectIdRef.PeeledNonTag(
Storage.PACKED, RefNames.patchSetRef(p.id()), p.commitId())));
}
/**
* Filters refs by visibility. Returns tags where visibility can't be trivially computed
* separately for later rev-walk-based visibility computation. Tags where visibility is trivial to
@@ -256,7 +288,8 @@ class DefaultRefFilter {
} else if ((changeId = Change.Id.fromRef(refName)) != null) {
// This is a mere performance optimization. RefVisibilityControl could determine the
// visibility of these refs just fine. But instead, we use highly-optimized logic that
// looks only on the last 10k most recent changes using the change index and a cache.
// looks only on the available changes in the change index and cache (which are the
// most recent changes).
if (hasAccessDatabase) {
resultRefs.add(ref);
} else if (!visible(repo, changeId)) {
@@ -315,6 +348,11 @@ class DefaultRefFilter {
}
private boolean visible(Repository repo, Change.Id changeId) throws PermissionBackendException {
return visibleChanges(repo).containsKey(changeId);
}
private Map<Change.Id, ChangeNotes> visibleChanges(Repository repo)
throws PermissionBackendException {
if (visibleChanges == null) {
if (changeCache == null) {
visibleChanges = visibleChangesByScan(repo);
@@ -323,7 +361,7 @@ class DefaultRefFilter {
}
logger.atFinest().log("Visible changes: %s", visibleChanges.keySet());
}
return visibleChanges.containsKey(changeId);
return visibleChanges;
}
private boolean visibleEdit(Repository repo, String name) throws PermissionBackendException {
@@ -340,15 +378,11 @@ class DefaultRefFilter {
return true;
}
// Initialize visibleChanges if it wasn't initialized yet.
if (visibleChanges == null) {
visible(repo, id);
}
if (visibleChanges.containsKey(id)) {
if (visible(repo, id)) {
try {
// Default to READ_PRIVATE_CHANGES as there is no special permission for reading edits.
permissionBackendForProject
.ref(visibleChanges.get(id).branch())
.ref(visibleChanges(repo).get(id).getChange().getDest().branch())
.check(RefPermission.READ_PRIVATE_CHANGES);
logger.atFinest().log("Foreign change edit ref is visible: %s", name);
return true;
@@ -362,17 +396,17 @@ class DefaultRefFilter {
return false;
}
private Map<Change.Id, BranchNameKey> visibleChangesBySearch() throws PermissionBackendException {
private Map<Change.Id, ChangeNotes> visibleChangesBySearch() throws PermissionBackendException {
Project.NameKey project = projectState.getNameKey();
try {
Map<Change.Id, BranchNameKey> visibleChanges = new HashMap<>();
Map<Change.Id, ChangeNotes> visibleChanges = new HashMap<>();
for (ChangeData cd : changeCache.getChangeData(project)) {
if (!projectState.statePermitsRead()) {
continue;
}
try {
permissionBackendForProject.change(cd).check(ChangePermission.READ);
visibleChanges.put(cd.getId(), cd.change().getDest());
visibleChanges.put(cd.getId(), cd.notes());
} catch (AuthException e) {
// Do nothing.
}
@@ -385,7 +419,7 @@ class DefaultRefFilter {
}
}
private Map<Change.Id, BranchNameKey> visibleChangesByScan(Repository repo)
private Map<Change.Id, ChangeNotes> visibleChangesByScan(Repository repo)
throws PermissionBackendException {
Project.NameKey p = projectState.getNameKey();
ImmutableList<ChangeNotesResult> changes;
@@ -397,11 +431,11 @@ class DefaultRefFilter {
return Collections.emptyMap();
}
Map<Change.Id, BranchNameKey> result = Maps.newHashMapWithExpectedSize(changes.size());
Map<Change.Id, ChangeNotes> result = Maps.newHashMapWithExpectedSize(changes.size());
for (ChangeNotesResult notesResult : changes) {
ChangeNotes notes = toNotes(notesResult);
if (notes != null) {
result.put(notes.getChangeId(), notes.getChange().getDest());
result.put(notes.getChangeId(), notes);
}
}
return result;

10
java/com/google/gerrit/server/permissions/PermissionBackend.java
View File

@@ -329,6 +329,13 @@ public abstract class PermissionBackend {
/** Remove all NoteDb refs (refs/changes/*, refs/users/*, edit refs) from the result. */
public abstract boolean filterMeta();
/**
* Return all of the visible change refs that are available in the change index (which are the
* most recent changes), even if they are not part of the List<Ref> passed. This allows the
* caller not to send all the refs/changes.
*/
public abstract boolean returnMostRecentRefChanges();
/**
* Select only refs with names matching prefixes per {@link
* org.eclipse.jgit.lib.RefDatabase#getRefsByPrefix}.
@@ -340,6 +347,7 @@ public abstract class PermissionBackend {
public static Builder builder() {
return new AutoValue_PermissionBackend_RefFilterOptions.Builder()
.setFilterMeta(false)
.setReturnMostRecentRefChanges(false)
.setPrefixes(Collections.singletonList(""));
}
@@ -347,6 +355,8 @@ public abstract class PermissionBackend {
public abstract static class Builder {
public abstract Builder setFilterMeta(boolean val);
public abstract Builder setReturnMostRecentRefChanges(boolean val);
public abstract Builder setPrefixes(List<String> prefixes);
public abstract RefFilterOptions build();

174
javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
View File

@@ -1422,6 +1422,180 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
}
}
@Test
public void advertiseMostRecentRefChangesEvenWhenNotInInputWithRefStarPermission()
throws Exception {
// admin has refs/* permission.
requestScopeOperations.setApiUser(admin.id());
try (Repository repo = repoManager.openRepository(project)) {
PermissionBackend.ForProject forProject = newFilter(project, admin);
assertThat(
names(
forProject.filter(
// set empty list of refs to filter
new ArrayList<>(),
repo,
RefFilterOptions.builder().setReturnMostRecentRefChanges(true).build())))
// all the change refs are still returned since returnMostRecentRefChanges = true
.containsExactlyElementsIn(
ImmutableList.of(
psRef1, metaRef1, psRef2, metaRef2, psRef3, metaRef3, psRef4, metaRef4));
}
}
@Test
public void advertiseMostRecentRefChangesEvenWhenNotInInputWithoutRefStarPermission()
throws Exception {
projectOperations
.project(project)
.forUpdate()
.add(allow(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS))
.update();
// user doesn't have refs/* permission.
requestScopeOperations.setApiUser(user.id());
try (Repository repo = repoManager.openRepository(project)) {
PermissionBackend.ForProject forProject = newFilter(project, admin);
assertThat(
names(
forProject.filter(
// set empty list of refs to filter
new ArrayList<>(),
repo,
RefFilterOptions.builder().setReturnMostRecentRefChanges(true).build())))
// all the change refs are still returned since returnMostRecentRefChanges = true
.containsExactlyElementsIn(
ImmutableList.of(
psRef1, metaRef1, psRef2, metaRef2, psRef3, metaRef3, psRef4, metaRef4));
}
}
@Test
public void advertiseMostRecentRefChangesOnlyOnceWithRefStarPermission() throws Exception {
// admin has refs/* permission.
requestScopeOperations.setApiUser(admin.id());
try (Repository repo = repoManager.openRepository(project)) {
PermissionBackend.ForProject forProject = newFilter(project, admin);
assertThat(
names(
forProject.filter(
repo.getRefDatabase().getRefs(),
repo,
RefFilterOptions.builder().setReturnMostRecentRefChanges(true).build())))
// all the change refs are still returned since returnMostRecentRefChanges = true. Make
// sure they are only returned once.
.containsExactlyElementsIn(
ImmutableList.of(
"HEAD",
psRef1,
metaRef1,
psRef2,
metaRef2,
psRef3,
metaRef3,
psRef4,
metaRef4,
"refs/heads/branch",
"refs/heads/master",
"refs/meta/config",
"refs/tags/branch-tag",
"refs/tags/master-tag",
"refs/tags/tree-tag"));
}
}
@Test
public void advertiseMostRecentRefChangesOnlyOnceWithoutRefStarPermission() throws Exception {
projectOperations
.project(project)
.forUpdate()
.add(allow(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS))
.update();
// user doesn't have refs/* permission.
requestScopeOperations.setApiUser(user.id());
try (Repository repo = repoManager.openRepository(project)) {
PermissionBackend.ForProject forProject = newFilter(project, admin);
assertThat(
names(
forProject.filter(
repo.getRefDatabase().getRefs(),
repo,
RefFilterOptions.builder().setReturnMostRecentRefChanges(true).build())))
// all the change refs are still returned since returnMostRecentRefChanges = true. Make
// sure they are only returned once.
.containsExactlyElementsIn(
ImmutableList.of(
"HEAD",
psRef1,
metaRef1,
psRef2,
metaRef2,
psRef3,
metaRef3,
psRef4,
metaRef4,
"refs/heads/branch",
"refs/heads/master",
"refs/meta/config",
"refs/tags/branch-tag",
"refs/tags/master-tag",
"refs/tags/tree-tag"));
}
}
@Test
public void advertiseMostRecentRefChangesWithSingleRequestedRefWithRefStarPermission()
throws Exception {
// admin has refs/* permission.
requestScopeOperations.setApiUser(admin.id());
try (Repository repo = repoManager.openRepository(project)) {
PermissionBackend.ForProject forProject = newFilter(project, admin);
assertThat(
names(
forProject.filter(
ImmutableList.of(repo.exactRef("HEAD")),
repo,
RefFilterOptions.builder().setReturnMostRecentRefChanges(true).build())))
// all the change refs are still returned since returnMostRecentRefChanges = true.
.containsExactlyElementsIn(
ImmutableList.of(
"HEAD", psRef1, metaRef1, psRef2, metaRef2, psRef3, metaRef3, psRef4, metaRef4));
}
}
@Test
public void advertiseMostRecentRefChangesWithSingleRefRequetedWithoutRefStarPermission()
throws Exception {
projectOperations
.project(project)
.forUpdate()
.add(allow(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS))
.update();
// user doesn't have refs/* permission.
requestScopeOperations.setApiUser(user.id());
try (Repository repo = repoManager.openRepository(project)) {
PermissionBackend.ForProject forProject = newFilter(project, admin);
assertThat(
names(
forProject.filter(
ImmutableList.of(repo.exactRef("HEAD")),
repo,
RefFilterOptions.builder().setReturnMostRecentRefChanges(true).build())))
// all the change refs are still returned since returnMostRecentRefChanges = true.
.containsExactlyElementsIn(
ImmutableList.of(
"HEAD", psRef1, metaRef1, psRef2, metaRef2, psRef3, metaRef3, psRef4, metaRef4));
}
}
@Test
public void fetchSingleChangeWithoutIndexAccess() throws Exception {
PushOneCommit.Result change = createChange();