opendev/gerrit
Code Issues Proposed changes

Move more functionality from elsewhere into ApprovalsUtil

Browse Source 
The commonality of these methods is they will have a different
implementation when approvals are read from an eventual notedb, so
it makes sense to keep them located together.

Change-Id: I5de63a4210f29863274111964480fb1f5720ca60
This commit is contained in:
Dave Borowitz
2013-12-18 13:12:44 -08:00
parent ab33191c2e
commit b5b6f615ec
12 changed files with 91 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files

40
gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
View File

@@ -17,11 +17,13 @@ package com.google.gerrit.server;
import static com.google.common.base.Preconditions.checkArgument;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.LinkedHashMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Multimaps;
import com.google.common.collect.Ordering;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
import com.google.gerrit.common.data.LabelType;
@@ -38,6 +40,7 @@ import com.google.gerrit.server.util.TimeUtil;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.sql.Timestamp;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
@@ -56,6 +59,19 @@ import java.util.Set;
* The methods in this class do not begin/commit transactions.
*/
public class ApprovalsUtil {
private static Ordering<PatchSetApproval> SORT_APPROVALS = Ordering.natural()
.onResultOf(new Function<PatchSetApproval, Timestamp>() {
@Override
public Timestamp apply(PatchSetApproval a) {
return a.getGranted();
}
});
public static List<PatchSetApproval> sortApprovals(
Iterable<PatchSetApproval> approvals) {
return SORT_APPROVALS.sortedCopy(approvals);
}
@VisibleForTesting
@Inject
public ApprovalsUtil() {
@@ -209,4 +225,28 @@ public class ApprovalsUtil {
db.patchSetApprovals().insert(cells);
return Collections.unmodifiableList(cells);
}
public List<PatchSetApproval> byPatchSet(ReviewDb db, PatchSet.Id psId)
throws OrmException {
return sortApprovals(db.patchSetApprovals().byPatchSet(psId));
}
public PatchSetApproval getSubmitter(ReviewDb db, PatchSet.Id c) {
if (c == null) {
return null;
}
PatchSetApproval submitter = null;
try {
for (PatchSetApproval a : db.patchSetApprovals().byPatchSet(c)) {
if (a.getValue() > 0 && a.isSubmit()) {
if (submitter == null
|| a.getGranted().compareTo(submitter.getGranted()) > 0) {
submitter = a;
}
}
}
} catch (OrmException e) {
}
return submitter;
}
}

3
gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java
View File

@@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.account.AccountInfo;
import com.google.gerrit.server.git.LabelNormalizer;
import com.google.gerrit.server.project.ChangeControl;
@@ -76,7 +77,7 @@ public class ReviewerJson {
PatchSet.Id psId = ctl.getChange().currentPatchSetId();
if (approvals == null) {
approvals = ChangeData.sortApprovals(db.get().patchSetApprovals()
approvals = ApprovalsUtil.sortApprovals(db.get().patchSetApprovals()
.byPatchSetUser(psId, out._id));
}
approvals = labelNormalizer.normalize(ctl, approvals);

3
gerrit-server/src/main/java/com/google/gerrit/server/git/CherryPick.java
View File

@@ -174,7 +174,8 @@ public class CherryPick extends SubmitStrategy {
args.db.changes().update(Collections.singletonList(n.change));
final List<PatchSetApproval> approvals = Lists.newArrayList();
for (PatchSetApproval a : args.mergeUtil.getApprovalsForCommit(n)) {
for (PatchSetApproval a
: args.approvalsUtil.byPatchSet(args.db, n.patchsetId)) {
approvals.add(new PatchSetApproval(ps.getId(), a));
}
args.db.patchSetApprovals().insert(approvals);

12
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@@ -14,7 +14,6 @@
package com.google.gerrit.server.git;
import static com.google.gerrit.server.git.MergeUtil.getSubmitter;
import static java.util.concurrent.TimeUnit.HOURS;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.MINUTES;
@@ -43,6 +42,7 @@ import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.Project.SubmitType;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
@@ -137,6 +137,7 @@ public class MergeOp {
private final ChangeControl.GenericFactory changeControlFactory;
private final MergeQueue mergeQueue;
private final MergeValidators.Factory mergeValidatorsFactory;
private final ApprovalsUtil approvalsUtil;
private final Branch.NameKey destBranch;
private ProjectState destProject;
@@ -177,7 +178,8 @@ public class MergeOp {
final WorkQueue workQueue,
final RequestScopePropagator requestScopePropagator,
final ChangeIndexer indexer,
final MergeValidators.Factory mergeValidatorsFactory) {
final MergeValidators.Factory mergeValidatorsFactory,
final ApprovalsUtil approvalsUtil) {
repoManager = grm;
schemaFactory = sf;
labelNormalizer = fs;
@@ -198,6 +200,7 @@ public class MergeOp {
this.requestScopePropagator = requestScopePropagator;
this.indexer = indexer;
this.mergeValidatorsFactory = mergeValidatorsFactory;
this.approvalsUtil = approvalsUtil;
destBranch = branch;
toMerge = ArrayListMultimap.create();
potentiallyStillSubmittable = new ArrayList<CodeReviewCommit>();
@@ -618,7 +621,8 @@ public class MergeOp {
gitRefUpdated.fire(destBranch.getParentKey(), branchUpdate);
Account account = null;
final PatchSetApproval submitter = getSubmitter(db, mergeTip.patchsetId);
PatchSetApproval submitter =
approvalsUtil.getSubmitter(db, mergeTip.patchsetId);
if (submitter != null) {
account = accountCache.get(submitter.getAccountId()).getAccount();
}
@@ -995,7 +999,7 @@ public class MergeOp {
boolean makeNew) {
PatchSetApproval submitter = null;
try {
submitter = getSubmitter(db, c.currentPatchSetId());
submitter = approvalsUtil.getSubmitter(db, c.currentPatchSetId());
} catch (Exception e) {
log.error("Cannot get submitter", e);
}

47
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
View File

@@ -24,6 +24,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetApproval.LabelId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.GerritServerConfig;
@@ -93,6 +94,7 @@ public class MergeUtil {
private final Provider<ReviewDb> db;
private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final Provider<String> urlProvider;
private final ApprovalsUtil approvalsUtil;
private final ProjectState project;
private final boolean useContentMerge;
private final boolean useRecursiveMerge;
@@ -102,9 +104,10 @@ public class MergeUtil {
final Provider<ReviewDb> db,
final IdentifiedUser.GenericFactory identifiedUserFactory,
@CanonicalWebUrl @Nullable final Provider<String> urlProvider,
final ApprovalsUtil approvalsUtil,
@Assisted final ProjectState project) {
this(serverConfig, db, identifiedUserFactory, urlProvider, project,
project.isUseContentMerge());
this(serverConfig, db, identifiedUserFactory, urlProvider, approvalsUtil,
project, project.isUseContentMerge());
}
@AssistedInject
@@ -112,11 +115,13 @@ public class MergeUtil {
final Provider<ReviewDb> db,
final IdentifiedUser.GenericFactory identifiedUserFactory,
@CanonicalWebUrl @Nullable final Provider<String> urlProvider,
final ApprovalsUtil approvalsUtil,
@Assisted final ProjectState project,
@Assisted boolean useContentMerge) {
this.db = db;
this.identifiedUserFactory = identifiedUserFactory;
this.urlProvider = urlProvider;
this.approvalsUtil = approvalsUtil;
this.project = project;
this.useContentMerge = useContentMerge;
this.useRecursiveMerge = useRecursiveMerge(serverConfig);
@@ -159,29 +164,7 @@ public class MergeUtil {
}
public PatchSetApproval getSubmitter(final PatchSet.Id c) {
return getSubmitter(db.get(), c);
}
public static PatchSetApproval getSubmitter(final ReviewDb reviewDb,
final PatchSet.Id c) {
if (c == null) {
return null;
}
PatchSetApproval submitter = null;
try {
final List<PatchSetApproval> approvals =
reviewDb.patchSetApprovals().byPatchSet(c).toList();
for (PatchSetApproval a : approvals) {
if (a.getValue() > 0 && a.isSubmit()) {
if (submitter == null
|| a.getGranted().compareTo(submitter.getGranted()) > 0) {
submitter = a;
}
}
}
} catch (OrmException e) {
}
return submitter;
return approvalsUtil.getSubmitter(db.get(), c);
}
public RevCommit createCherryPickFromCommit(Repository repo,
@@ -250,7 +233,7 @@ public class MergeUtil {
PatchSetApproval submitAudit = null;
for (final PatchSetApproval a : getApprovalsForCommit(n)) {
for (final PatchSetApproval a : safeGetApprovals(n)) {
if (a.getValue() <= 0) {
// Negative votes aren't counted.
continue;
@@ -324,17 +307,9 @@ public class MergeUtil {
return "Verified".equalsIgnoreCase(id.get());
}
public List<PatchSetApproval> getApprovalsForCommit(final CodeReviewCommit n) {
private List<PatchSetApproval> safeGetApprovals(CodeReviewCommit n) {
try {
List<PatchSetApproval> approvalList =
db.get().patchSetApprovals().byPatchSet(n.patchsetId).toList();
Collections.sort(approvalList, new Comparator<PatchSetApproval>() {
@Override
public int compare(final PatchSetApproval a, final PatchSetApproval b) {
return a.getGranted().compareTo(b.getGranted());
}
});
return approvalList;
return approvalsUtil.byPatchSet(db.get(), n.patchsetId);
} catch (OrmException e) {
log.error("Can't read approval records for " + n.patchsetId, e);
return Collections.emptyList();

3
gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseIfNecessary.java
View File

@@ -88,7 +88,8 @@ public class RebaseIfNecessary extends SubmitStrategy {
newMergeTip, args.mergeUtil, committerIdent,
false, false, ValidatePolicy.NONE);
List<PatchSetApproval> approvals = Lists.newArrayList();
for (PatchSetApproval a : args.mergeUtil.getApprovalsForCommit(n)) {
for (PatchSetApproval a
: args.approvalsUtil.byPatchSet(args.db, n.patchsetId)) {
approvals.add(new PatchSetApproval(newPatchSet.getId(), a));
}
args.db.patchSetApprovals().insert(approvals);

7
gerrit-server/src/main/java/com/google/gerrit/server/git/SubmitStrategy.java
View File

@@ -19,6 +19,7 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project.SubmitType;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.index.ChangeIndexer;
@@ -55,6 +56,7 @@ public abstract class SubmitStrategy {
protected final RevFlag canMergeFlag;
protected final Set<RevCommit> alreadyAccepted;
protected final Branch.NameKey destBranch;
protected final ApprovalsUtil approvalsUtil;
protected final MergeUtil mergeUtil;
protected final ChangeIndexer indexer;
protected final MergeSorter mergeSorter;
@@ -63,8 +65,8 @@ public abstract class SubmitStrategy {
final PersonIdent myIdent, final ReviewDb db, final Repository repo,
final RevWalk rw, final ObjectInserter inserter,
final RevFlag canMergeFlag, final Set<RevCommit> alreadyAccepted,
final Branch.NameKey destBranch, final MergeUtil mergeUtil,
final ChangeIndexer indexer) {
final Branch.NameKey destBranch, final ApprovalsUtil approvalsUtil,
final MergeUtil mergeUtil, final ChangeIndexer indexer) {
this.identifiedUserFactory = identifiedUserFactory;
this.myIdent = myIdent;
this.db = db;
@@ -75,6 +77,7 @@ public abstract class SubmitStrategy {
this.canMergeFlag = canMergeFlag;
this.alreadyAccepted = alreadyAccepted;
this.destBranch = destBranch;
this.approvalsUtil = approvalsUtil;
this.mergeUtil = mergeUtil;
this.indexer = indexer;
this.mergeSorter = new MergeSorter(rw, alreadyAccepted, canMergeFlag);

6
gerrit-server/src/main/java/com/google/gerrit/server/git/SubmitStrategyFactory.java
View File

@@ -18,6 +18,7 @@ import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Project.SubmitType;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.changedetail.RebaseChange;
@@ -53,6 +54,7 @@ public class SubmitStrategyFactory {
private final GitReferenceUpdated gitRefUpdated;
private final RebaseChange rebaseChange;
private final ProjectCache projectCache;
private final ApprovalsUtil approvalsUtil;
private final MergeUtil.Factory mergeUtilFactory;
private final ChangeIndexer indexer;
@@ -64,6 +66,7 @@ public class SubmitStrategyFactory {
@CanonicalWebUrl @Nullable final Provider<String> urlProvider,
final GitReferenceUpdated gitRefUpdated, final RebaseChange rebaseChange,
final ProjectCache projectCache,
final ApprovalsUtil approvalsUtil,
final MergeUtil.Factory mergeUtilFactory,
final ChangeIndexer indexer) {
this.identifiedUserFactory = identifiedUserFactory;
@@ -72,6 +75,7 @@ public class SubmitStrategyFactory {
this.gitRefUpdated = gitRefUpdated;
this.rebaseChange = rebaseChange;
this.projectCache = projectCache;
this.approvalsUtil = approvalsUtil;
this.mergeUtilFactory = mergeUtilFactory;
this.indexer = indexer;
}
@@ -85,7 +89,7 @@ public class SubmitStrategyFactory {
final SubmitStrategy.Arguments args =
new SubmitStrategy.Arguments(identifiedUserFactory, myIdent, db, repo,
rw, inserter, canMergeFlag, alreadyAccepted, destBranch,
mergeUtilFactory.create(project), indexer);
approvalsUtil, mergeUtilFactory.create(project), indexer);
switch (submitType) {
case CHERRY_PICK:
return new CherryPick(args, patchSetInfoFactory, gitRefUpdated);

10
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java
View File

@@ -14,8 +14,6 @@
package com.google.gerrit.server.git.validators;
import static com.google.gerrit.server.git.MergeUtil.getSubmitter;
import com.google.common.collect.Lists;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.reviewdb.client.Branch;
@@ -24,6 +22,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.git.CodeReviewCommit;
@@ -74,6 +73,7 @@ public class MergeValidators {
private final ReviewDb db;
private final ProjectCache projectCache;
private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final ApprovalsUtil approvalsUtil;
public interface Factory {
ProjectConfigValidator create();
@@ -82,11 +82,13 @@ public class MergeValidators {
@Inject
public ProjectConfigValidator(AllProjectsName allProjectsName,
ReviewDb db, ProjectCache projectCache,
IdentifiedUser.GenericFactory iuf) {
IdentifiedUser.GenericFactory iuf,
ApprovalsUtil approvalsUtil) {
this.allProjectsName = allProjectsName;
this.db = db;
this.projectCache = projectCache;
this.identifiedUserFactory = iuf;
this.approvalsUtil = approvalsUtil;
}
@Override
@@ -117,7 +119,7 @@ public class MergeValidators {
}
} else {
if (!oldParent.equals(newParent)) {
final PatchSetApproval psa = getSubmitter(db, patchSetId);
PatchSetApproval psa = approvalsUtil.getSubmitter(db, patchSetId);
if (psa == null) {
throw new MergeValidationException(CommitMergeStatus.
SETTING_PARENT_PROJECT_ONLY_ALLOWED_BY_ADMIN);

18
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@@ -14,14 +14,14 @@
package com.google.gerrit.server.query.change;
import com.google.common.base.Function;
import static com.google.gerrit.server.ApprovalsUtil.sortApprovals;
import com.google.common.base.Objects;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Ordering;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
import com.google.gerrit.common.data.SubmitRecord;
@@ -56,7 +56,6 @@ import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -65,19 +64,6 @@ import java.util.Map;
import java.util.Set;
public class ChangeData {
private static Ordering<PatchSetApproval> SORT_APPROVALS = Ordering.natural()
.onResultOf(new Function<PatchSetApproval, Timestamp>() {
@Override
public Timestamp apply(PatchSetApproval a) {
return a.getGranted();
}
});
public static List<PatchSetApproval> sortApprovals(
Iterable<PatchSetApproval> approvals) {
return SORT_APPROVALS.sortedCopy(approvals);
}
public static void ensureChangeLoaded(Provider<ReviewDb> db,
Iterable<ChangeData> changes) throws OrmException {
Map<Change.Id, ChangeData> missing = Maps.newHashMap();