opendev/gerrit
Code Issues Proposed changes

Merge "ChangeNotes.Factory: Add more methods to create change notes"

Browse Source
This commit is contained in:
Edwin Kempin
2016-02-15 08:56:22 +00:00
committed by Gerrit Code Review
parent a55ed3dba4 fc4fda286e
commit 88951509a6
14 changed files with 104 additions and 58 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
View File

@@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
@@ -263,7 +264,7 @@ public class PushOneCommit {
public void assertChange(Change.Status expectedStatus,
String expectedTopic, TestAccount... expectedReviewers)
throws OrmException {
throws OrmException, NoSuchChangeException {
Change c = getChange().change();
assertThat(c.getSubject()).isEqualTo(resSubj);
assertThat(c.getStatus()).isEqualTo(expectedStatus);
@@ -272,9 +273,9 @@ public class PushOneCommit {
}
private void assertReviewers(Change c, TestAccount... expectedReviewers)
throws OrmException {
throws OrmException, NoSuchChangeException {
Iterable<Account.Id> actualIds = approvalsUtil
.getReviewers(db, notesFactory.create(db, c.getProject(), c.getId()))
.getReviewers(db, notesFactory.createChecked(db, c))
.values();
assertThat(actualIds).containsExactlyElementsIn(
Sets.newHashSet(TestAccount.ids(expectedReviewers)));

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

@@ -29,6 +29,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -219,13 +220,15 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
}
private PatchSetApproval getSubmitter(PatchSet.Id patchSetId)
throws OrmException {
throws OrmException, NoSuchChangeException {
ChangeNotes notes =
notesFactory.create(db, project, patchSetId.getParentKey()).load();
notesFactory.createChecked(db, project, patchSetId.getParentKey())
.load();
return approvalsUtil.getSubmitter(db, notes, patchSetId);
}
private void assertSubmitApproval(PatchSet.Id patchSetId) throws OrmException {
private void assertSubmitApproval(PatchSet.Id patchSetId)
throws OrmException, NoSuchChangeException {
PatchSetApproval a = getSubmitter(patchSetId);
assertThat(a.isSubmit()).isTrue();
assertThat(a.getValue()).isEqualTo((short) 1);

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

@@ -34,7 +34,6 @@ 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;
@@ -187,16 +186,16 @@ public class VisibleRefFilterIT extends AbstractDaemonTest {
allow(Permission.READ, REGISTERED_USERS, "refs/heads/master");
deny(Permission.READ, REGISTERED_USERS, "refs/heads/branch");
ChangeNotes notes = notesFactory.create(db, project, c1);
Change c = notesFactory.createChecked(db, project, c1).getChange();
PatchSet ps1 = getPatchSet(new PatchSet.Id(c1, 1));
// Admin's edit is not visible.
setApiUser(admin);
editModifier.createEdit(notes.getChange(), ps1);
editModifier.createEdit(c, ps1);
// User's edit is visible.
setApiUser(user);
editModifier.createEdit(notes.getChange(), ps1);
editModifier.createEdit(c, ps1);
assertRefs(
"HEAD",
@@ -214,12 +213,12 @@ public class VisibleRefFilterIT extends AbstractDaemonTest {
deny(Permission.READ, REGISTERED_USERS, "refs/heads/master");
allow(Permission.READ, REGISTERED_USERS, "refs/heads/branch");
ChangeNotes notes = notesFactory.create(db, project, c1);
Change c = notesFactory.createChecked(db, project, c1).getChange();
PatchSet ps1 = getPatchSet(new PatchSet.Id(c1, 1));
setApiUser(admin);
editModifier.createEdit(notes.getChange(), ps1);
editModifier.createEdit(c, ps1);
setApiUser(user);
editModifier.createEdit(notes.getChange(), ps1);
editModifier.createEdit(c, ps1);
assertRefs(
// Change 1 is visible due to accessDatabase capability, even though

9
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
View File

@@ -57,6 +57,7 @@ import com.google.gerrit.server.data.PatchSetAttribute;
import com.google.gerrit.server.events.ChangeMergedEvent;
import com.google.gerrit.server.events.Event;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.testutil.ConfigSuite;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -311,10 +312,10 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
}
protected void assertSubmitter(String changeId, int psId)
throws OrmException {
throws OrmException, NoSuchChangeException {
Change c =
getOnlyElement(queryProvider.get().byKeyPrefix(changeId)).change();
ChangeNotes cn = notesFactory.create(db, c.getProject(), c.getId());
ChangeNotes cn = notesFactory.createChecked(db, c);
PatchSetApproval submitter = approvalsUtil.getSubmitter(
db, cn, new PatchSet.Id(cn.getChangeId(), psId));
assertThat(submitter).isNotNull();
@@ -323,10 +324,10 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
}
protected void assertNoSubmitter(String changeId, int psId)
throws OrmException {
throws OrmException, NoSuchChangeException {
Change c =
getOnlyElement(queryProvider.get().byKeyPrefix(changeId)).change();
ChangeNotes cn = notesFactory.create(db, c.getProject(), c.getId());
ChangeNotes cn = notesFactory.createChecked(db, c);
PatchSetApproval submitter = approvalsUtil.getSubmitter(
db, cn, new PatchSet.Id(cn.getChangeId(), psId));
assertThat(submitter).isNull();

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 = notesFactory.create(db, project, c.getId()).getChange();
c = notesFactory.createChecked(db, c).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 = notesFactory.create(db, project, c.getId()).getChange();
c = notesFactory.createChecked(db, c).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 = notesFactory.create(db, project, c.getId()).getChange();
c = notesFactory.createChecked(db, c).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 = notesFactory.create(db, project, c.getId()).getChange();
c = notesFactory.createChecked(db, c).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 = notesFactory.create(db, project, c.getId()).getChange();
c = notesFactory.createChecked(db, c).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 = notesFactory.create(db, project, c.getId()).getChange();
c = notesFactory.createChecked(db, c).getChange();
PatchSet.Id psId2 = new PatchSet.Id(c.getId(), 2);
assertThat(c.currentPatchSetId()).isEqualTo(psId2);
assertThat(getPatchSet(psId2).getRevision().get())

4
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
View File

@@ -54,6 +54,7 @@ import com.google.gerrit.server.change.Revisions;
import com.google.gerrit.server.change.SubmittedTogether;
import com.google.gerrit.server.change.SuggestReviewers;
import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -219,7 +220,8 @@ class ChangeApiImpl implements ChangeApi {
public ChangeApi revert(RevertInput in) throws RestApiException {
try {
return changeApi.id(revert.apply(change, in)._number);
} catch (OrmException | IOException | UpdateException e) {
} catch (OrmException | IOException | UpdateException
| NoSuchChangeException e) {
throw new RestApiException("Cannot revert change", e);
}
}

4
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
View File

@@ -59,6 +59,7 @@ import com.google.gerrit.server.change.Submit;
import com.google.gerrit.server.change.TestSubmitType;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -214,7 +215,8 @@ class RevisionApiImpl implements RevisionApi {
public ChangeApi rebase(RebaseInput in) throws RestApiException {
try {
return changes.id(rebase.apply(revision, in)._number);
} catch (OrmException | EmailException | UpdateException | IOException e) {
} catch (OrmException | EmailException | UpdateException | IOException
| NoSuchChangeException e) {
throw new RestApiException("Cannot rebase ps", e);
}
}

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

@@ -100,6 +100,7 @@ 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;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
import com.google.gerrit.server.query.change.ChangeData;
@@ -157,7 +158,7 @@ public class ChangeJson {
private final Provider<ConsistencyChecker> checkerProvider;
private final ActionJson actionJson;
private final GpgApiAdapter gpgApi;
private final ChangeNotes.Factory changeNotesFactory;
private final ChangeNotes.Factory notesFactory;
private AccountLoader accountLoader;
private Map<Change.Id, List<SubmitRecord>> submitRecords;
@@ -183,7 +184,7 @@ public class ChangeJson {
Provider<ConsistencyChecker> checkerProvider,
ActionJson actionJson,
GpgApiAdapter gpgApi,
ChangeNotes.Factory changeNotesFactory,
ChangeNotes.Factory notesFactory,
@Assisted Set<ListChangesOption> options) {
this.db = db;
this.labelNormalizer = ln;
@@ -203,7 +204,7 @@ public class ChangeJson {
this.checkerProvider = checkerProvider;
this.actionJson = actionJson;
this.gpgApi = gpgApi;
this.changeNotesFactory = changeNotesFactory;
this.notesFactory = notesFactory;
this.options = options.isEmpty()
? EnumSet.noneOf(ListChangesOption.class)
: EnumSet.copyOf(options);
@@ -223,17 +224,17 @@ public class ChangeJson {
}
public ChangeInfo format(Project.NameKey project, Change.Id id)
throws OrmException {
ChangeNotes notes;
throws OrmException, NoSuchChangeException {
Change c;
try {
notes = changeNotesFactory.create(db.get(), project, id);
} catch (OrmException e) {
c = notesFactory.createChecked(db.get(), project, id).getChange();
} catch (OrmException | NoSuchChangeException e) {
if (!has(CHECK)) {
throw e;
}
return checkOnly(changeDataFactory.create(db.get(), project, id));
}
return format(changeDataFactory.create(db.get(), notes.getChange()));
return format(changeDataFactory.create(db.get(), c));
}
public ChangeInfo format(ChangeData cd) throws OrmException {

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

@@ -426,13 +426,12 @@ public class ConsistencyChecker {
continue;
}
try {
Change c = notesFactory
.create(db.get(), change.getProject(), psId.getParentKey())
.getChange();
if (c == null || !c.getDest().equals(change.getDest())) {
Change c = notesFactory.createChecked(
db.get(), change.getProject(), psId.getParentKey()).getChange();
if (!c.getDest().equals(change.getDest())) {
continue;
}
} catch (OrmException e) {
} catch (OrmException | NoSuchChangeException e) {
warn(e);
// Include this patch set; should cause an error below, which is good.
}
@@ -593,12 +592,8 @@ public class ConsistencyChecker {
try {
db.changes().beginTransaction(cid);
try {
ChangeNotes notes = notesFactory.create(db, project, cid);
ChangeNotes notes = notesFactory.createChecked(db, project, cid);
Change c = notes.getChange();
if (c == null) {
throw new OrmException("Change missing: " + cid);
}
if (psId.equals(c.currentPatchSetId())) {
List<PatchSet> all = Lists.newArrayList(db.patchSets().byChange(cid));
if (all.size() == 1 && all.get(0).getId().equals(psId)) {
@@ -638,7 +633,8 @@ public class ConsistencyChecker {
} finally {
db.rollback();
}
} catch (PatchSetInfoNotAvailableException | OrmException e) {
} catch (PatchSetInfoNotAvailableException | OrmException
| NoSuchChangeException e) {
String msg = "Error deleting patch set";
log.warn(msg + ' ' + psId, e);
p.status = Status.FIX_FAILED;

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

@@ -233,7 +233,7 @@ public class CreateChange implements
bu.execute();
}
ChangeJson json = jsonFactory.create(ChangeJson.NO_OPTIONS);
return Response.created(json.format(project, changeId));
return Response.created(json.format(ins.getChange()));
}
}

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

@@ -38,6 +38,7 @@ 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.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
@@ -98,7 +99,7 @@ public class Rebase implements RestModifyView<RevisionResource, RebaseInput>,
@Override
public ChangeInfo apply(RevisionResource rsrc, RebaseInput input)
throws EmailException, OrmException, UpdateException, RestApiException,
IOException {
IOException, NoSuchChangeException {
ChangeControl control = rsrc.getControl();
Change change = rsrc.getChange();
try (Repository repo = repoManager.openRepository(change.getProject());
@@ -297,7 +298,7 @@ public class Rebase implements RestModifyView<RevisionResource, RebaseInput>,
@Override
public ChangeInfo apply(ChangeResource rsrc, RebaseInput input)
throws EmailException, OrmException, UpdateException, RestApiException,
IOException {
IOException, NoSuchChangeException {
PatchSet ps =
rebase.dbProvider.get().patchSets()
.get(rsrc.getChange().currentPatchSetId());

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

@@ -58,7 +58,7 @@ public class Revert implements RestModifyView<ChangeResource, RevertInput>,
@Override
public ChangeInfo apply(ChangeResource req, RevertInput input)
throws IOException, OrmException, RestApiException,
UpdateException {
UpdateException, NoSuchChangeException {
RefControl refControl = req.getControl().getRefControl();
Change change = req.getChange();
if (!refControl.canUpload()) {

47
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@@ -45,8 +45,12 @@ import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -55,9 +59,12 @@ import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.RevWalk;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.List;
import java.util.Map;
/** View of a single {@link Change} based on the log of its notes branch. */
@@ -103,18 +110,56 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
@Singleton
public static class Factory {
private static final Logger log = LoggerFactory.getLogger(Factory.class);
private final GitRepositoryManager repoManager;
private final NotesMigration migration;
private final AllUsersNameProvider allUsersProvider;
private final Provider<InternalChangeQuery> queryProvider;
@VisibleForTesting
@Inject
public Factory(GitRepositoryManager repoManager,
NotesMigration migration,
AllUsersNameProvider allUsersProvider) {
AllUsersNameProvider allUsersProvider,
Provider<InternalChangeQuery> queryProvider) {
this.repoManager = repoManager;
this.migration = migration;
this.allUsersProvider = allUsersProvider;
this.queryProvider = queryProvider;
}
public ChangeNotes createChecked(ReviewDb db, Change c)
throws OrmException, NoSuchChangeException {
ChangeNotes notes = create(db, c.getProject(), c.getId());
if (notes.getChange() == null) {
throw new NoSuchChangeException(c.getId());
}
return notes;
}
public ChangeNotes createChecked(ReviewDb db, Project.NameKey project,
Change.Id changeId) throws OrmException, NoSuchChangeException {
ChangeNotes notes = create(db, project, changeId);
if (notes.getChange() == null) {
throw new NoSuchChangeException(changeId);
}
return notes;
}
public ChangeNotes createChecked(Change.Id changeId)
throws OrmException, NoSuchChangeException {
InternalChangeQuery query = queryProvider.get().noFields();
List<ChangeData> changes = query.byLegacyChangeId(changeId);
if (changes.isEmpty()) {
throw new NoSuchChangeException(changeId);
}
if (changes.size() != 1) {
log.error(
String.format("Multiple changes found for %d", changeId.get()));
throw new NoSuchChangeException(changeId);
}
return changes.get(0).notes();
}
public ChangeNotes create(ReviewDb db, Project.NameKey project,

13
gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View File

@@ -29,7 +29,6 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeFinder;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.query.change.ChangeData;
@@ -48,16 +47,13 @@ public class ChangeControl {
public static class GenericFactory {
private final ProjectControl.GenericFactory projectControl;
private final ChangeNotes.Factory notesFactory;
private final ChangeFinder changeFinder;
@Inject
GenericFactory(
ProjectControl.GenericFactory p,
ChangeNotes.Factory n,
ChangeFinder f) {
ChangeNotes.Factory n) {
projectControl = p;
notesFactory = n;
changeFinder = f;
}
public ChangeControl controlFor(ReviewDb db, Project.NameKey project,
@@ -92,13 +88,12 @@ public class ChangeControl {
public ChangeControl validateFor(ReviewDb db, Change.Id changeId,
CurrentUser user) throws NoSuchChangeException, OrmException {
ChangeControl ctl = changeFinder.findOne(changeId, user);
return validateFor(db, ctl.getChange(), user);
return validateFor(db, notesFactory.createChecked(changeId), user);
}
public ChangeControl validateFor(ReviewDb db, Change change,
public ChangeControl validateFor(ReviewDb db, ChangeNotes notes,
CurrentUser user) throws NoSuchChangeException, OrmException {
ChangeControl c = controlFor(db, change, user);
ChangeControl c = controlFor(notes, user);
if (!c.isVisible(db)) {
throw new NoSuchChangeException(c.getId());
}