From 0fb0983b9b3ec53e3c928465a4cb533987c51ead Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 1 Feb 2016 18:17:15 +0100 Subject: [PATCH] Use ChangeNotes in more places to load the change Changes should be loaded via ChangeNotes.Factory and not directly from the database. Change-Id: I36224208428092ea4cd86843f72f9f72e8c08535 Signed-off-by: Edwin Kempin --- .../gerrit/acceptance/AbstractDaemonTest.java | 4 ++++ .../gerrit/acceptance/git/SubmitOnPushIT.java | 7 ++----- .../gerrit/acceptance/git/VisibleRefFilterIT.java | 13 +++++++------ .../server/change/ConsistencyCheckerIT.java | 12 ++++++------ .../google/gerrit/server/change/ChangeJson.java | 13 +++++++++---- .../google/gerrit/server/change/CherryPick.java | 3 ++- .../gerrit/server/change/ConsistencyChecker.java | 9 +++++---- .../google/gerrit/server/change/CreateChange.java | 2 +- .../com/google/gerrit/server/change/Rebase.java | 14 ++++++++------ .../com/google/gerrit/server/change/Revert.java | 3 ++- .../com/google/gerrit/server/change/Submit.java | 7 ++++++- .../google/gerrit/server/events/EventFactory.java | 4 ++-- .../google/gerrit/server/git/ReceiveCommits.java | 9 ++++++--- plugins/reviewnotes | 2 +- 14 files changed, 61 insertions(+), 41 deletions(-) diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 74abe81f51..78fa0a49c5 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -57,6 +57,7 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.index.ChangeIndexer; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.Util; @@ -187,6 +188,9 @@ public abstract class AbstractDaemonTest { @Inject protected NotesMigration notesMigration; + @Inject + protected ChangeNotes.Factory notesFactory; + @Rule public ExpectedException exception = ExpectedException.none(); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java index 747c0ad2d1..813c9c9176 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java @@ -50,9 +50,6 @@ public class SubmitOnPushIT extends AbstractDaemonTest { @Inject private ApprovalsUtil approvalsUtil; - @Inject - private ChangeNotes.Factory changeNotesFactory; - @Test public void submitOnPush() throws Exception { grant(Permission.SUBMIT, project, "refs/for/refs/heads/master"); @@ -223,8 +220,8 @@ public class SubmitOnPushIT extends AbstractDaemonTest { private PatchSetApproval getSubmitter(PatchSet.Id patchSetId) throws OrmException { - ChangeNotes notes = changeNotesFactory - .create(db, project, patchSetId.getParentKey()).load(); + ChangeNotes notes = + notesFactory.create(db, project, patchSetId.getParentKey()).load(); return approvalsUtil.getSubmitter(db, notes, patchSetId); } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/VisibleRefFilterIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/VisibleRefFilterIT.java index 796d4d6e5e..0e47123f58 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/VisibleRefFilterIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/VisibleRefFilterIT.java @@ -34,6 +34,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.edit.ChangeEditModifier; import com.google.gerrit.server.git.ProjectConfig; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.project.Util; import com.google.inject.Inject; @@ -186,16 +187,16 @@ public class VisibleRefFilterIT extends AbstractDaemonTest { allow(Permission.READ, REGISTERED_USERS, "refs/heads/master"); deny(Permission.READ, REGISTERED_USERS, "refs/heads/branch"); - Change change1 = db.changes().get(c1); + ChangeNotes notes = notesFactory.create(db, project, c1); PatchSet ps1 = getPatchSet(new PatchSet.Id(c1, 1)); // Admin's edit is not visible. setApiUser(admin); - editModifier.createEdit(change1, ps1); + editModifier.createEdit(notes.getChange(), ps1); // User's edit is visible. setApiUser(user); - editModifier.createEdit(change1, ps1); + editModifier.createEdit(notes.getChange(), ps1); assertRefs( "HEAD", @@ -213,12 +214,12 @@ public class VisibleRefFilterIT extends AbstractDaemonTest { deny(Permission.READ, REGISTERED_USERS, "refs/heads/master"); allow(Permission.READ, REGISTERED_USERS, "refs/heads/branch"); - Change change1 = db.changes().get(c1); + ChangeNotes notes = notesFactory.create(db, project, c1); PatchSet ps1 = getPatchSet(new PatchSet.Id(c1, 1)); setApiUser(admin); - editModifier.createEdit(change1, ps1); + editModifier.createEdit(notes.getChange(), ps1); setApiUser(user); - editModifier.createEdit(change1, ps1); + editModifier.createEdit(notes.getChange(), ps1); assertRefs( // Change 1 is visible due to accessDatabase capability, even though diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index db93f29178..34d42f172f 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -223,7 +223,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); assertThat(p.outcome).isEqualTo("Deleted patch set"); - c = db.changes().get(c.getId()); + c = notesFactory.create(db, project, c.getId()).getChange(); assertThat(c.currentPatchSetId().get()).isEqualTo(1); assertThat(getPatchSet(ps1.getId())).isNotNull(); assertThat(getPatchSet(ps2.getId())).isNull(); @@ -271,7 +271,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); assertThat(p.outcome).isEqualTo("Deleted patch set"); - c = db.changes().get(c.getId()); + c = notesFactory.create(db, project, c.getId()).getChange(); assertThat(c.currentPatchSetId().get()).isEqualTo(3); assertThat(getPatchSet(ps1.getId())).isNotNull(); assertThat(getPatchSet(ps2.getId())).isNull(); @@ -299,7 +299,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { assertThat(p.outcome) .isEqualTo("Cannot delete patch set; no patch sets would remain"); - c = db.changes().get(c.getId()); + c = notesFactory.create(db, project, c.getId()).getChange(); assertThat(c.currentPatchSetId().get()).isEqualTo(1); assertThat(getPatchSet(ps1.getId())).isNotNull(); } @@ -387,7 +387,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); assertThat(p.outcome).isEqualTo("Marked change as merged"); - c = db.changes().get(c.getId()); + c = notesFactory.create(db, project, c.getId()).getChange(); assertThat(c.getStatus()).isEqualTo(Change.Status.MERGED); assertProblems(c); } @@ -476,7 +476,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); assertThat(p.outcome).isEqualTo("Inserted as patch set 2"); - c = db.changes().get(c.getId()); + c = notesFactory.create(db, project, c.getId()).getChange(); PatchSet.Id psId2 = new PatchSet.Id(c.getId(), 2); assertThat(c.currentPatchSetId()).isEqualTo(psId2); assertThat(getPatchSet(psId2).getRevision().get()) @@ -518,7 +518,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); assertThat(p.outcome).isEqualTo("Inserted as patch set 2"); - c = db.changes().get(c.getId()); + c = notesFactory.create(db, project, c.getId()).getChange(); PatchSet.Id psId2 = new PatchSet.Id(c.getId(), 2); assertThat(c.currentPatchSetId()).isEqualTo(psId2); assertThat(getPatchSet(psId2).getRevision().get()) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 28328970b0..138377d7a2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -96,6 +96,7 @@ import com.google.gerrit.server.api.accounts.GpgApiAdapter; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.LabelNormalizer; import com.google.gerrit.server.git.MergeUtil; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.project.ChangeControl; @@ -156,6 +157,7 @@ public class ChangeJson { private final Provider checkerProvider; private final ActionJson actionJson; private final GpgApiAdapter gpgApi; + private final ChangeNotes.Factory changeNotesFactory; private AccountLoader accountLoader; private Map> submitRecords; @@ -181,6 +183,7 @@ public class ChangeJson { Provider checkerProvider, ActionJson actionJson, GpgApiAdapter gpgApi, + ChangeNotes.Factory changeNotesFactory, @Assisted Set options) { this.db = db; this.labelNormalizer = ln; @@ -200,6 +203,7 @@ public class ChangeJson { this.checkerProvider = checkerProvider; this.actionJson = actionJson; this.gpgApi = gpgApi; + this.changeNotesFactory = changeNotesFactory; this.options = options.isEmpty() ? EnumSet.noneOf(ListChangesOption.class) : EnumSet.copyOf(options); @@ -218,17 +222,18 @@ public class ChangeJson { return format(changeDataFactory.create(db.get(), change)); } - public ChangeInfo format(Change.Id id) throws OrmException { - Change c; + public ChangeInfo format(Project.NameKey project, Change.Id id) + throws OrmException { + ChangeNotes notes; try { - c = db.get().changes().get(id); + notes = changeNotesFactory.create(db.get(), project, id); } catch (OrmException e) { if (!has(CHECK)) { throw e; } return checkOnly(changeDataFactory.create(db.get(), id)); } - return format(changeDataFactory.create(db.get(), c)); + return format(changeDataFactory.create(db.get(), notes.getChange())); } public ChangeInfo format(ChangeData cd) throws OrmException { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java index 82aaecbe28..545ea17a81 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java @@ -84,7 +84,8 @@ public class CherryPick implements RestModifyView all = Lists.newArrayList(db.patchSets().byChange(cid)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java index b280c48789..5a1e6ffa90 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java @@ -217,7 +217,7 @@ public class CreateChange implements bu.execute(); } ChangeJson json = jsonFactory.create(ChangeJson.NO_OPTIONS); - return Response.created(json.format(changeId)); + return Response.created(json.format(project, changeId)); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java index 7101986b6f..6e55d39156 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java @@ -36,6 +36,7 @@ import com.google.gerrit.server.git.BatchUpdate; 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.notedb.ChangeNotes; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; @@ -68,6 +69,7 @@ public class Rebase implements RestModifyView, private final RebaseChangeOp.Factory rebaseFactory; private final RebaseUtil rebaseUtil; private final ChangeJson.Factory json; + private final ChangeNotes.Factory notesFactory; private final Provider dbProvider; private final Provider queryProvider; private final PatchSetUtil psUtil; @@ -78,6 +80,7 @@ public class Rebase implements RestModifyView, RebaseChangeOp.Factory rebaseFactory, RebaseUtil rebaseUtil, ChangeJson.Factory json, + ChangeNotes.Factory notesFactory, Provider dbProvider, Provider queryProvider, PatchSetUtil psUtil) { @@ -86,6 +89,7 @@ public class Rebase implements RestModifyView, this.rebaseFactory = rebaseFactory; this.rebaseUtil = rebaseUtil; this.json = json; + this.notesFactory = notesFactory; this.dbProvider = dbProvider; this.queryProvider = queryProvider; this.psUtil = psUtil; @@ -120,7 +124,7 @@ public class Rebase implements RestModifyView, .setValidatePolicy(CommitValidators.Policy.GERRIT)); bu.execute(); } - return json.create(OPTIONS).format(change.getId()); + return json.create(OPTIONS).format(change.getProject(), change.getId()); } private String findBaseRev(RevWalk rw, RevisionResource rsrc, @@ -236,11 +240,9 @@ public class Rebase implements RestModifyView, if (rsrc.getChange().getId().equals(id)) { return rsrc.getControl(); } - Change c = dbProvider.get().changes().get(id); - if (c == null) { - return null; - } - return rsrc.getControl().getProjectControl().controlFor(c); + ChangeNotes notes = + notesFactory.create(dbProvider.get(), rsrc.getProject(), id); + return rsrc.getControl().getProjectControl().controlFor(notes); } private boolean hasOneParent(RevWalk rw, PatchSet ps) throws IOException { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java index 7e3845cd72..d702fc6026 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java @@ -76,7 +76,8 @@ public class Revert implements RestModifyView, } catch (NoSuchChangeException e) { throw new ResourceNotFoundException(e.getMessage()); } - return json.create(ChangeJson.NO_OPTIONS).format(revertedChangeId); + return json.create(ChangeJson.NO_OPTIONS).format(req.getProject(), + revertedChangeId); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index 8ba80bd719..488a92e729 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -44,6 +44,7 @@ import com.google.gerrit.server.git.ChangeSet; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeOp; import com.google.gerrit.server.git.MergeSuperSet; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.query.change.ChangeData; @@ -116,6 +117,7 @@ public class Submit implements RestModifyView, private final ChangeData.Factory changeDataFactory; private final ChangeMessagesUtil cmUtil; private final ChangeControl.GenericFactory changeControlFactory; + private final ChangeNotes.Factory changeNotesFactory; private final Provider mergeOpProvider; private final MergeSuperSet mergeSuperSet; private final AccountsCollection accounts; @@ -135,6 +137,7 @@ public class Submit implements RestModifyView, ChangeData.Factory changeDataFactory, ChangeMessagesUtil cmUtil, ChangeControl.GenericFactory changeControlFactory, + ChangeNotes.Factory changeNotesFactory, Provider mergeOpProvider, MergeSuperSet mergeSuperSet, AccountsCollection accounts, @@ -146,6 +149,7 @@ public class Submit implements RestModifyView, this.changeDataFactory = changeDataFactory; this.cmUtil = cmUtil; this.changeControlFactory = changeControlFactory; + this.changeNotesFactory = changeNotesFactory; this.mergeOpProvider = mergeOpProvider; this.mergeSuperSet = mergeSuperSet; this.accounts = accounts; @@ -203,7 +207,8 @@ public class Submit implements RestModifyView, try (MergeOp op = mergeOpProvider.get()) { ReviewDb db = dbProvider.get(); op.merge(db, change, caller, true, input); - change = db.changes().get(change.getId()); + change = changeNotesFactory + .create(db, change.getProject(), change.getId()).getChange(); } if (change == null) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java index 758fdbabc0..09a8452d27 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java @@ -496,7 +496,7 @@ public class EventFactory { p.author = asAccountAttribute(author.getAccount()); } - Change change = db.changes().get(pId.getParentKey()); + Change change = notes.getChange(); List list = patchListCache.get(change, patchSet).toPatchList(pId); for (Patch pe : list) { @@ -506,7 +506,7 @@ public class EventFactory { } } p.kind = changeKindCache.getChangeKind(db, change, patchSet); - } catch (OrmException | IOException e) { + } catch (IOException e) { log.error("Cannot load patch set data for " + patchSet.getId(), e); } catch (PatchSetInfoNotAvailableException e) { log.error(String.format("Cannot get authorEmail for %s.", pId), e); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 86ae1ab286..89eb49fc3c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -1486,7 +1486,8 @@ public class ReceiveCommits { final Change changeEnt; try { - changeEnt = db.changes().get(changeId); + changeEnt = + notesFactory.create(db, project.getNameKey(), changeId).getChange(); } catch (OrmException e) { log.error("Cannot lookup existing change " + changeId, e); reject(cmd, "database error"); @@ -1837,7 +1838,8 @@ public class ReceiveCommits { changeCtl.getUser().asIdentifiedUser(), false, null); } addMessage(""); - Change c = db.changes().get(rsrc.getChange().getId()); + Change c = notesFactory + .create(db, project.getNameKey(), rsrc.getChange().getId()).getChange(); switch (c.getStatus()) { case MERGED: addMessage("Change " + c.getChangeId() + " merged."); @@ -2721,7 +2723,8 @@ public class ReceiveCommits { String refName = cmd.getRefName(); Change.Id cid = psi.getParentKey(); - Change change = db.changes().get(cid); + Change change = + notesFactory.create(db, project.getNameKey(), cid).getChange(); ChangeControl ctl = projectControl.controlFor(change); PatchSet ps = psUtil.get(db, ctl.getNotes(), psi); if (change == null || ps == null) { diff --git a/plugins/reviewnotes b/plugins/reviewnotes index 34678f04b2..552001eeaf 160000 --- a/plugins/reviewnotes +++ b/plugins/reviewnotes @@ -1 +1 @@ -Subproject commit 34678f04b28c02cda754dd49c9dd99ff4db60415 +Subproject commit 552001eeaf6ffbfdb1f1ecd08cd9b34c94ca7ed8