diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java index dd64d80cc6..9909069cf9 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java @@ -185,7 +185,7 @@ class PatchSetDetailFactory extends Handler { 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(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java index 52aa2ca44e..3659db6adb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java +++ b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java @@ -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 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 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 uploader = getAccountSupplier(patchSet.getUploader()); Supplier 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 uploader = getAccountSupplier(patchSet.getUploader()); Supplier 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 approvals, ReviewDb db) throws OrmException { + ChangeNotes notes = newNotes(change); CommentAddedEvent event = new CommentAddedEvent(change); Supplier 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() { @@ -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 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 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 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 patchSetAttributeSupplier( - final Change change, final PatchSet patchSet) { + final ChangeNotes notes, final PatchSet patchSet) { return Suppliers.memoize( new Supplier() { @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); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java index 4447c50332..f88d60f133 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -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 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)); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java index aabb1237de..9d4b91745f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java @@ -164,7 +164,7 @@ public class DeleteDraftPatchSet implements RestModifyView ps, Map> 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(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 3509b9cfa8..aacc5e2256 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -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,27 +1072,29 @@ public class MergeOp implements AutoCloseable { } } - private Change setMergedPatchSet(Change.Id changeId, final PatchSet.Id merged) - throws OrmException { - return db.changes().atomicUpdate(changeId, new AtomicUpdate() { - @Override - public Change update(Change c) { - c.setStatus(Change.Status.MERGED); - c.setSubmissionId(submissionId); - if (!merged.equals(c.currentPatchSetId())) { - // Uncool; the patch set changed after we merged it. - // Go back to the patch set that was actually merged. - // - try { - c.setCurrentPatchSet(patchSetInfoFactory.get(db, merged)); - } catch (PatchSetInfoNotAvailableException e1) { - logError("Cannot read merged patch set " + merged, e1); + private Change setMergedPatchSet(final ChangeNotes notes, + final PatchSet.Id merged) throws OrmException { + return db.changes().atomicUpdate(notes.getChangeId(), + new AtomicUpdate() { + @Override + public Change update(Change c) { + c.setStatus(Change.Status.MERGED); + c.setSubmissionId(submissionId); + if (!merged.equals(c.currentPatchSetId())) { + // Uncool; the patch set changed after we merged it. + // Go back to the patch set that was actually merged. + // + try { + c.setCurrentPatchSet( + patchSetInfoFactory.get(db, notes, merged)); + } catch (PatchSetInfoNotAvailableException e1) { + logError("Cannot read merged patch set " + merged, e1); + } + } + ChangeUtil.updated(c); + return c; } - } - ChangeUtil.updated(c); - return c; - } - }); + }); } private void setApproval(ChangeData cd) throws OrmException, IOException { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java index d36a409ca7..c02ec1dbe6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java @@ -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; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java index 16598b7310..b1ed4ca2e2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java @@ -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); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java index 237bbf2097..cac62e24fd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java @@ -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);