From 41c36201f94d737a3c166d18f6f19cb401cf8f31 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Sat, 1 Jul 2017 16:09:17 -0700 Subject: [PATCH] Optimize BranchesCollection.parse for large repositories Repositories with excessively large number of branches suffer from very slow times looking up a branch in either the REST API, or the internal GerritApi.projects().project(p).branch(b) API that may be commonly used by plugins. Remove BranchInfo from BranchResource; it only needs to know the name and the revision. Defer construction of the full BranchInfo to GetBranch.apply(), where the action map and additional data is actually required. During BranchesCollection.parse(), verify the branch exists using the single-ref exactRef() operation, and check the caller has access to the branch according to PermissionBackend. This avoids considering every single branch in the repository when the operation only needs information on a single branch to continue. Change-Id: If2284dbcc45dcac8b2467426c5719e39be83e30a --- .../server/api/projects/BranchApiImpl.java | 12 +++-- .../gerrit/server/project/BranchResource.java | 20 ++++--- .../server/project/BranchesCollection.java | 53 ++++++++++++++----- .../gerrit/server/project/GetBranch.java | 14 ++++- .../gerrit/server/project/ListBranches.java | 18 ++++++- .../gerrit/server/project/PutBranch.java | 3 +- 6 files changed, 87 insertions(+), 33 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/projects/BranchApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/projects/BranchApiImpl.java index afcd273d42..642791a683 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/projects/BranchApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/projects/BranchApiImpl.java @@ -23,12 +23,14 @@ import com.google.gerrit.extensions.api.projects.ReflogEntryInfo; import com.google.gerrit.extensions.restapi.BinaryResult; import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.BranchResource; import com.google.gerrit.server.project.BranchesCollection; import com.google.gerrit.server.project.CreateBranch; import com.google.gerrit.server.project.DeleteBranch; import com.google.gerrit.server.project.FileResource; import com.google.gerrit.server.project.FilesCollection; +import com.google.gerrit.server.project.GetBranch; import com.google.gerrit.server.project.GetContent; import com.google.gerrit.server.project.GetReflog; import com.google.gerrit.server.project.ProjectResource; @@ -46,6 +48,7 @@ public class BranchApiImpl implements BranchApi { private final CreateBranch.Factory createBranchFactory; private final DeleteBranch deleteBranch; private final FilesCollection filesCollection; + private final GetBranch getBranch; private final GetContent getContent; private final GetReflog getReflog; private final String ref; @@ -57,6 +60,7 @@ public class BranchApiImpl implements BranchApi { CreateBranch.Factory createBranchFactory, DeleteBranch deleteBranch, FilesCollection filesCollection, + GetBranch getBranch, GetContent getContent, GetReflog getReflog, @Assisted ProjectResource project, @@ -65,6 +69,7 @@ public class BranchApiImpl implements BranchApi { this.createBranchFactory = createBranchFactory; this.deleteBranch = deleteBranch; this.filesCollection = filesCollection; + this.getBranch = getBranch; this.getContent = getContent; this.getReflog = getReflog; this.project = project; @@ -84,7 +89,7 @@ public class BranchApiImpl implements BranchApi { @Override public BranchInfo get() throws RestApiException { try { - return resource().getBranchInfo(); + return getBranch.apply(resource()); } catch (Exception e) { throw asRestApiException("Cannot read branch", e); } @@ -113,12 +118,13 @@ public class BranchApiImpl implements BranchApi { public List reflog() throws RestApiException { try { return getReflog.apply(resource()); - } catch (IOException e) { + } catch (IOException | PermissionBackendException e) { throw new RestApiException("Cannot retrieve reflog", e); } } - private BranchResource resource() throws RestApiException, IOException { + private BranchResource resource() + throws RestApiException, IOException, PermissionBackendException { return branches.parse(project, IdString.fromDecoded(ref)); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/BranchResource.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/BranchResource.java index db239679e1..2e81af3f15 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/BranchResource.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/BranchResource.java @@ -14,37 +14,35 @@ package com.google.gerrit.server.project; -import com.google.gerrit.extensions.api.projects.BranchInfo; import com.google.gerrit.extensions.restapi.RestView; import com.google.gerrit.reviewdb.client.Branch; import com.google.inject.TypeLiteral; +import org.eclipse.jgit.lib.Ref; public class BranchResource extends RefResource { public static final TypeLiteral> BRANCH_KIND = new TypeLiteral>() {}; - private final BranchInfo branchInfo; + private final String refName; + private final String revision; - public BranchResource(ProjectControl control, BranchInfo branchInfo) { + public BranchResource(ProjectControl control, Ref ref) { super(control); - this.branchInfo = branchInfo; - } - - public BranchInfo getBranchInfo() { - return branchInfo; + this.refName = ref.getName(); + this.revision = ref.getObjectId() != null ? ref.getObjectId().name() : null; } public Branch.NameKey getBranchKey() { - return new Branch.NameKey(getNameKey(), branchInfo.ref); + return new Branch.NameKey(getNameKey(), refName); } @Override public String getRef() { - return branchInfo.ref; + return refName; } @Override public String getRevision() { - return branchInfo.revision; + return revision; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/BranchesCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/BranchesCollection.java index 6867dce06e..77cc903128 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/BranchesCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/BranchesCollection.java @@ -14,36 +14,51 @@ package com.google.gerrit.server.project; -import com.google.gerrit.extensions.api.projects.BranchInfo; import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.restapi.AcceptsCreate; -import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.ChildCollection; import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestView; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.RefPermission; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; -import java.util.List; -import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.errors.RepositoryNotFoundException; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Repository; @Singleton public class BranchesCollection implements ChildCollection, AcceptsCreate { private final DynamicMap> views; private final Provider list; + private final PermissionBackend permissionBackend; + private final Provider user; + private final GitRepositoryManager repoManager; private final CreateBranch.Factory createBranchFactory; @Inject BranchesCollection( DynamicMap> views, Provider list, + PermissionBackend permissionBackend, + Provider user, + GitRepositoryManager repoManager, CreateBranch.Factory createBranchFactory) { this.views = views; this.list = list; + this.permissionBackend = permissionBackend; + this.user = user; + this.repoManager = repoManager; this.createBranchFactory = createBranchFactory; } @@ -54,18 +69,28 @@ public class BranchesCollection @Override public BranchResource parse(ProjectResource parent, IdString id) - throws ResourceNotFoundException, IOException, BadRequestException { - String branchName = id.get(); - if (!branchName.equals(Constants.HEAD)) { - branchName = RefNames.fullName(branchName); - } - List branches = list.get().apply(parent); - for (BranchInfo b : branches) { - if (branchName.equals(b.ref)) { - return new BranchResource(parent.getControl(), b); + throws ResourceNotFoundException, IOException, PermissionBackendException { + Project.NameKey project = parent.getNameKey(); + try (Repository repo = repoManager.openRepository(project)) { + Ref ref = repo.exactRef(RefNames.fullName(id.get())); + if (ref == null) { + throw new ResourceNotFoundException(id); } + + // ListBranches checks the target of a symbolic reference to determine access + // rights on the symbolic reference itself. This check prevents seeing a hidden + // branch simply because the symbolic reference name was visible. + permissionBackend + .user(user) + .project(project) + .ref(ref.isSymbolic() ? ref.getTarget().getName() : ref.getName()) + .check(RefPermission.READ); + return new BranchResource(parent.getControl(), ref); + } catch (AuthException notAllowed) { + throw new ResourceNotFoundException(id); + } catch (RepositoryNotFoundException noRepo) { + throw new ResourceNotFoundException(); } - throw new ResourceNotFoundException(); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/GetBranch.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/GetBranch.java index 78878a7212..456263043c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/GetBranch.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/GetBranch.java @@ -15,14 +15,24 @@ package com.google.gerrit.server.project; import com.google.gerrit.extensions.api.projects.BranchInfo; +import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestReadView; +import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.Singleton; +import java.io.IOException; @Singleton public class GetBranch implements RestReadView { + private final Provider list; + + @Inject + GetBranch(Provider list) { + this.list = list; + } @Override - public BranchInfo apply(BranchResource rsrc) { - return rsrc.getBranchInfo(); + public BranchInfo apply(BranchResource rsrc) throws ResourceNotFoundException, IOException { + return list.get().toBranchInfo(rsrc); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListBranches.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListBranches.java index ecb3ea69b7..1a0aff09c7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListBranches.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListBranches.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.project; import com.google.common.collect.ComparisonChain; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Sets; import com.google.gerrit.extensions.api.projects.BranchInfo; import com.google.gerrit.extensions.common.ActionInfo; @@ -128,6 +129,18 @@ public class ListBranches implements RestReadView { .filter(allBranches(rsrc)); } + BranchInfo toBranchInfo(BranchResource rsrc) throws IOException, ResourceNotFoundException { + try (Repository db = repoManager.openRepository(rsrc.getNameKey())) { + Ref r = db.exactRef(rsrc.getRef()); + if (r == null) { + throw new ResourceNotFoundException(); + } + return toBranchInfo(rsrc, ImmutableList.of(r)).get(0); + } catch (RepositoryNotFoundException noRepo) { + throw new ResourceNotFoundException(); + } + } + private List allBranches(ProjectResource rsrc) throws IOException, ResourceNotFoundException { List refs; @@ -142,7 +155,10 @@ public class ListBranches implements RestReadView { } catch (RepositoryNotFoundException noGitRepository) { throw new ResourceNotFoundException(); } + return toBranchInfo(rsrc, refs); + } + private List toBranchInfo(ProjectResource rsrc, List refs) { Set targets = Sets.newHashSetWithExpectedSize(1); for (Ref ref : refs) { if (ref.isSymbolic()) { @@ -213,7 +229,7 @@ public class ListBranches implements RestReadView { info.canDelete = !targets.contains(ref.getName()) && perm.testOrFalse(RefPermission.DELETE) ? true : null; - BranchResource rsrc = new BranchResource(pctl, info); + BranchResource rsrc = new BranchResource(pctl, ref); for (UiAction.Description d : uiActions.from(branchViews, rsrc)) { if (info.actions == null) { info.actions = new TreeMap<>(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/PutBranch.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/PutBranch.java index 9e0db6e3f4..8e8efafe78 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/PutBranch.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/PutBranch.java @@ -25,7 +25,6 @@ public class PutBranch implements RestModifyView { @Override public BranchInfo apply(BranchResource rsrc, BranchInput input) throws ResourceConflictException { - throw new ResourceConflictException( - "Branch \"" + rsrc.getBranchInfo().ref + "\" already exists"); + throw new ResourceConflictException("Branch \"" + rsrc.getRef() + "\" already exists"); } }