Refuse to rebuild changes with no patch sets
The implementation of NoteDb requires every commit in the meta graph to have a patch set ID. The implementation of ChangeRebuildImpl was unintentionally setting the patch set to the current patch set of the change, even if there were no actual PatchSet entities in ReviewDb to back that up. Also unintentionally, parsing this result back would succeed, and even produce the correct Change fields to make ChangeBundle's diff happy. Changes in this state are really corrupt, though, and can't be shown in the UI, among other things. We should just ignore them when converting to NoteDb, which is equivalent to obliterating them. Change-Id: I376d89770fb3b3870e7111e5170882a14419e0a2
This commit is contained in:
@@ -59,6 +59,7 @@ import com.google.gerrit.server.git.RepoRefCache;
|
|||||||
import com.google.gerrit.server.git.UpdateException;
|
import com.google.gerrit.server.git.UpdateException;
|
||||||
import com.google.gerrit.server.notedb.ChangeBundle;
|
import com.google.gerrit.server.notedb.ChangeBundle;
|
||||||
import com.google.gerrit.server.notedb.ChangeNotes;
|
import com.google.gerrit.server.notedb.ChangeNotes;
|
||||||
|
import com.google.gerrit.server.notedb.ChangeRebuilder.NoPatchSetsException;
|
||||||
import com.google.gerrit.server.notedb.NoteDbChangeState;
|
import com.google.gerrit.server.notedb.NoteDbChangeState;
|
||||||
import com.google.gerrit.server.notedb.NoteDbUpdateManager;
|
import com.google.gerrit.server.notedb.NoteDbUpdateManager;
|
||||||
import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper;
|
import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper;
|
||||||
@@ -954,6 +955,22 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
|
|||||||
assertChangeUpToDate(false, id);
|
assertChangeUpToDate(false, id);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
public void rebuildChangeWithNoPatchSets() throws Exception {
|
||||||
|
PushOneCommit.Result r = createChange();
|
||||||
|
Change.Id id = r.getPatchSetId().getParentKey();
|
||||||
|
db.changes().beginTransaction(id);
|
||||||
|
try {
|
||||||
|
db.patchSets().delete(db.patchSets().byChange(id));
|
||||||
|
db.commit();
|
||||||
|
} finally {
|
||||||
|
db.rollback();
|
||||||
|
}
|
||||||
|
|
||||||
|
exception.expect(NoPatchSetsException.class);
|
||||||
|
checker.rebuildAndCheckChanges(id);
|
||||||
|
}
|
||||||
|
|
||||||
private void assertChangesReadOnly(RestApiException e) throws Exception {
|
private void assertChangesReadOnly(RestApiException e) throws Exception {
|
||||||
Throwable cause = e.getCause();
|
Throwable cause = e.getCause();
|
||||||
assertThat(cause).isInstanceOf(UpdateException.class);
|
assertThat(cause).isInstanceOf(UpdateException.class);
|
||||||
|
|||||||
@@ -32,6 +32,15 @@ import java.io.IOException;
|
|||||||
import java.util.concurrent.Callable;
|
import java.util.concurrent.Callable;
|
||||||
|
|
||||||
public abstract class ChangeRebuilder {
|
public abstract class ChangeRebuilder {
|
||||||
|
public static class NoPatchSetsException extends OrmException {
|
||||||
|
private static final long serialVersionUID = 1L;
|
||||||
|
|
||||||
|
NoPatchSetsException(Change.Id changeId) {
|
||||||
|
super("Change " + changeId
|
||||||
|
+ " cannot be rebuilt because it has no patch sets");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
private final SchemaFactory<ReviewDb> schemaFactory;
|
private final SchemaFactory<ReviewDb> schemaFactory;
|
||||||
|
|
||||||
protected ChangeRebuilder(SchemaFactory<ReviewDb> schemaFactory) {
|
protected ChangeRebuilder(SchemaFactory<ReviewDb> schemaFactory) {
|
||||||
|
|||||||
@@ -287,6 +287,8 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
|
|||||||
for (Change.Id changeId : allChanges.get(project)) {
|
for (Change.Id changeId : allChanges.get(project)) {
|
||||||
try {
|
try {
|
||||||
buildUpdates(manager, ChangeBundle.fromReviewDb(db, changeId));
|
buildUpdates(manager, ChangeBundle.fromReviewDb(db, changeId));
|
||||||
|
} catch (NoPatchSetsException e) {
|
||||||
|
log.warn(e.getMessage());
|
||||||
} catch (Throwable t) {
|
} catch (Throwable t) {
|
||||||
log.error("Failed to rebuild change " + changeId, t);
|
log.error("Failed to rebuild change " + changeId, t);
|
||||||
ok = false;
|
ok = false;
|
||||||
@@ -304,6 +306,10 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
|
|||||||
throws IOException, OrmException {
|
throws IOException, OrmException {
|
||||||
manager.setCheckExpectedState(false);
|
manager.setCheckExpectedState(false);
|
||||||
Change change = new Change(bundle.getChange());
|
Change change = new Change(bundle.getChange());
|
||||||
|
if (bundle.getPatchSets().isEmpty()) {
|
||||||
|
throw new NoPatchSetsException(change.getId());
|
||||||
|
}
|
||||||
|
|
||||||
PatchSet.Id currPsId = change.currentPatchSetId();
|
PatchSet.Id currPsId = change.currentPatchSetId();
|
||||||
// We will rebuild all events, except for draft comments, in buckets based
|
// We will rebuild all events, except for draft comments, in buckets based
|
||||||
// on author and timestamp.
|
// on author and timestamp.
|
||||||
|
|||||||
Reference in New Issue
Block a user