opendev/gerrit
Code Issues Proposed changes

Use ChangeNotes in more places to load the change

Browse Source 
Changes should be loaded via ChangeNotes.Factory and not directly from
the database.

Change-Id: I36224208428092ea4cd86843f72f9f72e8c08535
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin 2016-02-01 18:17:15 +01:00
parent c89c21cd6d
commit 0fb0983b9b
14 changed files with 61 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@ -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();

7
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java
View File

@ -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);
}

13
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/VisibleRefFilterIT.java
View File

@ -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

12
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
View File

@ -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())

13
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
View File

@ -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<ConsistencyChecker> checkerProvider;
private final ActionJson actionJson;
private final GpgApiAdapter gpgApi;
private final ChangeNotes.Factory changeNotesFactory;
private AccountLoader accountLoader;
private Map<Change.Id, List<SubmitRecord>> submitRecords;
@ -181,6 +183,7 @@ public class ChangeJson {
Provider<ConsistencyChecker> checkerProvider,
ActionJson actionJson,
GpgApiAdapter gpgApi,
ChangeNotes.Factory changeNotesFactory,
@Assisted Set<ListChangesOption> 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 {

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

@ -84,7 +84,8 @@ public class CherryPick implements RestModifyView<RevisionResource, CherryPickIn
cherryPickChange.cherryPick(revision.getChange(),
revision.getPatchSet(), input.message, refName,
refControl);
return json.create(ChangeJson.NO_OPTIONS).format(cherryPickedChangeId);
return json.create(ChangeJson.NO_OPTIONS).format(revision.getProject(),
cherryPickedChangeId);
} catch (InvalidChangeOperationException e) {
throw new BadRequestException(e.getMessage());
} catch (IntegrationException | NoSuchChangeException e) {

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

@ -312,7 +312,7 @@ public class ConsistencyChecker {
objId, String.format("patch set %d", psNum));
if (psCommit == null) {
if (fix != null && fix.deletePatchSetIfCommitMissing) {
deletePatchSet(lastProblem(), ps.getId());
deletePatchSet(lastProblem(), change.getProject(), ps.getId());
}
continue;
} else if (refProblem != null && fix != null) {
@ -577,17 +577,18 @@ public class ConsistencyChecker {
}
}
private void deletePatchSet(ProblemInfo p, PatchSet.Id psId) {
private void deletePatchSet(ProblemInfo p, Project.NameKey project,
PatchSet.Id psId) {
ReviewDb db = this.db.get();
Change.Id cid = psId.getParentKey();
try {
db.changes().beginTransaction(cid);
try {
Change c = db.changes().get(cid);
ChangeNotes notes = notesFactory.create(db, project, cid);
Change c = notes.getChange();
if (c == null) {
throw new OrmException("Change missing: " + cid);
}
ChangeNotes notes = notesFactory.create(db, c.getProject(), cid);
if (psId.equals(c.currentPatchSetId())) {
List<PatchSet> all = Lists.newArrayList(db.patchSets().byChange(cid));

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

@ -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));
}
}

14
gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java
View File

@ -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<RevisionResource, RebaseInput>,
private final RebaseChangeOp.Factory rebaseFactory;
private final RebaseUtil rebaseUtil;
private final ChangeJson.Factory json;
private final ChangeNotes.Factory notesFactory;
private final Provider<ReviewDb> dbProvider;
private final Provider<InternalChangeQuery> queryProvider;
private final PatchSetUtil psUtil;
@ -78,6 +80,7 @@ public class Rebase implements RestModifyView<RevisionResource, RebaseInput>,
RebaseChangeOp.Factory rebaseFactory,
RebaseUtil rebaseUtil,
ChangeJson.Factory json,
ChangeNotes.Factory notesFactory,
Provider<ReviewDb> dbProvider,
Provider<InternalChangeQuery> queryProvider,
PatchSetUtil psUtil) {
@ -86,6 +89,7 @@ public class Rebase implements RestModifyView<RevisionResource, RebaseInput>,
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<RevisionResource, RebaseInput>,
.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<RevisionResource, RebaseInput>,
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 {

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

@ -76,7 +76,8 @@ public class Revert implements RestModifyView<ChangeResource, RevertInput>,
} 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

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

@ -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<RevisionResource, SubmitInput>,
private final ChangeData.Factory changeDataFactory;
private final ChangeMessagesUtil cmUtil;
private final ChangeControl.GenericFactory changeControlFactory;
private final ChangeNotes.Factory changeNotesFactory;
private final Provider<MergeOp> mergeOpProvider;
private final MergeSuperSet mergeSuperSet;
private final AccountsCollection accounts;
@ -135,6 +137,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
ChangeData.Factory changeDataFactory,
ChangeMessagesUtil cmUtil,
ChangeControl.GenericFactory changeControlFactory,
ChangeNotes.Factory changeNotesFactory,
Provider<MergeOp> mergeOpProvider,
MergeSuperSet mergeSuperSet,
AccountsCollection accounts,
@ -146,6 +149,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
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<RevisionResource, SubmitInput>,
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) {

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

@ -496,7 +496,7 @@ public class EventFactory {
p.author = asAccountAttribute(author.getAccount());
}
Change change = db.changes().get(pId.getParentKey());
Change change = notes.getChange();
List<Patch> 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);

9
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@ -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) {

2
plugins/reviewnotes

@ -1 +1 @@
Subproject commit 34678f04b28c02cda754dd49c9dd99ff4db60415
Subproject commit 552001eeaf6ffbfdb1f1ecd08cd9b34c94ca7ed8