opendev/gerrit
Code Issues Proposed changes

ChangeNotes: Enforce project passed to factory matches change

Browse Source 
In Ib164511e20 we started passing project to ChangeData.Factory
methods. Unfortunately during review we missed the fact that we're
passing the wrong project to the factory in
Submit#problemsForSubmittingChanges. This has subtle and unfortunate
consequences: using the wrong repository for the mergeability check
will always fail, since the commit to test does not belong to that
repository. The end result is that submitWholeTopic across projects
was intermittently broken, since we could never get the mergeability
check to pass on any change not in the same project as the one where
submit was clicked.

(Except that, maddeningly, when commits are shared between
projects using submitWholeTopic, as was the case before I71efad9d06,
the mergeability checks would succeed anyway, making tests pass when
they shouldn't have.)

Add a sanity check in this factory method. Fix the issue in Submit by
just using the existing ChangeData from the ChangeSet, which it's
unclear why we weren't doing that in the first place. To do so we also
need to teach MergeSuperSet to prepopulate the ChangeControls with the
right user, which are used for visibility checks later.

Change-Id: I3fa88121dde5b5eaaf72ead916466cf6d3e3c9f1
This commit is contained in:
Dave Borowitz
2016-02-18 20:49:54 -05:00
parent 1a245bda61
commit 3e4b231afc
7 changed files with 41 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -68,7 +68,8 @@ public class GetRevisionActions implements ETagView<RevisionResource> {
rsrc.getChangeResource().prepareETag(h, user);
h.putBoolean(Submit.wholeTopicEnabled(config));
ReviewDb db = dbProvider.get();
ChangeSet cs = mergeSuperSet.completeChangeSet(db, rsrc.getChange());
ChangeSet cs =
mergeSuperSet.completeChangeSet(db, rsrc.getChange(), user);
for (ChangeData cd : cs.changes()) {
new ChangeResource(cd.changeControl()).prepareETag(h, user);
}

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

@@ -32,7 +32,6 @@ import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeMessagesUtil;
@@ -47,7 +46,6 @@ 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;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
@@ -117,7 +115,6 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
private final GitRepositoryManager repoManager;
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;
@@ -137,7 +134,6 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
GitRepositoryManager repoManager,
ChangeData.Factory changeDataFactory,
ChangeMessagesUtil cmUtil,
ChangeControl.GenericFactory changeControlFactory,
ChangeNotes.Factory changeNotesFactory,
Provider<MergeOp> mergeOpProvider,
MergeSuperSet mergeSuperSet,
@@ -149,7 +145,6 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
this.repoManager = repoManager;
this.changeDataFactory = changeDataFactory;
this.cmUtil = cmUtil;
this.changeControlFactory = changeControlFactory;
this.changeNotesFactory = changeNotesFactory;
this.mergeOpProvider = mergeOpProvider;
this.mergeSuperSet = mergeSuperSet;
@@ -233,19 +228,16 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
/**
* @param cs set of changes to be submitted at once
* @param project the name of the project
* @param identifiedUser the user who is checking to submit
* @return a reason why any of the changes is not submittable or null
*/
private String problemsForSubmittingChangeset(ChangeSet cs,
Project.NameKey project, IdentifiedUser identifiedUser) {
IdentifiedUser identifiedUser) {
try {
@SuppressWarnings("resource")
ReviewDb db = dbProvider.get();
for (PatchSet.Id psId : cs.patchIds()) {
ChangeControl changeControl = changeControlFactory
.controlFor(db, project, psId.getParentKey(), identifiedUser);
ChangeData c = changeDataFactory.create(db, changeControl);
for (ChangeData c : cs.changes()) {
ChangeControl changeControl = c.changeControl(identifiedUser);
if (!changeControl.isVisible(db)) {
return BLOCKED_HIDDEN_SUBMIT_TOOLTIP;
@@ -270,7 +262,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
}
} catch (ResourceConflictException e) {
return BLOCKED_SUBMIT_TOOLTIP;
} catch (NoSuchChangeException | OrmException e) {
} catch (OrmException e) {
log.error("Error checking if change is submittable", e);
throw new OrmRuntimeException("Could not determine problems for the change", e);
}
@@ -329,7 +321,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
ChangeSet cs;
try {
cs = mergeSuperSet.completeChangeSet(db, cd.change());
cs = mergeSuperSet.completeChangeSet(
db, cd.change(), resource.getControl().getUser());
} catch (OrmException | IOException e) {
throw new OrmRuntimeException("Could not determine complete set of " +
"changes to be submitted", e);
@@ -343,8 +336,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
&& !Strings.isNullOrEmpty(topic)
&& topicSize > 1;
String submitProblems = problemsForSubmittingChangeset(cs,
resource.getProject(), resource.getUser());
String submitProblems =
problemsForSubmittingChangeset(cs, resource.getUser());
if (submitProblems != null) {
return new UiAction.Description()
.setLabel(treatWithTopic

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

@@ -23,6 +23,7 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.WalkSorter.PatchSetData;
import com.google.gerrit.server.git.ChangeSet;
import com.google.gerrit.server.git.MergeSuperSet;
@@ -74,7 +75,7 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
Change c = resource.getChange();
List<ChangeData> cds;
if (c.getStatus().isOpen()) {
cds = getForOpenChange(c);
cds = getForOpenChange(c, resource.getControl().getUser());
} else if (c.getStatus().asChangeStatus() == ChangeStatus.MERGED) {
cds = getForMergedChange(c);
} else {
@@ -99,9 +100,9 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
}
}
private List<ChangeData> getForOpenChange(Change c)
private List<ChangeData> getForOpenChange(Change c, CurrentUser user)
throws OrmException, IOException {
ChangeSet cs = mergeSuperSet.completeChangeSet(dbProvider.get(), c);
ChangeSet cs = mergeSuperSet.completeChangeSet(dbProvider.get(), c, user);
return cs.changes().asList();
}

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

@@ -547,7 +547,7 @@ public class MergeOp implements AutoCloseable {
this.db = db;
logDebug("Beginning integration of {}", change);
try {
ChangeSet cs = mergeSuperSet.completeChangeSet(db, change);
ChangeSet cs = mergeSuperSet.completeChangeSet(db, change, caller);
checkState(cs.ids().contains(change.getId()),
"change %s missing from %s", change.getId(), cs);
this.commits = new CommitStatus(cs);

24
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java
View File

@@ -24,6 +24,7 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.Submit;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.index.ChangeField;
@@ -90,21 +91,22 @@ public class MergeSuperSet {
this.repoManager = repoManager;
}
public ChangeSet completeChangeSet(ReviewDb db, Change change)
public ChangeSet completeChangeSet(ReviewDb db, Change change, CurrentUser user)
throws MissingObjectException, IncorrectObjectTypeException, IOException,
OrmException {
ChangeData cd =
changeDataFactory.create(db, change.getProject(), change.getId());
cd.changeControl(user);
if (Submit.wholeTopicEnabled(cfg)) {
return completeChangeSetIncludingTopics(db, new ChangeSet(cd));
return completeChangeSetIncludingTopics(db, new ChangeSet(cd), user);
} else {
return completeChangeSetWithoutTopic(db, new ChangeSet(cd));
return completeChangeSetWithoutTopic(db, new ChangeSet(cd), user);
}
}
private ChangeSet completeChangeSetWithoutTopic(ReviewDb db, ChangeSet changes)
throws MissingObjectException, IncorrectObjectTypeException, IOException,
OrmException {
private ChangeSet completeChangeSetWithoutTopic(ReviewDb db, ChangeSet changes,
CurrentUser user) throws MissingObjectException,
IncorrectObjectTypeException, IOException, OrmException {
List<ChangeData> ret = new ArrayList<>();
Multimap<Project.NameKey, Change.Id> pc = changes.changesByProject();
@@ -113,6 +115,7 @@ public class MergeSuperSet {
RevWalk rw = CodeReviewCommit.newRevWalk(repo)) {
for (Change.Id cId : pc.get(project)) {
ChangeData cd = changeDataFactory.create(db, project, cId);
cd.changeControl(user);
SubmitTypeRecord str = cd.submitTypeRecord();
if (!str.isOk()) {
@@ -169,11 +172,12 @@ public class MergeSuperSet {
}
private ChangeSet completeChangeSetIncludingTopics(
ReviewDb db, ChangeSet changes) throws MissingObjectException,
IncorrectObjectTypeException, IOException, OrmException {
ReviewDb db, ChangeSet changes, CurrentUser user)
throws MissingObjectException, IncorrectObjectTypeException, IOException,
OrmException {
Set<String> topicsTraversed = new HashSet<>();
boolean done = false;
ChangeSet newCs = completeChangeSetWithoutTopic(db, changes);
ChangeSet newCs = completeChangeSetWithoutTopic(db, changes, user);
while (!done) {
List<ChangeData> chgs = new ArrayList<>();
done = true;
@@ -187,7 +191,7 @@ public class MergeSuperSet {
}
}
changes = new ChangeSet(chgs);
newCs = completeChangeSetWithoutTopic(db, changes);
newCs = completeChangeSetWithoutTopic(db, changes, user);
}
return newCs;
}

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

@@ -14,6 +14,7 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST;
@@ -174,6 +175,10 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
public ChangeNotes create(ReviewDb db, Project.NameKey project,
Change.Id changeId) throws OrmException {
Change change = db.changes().get(changeId);
checkArgument(change.getProject().equals(project),
"passed project %s when creating ChangeNotes for %s, but actual"
+ " project is %s",
project, changeId, change.getProject());
// TODO: Throw NoSuchChangeException when the change is not found in the
// database
return new ChangeNotes(repoManager, migration, allUsersProvider, project,

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

@@ -618,6 +618,13 @@ public class ChangeData {
public ChangeControl changeControl(CurrentUser user) throws OrmException {
if (changeControl != null) {
CurrentUser oldUser = user;
// TODO(dborowitz): This is a hack; general CurrentUser equality would be
// better.
if (user.isIdentifiedUser() && oldUser.isIdentifiedUser()
&& user.getAccountId().equals(oldUser.getAccountId())) {
return changeControl;
}
throw new IllegalStateException(
"user already specified: " + changeControl.getUser());
}