opendev
/
gerrit
Code Issues Proposed changes

Support ChangeUpdates without providing a CurrentUser 

We want to thin out the dependency stack of ChangeRebuilderImpl in our
custom online rebuilding implementation. Using full ChangeControls and
IdentifiedUsers brings in way more than we need to just create author
idents containing the user's full name.

Rework AbstractChangeUpdate to just take an Account.Id and
prepopulated PersonIdent, and teach ChangeRebuilderImpl to use that
path.

Change-Id: I79af43ff21fbd27ad682134d37fae75f043a586a
This commit is contained in:
Dave Borowitz 2016-04-01 16:37:02 -04:00
parent 5a18df0286
commit ebdba50b40
10 changed files with 217 additions and 147 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -72,10 +72,9 @@ public class ChangeMessagesUtil {
public void addChangeMessage(ReviewDb db, ChangeUpdate update,
ChangeMessage changeMessage) throws OrmException {
checkState(
Objects.equals(changeMessage.getAuthor(),
update.getUser().getAccountId()),
"cannot store change message of %s in update of %s",
changeMessage.getAuthor(), update.getUser().getAccountId());
Objects.equals(changeMessage.getAuthor(), update.getAccountId()),
"cannot store change message by %s in update by %s",
changeMessage.getAuthor(), update.getAccountId());
update.setChangeMessage(changeMessage.getMessage());
db.changeMessages().insert(Collections.singleton(changeMessage));
}

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

