Merge "Optimize BranchesCollection.parse for large repositories"

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<ReflogEntryInfo> 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));
   }
 }

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<RestView<BranchResource>> BRANCH_KIND =
       new TypeLiteral<RestView<BranchResource>>() {};
 
-  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;
+    this.refName = ref.getName();
-  }
+    this.revision = ref.getObjectId() != null ? ref.getObjectId().name() : null;
-
-  public BranchInfo getBranchInfo() {
-    return branchInfo;
   }
 
   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;
   }
 }

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.errors.RepositoryNotFoundException;
-import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
 
 @Singleton
 public class BranchesCollection
     implements ChildCollection<ProjectResource, BranchResource>, AcceptsCreate<ProjectResource> {
   private final DynamicMap<RestView<BranchResource>> views;
   private final Provider<ListBranches> list;
+  private final PermissionBackend permissionBackend;
+  private final Provider<CurrentUser> user;
+  private final GitRepositoryManager repoManager;
   private final CreateBranch.Factory createBranchFactory;
 
   @Inject
   BranchesCollection(
       DynamicMap<RestView<BranchResource>> views,
       Provider<ListBranches> list,
+      PermissionBackend permissionBackend,
+      Provider<CurrentUser> 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,19 +69,29 @@ public class BranchesCollection
 
   @Override
   public BranchResource parse(ProjectResource parent, IdString id)
-      throws ResourceNotFoundException, IOException, BadRequestException {
+      throws ResourceNotFoundException, IOException, PermissionBackendException {
-    String branchName = id.get();
+    Project.NameKey project = parent.getNameKey();
-    if (!branchName.equals(Constants.HEAD)) {
+    try (Repository repo = repoManager.openRepository(project)) {
-      branchName = RefNames.fullName(branchName);
+      Ref ref = repo.exactRef(RefNames.fullName(id.get()));
-    }
+      if (ref == null) {
-    List<BranchInfo> branches = list.get().apply(parent);
+        throw new ResourceNotFoundException(id);
-    for (BranchInfo b : branches) {
-      if (branchName.equals(b.ref)) {
-        return new BranchResource(parent.getControl(), b);
-      }
       }
+
+      // 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();
     }
+  }
 
   @Override
   public DynamicMap<RestView<BranchResource>> views() {

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<BranchResource> {
+  private final Provider<ListBranches> list;
+
+  @Inject
+  GetBranch(Provider<ListBranches> list) {
+    this.list = list;
+  }
 
   @Override
-  public BranchInfo apply(BranchResource rsrc) {
+  public BranchInfo apply(BranchResource rsrc) throws ResourceNotFoundException, IOException {
-    return rsrc.getBranchInfo();
+    return list.get().toBranchInfo(rsrc);
   }
 }

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<ProjectResource> {
         .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<BranchInfo> allBranches(ProjectResource rsrc)
       throws IOException, ResourceNotFoundException {
     List<Ref> refs;
@@ -142,7 +155,10 @@ public class ListBranches implements RestReadView<ProjectResource> {
     } catch (RepositoryNotFoundException noGitRepository) {
       throw new ResourceNotFoundException();
     }
+    return toBranchInfo(rsrc, refs);
+  }
 
+  private List<BranchInfo> toBranchInfo(ProjectResource rsrc, List<Ref> refs) {
     Set<String> targets = Sets.newHashSetWithExpectedSize(1);
     for (Ref ref : refs) {
       if (ref.isSymbolic()) {
@@ -213,7 +229,7 @@ public class ListBranches implements RestReadView<ProjectResource> {
     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<>();

gerrit-server/src/main/java/com/google/gerrit/server/project/PutBranch.java

@@ -25,7 +25,6 @@ public class PutBranch implements RestModifyView<BranchResource, BranchInput> {
 
   @Override
   public BranchInfo apply(BranchResource rsrc, BranchInput input) throws ResourceConflictException {
-    throw new ResourceConflictException(
+    throw new ResourceConflictException("Branch \"" + rsrc.getRef() + "\" already exists");
-        "Branch \"" + rsrc.getBranchInfo().ref + "\" already exists");
   }
 }