opendev/gerrit
Code Issues Proposed changes

PatchSetInfoFactory: Use PatchSetUtil

Browse Source 
Some public methods now need to take ChangeNotes. A significant
affected caller was ChangeHookRunner, which now needs to construct
notes on many event types. This might be able to be avoided if we
changed the ChangeHooks interface, but that is saved for later
cleanup.

Change-Id: Ia2f0b9b07bc9e32b8c010191d3a6c3bbf848a5d1
This commit is contained in:
Dave Borowitz
2016-01-19 12:55:50 -05:00
parent 0dfabcc1ac
commit 3a6dea327c
9 changed files with 87 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java
View File

@@ -185,7 +185,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
detail.setPatchSet(patchSet);
detail.setProject(project);
detail.setInfo(infoFactory.get(db, patchSet.getId()));
detail.setInfo(infoFactory.get(db, notes, patchSet.getId()));
detail.setPatches(patches);
final CurrentUser user = control.getUser();

42
gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java
View File

@@ -58,6 +58,7 @@ import com.google.gerrit.server.events.ReviewerAddedEvent;
import com.google.gerrit.server.events.TopicChangedEvent;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.WorkQueue;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.ProjectState;
@@ -227,6 +228,8 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
/** Repository Manager. */
private final GitRepositoryManager repoManager;
private final ChangeNotes.Factory notesFactory;
/** Queue of hooks that need to run. */
private final WorkQueue.Executor hookQueue;
@@ -256,6 +259,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
@Inject
public ChangeHookRunner(WorkQueue queue,
GitRepositoryManager repoManager,
ChangeNotes.Factory notesFactory,
@GerritServerConfig Config config,
@AnonymousCowardName String anonymousCowardName,
SitePaths sitePath,
@@ -265,6 +269,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
DynamicSet<EventListener> unrestrictedListeners) {
this.anonymousCowardName = anonymousCowardName;
this.repoManager = repoManager;
this.notesFactory = notesFactory;
this.hookQueue = queue.createQueue(1, "hook");
this.projectCache = projectCache;
this.accountCache = accountCache;
@@ -304,6 +309,10 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
.build());
}
private ChangeNotes newNotes(Change change) {
return notesFactory.create(change);
}
private static Optional<Path> hook(Config config, Path path, String name) {
String setting = name.replace("-", "") + "hook";
String value = config.getString("hooks", null, setting);
@@ -394,13 +403,14 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
@Override
public void doPatchsetCreatedHook(Change change,
PatchSet patchSet, ReviewDb db) throws OrmException {
ChangeNotes notes = newNotes(change);
PatchSetCreatedEvent event = new PatchSetCreatedEvent(change);
Supplier<AccountState> uploader =
getAccountSupplier(patchSet.getUploader());
Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
event.change = changeAttributeSupplier(change);
event.patchSet = patchSetAttributeSupplier(change, patchSet);
event.patchSet = patchSetAttributeSupplier(notes, patchSet);
event.uploader = accountAttributeSupplier(uploader);
fireEvent(change, event, db);
@@ -431,13 +441,14 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
@Override
public void doDraftPublishedHook(Change change, PatchSet patchSet,
ReviewDb db) throws OrmException {
ChangeNotes notes = newNotes(change);
DraftPublishedEvent event = new DraftPublishedEvent(change);
Supplier<AccountState> uploader =
getAccountSupplier(patchSet.getUploader());
Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
event.change = changeAttributeSupplier(change);
event.patchSet = patchSetAttributeSupplier(change, patchSet);
event.patchSet = patchSetAttributeSupplier(notes, patchSet);
event.uploader = accountAttributeSupplier(uploader);
fireEvent(change, event, db);
@@ -467,12 +478,13 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
public void doCommentAddedHook(final Change change, Account account,
PatchSet patchSet, String comment, final Map<String, Short> approvals,
ReviewDb db) throws OrmException {
ChangeNotes notes = newNotes(change);
CommentAddedEvent event = new CommentAddedEvent(change);
Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
event.change = changeAttributeSupplier(change);
event.author = accountAttributeSupplier(account);
event.patchSet = patchSetAttributeSupplier(change, patchSet);
event.patchSet = patchSetAttributeSupplier(notes, patchSet);
event.comment = comment;
event.approvals = Suppliers.memoize(
new Supplier<ApprovalAttribute[]>() {
@@ -528,12 +540,13 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
public void doChangeMergedHook(Change change, Account account,
PatchSet patchSet, ReviewDb db, String mergeResultRev)
throws OrmException {
ChangeNotes notes = newNotes(change);
ChangeMergedEvent event = new ChangeMergedEvent(change);
Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
event.change = changeAttributeSupplier(change);
event.submitter = accountAttributeSupplier(account);
event.patchSet = patchSetAttributeSupplier(change, patchSet);
event.patchSet = patchSetAttributeSupplier(notes, patchSet);
event.newRev = mergeResultRev;
fireEvent(change, event, db);
@@ -563,12 +576,13 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
public void doMergeFailedHook(Change change, Account account,
PatchSet patchSet, String reason,
ReviewDb db) throws OrmException {
ChangeNotes notes = newNotes(change);
MergeFailedEvent event = new MergeFailedEvent(change);
Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
event.change = changeAttributeSupplier(change);
event.submitter = accountAttributeSupplier(account);
event.patchSet = patchSetAttributeSupplier(change, patchSet);
event.patchSet = patchSetAttributeSupplier(notes, patchSet);
event.reason = reason;
fireEvent(change, event, db);
@@ -598,12 +612,13 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
public void doChangeAbandonedHook(Change change, Account account,
PatchSet patchSet, String reason, ReviewDb db)
throws OrmException {
ChangeNotes notes = newNotes(change);
ChangeAbandonedEvent event = new ChangeAbandonedEvent(change);
AccountState owner = accountCache.get(change.getOwner());
event.change = changeAttributeSupplier(change);
event.abandoner = accountAttributeSupplier(account);
event.patchSet = patchSetAttributeSupplier(change, patchSet);
event.patchSet = patchSetAttributeSupplier(notes, patchSet);
event.reason = reason;
fireEvent(change, event, db);
@@ -633,12 +648,13 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
public void doChangeRestoredHook(Change change, Account account,
PatchSet patchSet, String reason, ReviewDb db)
throws OrmException {
ChangeNotes notes = newNotes(change);
ChangeRestoredEvent event = new ChangeRestoredEvent(change);
AccountState owner = accountCache.get(change.getOwner());
event.change = changeAttributeSupplier(change);
event.restorer = accountAttributeSupplier(account);
event.patchSet = patchSetAttributeSupplier(change, patchSet);
event.patchSet = patchSetAttributeSupplier(notes, patchSet);
event.reason = reason;
fireEvent(change, event, db);
@@ -709,11 +725,12 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
@Override
public void doReviewerAddedHook(Change change, Account account,
PatchSet patchSet, ReviewDb db) throws OrmException {
ChangeNotes notes = newNotes(change);
ReviewerAddedEvent event = new ReviewerAddedEvent(change);
Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
event.change = changeAttributeSupplier(change);
event.patchSet = patchSetAttributeSupplier(change, patchSet);
event.patchSet = patchSetAttributeSupplier(notes, patchSet);
event.reviewer = accountAttributeSupplier(account);
fireEvent(change, event, db);
@@ -880,15 +897,16 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
}
private Supplier<PatchSetAttribute> patchSetAttributeSupplier(
final Change change, final PatchSet patchSet) {
final ChangeNotes notes, final PatchSet patchSet) {
return Suppliers.memoize(
new Supplier<PatchSetAttribute>() {
@Override
public PatchSetAttribute get() {
try (Repository repo
= repoManager.openRepository(change.getProject());
try (Repository repo =
repoManager.openRepository(notes.getChange().getProject());
RevWalk revWalk = new RevWalk(repo)) {
return eventFactory.asPatchSetAttribute(revWalk, patchSet);
return eventFactory.asPatchSetAttribute(
revWalk, notes, patchSet);
} catch (IOException e) {
throw Throwables.propagate(e);
}

7
gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
View File

@@ -45,6 +45,7 @@ import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
@@ -119,6 +120,7 @@ public class ConsistencyChecker {
private final BatchUpdate.Factory updateFactory;
private final ChangeIndexer indexer;
private final ChangeControl.GenericFactory changeControlFactory;
private final ChangeNotes.Factory notesFactory;
private final ChangeUpdate.Factory changeUpdateFactory;
private FixInput fix;
@@ -144,6 +146,7 @@ public class ConsistencyChecker {
BatchUpdate.Factory updateFactory,
ChangeIndexer indexer,
ChangeControl.GenericFactory changeControlFactory,
ChangeNotes.Factory notesFactory,
ChangeUpdate.Factory changeUpdateFactory) {
this.db = db;
this.repoManager = repoManager;
@@ -155,6 +158,7 @@ public class ConsistencyChecker {
this.updateFactory = updateFactory;
this.indexer = indexer;
this.changeControlFactory = changeControlFactory;
this.notesFactory = notesFactory;
this.changeUpdateFactory = changeUpdateFactory;
reset();
}
@@ -578,6 +582,7 @@ public class ConsistencyChecker {
if (c == null) {
throw new OrmException("Change missing: " + cid);
}
ChangeNotes notes = notesFactory.create(c);
if (psId.equals(c.currentPatchSetId())) {
List<PatchSet> all = Lists.newArrayList(db.patchSets().byChange(cid));
@@ -597,7 +602,7 @@ public class ConsistencyChecker {
break;
}
}
c.setCurrentPatchSet(patchSetInfoFactory.get(db, latest));
c.setCurrentPatchSet(patchSetInfoFactory.get(db, notes, latest));
db.changes().update(Collections.singleton(c));
}

2
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java
View File

@@ -164,7 +164,7 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
try {
// TODO(dborowitz): Get this in a way that doesn't involve re-opening
// the repo after the updateRepo phase.
return patchSetInfoFactory.get(ctx.getDb(),
return patchSetInfoFactory.get(ctx.getDb(), ctx.getNotes(),
new PatchSet.Id(psId.getParentKey(), psId.get() - 1));
} catch (PatchSetInfoNotAvailableException e) {
throw new OrmException(e);

16
gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java
View File

@@ -382,17 +382,17 @@ public class EventFactory {
public void addPatchSets(ReviewDb db, RevWalk revWalk, ChangeAttribute ca,
Collection<PatchSet> ps,
Map<PatchSet.Id, Collection<PatchSetApproval>> approvals,
boolean includeFiles, Change change, LabelTypes labelTypes) {
boolean includeFiles, ChangeNotes notes, LabelTypes labelTypes) {
if (!ps.isEmpty()) {
ca.patchSets = new ArrayList<>(ps.size());
for (PatchSet p : ps) {
PatchSetAttribute psa = asPatchSetAttribute(db, revWalk, p);
PatchSetAttribute psa = asPatchSetAttribute(db, revWalk, notes, p);
if (approvals != null) {
addApprovals(psa, p.getId(), approvals, labelTypes);
}
ca.patchSets.add(psa);
if (includeFiles && change != null) {
addPatchSetFileNames(psa, change, p);
if (includeFiles && notes != null) {
addPatchSetFileNames(psa, notes.getChange(), p);
}
}
}
@@ -452,9 +452,9 @@ public class EventFactory {
* @return object suitable for serialization to JSON
*/
public PatchSetAttribute asPatchSetAttribute(RevWalk revWalk,
PatchSet patchSet) {
ChangeNotes notes, PatchSet patchSet) {
try (ReviewDb db = schema.open()) {
return asPatchSetAttribute(db, revWalk, patchSet);
return asPatchSetAttribute(db, revWalk, notes, patchSet);
} catch (OrmException e) {
log.error("Cannot open database connection", e);
return new PatchSetAttribute();
@@ -470,7 +470,7 @@ public class EventFactory {
* @return object suitable for serialization to JSON
*/
public PatchSetAttribute asPatchSetAttribute(ReviewDb db, RevWalk revWalk,
PatchSet patchSet) {
ChangeNotes notes, PatchSet patchSet) {
PatchSetAttribute p = new PatchSetAttribute();
p.revision = patchSet.getRevision().get();
p.number = Integer.toString(patchSet.getPatchSetId());
@@ -486,7 +486,7 @@ public class EventFactory {
p.parents.add(parent.name());
}
UserIdentity author = psInfoFactory.get(db, pId).getAuthor();
UserIdentity author = psInfoFactory.get(db, notes, pId).getAuthor();
if (author.getAccount() == null) {
p.author = new AccountAttribute();
p.author.email = author.getEmail();

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

@@ -65,6 +65,7 @@ import com.google.gerrit.server.git.strategy.SubmitStrategyFactory;
import com.google.gerrit.server.git.validators.MergeValidationException;
import com.google.gerrit.server.git.validators.MergeValidators;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
@@ -1034,7 +1035,7 @@ public class MergeOp implements AutoCloseable {
PatchSet.Id mergedId = commit.change().currentPatchSetId();
// TODO(dborowitz): Use PatchSetUtil after BatchUpdate migration.
merged = db.patchSets().get(mergedId);
c = setMergedPatchSet(c.getId(), mergedId);
c = setMergedPatchSet(commit.notes(), mergedId);
submitter = approvalsUtil.getSubmitter(db, commit.notes(), mergedId);
ChangeControl control = commit.getControl();
update = updateFactory.create(control, c.getLastUpdatedOn());
@@ -1071,9 +1072,10 @@ public class MergeOp implements AutoCloseable {
}
}
private Change setMergedPatchSet(Change.Id changeId, final PatchSet.Id merged)
throws OrmException {
return db.changes().atomicUpdate(changeId, new AtomicUpdate<Change>() {
private Change setMergedPatchSet(final ChangeNotes notes,
final PatchSet.Id merged) throws OrmException {
return db.changes().atomicUpdate(notes.getChangeId(),
new AtomicUpdate<Change>() {
@Override
public Change update(Change c) {
c.setStatus(Change.Status.MERGED);
@@ -1083,7 +1085,8 @@ public class MergeOp implements AutoCloseable {
// Go back to the patch set that was actually merged.
//
try {
c.setCurrentPatchSet(patchSetInfoFactory.get(db, merged));
c.setCurrentPatchSet(
patchSetInfoFactory.get(db, notes, merged));
} catch (PatchSetInfoNotAvailableException e1) {
logError("Cannot read merged patch set " + merged, e1);
}

5
gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java
View File

@@ -148,8 +148,9 @@ public abstract class ChangeEmail extends NotificationEmail {
if (patchSet != null && patchSetInfo == null) {
try {
patchSetInfo = args.patchSetInfoFactory.get(args.db.get(), patchSet.getId());
} catch (PatchSetInfoNotAvailableException err) {
patchSetInfo = args.patchSetInfoFactory.get(
args.db.get(), changeData.notes(), patchSet.getId());
} catch (PatchSetInfoNotAvailableException | OrmException err) {
patchSetInfo = null;
}
}

18
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java
View File

@@ -21,8 +21,10 @@ import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.client.UserIdentity;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.account.AccountByEmailCache;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -47,12 +49,16 @@ import java.util.Set;
@Singleton
public class PatchSetInfoFactory {
private final GitRepositoryManager repoManager;
private final PatchSetUtil psUtil;
private final AccountByEmailCache byEmailCache;
@Inject
public PatchSetInfoFactory(final GitRepositoryManager grm,
final AccountByEmailCache byEmailCache) {
this.repoManager = grm;
public PatchSetInfoFactory(
GitRepositoryManager repoManager,
PatchSetUtil psUtil,
AccountByEmailCache byEmailCache) {
this.repoManager = repoManager;
this.psUtil = psUtil;
this.byEmailCache = byEmailCache;
}
@@ -68,11 +74,11 @@ public class PatchSetInfoFactory {
return info;
}
public PatchSetInfo get(ReviewDb db, PatchSet.Id patchSetId)
public PatchSetInfo get(ReviewDb db, ChangeNotes notes, PatchSet.Id psId)
throws PatchSetInfoNotAvailableException {
try {
final PatchSet patchSet = db.patchSets().get(patchSetId);
final Change change = db.changes().get(patchSet.getId().getParentKey());
PatchSet patchSet = psUtil.get(db, notes, psId);
Change change = db.changes().get(psId.getParentKey());
return get(change, patchSet);
} catch (OrmException e) {
throw new PatchSetInfoNotAvailableException(e);

4
gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
View File

@@ -278,14 +278,14 @@ public class OutputStreamQuery {
if (includePatchSets) {
eventFactory.addPatchSets(db.get(), rw, c, d.patchSets(),
includeApprovals ? d.approvals().asMap() : null,
includeFiles, d.change(), labelTypes);
includeFiles, d.notes(), labelTypes);
}
if (includeCurrentPatchSet) {
PatchSet current = d.currentPatchSet();
if (current != null) {
c.currentPatchSet =
eventFactory.asPatchSetAttribute(db.get(), rw, current);
eventFactory.asPatchSetAttribute(db.get(), rw, d.notes(), current);
eventFactory.addApprovals(c.currentPatchSet,
d.currentApprovals(), labelTypes);