@ -96,7 +96,7 @@ public class PatchSetUtil {
PatchSet ps = new PatchSet(psId);
ps.setRevision(new RevId(commit.name()));
ps.setUploader(update.getUser().getAccountId());
ps.setUploader(update.getAccountId());
ps.setCreatedOn(new Timestamp(update.getWhen().getTime()));
ps.setDraft(draft);
ps.setGroups(groups);

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

@ -93,7 +93,7 @@ public class SetHashtagsOp extends BatchUpdate.Op {
}
change = ctx.getChange();
ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
ChangeNotes notes = update.getChangeNotes().load();
ChangeNotes notes = update.getNotes().load();
Set<String> existingHashtags = notes.getHashtags();
Set<String> updated = new HashSet<>();

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

@ -15,6 +15,7 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
@ -23,7 +24,6 @@ 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;
@ -41,63 +41,69 @@ import java.util.Date;
/** A single delta related to a specific patch-set of a change. */
public abstract class AbstractChangeUpdate {
protected final NotesMigration migration;
protected final GitRepositoryManager repoManager;
protected final ChangeControl ctl;
protected final String anonymousCowardName;
protected final ChangeNoteUtil noteUtil;
protected final String anonymousCowardName;
protected final ChangeNotes notes;
protected final Account.Id accountId;
protected final PersonIdent authorIdent;
protected final Date when;
private final PersonIdent serverIdent;
protected PatchSet.Id psId;
private ObjectId result;
protected AbstractChangeUpdate(NotesMigration migration,
GitRepositoryManager repoManager,
protected AbstractChangeUpdate(
NotesMigration migration,
ChangeControl ctl,
PersonIdent serverIdent,
String anonymousCowardName,
ChangeNoteUtil noteUtil,
Date when) {
this(
migration,
noteUtil,
serverIdent,
anonymousCowardName,
ctl.getNotes(),
accountId(ctl.getUser()),
ident(noteUtil, serverIdent, anonymousCowardName, ctl.getUser(), when),
when);
}
protected AbstractChangeUpdate(
NotesMigration migration,
ChangeNoteUtil noteUtil,
PersonIdent serverIdent,
String anonymousCowardName,
ChangeNotes notes,
Account.Id accountId,
PersonIdent authorIdent,
Date when) {
this.migration = migration;
this.repoManager = repoManager;
this.ctl = ctl;
this.serverIdent = serverIdent;
this.anonymousCowardName = anonymousCowardName;
this.noteUtil = noteUtil;
this.serverIdent = new PersonIdent(serverIdent, when);
this.anonymousCowardName = anonymousCowardName;
this.notes = notes;
this.accountId = accountId;
this.authorIdent = authorIdent;
this.when = when;
}
private static void checkUserType(CurrentUser user) {
checkArgument(
(ctl.getUser() instanceof IdentifiedUser)
|| (ctl.getUser() instanceof InternalUser),
"user must be IdentifiedUser or InternalUser: %s", ctl.getUser());
(user instanceof IdentifiedUser) || (user instanceof InternalUser),
"user must be IdentifiedUser or InternalUser: %s", user);
}
public ChangeNotes getChangeNotes() {
return ctl.getNotes();
private static Account.Id accountId(CurrentUser u) {
checkUserType(u);
return (u instanceof IdentifiedUser) ? u.getAccountId() : null;
}
public Change getChange() {
return ctl.getChange();
}
public Date getWhen() {
return when;
}
public CurrentUser getUser() {
return ctl.getUser();
}
public PatchSet.Id getPatchSetId() {
return psId;
}
public void setPatchSetId(PatchSet.Id psId) {
checkArgument(psId == null || psId.getParentKey().equals(ctl.getId()));
this.psId = psId;
}
private PersonIdent newAuthorIdent() {
CurrentUser u = getUser();
private static PersonIdent ident(ChangeNoteUtil noteUtil,
PersonIdent serverIdent, String anonymousCowardName, CurrentUser u,
Date when) {
checkUserType(u);
if (u instanceof IdentifiedUser) {
return noteUtil.newIdent(u.asIdentifiedUser().getAccount(), when,
serverIdent, anonymousCowardName);
@ -107,6 +113,42 @@ public abstract class AbstractChangeUpdate {
throw new IllegalStateException();
}
public Change.Id getId() {
return notes.getChangeId();
}
public ChangeNotes getNotes() {
return notes;
}
public Change getChange() {
return notes.getChange();
}
public Date getWhen() {
return when;
}
public PatchSet.Id getPatchSetId() {
return psId;
}
public void setPatchSetId(PatchSet.Id psId) {
checkArgument(psId == null || psId.getParentKey().equals(getId()));
this.psId = psId;
}
public Account.Id getAccountId() {
checkState(accountId != null,
"author identity for %s is not from an IdentifiedUser: %s",
getClass().getSimpleName(), authorIdent.toExternalString());
return accountId;
}
public Account.Id getNullableAccountId() {
return accountId;
}
protected PersonIdent newIdent(Account author, Date when) {
return noteUtil.newIdent(author, when, serverIdent, anonymousCowardName);
}
@ -145,7 +187,7 @@ public abstract class AbstractChangeUpdate {
result = z;
return z; // Impl intends to delete the ref.
}
cb.setAuthor(newAuthorIdent());
cb.setAuthor(authorIdent);
cb.setCommitter(new PersonIdent(serverIdent, when));
if (!curr.equals(z)) {
cb.setParentId(curr);

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

@ -17,7 +17,6 @@ package com.google.gerrit.server.notedb;
import static com.google.common.base.MoreObjects.firstNonNull;
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 org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import com.google.auto.value.AutoValue;
@ -28,11 +27,8 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gwtorm.server.OrmException;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
@ -63,7 +59,8 @@ import java.util.Set;
*/
public class ChangeDraftUpdate extends AbstractChangeUpdate {
public interface Factory {
ChangeDraftUpdate create(ChangeControl ctl, Date when);
ChangeDraftUpdate create(ChangeNotes notes, Account.Id accountId,
PersonIdent authorIdent, Date when);
}
@AutoValue
@ -77,7 +74,6 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
}
private final AllUsersName draftsProject;
private final Account.Id accountId;
// TODO: can go back to a list?
private Map<Key, PatchLineComment> put;
@ -87,20 +83,16 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
private ChangeDraftUpdate(
@GerritPersonIdent PersonIdent serverIdent,
@AnonymousCowardName String anonymousCowardName,
GitRepositoryManager repoManager,
NotesMigration migration,
AllUsersName allUsers,
ChangeNoteUtil noteUtil,
@Assisted ChangeControl ctl,
@Assisted ChangeNotes notes,
@Assisted Account.Id accountId,
@Assisted PersonIdent authorIdent,
@Assisted Date when) {
super(migration, repoManager, ctl, serverIdent, anonymousCowardName,
noteUtil, when);
super(migration, noteUtil, serverIdent, anonymousCowardName, notes,
accountId, authorIdent, when);
this.draftsProject = allUsers;
checkState(ctl.getUser().isIdentifiedUser(),
"Current user must be identified");
IdentifiedUser user = ctl.getUser().asIdentifiedUser();
this.accountId = user.getAccountId();
this.put = new HashMap<>();
this.delete = new HashSet<>();
}
@ -176,12 +168,12 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
// already parsed the revision notes. We can reuse them as long as the ref
// hasn't advanced.
DraftCommentNotes draftNotes =
ctl.getNotes().load().getDraftCommentNotes();
getNotes().load().getDraftCommentNotes();
if (draftNotes != null) {
ObjectId idFromNotes =
firstNonNull(draftNotes.getRevision(), ObjectId.zeroId());
if (idFromNotes.equals(curr)) {
return checkNotNull(ctl.getNotes().revisionNoteMap);
return checkNotNull(getNotes().revisionNoteMap);
}
}
}
@ -194,7 +186,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
// Even though reading from changes might not be enabled, we need to
// parse any existing revision notes so we can merge them.
return RevisionNoteMap.parse(
noteUtil, ctl.getId(), rw.getObjectReader(), noteMap, true);
noteUtil, getId(), rw.getObjectReader(), noteMap, true);
}
@Override
@ -221,7 +213,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
@Override
protected String getRefName() {
return RefNames.refsDraftComments(accountId, ctl.getId());
return RefNames.refsDraftComments(accountId, getId());
}
@Override

108
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java
View File

@ -33,6 +33,7 @@ import com.google.common.collect.Multimap;
import com.google.common.collect.Ordering;
import com.google.common.collect.Sets;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
@ -41,24 +42,24 @@ import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
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.GerritPersonIdent;
import com.google.gerrit.server.PatchLineCommentsUtil;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.git.ChainedReceiveCommands;
import com.google.gerrit.server.patch.PatchListCache;
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.schema.DisabledChangesReviewDbWrapper;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
import com.google.inject.util.Providers;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
@ -68,6 +69,7 @@ import java.io.IOException;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.List;
import java.util.Objects;
import java.util.Set;
@ -92,37 +94,40 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
*/
private static final long MAX_DELTA_MS = SECONDS.toMillis(1);
private final ChangeControl.GenericFactory controlFactory;
private final IdentifiedUser.GenericFactory userFactory;
private final InternalUser.Factory internalUserFactory;
private final PatchListCache patchListCache;
private final ChangeNotes.Factory notesFactory;
private final ChangeUpdate.Factory updateFactory;
private final AccountCache accountCache;
private final ChangeDraftUpdate.Factory draftUpdateFactory;
private final NoteDbUpdateManager.Factory updateManagerFactory;
private final ChangeNotes.Factory notesFactory;
private final ChangeNoteUtil changeNoteUtil;
private final ChangeUpdate.Factory updateFactory;
private final NoteDbUpdateManager.Factory updateManagerFactory;
private final PatchListCache patchListCache;
private final PersonIdent serverIdent;
private final ProjectCache projectCache;
private final String anonymousCowardName;
@Inject
ChangeRebuilderImpl(SchemaFactory<ReviewDb> schemaFactory,
ChangeControl.GenericFactory controlFactory,
IdentifiedUser.GenericFactory userFactory,
InternalUser.Factory internalUserFactory,
PatchListCache patchListCache,
ChangeNotes.Factory notesFactory,
ChangeUpdate.Factory updateFactory,
AccountCache accountCache,
ChangeDraftUpdate.Factory draftUpdateFactory,
ChangeNotes.Factory notesFactory,
ChangeNoteUtil changeNoteUtil,
ChangeUpdate.Factory updateFactory,
NoteDbUpdateManager.Factory updateManagerFactory,
ChangeNoteUtil changeNoteUtil) {
PatchListCache patchListCache,
@GerritPersonIdent PersonIdent serverIdent,
@Nullable ProjectCache projectCache,
@AnonymousCowardName String anonymousCowardName) {
super(schemaFactory);
this.controlFactory = controlFactory;
this.userFactory = userFactory;
this.internalUserFactory = internalUserFactory;
this.patchListCache = patchListCache;
this.notesFactory = notesFactory;
this.updateFactory = updateFactory;
this.accountCache = accountCache;
this.draftUpdateFactory = draftUpdateFactory;
this.updateManagerFactory = updateManagerFactory;
this.notesFactory = notesFactory;
this.changeNoteUtil = changeNoteUtil;
this.updateFactory = updateFactory;
this.updateManagerFactory = updateManagerFactory;
this.patchListCache = patchListCache;
this.serverIdent = serverIdent;
this.projectCache = projectCache;
this.anonymousCowardName = anonymousCowardName;
}
@Override
@ -174,8 +179,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
}
private void buildUpdates(NoteDbUpdateManager manager, ChangeBundle bundle)
throws NoSuchChangeException, IOException, OrmException,
ConfigInvalidException {
throws IOException, OrmException, ConfigInvalidException {
Change change = new Change(bundle.getChange());
// We will rebuild all events, except for draft comments, in buckets based
// on author and timestamp.
@ -251,16 +255,25 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
}
private void flushEventsToUpdate(NoteDbUpdateManager manager,
EventList<Event> events, Change change)
throws NoSuchChangeException, OrmException, IOException {
EventList<Event> events, Change change) throws OrmException, IOException {
if (events.isEmpty()) {
return;
}
Comparator<String> labelNameComparator;
if (projectCache != null) {
labelNameComparator = projectCache.get(change.getProject())
.getLabelTypes().nameComparator();
} else {
// No project cache available, bail and use natural ordering; there's no
// semantic difference anyway difference.
labelNameComparator = Ordering.natural();
}
ChangeUpdate update = updateFactory.create(
controlFactory.controlFor(
notesFactory.createWithAutoRebuildingDisabled(change),
events.getUser()),
events.getWhen());
notesFactory.createWithAutoRebuildingDisabled(change),
events.getAccountId(),
events.newAuthorIdent(),
events.getWhen(),
labelNameComparator);
update.setAllowWriteToNewRef(true);
update.setPatchSetId(events.getPatchSetId());
for (Event e : events) {
@ -272,14 +285,14 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
private void flushEventsToDraftUpdate(NoteDbUpdateManager manager,
EventList<PatchLineCommentEvent> events, Change change)
throws NoSuchChangeException, OrmException {
throws OrmException {
if (events.isEmpty()) {
return;
}
ChangeDraftUpdate update = draftUpdateFactory.create(
controlFactory.controlFor(
notesFactory.createWithAutoRebuildingDisabled(change),
events.getUser()),
notesFactory.createWithAutoRebuildingDisabled(change),
events.getAccountId(),
events.newAuthorIdent(),
events.getWhen());
update.setPatchSetId(events.getPatchSetId());
for (PatchLineCommentEvent e : events) {
@ -389,9 +402,9 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
checkState(when.getTime() - update.getWhen().getTime() <= MAX_WINDOW_MS,
"event at %s outside update window starting at %s",
when, update.getWhen());
checkState(Objects.equals(update.getUser().getAccountId(), who),
checkState(Objects.equals(update.getNullableAccountId(), who),
"cannot apply event by %s to update by %s",
who, update.getUser().getAccountId());
who, update.getNullableAccountId());
}
/**
@ -478,18 +491,23 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
return id;
}
CurrentUser getUser() {
Account.Id getAccountId() {
Account.Id id = get(0).who;
for (int i = 1; i < size(); i++) {
checkState(Objects.equals(id, get(i).who),
"mismatched users in EventList: %s != %s", id, get(i).who);
}
return id;
}
PersonIdent newAuthorIdent() {
Account.Id id = getAccountId();
if (id == null) {
return internalUserFactory.create();
return new PersonIdent(serverIdent, getWhen());
}
// db is only used by IdentifiedUser to look up project watches, which are
// not needed for rebuilding changes.
return userFactory.create(Providers.of((ReviewDb) null), id);
return changeNoteUtil.newIdent(
accountCache.get(id).getAccount(), getWhen(), serverIdent,
anonymousCowardName);
}
}

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

@ -33,26 +33,23 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
import com.google.common.base.Joiner;
import com.google.common.base.Optional;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Ordering;
import com.google.common.collect.Table;
import com.google.common.collect.TreeBasedTable;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.LabelVote;
@ -71,9 +68,11 @@ import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.Date;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
@ -94,6 +93,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
public interface Factory {
ChangeUpdate create(ChangeControl ctl);
ChangeUpdate create(ChangeControl ctl, Date when);
ChangeUpdate create(ChangeNotes notes, @Nullable Account.Id accountId,
PersonIdent authorIdent, Date when,
Comparator<String> labelNameComparator);
@VisibleForTesting
ChangeUpdate create(ChangeControl ctl, Date when,
Comparator<String> labelNameComparator);
@ -104,7 +107,8 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private final NoteDbUpdateManager.Factory updateManagerFactory;
private final Table<String, Account.Id, Optional<Short>> approvals;
private final Map<Account.Id, ReviewerStateInternal> reviewers;
private final Map<Account.Id, ReviewerStateInternal> reviewers = new LinkedHashMap<>();
private final List<PatchLineComment> comments = new ArrayList<>();
private String commitSubject;
private String subject;
@ -113,7 +117,6 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private Change.Status status;
private List<SubmitRecord> submitRecords;
private String submissionId;
private List<PatchLineComment> comments;
private String topic;
private String commit;
private Set<String> hashtags;
@ -129,7 +132,6 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private ChangeUpdate(
@GerritPersonIdent PersonIdent serverIdent,
@AnonymousCowardName String anonymousCowardName,
GitRepositoryManager repoManager,
NotesMigration migration,
AccountCache accountCache,
NoteDbUpdateManager.Factory updateManagerFactory,
@ -137,7 +139,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
ProjectCache projectCache,
@Assisted ChangeControl ctl,
ChangeNoteUtil noteUtil) {
this(serverIdent, anonymousCowardName, repoManager, migration, accountCache,
this(serverIdent, anonymousCowardName, migration, accountCache,
updateManagerFactory, draftUpdateFactory,
projectCache, ctl, serverIdent.getWhen(), noteUtil);
}
@ -146,7 +148,6 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private ChangeUpdate(
@GerritPersonIdent PersonIdent serverIdent,
@AnonymousCowardName String anonymousCowardName,
GitRepositoryManager repoManager,
NotesMigration migration,
AccountCache accountCache,
NoteDbUpdateManager.Factory updateManagerFactory,
@ -155,7 +156,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
@Assisted ChangeControl ctl,
@Assisted Date when,
ChangeNoteUtil noteUtil) {
this(serverIdent, anonymousCowardName, repoManager, migration, accountCache,
this(serverIdent, anonymousCowardName, migration, accountCache,
updateManagerFactory, draftUpdateFactory, ctl,
when,
projectCache.get(getProjectName(ctl)).getLabelTypes().nameComparator(),
@ -166,11 +167,15 @@ public class ChangeUpdate extends AbstractChangeUpdate {
return ctl.getProject().getNameKey();
}
private static Table<String, Account.Id, Optional<Short>> approvals(
Comparator<String> nameComparator) {
return TreeBasedTable.create(nameComparator, ReviewDbUtil.intKeyOrdering());
}
@AssistedInject
private ChangeUpdate(
@GerritPersonIdent PersonIdent serverIdent,
@AnonymousCowardName String anonymousCowardName,
GitRepositoryManager repoManager,
NotesMigration migration,
AccountCache accountCache,
NoteDbUpdateManager.Factory updateManagerFactory,
@ -179,22 +184,34 @@ public class ChangeUpdate extends AbstractChangeUpdate {
@Assisted Date when,
@Assisted Comparator<String> labelNameComparator,
ChangeNoteUtil noteUtil) {
super(migration, repoManager, ctl, serverIdent,
super(migration, ctl, serverIdent,
anonymousCowardName, noteUtil, when);
this.accountCache = accountCache;
this.draftUpdateFactory = draftUpdateFactory;
this.updateManagerFactory = updateManagerFactory;
this.approvals = approvals(labelNameComparator);
}
this.approvals = TreeBasedTable.create(
labelNameComparator,
Ordering.natural().onResultOf(new Function<Account.Id, Integer>() {
@Override
public Integer apply(Account.Id in) {
return in.get();
}
}));
this.reviewers = Maps.newLinkedHashMap();
this.comments = Lists.newArrayList();
@AssistedInject
private ChangeUpdate(
@GerritPersonIdent PersonIdent serverIdent,
@AnonymousCowardName String anonymousCowardName,
NotesMigration migration,
AccountCache accountCache,
NoteDbUpdateManager.Factory updateManagerFactory,
ChangeDraftUpdate.Factory draftUpdateFactory,
ChangeNoteUtil noteUtil,
@Assisted ChangeNotes notes,
@Assisted @Nullable Account.Id accountId,
@Assisted PersonIdent authorIdent,
@Assisted Date when,
@Assisted Comparator<String> labelNameComparator) {
super(migration, noteUtil, serverIdent, anonymousCowardName, notes,
accountId, authorIdent, when);
this.accountCache = accountCache;
this.draftUpdateFactory = draftUpdateFactory;
this.updateManagerFactory = updateManagerFactory;
this.approvals = approvals(labelNameComparator);
}
public ObjectId commit() throws IOException, OrmException {
@ -203,13 +220,13 @@ public class ChangeUpdate extends AbstractChangeUpdate {
updateManager.add(this);
NoteDbChangeState.applyDelta(
getChange(),
updateManager.stage().get(ctl.getId()));
updateManager.stage().get(getId()));
updateManager.execute();
return getResult();
}
public void setChangeId(String changeId) {
String old = ctl.getChange().getKey().get();
String old = getChange().getKey().get();
checkArgument(old.equals(changeId),
"The Change-Id was already set to %s, so we cannot set this Change-Id: %s",
old, changeId);
@ -231,7 +248,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
}
public void putApproval(String label, short value) {
putApprovalFor(getUser().getAccountId(), label, value);
putApprovalFor(getAccountId(), label, value);
}
public void putApprovalFor(Account.Id reviewer, String label, short value) {
@ -239,7 +256,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
}
public void removeApproval(String label) {
removeApprovalFor(getUser().getAccountId(), label);
removeApprovalFor(getAccountId(), label);
}
public void removeApprovalFor(Account.Id reviewer, String label) {
@ -304,16 +321,17 @@ public class ChangeUpdate extends AbstractChangeUpdate {
@VisibleForTesting
ChangeDraftUpdate createDraftUpdateIfNull() {
if (draftUpdate == null) {
draftUpdate = draftUpdateFactory.create(ctl, when);
draftUpdate =
draftUpdateFactory.create(getNotes(), accountId, authorIdent, when);
}
return draftUpdate;
}
private void verifyComment(PatchLineComment c) {
checkArgument(c.getRevId() != null, "RevId required for comment: %s", c);
checkArgument(c.getAuthor().equals(getUser().getAccountId()),
checkArgument(c.getAuthor().equals(getAccountId()),
"The author for the following comment does not match the author of"
+ " this ChangeDraftUpdate (%s): %s", getUser().getAccountId(), c);
+ " this ChangeDraftUpdate (%s): %s", getAccountId(), c);
}
@ -403,16 +421,16 @@ public class ChangeUpdate extends AbstractChangeUpdate {
// parsed the revision notes. We can reuse them as long as the ref hasn't
// advanced.
ObjectId idFromNotes =
firstNonNull(ctl.getNotes().load().getRevision(), ObjectId.zeroId());
firstNonNull(getNotes().load().getRevision(), ObjectId.zeroId());
if (idFromNotes.equals(curr)) {
return checkNotNull(ctl.getNotes().revisionNoteMap);
return checkNotNull(getNotes().revisionNoteMap);
}
}
NoteMap noteMap = NoteMap.read(rw.getObjectReader(), rw.parseCommit(curr));
// Even though reading from changes might not be enabled, we need to
// parse any existing revision notes so we can merge them.
return RevisionNoteMap.parse(
noteUtil, ctl.getId(), rw.getObjectReader(), noteMap, false);
noteUtil, getId(), rw.getObjectReader(), noteMap, false);
}
private void checkComments(Map<RevId, RevisionNote> existingNotes,
@ -455,7 +473,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
@Override
protected String getRefName() {
return ChangeNoteUtil.changeRefName(ctl.getId());
return ChangeNoteUtil.changeRefName(getId());
}
@Override
@ -527,7 +545,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
c.getRowKey(), c.getValue().get()).formatWithEquals());
}
Account.Id id = c.getColumnKey();
if (!id.equals(ctl.getUser().getAccountId())) {
if (!id.equals(getAccountId())) {
addIdent(msg.append(' '), id);
}
msg.append('\n');
@ -584,7 +602,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
@Override
protected Project.NameKey getProjectName() {
return getProjectName(ctl);
return getChange().getProject();
}
@Override

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

@ -207,6 +207,7 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
protected ChangeUpdate newUpdate(Change c, CurrentUser user)
throws Exception {
ChangeUpdate update = TestChanges.newUpdate(injector, c, user);
update.setPatchSetId(c.currentPatchSetId());
update.setAllowWriteToNewRef(true);
return update;
}

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

@ -1760,7 +1760,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(msg.getAuthor()).isNull();
update = newUpdate(c, internalUser);
exception.expect(UnsupportedOperationException.class);
exception.expect(IllegalStateException.class);
update.putApproval("Code-Review", (short) 1);
}

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

@ -101,7 +101,7 @@ public class TestChanges {
user),
TimeUtil.nowTs(), Ordering.<String> natural());
ChangeNotes notes = update.getChangeNotes();
ChangeNotes notes = update.getNotes();
boolean hasPatchSets = notes.getPatchSets() != null
&& !notes.getPatchSets().isEmpty();
NotesMigration migration = injector.getInstance(NotesMigration.class);