Use Guice to feed dependencies to ApprovalsUtil
Classes shouldn't use static methods with long dependency lists. They should use Guice injection to get their dependencies, especially if they are very common ones like ReviewDb or ApprovalTypes. ChangeUtil is a disaster so just paper over the one call site that is harder to clean up. Change-Id: Ia32fbf32175dcc84f8ae78361b128ce527ea99fa
This commit is contained in:
@@ -15,12 +15,12 @@
|
|||||||
package com.google.gerrit.httpd.rpc.changedetail;
|
package com.google.gerrit.httpd.rpc.changedetail;
|
||||||
|
|
||||||
import com.google.gerrit.common.ChangeHookRunner;
|
import com.google.gerrit.common.ChangeHookRunner;
|
||||||
import com.google.gerrit.common.data.ApprovalTypes;
|
|
||||||
import com.google.gerrit.common.data.ChangeDetail;
|
import com.google.gerrit.common.data.ChangeDetail;
|
||||||
import com.google.gerrit.common.errors.NoSuchEntityException;
|
import com.google.gerrit.common.errors.NoSuchEntityException;
|
||||||
import com.google.gerrit.httpd.rpc.Handler;
|
import com.google.gerrit.httpd.rpc.Handler;
|
||||||
import com.google.gerrit.reviewdb.client.PatchSet;
|
import com.google.gerrit.reviewdb.client.PatchSet;
|
||||||
import com.google.gerrit.reviewdb.server.ReviewDb;
|
import com.google.gerrit.reviewdb.server.ReviewDb;
|
||||||
|
import com.google.gerrit.server.ApprovalsUtil;
|
||||||
import com.google.gerrit.server.ChangeUtil;
|
import com.google.gerrit.server.ChangeUtil;
|
||||||
import com.google.gerrit.server.GerritPersonIdent;
|
import com.google.gerrit.server.GerritPersonIdent;
|
||||||
import com.google.gerrit.server.IdentifiedUser;
|
import com.google.gerrit.server.IdentifiedUser;
|
||||||
@@ -65,7 +65,7 @@ class RebaseChange extends Handler<ChangeDetail> {
|
|||||||
|
|
||||||
private final PersonIdent myIdent;
|
private final PersonIdent myIdent;
|
||||||
|
|
||||||
private final ApprovalTypes approvalTypes;
|
private final ApprovalsUtil approvalsUtil;
|
||||||
|
|
||||||
@Inject
|
@Inject
|
||||||
RebaseChange(final ChangeControl.Factory changeControlFactory,
|
RebaseChange(final ChangeControl.Factory changeControlFactory,
|
||||||
@@ -77,7 +77,7 @@ class RebaseChange extends Handler<ChangeDetail> {
|
|||||||
final PatchSetInfoFactory patchSetInfoFactory,
|
final PatchSetInfoFactory patchSetInfoFactory,
|
||||||
final ReplicationQueue replication,
|
final ReplicationQueue replication,
|
||||||
@GerritPersonIdent final PersonIdent myIdent,
|
@GerritPersonIdent final PersonIdent myIdent,
|
||||||
final ApprovalTypes approvalTypes) {
|
final ApprovalsUtil approvalsUtil) {
|
||||||
this.changeControlFactory = changeControlFactory;
|
this.changeControlFactory = changeControlFactory;
|
||||||
this.db = db;
|
this.db = db;
|
||||||
this.currentUser = currentUser;
|
this.currentUser = currentUser;
|
||||||
@@ -92,7 +92,7 @@ class RebaseChange extends Handler<ChangeDetail> {
|
|||||||
this.replication = replication;
|
this.replication = replication;
|
||||||
this.myIdent = myIdent;
|
this.myIdent = myIdent;
|
||||||
|
|
||||||
this.approvalTypes = approvalTypes;
|
this.approvalsUtil = approvalsUtil;
|
||||||
}
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
@@ -103,7 +103,7 @@ class RebaseChange extends Handler<ChangeDetail> {
|
|||||||
|
|
||||||
ChangeUtil.rebaseChange(patchSetId, currentUser, db,
|
ChangeUtil.rebaseChange(patchSetId, currentUser, db,
|
||||||
rebasedPatchSetSenderFactory, hooks, gitManager, patchSetInfoFactory,
|
rebasedPatchSetSenderFactory, hooks, gitManager, patchSetInfoFactory,
|
||||||
replication, myIdent, changeControlFactory, approvalTypes);
|
replication, myIdent, changeControlFactory, approvalsUtil);
|
||||||
|
|
||||||
return changeDetailFactory.create(patchSetId.getParentKey()).call();
|
return changeDetailFactory.create(patchSetId.getParentKey()).call();
|
||||||
}
|
}
|
||||||
|
@@ -22,15 +22,27 @@ import com.google.gerrit.reviewdb.client.PatchSet;
|
|||||||
import com.google.gerrit.reviewdb.client.PatchSetApproval;
|
import com.google.gerrit.reviewdb.client.PatchSetApproval;
|
||||||
import com.google.gerrit.reviewdb.server.ReviewDb;
|
import com.google.gerrit.reviewdb.server.ReviewDb;
|
||||||
import com.google.gwtorm.server.OrmException;
|
import com.google.gwtorm.server.OrmException;
|
||||||
|
import com.google.inject.Inject;
|
||||||
|
|
||||||
import java.io.IOException;
|
import java.io.IOException;
|
||||||
import java.util.Collections;
|
import java.util.Collections;
|
||||||
import java.util.List;
|
import java.util.List;
|
||||||
|
|
||||||
public class ApprovalsUtil {
|
public class ApprovalsUtil {
|
||||||
/* Resync the changeOpen status which is cached in the approvals table for
|
private final ReviewDb db;
|
||||||
performance reasons*/
|
private final ApprovalTypes approvalTypes;
|
||||||
public static void syncChangeStatus(final ReviewDb db, final Change change)
|
|
||||||
|
@Inject
|
||||||
|
ApprovalsUtil(ReviewDb db, ApprovalTypes approvalTypes) {
|
||||||
|
this.db = db;
|
||||||
|
this.approvalTypes = approvalTypes;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Resync the changeOpen status which is cached in the approvals table for
|
||||||
|
* performance reasons
|
||||||
|
*/
|
||||||
|
public void syncChangeStatus(final Change change)
|
||||||
throws OrmException {
|
throws OrmException {
|
||||||
final List<PatchSetApproval> approvals =
|
final List<PatchSetApproval> approvals =
|
||||||
db.patchSetApprovals().byChange(change.getId()).toList();
|
db.patchSetApprovals().byChange(change.getId()).toList();
|
||||||
@@ -44,14 +56,12 @@ public class ApprovalsUtil {
|
|||||||
* Moves the PatchSetApprovals to the last PatchSet on the change while
|
* Moves the PatchSetApprovals to the last PatchSet on the change while
|
||||||
* keeping the vetos.
|
* keeping the vetos.
|
||||||
*
|
*
|
||||||
* @param db The review database
|
|
||||||
* @param change Change to update
|
* @param change Change to update
|
||||||
* @param approvalTypes The approval types
|
|
||||||
* @throws OrmException
|
* @throws OrmException
|
||||||
* @throws IOException
|
* @throws IOException
|
||||||
*/
|
*/
|
||||||
public static void copyVetosToLatestPatchSet(final ReviewDb db, Change change,
|
public void copyVetosToLatestPatchSet(Change change)
|
||||||
ApprovalTypes approvalTypes) throws OrmException, IOException {
|
throws OrmException, IOException {
|
||||||
PatchSet.Id source;
|
PatchSet.Id source;
|
||||||
if (change.getNumberOfPatchSets() > 1) {
|
if (change.getNumberOfPatchSets() > 1) {
|
||||||
source = new PatchSet.Id(change.getId(), change.getNumberOfPatchSets() - 1);
|
source = new PatchSet.Id(change.getId(), change.getNumberOfPatchSets() - 1);
|
||||||
|
@@ -16,7 +16,6 @@ package com.google.gerrit.server;
|
|||||||
|
|
||||||
import com.google.gerrit.common.ChangeHookRunner;
|
import com.google.gerrit.common.ChangeHookRunner;
|
||||||
import com.google.gerrit.common.ChangeHooks;
|
import com.google.gerrit.common.ChangeHooks;
|
||||||
import com.google.gerrit.common.data.ApprovalTypes;
|
|
||||||
import com.google.gerrit.reviewdb.client.Account;
|
import com.google.gerrit.reviewdb.client.Account;
|
||||||
import com.google.gerrit.reviewdb.client.Change;
|
import com.google.gerrit.reviewdb.client.Change;
|
||||||
import com.google.gerrit.reviewdb.client.Change.Status;
|
import com.google.gerrit.reviewdb.client.Change.Status;
|
||||||
@@ -243,7 +242,7 @@ public class ChangeUtil {
|
|||||||
final PatchSetInfoFactory patchSetInfoFactory,
|
final PatchSetInfoFactory patchSetInfoFactory,
|
||||||
final ReplicationQueue replication, PersonIdent myIdent,
|
final ReplicationQueue replication, PersonIdent myIdent,
|
||||||
final ChangeControl.Factory changeControlFactory,
|
final ChangeControl.Factory changeControlFactory,
|
||||||
final ApprovalTypes approvalTypes) throws NoSuchChangeException,
|
final ApprovalsUtil approvalsUtil) throws NoSuchChangeException,
|
||||||
EmailException, OrmException, MissingObjectException,
|
EmailException, OrmException, MissingObjectException,
|
||||||
IncorrectObjectTypeException, IOException,
|
IncorrectObjectTypeException, IOException,
|
||||||
PatchSetInfoNotAvailableException, InvalidChangeOperationException {
|
PatchSetInfoNotAvailableException, InvalidChangeOperationException {
|
||||||
@@ -384,7 +383,7 @@ public class ChangeUtil {
|
|||||||
|
|
||||||
replication.scheduleUpdate(change.getProject(), ru.getName());
|
replication.scheduleUpdate(change.getProject(), ru.getName());
|
||||||
|
|
||||||
ApprovalsUtil.copyVetosToLatestPatchSet(db, change, approvalTypes);
|
approvalsUtil.copyVetosToLatestPatchSet(change);
|
||||||
|
|
||||||
final ChangeMessage cmsg =
|
final ChangeMessage cmsg =
|
||||||
new ChangeMessage(new ChangeMessage.Key(changeId,
|
new ChangeMessage(new ChangeMessage.Key(changeId,
|
||||||
@@ -594,7 +593,7 @@ public class ChangeUtil {
|
|||||||
}
|
}
|
||||||
db.changeMessages().insert(Collections.singleton(cmsg));
|
db.changeMessages().insert(Collections.singleton(cmsg));
|
||||||
|
|
||||||
ApprovalsUtil.syncChangeStatus(db, change);
|
new ApprovalsUtil(db, null).syncChangeStatus(change);
|
||||||
|
|
||||||
// Email the reviewers
|
// Email the reviewers
|
||||||
final ReplyToChangeSender cm = senderFactory.create(change);
|
final ReplyToChangeSender cm = senderFactory.create(change);
|
||||||
|
@@ -18,6 +18,7 @@ import static com.google.inject.Scopes.SINGLETON;
|
|||||||
|
|
||||||
import com.google.gerrit.reviewdb.server.ReviewDb;
|
import com.google.gerrit.reviewdb.server.ReviewDb;
|
||||||
import com.google.gerrit.server.AnonymousUser;
|
import com.google.gerrit.server.AnonymousUser;
|
||||||
|
import com.google.gerrit.server.ApprovalsUtil;
|
||||||
import com.google.gerrit.server.IdentifiedUser;
|
import com.google.gerrit.server.IdentifiedUser;
|
||||||
import com.google.gerrit.server.RequestCleanup;
|
import com.google.gerrit.server.RequestCleanup;
|
||||||
import com.google.gerrit.server.account.AccountControl;
|
import com.google.gerrit.server.account.AccountControl;
|
||||||
@@ -73,6 +74,7 @@ public class GerritRequestModule extends FactoryModule {
|
|||||||
bind(AccountResolver.class);
|
bind(AccountResolver.class);
|
||||||
bind(ChangeQueryRewriter.class);
|
bind(ChangeQueryRewriter.class);
|
||||||
bind(ListProjects.class);
|
bind(ListProjects.class);
|
||||||
|
bind(ApprovalsUtil.class);
|
||||||
|
|
||||||
bind(AnonymousUser.class).in(RequestScoped.class);
|
bind(AnonymousUser.class).in(RequestScoped.class);
|
||||||
bind(PerRequestProjectControlCache.class).in(RequestScoped.class);
|
bind(PerRequestProjectControlCache.class).in(RequestScoped.class);
|
||||||
|
@@ -183,6 +183,7 @@ public class ReceiveCommits {
|
|||||||
private final ReplicationQueue replication;
|
private final ReplicationQueue replication;
|
||||||
private final PatchSetInfoFactory patchSetInfoFactory;
|
private final PatchSetInfoFactory patchSetInfoFactory;
|
||||||
private final ChangeHooks hooks;
|
private final ChangeHooks hooks;
|
||||||
|
private final ApprovalsUtil approvalsUtil;
|
||||||
private final GitRepositoryManager repoManager;
|
private final GitRepositoryManager repoManager;
|
||||||
private final ProjectCache projectCache;
|
private final ProjectCache projectCache;
|
||||||
private final String canonicalWebUrl;
|
private final String canonicalWebUrl;
|
||||||
@@ -230,6 +231,7 @@ public class ReceiveCommits {
|
|||||||
final ReplicationQueue replication,
|
final ReplicationQueue replication,
|
||||||
final PatchSetInfoFactory patchSetInfoFactory,
|
final PatchSetInfoFactory patchSetInfoFactory,
|
||||||
final ChangeHooks hooks,
|
final ChangeHooks hooks,
|
||||||
|
final ApprovalsUtil approvalsUtil,
|
||||||
final ProjectCache projectCache,
|
final ProjectCache projectCache,
|
||||||
final GitRepositoryManager repoManager,
|
final GitRepositoryManager repoManager,
|
||||||
final TagCache tagCache,
|
final TagCache tagCache,
|
||||||
@@ -252,6 +254,7 @@ public class ReceiveCommits {
|
|||||||
this.replication = replication;
|
this.replication = replication;
|
||||||
this.patchSetInfoFactory = patchSetInfoFactory;
|
this.patchSetInfoFactory = patchSetInfoFactory;
|
||||||
this.hooks = hooks;
|
this.hooks = hooks;
|
||||||
|
this.approvalsUtil = approvalsUtil;
|
||||||
this.projectCache = projectCache;
|
this.projectCache = projectCache;
|
||||||
this.repoManager = repoManager;
|
this.repoManager = repoManager;
|
||||||
this.canonicalWebUrl = canonicalWebUrl;
|
this.canonicalWebUrl = canonicalWebUrl;
|
||||||
@@ -1967,7 +1970,7 @@ public class ReceiveCommits {
|
|||||||
change.setStatus(Change.Status.MERGED);
|
change.setStatus(Change.Status.MERGED);
|
||||||
ChangeUtil.updated(change);
|
ChangeUtil.updated(change);
|
||||||
|
|
||||||
ApprovalsUtil.syncChangeStatus(db, change);
|
approvalsUtil.syncChangeStatus(change);
|
||||||
|
|
||||||
final StringBuilder msgBuf = new StringBuilder();
|
final StringBuilder msgBuf = new StringBuilder();
|
||||||
msgBuf.append("Change has been successfully pushed");
|
msgBuf.append("Change has been successfully pushed");
|
||||||
|
Reference in New Issue
Block a user