From ebdba50b403b2fe7cc29c3a462538442c58074ba Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 1 Apr 2016 16:37:02 -0400 Subject: [PATCH] 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 --- .../gerrit/server/ChangeMessagesUtil.java | 7 +- .../google/gerrit/server/PatchSetUtil.java | 2 +- .../gerrit/server/change/SetHashtagsOp.java | 2 +- .../server/notedb/AbstractChangeUpdate.java | 120 ++++++++++++------ .../server/notedb/ChangeDraftUpdate.java | 30 ++--- .../server/notedb/ChangeRebuilderImpl.java | 108 +++++++++------- .../gerrit/server/notedb/ChangeUpdate.java | 90 +++++++------ .../notedb/AbstractChangeNotesTest.java | 1 + .../gerrit/server/notedb/ChangeNotesTest.java | 2 +- .../google/gerrit/testutil/TestChanges.java | 2 +- 10 files changed, 217 insertions(+), 147 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeMessagesUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeMessagesUtil.java index 07d4326ad0..af953e0c05 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeMessagesUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeMessagesUtil.java @@ -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)); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java index 1bbd735f0d..0dcf3bf928 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java @@ -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); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java index 2b0a5d9d10..58464009f3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java @@ -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 existingHashtags = notes.getHashtags(); Set updated = new HashSet<>(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java index f2d3a09a94..a55888d094 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java @@ -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); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java index 1ab1d70abf..2644528189 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java @@ -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 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 diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java index a08a6c685b..d6dfd7650b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java @@ -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 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 events, Change change) - throws NoSuchChangeException, OrmException, IOException { + EventList events, Change change) throws OrmException, IOException { if (events.isEmpty()) { return; } + Comparator 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 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); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 1388ee8e55..b67fb59654 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -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 labelNameComparator); + @VisibleForTesting ChangeUpdate create(ChangeControl ctl, Date when, Comparator labelNameComparator); @@ -104,7 +107,8 @@ public class ChangeUpdate extends AbstractChangeUpdate { private final NoteDbUpdateManager.Factory updateManagerFactory; private final Table> approvals; - private final Map reviewers; + private final Map reviewers = new LinkedHashMap<>(); + private final List comments = new ArrayList<>(); private String commitSubject; private String subject; @@ -113,7 +117,6 @@ public class ChangeUpdate extends AbstractChangeUpdate { private Change.Status status; private List submitRecords; private String submissionId; - private List comments; private String topic; private String commit; private Set 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> approvals( + Comparator 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 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() { - @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 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 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 diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java index ac4bf718fd..5a11bfa38e 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java @@ -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; } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java index 8cca245fe8..d2dd48bab0 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -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); } diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java index 7ac6cbe467..84d9b6cc50 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java @@ -101,7 +101,7 @@ public class TestChanges { user), TimeUtil.nowTs(), Ordering. natural()); - ChangeNotes notes = update.getChangeNotes(); + ChangeNotes notes = update.getNotes(); boolean hasPatchSets = notes.getPatchSets() != null && !notes.getPatchSets().isEmpty(); NotesMigration migration = injector.getInstance(NotesMigration.class);