opendev/gerrit
Code Issues Proposed changes

Support ChangeUpdates from InternalUser

Browse Source 
In a few codepaths, such as auto-abandoning open changes on deleted
projects, we might have ChangeMessages with no Account.Id, indicating
the message was written by the server. Support this in notedb by
comparing the author and committer on the meta commit. Fail during
parsing if we would need to store a null account anywhere other than
the ChangeMessage.

Change-Id: Ie3f087e5bd326747906a6d824f721ee97ab9f9c6
This commit is contained in:
Dave Borowitz
2016-02-19 16:28:18 -05:00
parent d8bce6c90b
commit 15cddfecc0
9 changed files with 113 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java
View File

@@ -20,7 +20,9 @@ import com.google.gerrit.reviewdb.client.Account;
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.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gwtorm.server.OrmException;
@@ -60,6 +62,10 @@ public abstract class AbstractChangeUpdate {
this.serverIdent = serverIdent;
this.anonymousCowardName = anonymousCowardName;
this.when = when;
checkArgument(
(ctl.getUser() instanceof IdentifiedUser)
|| (ctl.getUser() instanceof InternalUser),
"user must be IdentifiedUser or InternalUser: %s", ctl.getUser());
}
public ChangeNotes getChangeNotes() {
@@ -74,8 +80,8 @@ public abstract class AbstractChangeUpdate {
return when;
}
public IdentifiedUser getUser() {
return ctl.getUser().asIdentifiedUser();
public CurrentUser getUser() {
return ctl.getUser();
}
public PatchSet.Id getPatchSetId() {
@@ -87,6 +93,17 @@ public abstract class AbstractChangeUpdate {
this.psId = psId;
}
protected PersonIdent newAuthorIdent() {
CurrentUser u = getUser();
if (u instanceof IdentifiedUser) {
return ChangeNoteUtil.newIdent(u.asIdentifiedUser().getAccount(), when,
serverIdent, anonymousCowardName);
} else if (u instanceof InternalUser) {
return serverIdent;
}
throw new IllegalStateException();
}
protected PersonIdent newIdent(Account author, Date when) {
return ChangeNoteUtil.newIdent(author, when, serverIdent,
anonymousCowardName);

2
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java
View File

@@ -234,7 +234,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
protected CommitBuilder applyImpl(ObjectInserter ins)
throws OrmException, IOException {
CommitBuilder cb = new CommitBuilder();
cb.setAuthor(newIdent(getUser().getAccount(), when));
cb.setAuthor(newAuthorIdent());
cb.setCommitter(new PersonIdent(serverIdent, when));
cb.setMessage("Update draft comments");
AtomicBoolean removedAllComments = new AtomicBoolean();

20
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
View File

@@ -217,7 +217,9 @@ class ChangeNotesParser implements AutoCloseable {
}
Account.Id accountId = parseIdent(commit);
ownerId = accountId;
if (accountId != null) {
ownerId = accountId;
}
if (changeId == null) {
changeId = parseChangeId(commit);
@@ -339,6 +341,10 @@ class ChangeNotesParser implements AutoCloseable {
private void parsePatchSet(PatchSet.Id psId, ObjectId rev,
Account.Id accountId, Timestamp ts) throws ConfigInvalidException {
if (accountId == null) {
throw parseException(
"patch set %s requires an identified user as uploader", psId.get());
}
PatchSet ps = patchSets.get(psId);
if (ps == null) {
ps = new PatchSet(psId);
@@ -518,6 +524,10 @@ class ChangeNotesParser implements AutoCloseable {
private void parseApproval(PatchSet.Id psId, Account.Id accountId,
Timestamp ts, String line) throws ConfigInvalidException {
if (accountId == null) {
throw parseException(
"patch set %s requires an identified user as uploader", psId.get());
}
if (line.startsWith("-")) {
parseRemoveApproval(psId, accountId, line);
} else {
@@ -665,6 +675,14 @@ class ChangeNotesParser implements AutoCloseable {
private Account.Id parseIdent(RevCommit commit)
throws ConfigInvalidException {
// Check if the author name/email is the same as the committer name/email,
// i.e. was the server ident at the time this commit was made.
PersonIdent a = commit.getAuthorIdent();
PersonIdent c = commit.getCommitterIdent();
if (a.getName().equals(c.getName())
&& a.getEmailAddress().equals(c.getEmailAddress())) {
return null;
}
return parseIdent(commit.getAuthorIdent());
}

9
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java
View File

@@ -40,7 +40,9 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.StarredChange;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.PatchLineCommentsUtil;
import com.google.gerrit.server.git.ChainedReceiveCommands;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -81,6 +83,7 @@ public class ChangeRebuilder {
private final GitRepositoryManager repoManager;
private final ChangeControl.GenericFactory controlFactory;
private final IdentifiedUser.GenericFactory userFactory;
private final InternalUser.Factory internalUserFactory;
private final PatchListCache patchListCache;
private final ChangeUpdate.Factory updateFactory;
private final ChangeDraftUpdate.Factory draftUpdateFactory;
@@ -91,6 +94,7 @@ public class ChangeRebuilder {
GitRepositoryManager repoManager,
ChangeControl.GenericFactory controlFactory,
IdentifiedUser.GenericFactory userFactory,
InternalUser.Factory internalUserFactory,
PatchListCache patchListCache,
ChangeUpdate.Factory updateFactory,
ChangeDraftUpdate.Factory draftUpdateFactory,
@@ -99,6 +103,7 @@ public class ChangeRebuilder {
this.repoManager = repoManager;
this.controlFactory = controlFactory;
this.userFactory = userFactory;
this.internalUserFactory = internalUserFactory;
this.patchListCache = patchListCache;
this.updateFactory = updateFactory;
this.draftUpdateFactory = draftUpdateFactory;
@@ -172,7 +177,9 @@ public class ChangeRebuilder {
if (update != null) {
manager.add(update);
}
IdentifiedUser user = userFactory.create(Providers.of(db), e.who);
CurrentUser user = e.who != null
? userFactory.create(Providers.of(db), e.who)
: internalUserFactory.create();
update = updateFactory.create(
controlFactory.controlFor(db, change, user), e.when);
update.setPatchSetId(e.psId);

2
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
View File

@@ -479,7 +479,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
protected CommitBuilder applyImpl(ObjectInserter ins)
throws OrmException, IOException {
CommitBuilder cb = new CommitBuilder();
cb.setAuthor(newIdent(getUser().getAccount(), when));
cb.setAuthor(newAuthorIdent());
cb.setCommitter(new PersonIdent(serverIdent, when));
int ps = psId != null ? psId.get() : getChange().currentPatchSetId().get();

6
gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
View File

@@ -29,8 +29,10 @@ import com.google.gerrit.reviewdb.client.PatchLineComment;
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.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.CapabilityControl;
@@ -88,6 +90,7 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
protected InMemoryRepository repo;
protected InMemoryRepositoryManager repoManager;
protected PersonIdent serverIdent;
protected InternalUser internalUser;
protected Project.NameKey project;
protected RevWalk rw;
protected TestRepository<InMemoryRepository> tr;
@@ -158,6 +161,7 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
changeOwner = userFactory.create(co.getId());
otherUser = userFactory.create(ou.getId());
otherUserId = otherUser.getAccountId();
internalUser = new InternalUser(null);
}
private void setTimeForTesting() {
@@ -180,7 +184,7 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
return c;
}
protected ChangeUpdate newUpdate(Change c, IdentifiedUser user)
protected ChangeUpdate newUpdate(Change c, CurrentUser user)
throws Exception {
ChangeUpdate update = TestChanges.newUpdate(
injector, repoManager, MIGRATION, c, allUsers, user);

39
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
View File

@@ -381,6 +381,39 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
+ "Groups: d,e,f\n");
}
@Test
public void parseServerIdent() throws Exception {
String msg = "Update change\n"
+ "\n"
+ "Patch-set: 1\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Subject: Change subject\n";
assertParseSucceeds(msg);
assertParseSucceeds(writeCommit(msg, serverIdent));
msg = "Update change\n"
+ "\n"
+ "With a message."
+ "\n"
+ "Patch-set: 1\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Subject: Change subject\n";
assertParseSucceeds(msg);
assertParseSucceeds(writeCommit(msg, serverIdent));
msg = "Update change\n"
+ "\n"
+ "Patch-set: 1\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Subject: Change subject\n"
+ "Label: Label1=+1\n";
assertParseSucceeds(msg);
assertParseFails(writeCommit(msg, serverIdent));
}
private RevCommit writeCommit(String body) throws Exception {
return writeCommit(body, ChangeNoteUtil.newIdent(
changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent,
@@ -404,7 +437,11 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
}
private void assertParseSucceeds(String body) throws Exception {
try (ChangeNotesParser parser = newParser(writeCommit(body))) {
assertParseSucceeds(writeCommit(body));
}
private void assertParseSucceeds(RevCommit commit) throws Exception {
try (ChangeNotesParser parser = newParser(commit)) {
parser.parseAll();
}
}

16
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@@ -1702,6 +1702,22 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(notes.getComments()).hasSize(2);
}
@Test
public void updateWithServerIdent() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, internalUser);
update.setChangeMessage("A message.");
update.commit();
ChangeMessage msg = Iterables.getLast(newNotes(c).getChangeMessages());
assertThat(msg.getMessage()).isEqualTo("A message.");
assertThat(msg.getAuthor()).isNull();
update = newUpdate(c, internalUser);
exception.expect(UnsupportedOperationException.class);
update.putApproval("Code-Review", (short) 1);
}
private String readNote(ChangeNotes notes, ObjectId noteId) throws Exception {
ObjectId dataId = notes.getNoteMap().getNote(noteId).getData();
return new String(

12
gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java
View File

@@ -27,7 +27,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeDraftUpdate;
@@ -88,14 +88,14 @@ public class TestChanges {
public static ChangeUpdate newUpdate(Injector injector,
GitRepositoryManager repoManager, NotesMigration migration, Change c,
final AllUsersName allUsers, final IdentifiedUser user)
final AllUsersName allUsers, final CurrentUser user)
throws Exception {
ChangeUpdate update = injector.createChildInjector(new FactoryModule() {
@Override
public void configure() {
factory(ChangeUpdate.Factory.class);
factory(ChangeDraftUpdate.Factory.class);
bind(IdentifiedUser.class).toInstance(user);
bind(CurrentUser.class).toInstance(user);
}
}).getInstance(ChangeUpdate.Factory.class).create(
stubChangeControl(repoManager, migration, c, allUsers, user),
@@ -112,8 +112,8 @@ public class TestChanges {
// first patch set, so create one.
try (Repository repo = repoManager.openRepository(c.getProject())) {
TestRepository<Repository> tr = new TestRepository<>(repo);
PersonIdent ident =
user.newCommitterIdent(update.getWhen(), TimeZone.getDefault());
PersonIdent ident = user.asIdentifiedUser()
.newCommitterIdent(update.getWhen(), TimeZone.getDefault());
TestRepository<Repository>.CommitBuilder cb = tr.commit()
.author(ident)
.committer(ident)
@@ -132,7 +132,7 @@ public class TestChanges {
private static ChangeControl stubChangeControl(
GitRepositoryManager repoManager, NotesMigration migration,
Change c, AllUsersName allUsers,
IdentifiedUser user) throws OrmException {
CurrentUser user) throws OrmException {
ChangeControl ctl = EasyMock.createMock(ChangeControl.class);
expect(ctl.getChange()).andStubReturn(c);
expect(ctl.getProject()).andStubReturn(new Project(c.getProject()));