ChangeMessagesUtil: Remove ReviewDb-specific logic

ReviewDb is gone.

Change-Id: I4e86d944bafa2904857e1111ac063f21876981a6
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-12-11 14:54:16 +01:00
parent d97f8eae9e
commit a951386a7f
27 changed files with 44 additions and 135 deletions

View File

@@ -15,37 +15,24 @@
package com.google.gerrit.server;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.reviewdb.server.ReviewDbUtil.unwrapDb;
import static java.util.Objects.requireNonNull;
import com.google.common.annotations.VisibleForTesting;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.update.BatchUpdateReviewDb;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.sql.Timestamp;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
/**
* Utility functions to manipulate ChangeMessages.
*
* <p>These methods either query for and update ChangeMessages in the NoteDb or ReviewDb, depending
* on the state of the NotesMigration.
*/
/** Utility functions to manipulate ChangeMessages. */
@Singleton
public class ChangeMessagesUtil {
public static final String AUTOGENERATED_TAG_PREFIX = "autogenerated:";
@@ -100,27 +87,11 @@ public class ChangeMessagesUtil {
return workInProgress ? TAG_UPLOADED_WIP_PATCH_SET : TAG_UPLOADED_PATCH_SET;
}
private static List<ChangeMessage> sortChangeMessages(Iterable<ChangeMessage> changeMessage) {
return ChangeNotes.MESSAGE_BY_TIME.sortedCopy(changeMessage);
}
private final NotesMigration migration;
@VisibleForTesting
@Inject
public ChangeMessagesUtil(NotesMigration migration) {
this.migration = migration;
}
public List<ChangeMessage> byChange(ReviewDb db, ChangeNotes notes) throws OrmException {
if (!migration.readChanges()) {
return sortChangeMessages(db.changeMessages().byChange(notes.getChangeId()));
}
public List<ChangeMessage> byChange(ChangeNotes notes) throws OrmException {
return notes.load().getChangeMessages();
}
public void addChangeMessage(ReviewDb db, ChangeUpdate update, ChangeMessage changeMessage)
throws OrmException {
public void addChangeMessage(ChangeUpdate update, ChangeMessage changeMessage) {
checkState(
Objects.equals(changeMessage.getAuthor(), update.getNullableAccountId()),
"cannot store change message by %s in update by %s",
@@ -128,7 +99,6 @@ public class ChangeMessagesUtil {
update.getNullableAccountId());
update.setChangeMessage(changeMessage.getMessage());
update.setTag(changeMessage.getTag());
db.changeMessages().insert(Collections.singleton(changeMessage));
}
/**
@@ -139,63 +109,14 @@ public class ChangeMessagesUtil {
* deleted from both NoteDb and ReviewDb, the index of the change message must be used rather than
* its ID.
*
* @param db the {@code ReviewDb} instance to update.
* @param update change update.
* @param targetMessageIdx the index of the target change message.
* @param newMessage the new message which is going to replace the old.
* @throws OrmException
*/
public void replaceChangeMessage(
ReviewDb db, ChangeUpdate update, int targetMessageIdx, String newMessage)
throws OrmException {
if (PrimaryStorage.of(update.getChange()).equals(PrimaryStorage.REVIEW_DB)) {
if (db instanceof BatchUpdateReviewDb) {
db = ((BatchUpdateReviewDb) db).unsafeGetDelegate();
}
db = unwrapDb(db);
List<ChangeMessage> messagesInReviewDb =
sortChangeMessages(db.changeMessages().byChange(update.getId()));
if (migration.readChanges()) {
sanityCheckForChangeMessages(messagesInReviewDb, update.getNotes().getChangeMessages());
}
ChangeMessage targetMessage = messagesInReviewDb.get(targetMessageIdx);
targetMessage.setMessage(newMessage);
db.changeMessages().upsert(Collections.singleton(targetMessage));
}
public void replaceChangeMessage(ChangeUpdate update, int targetMessageIdx, String newMessage) {
update.deleteChangeMessageByRewritingHistory(targetMessageIdx, newMessage);
}
private static void sanityCheckForChangeMessages(
List<ChangeMessage> messagesInReviewDb, List<ChangeMessage> messagesInNoteDb) {
String message =
String.format(
"Change messages in ReivewDb and NoteDb don't match: NoteDb %s; ReviewDb %s",
messagesInNoteDb, messagesInReviewDb);
if (messagesInReviewDb.size() != messagesInNoteDb.size()) {
throw new IllegalStateException(message);
}
for (int i = 0; i < messagesInReviewDb.size(); i++) {
ChangeMessage messageInReviewDb = messagesInReviewDb.get(i);
ChangeMessage messageInNoteDb = messagesInNoteDb.get(i);
// Don't compare the keys because they are different for the same change message in NoteDb and
// ReviewDb.
boolean isEqual =
Objects.equals(messageInReviewDb.getAuthor(), messageInNoteDb.getAuthor())
&& Objects.equals(messageInReviewDb.getWrittenOn(), messageInNoteDb.getWrittenOn())
&& Objects.equals(messageInReviewDb.getMessage(), messageInNoteDb.getMessage())
&& Objects.equals(messageInReviewDb.getPatchSetId(), messageInNoteDb.getPatchSetId())
&& Objects.equals(messageInReviewDb.getTag(), messageInNoteDb.getTag())
&& Objects.equals(messageInReviewDb.getRealAuthor(), messageInNoteDb.getRealAuthor());
if (!isEqual) {
throw new IllegalStateException(message);
}
}
}
/**
* @param tag value of a tag, or null.
* @return whether the tag starts with the autogenerated prefix.

View File

@@ -105,7 +105,7 @@ public class AbandonOp implements BatchUpdateOp {
update.setStatus(change.getStatus());
message = newMessage(ctx);
cmUtil.addChangeMessage(ctx.getDb(), update, message);
cmUtil.addChangeMessage(update, message);
return true;
}

View File

@@ -459,7 +459,7 @@ public class ChangeInserter implements InsertChangeOp {
patchSet.getCreatedOn(),
message,
ChangeMessagesUtil.uploadedPatchSetTag(workInProgress));
cmUtil.addChangeMessage(db, update, changeMessage);
cmUtil.addChangeMessage(update, changeMessage);
}
return true;
}

View File

@@ -667,7 +667,7 @@ public class ChangeJson {
}
private Collection<ChangeMessageInfo> messages(ChangeData cd) throws OrmException {
List<ChangeMessage> messages = cmUtil.byChange(db.get(), cd.notes());
List<ChangeMessage> messages = cmUtil.byChange(cd.notes());
if (messages.isEmpty()) {
return Collections.emptyList();
}

View File

@@ -275,7 +275,7 @@ public class PatchSetInserter implements BatchUpdateOp {
db, ctx.getNotes(), patchSet, ctx.getRevWalk(), ctx.getRepoView().getConfig());
}
if (changeMessage != null) {
cmUtil.addChangeMessage(db, update, changeMessage);
cmUtil.addChangeMessage(update, changeMessage);
}
return true;
}

View File

@@ -99,7 +99,7 @@ public class SetAssigneeOp implements BatchUpdateOp {
return true;
}
private void addMessage(ChangeContext ctx, ChangeUpdate update) throws OrmException {
private void addMessage(ChangeContext ctx, ChangeUpdate update) {
StringBuilder msg = new StringBuilder();
msg.append("Assignee ");
if (oldAssignee == null) {
@@ -113,7 +113,7 @@ public class SetAssigneeOp implements BatchUpdateOp {
}
ChangeMessage cmsg =
ChangeMessagesUtil.newMessage(ctx, msg.toString(), ChangeMessagesUtil.TAG_SET_ASSIGNEE);
cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
cmUtil.addChangeMessage(update, cmsg);
}
@Override

View File

@@ -125,13 +125,13 @@ public class SetHashtagsOp implements BatchUpdateOp {
}
}
private void addMessage(ChangeContext ctx, ChangeUpdate update) throws OrmException {
private void addMessage(ChangeContext ctx, ChangeUpdate update) {
StringBuilder msg = new StringBuilder();
appendHashtagMessage(msg, "added", toAdd);
appendHashtagMessage(msg, "removed", toRemove);
ChangeMessage cmsg =
ChangeMessagesUtil.newMessage(ctx, msg.toString(), ChangeMessagesUtil.TAG_SET_HASHTAGS);
cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
cmUtil.addChangeMessage(update, cmsg);
}
private void appendHashtagMessage(StringBuilder b, String action, Set<String> hashtags) {

View File

@@ -135,7 +135,7 @@ public class WorkInProgressOp implements BatchUpdateOp {
return true;
}
private void addMessage(ChangeContext ctx, ChangeUpdate update) throws OrmException {
private void addMessage(ChangeContext ctx, ChangeUpdate update) {
Change c = ctx.getChange();
StringBuilder buf =
new StringBuilder(c.isWorkInProgress() ? "Set Work In Progress" : "Set Ready For Review");
@@ -154,7 +154,7 @@ public class WorkInProgressOp implements BatchUpdateOp {
? ChangeMessagesUtil.TAG_SET_WIP
: ChangeMessagesUtil.TAG_SET_READY);
cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
cmUtil.addChangeMessage(update, cmsg);
}
@Override

View File

@@ -151,7 +151,7 @@ public class MergedByPushOp implements BatchUpdateOp {
ChangeMessage msg =
ChangeMessagesUtil.newMessage(
psId, ctx.getUser(), ctx.getWhen(), msgBuf.toString(), ChangeMessagesUtil.TAG_MERGED);
cmUtil.addChangeMessage(ctx.getDb(), update, msg);
cmUtil.addChangeMessage(update, msg);
PatchSetApproval submitter =
ApprovalsUtil.newApproval(

View File

@@ -353,7 +353,7 @@ public class ReplaceOp implements BatchUpdateOp {
}
msg = createChangeMessage(ctx, reviewMessage);
cmUtil.addChangeMessage(ctx.getDb(), update, msg);
cmUtil.addChangeMessage(update, msg);
if (mergedByPushOp == null) {
resetChange(ctx);

View File

@@ -290,7 +290,7 @@ public class MailProcessor {
}
changeMessage = generateChangeMessage(ctx);
changeMessagesUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId), changeMessage);
changeMessagesUtil.addChangeMessage(ctx.getUpdate(psId), changeMessage);
comments = new ArrayList<>();
for (MailComment c : parsedComments) {

View File

@@ -949,7 +949,7 @@ public class ChangeData {
if (!lazyLoad) {
return Collections.emptyList();
}
messages = cmUtil.byChange(db, notes());
messages = cmUtil.byChange(notes());
}
return messages;
}

View File

@@ -114,14 +114,14 @@ public class DeleteAssignee
return deletedAssignee != null ? deletedAssignee.getAccount().getId() : null;
}
private void addMessage(ChangeContext ctx, ChangeUpdate update, IdentifiedUser deletedAssignee)
throws OrmException {
private void addMessage(
ChangeContext ctx, ChangeUpdate update, IdentifiedUser deletedAssignee) {
ChangeMessage cmsg =
ChangeMessagesUtil.newMessage(
ctx,
"Assignee deleted: " + deletedAssignee.getNameEmail(),
ChangeMessagesUtil.TAG_DELETE_ASSIGNEE);
cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
cmUtil.addChangeMessage(update, cmsg);
}
@Override

View File

@@ -109,8 +109,7 @@ public class DeleteChangeMessage
private ChangeMessageInfo createUpdatedChangeMessageInfo(Change.Id id, int targetIdx)
throws OrmException, PermissionBackendException {
List<ChangeMessage> messages =
changeMessagesUtil.byChange(dbProvider.get(), notesFactory.createChecked(id));
List<ChangeMessage> messages = changeMessagesUtil.byChange(notesFactory.createChecked(id));
ChangeMessage updatedChangeMessage = messages.get(targetIdx);
AccountLoader accountLoader = accountLoaderFactory.create(true);
ChangeMessageInfo info = createChangeMessageInfo(updatedChangeMessage, accountLoader);
@@ -145,10 +144,9 @@ public class DeleteChangeMessage
}
@Override
public boolean updateChange(ChangeContext ctx) throws OrmException {
public boolean updateChange(ChangeContext ctx) {
PatchSet.Id psId = ctx.getChange().currentPatchSetId();
changeMessagesUtil.replaceChangeMessage(
ctx.getDb(), ctx.getUpdate(psId), targetMessageIdx, newMessage);
changeMessagesUtil.replaceChangeMessage(ctx.getUpdate(psId), targetMessageIdx, newMessage);
return true;
}
}

View File

@@ -171,7 +171,7 @@ public class DeleteReviewerOp implements BatchUpdateOp {
changeMessage =
ChangeMessagesUtil.newMessage(ctx, msg.toString(), ChangeMessagesUtil.TAG_DELETE_REVIEWER);
cmUtil.addChangeMessage(ctx.getDb(), update, changeMessage);
cmUtil.addChangeMessage(update, changeMessage);
return true;
}

View File

@@ -218,7 +218,7 @@ public class DeleteVote extends RetryingRestModifyView<VoteResource, DeleteVoteI
msg.append(" by ").append(userFactory.create(accountId).getNameEmail()).append("\n");
changeMessage =
ChangeMessagesUtil.newMessage(ctx, msg.toString(), ChangeMessagesUtil.TAG_DELETE_VOTE);
cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId), changeMessage);
cmUtil.addChangeMessage(ctx.getUpdate(psId), changeMessage);
return true;
}

View File

@@ -19,30 +19,24 @@ import static com.google.gerrit.server.ChangeMessagesUtil.createChangeMessageInf
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.util.List;
import java.util.stream.Collectors;
@Singleton
public class ListChangeMessages implements RestReadView<ChangeResource> {
private final Provider<ReviewDb> dbProvider;
private final ChangeMessagesUtil changeMessagesUtil;
private final AccountLoader accountLoader;
@Inject
public ListChangeMessages(
Provider<ReviewDb> dbProvider,
ChangeMessagesUtil changeMessagesUtil,
AccountLoader.Factory accountLoaderFactory) {
this.dbProvider = dbProvider;
ChangeMessagesUtil changeMessagesUtil, AccountLoader.Factory accountLoaderFactory) {
this.changeMessagesUtil = changeMessagesUtil;
this.accountLoader = accountLoaderFactory.create(true);
}
@@ -50,8 +44,7 @@ public class ListChangeMessages implements RestReadView<ChangeResource> {
@Override
public List<ChangeMessageInfo> apply(ChangeResource resource)
throws OrmException, PermissionBackendException {
List<ChangeMessage> messages =
changeMessagesUtil.byChange(dbProvider.get(), resource.getNotes());
List<ChangeMessage> messages = changeMessagesUtil.byChange(resource.getNotes());
List<ChangeMessageInfo> messageInfos =
messages
.stream()

View File

@@ -252,7 +252,7 @@ public class Move extends RetryingRestModifyView<ChangeResource, MoveInput, Chan
}
ChangeMessage cmsg =
ChangeMessagesUtil.newMessage(ctx, msgBuf.toString(), ChangeMessagesUtil.TAG_MOVE);
cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
cmUtil.addChangeMessage(update, cmsg);
return true;
}

View File

@@ -1385,7 +1385,7 @@ public class PostReview
return current;
}
private boolean insertMessage(ChangeContext ctx) throws OrmException {
private boolean insertMessage(ChangeContext ctx) {
String msg = Strings.nullToEmpty(in.message).trim();
StringBuilder buf = new StringBuilder();
@@ -1409,7 +1409,7 @@ public class PostReview
message =
ChangeMessagesUtil.newMessage(
psId, user, ctx.getWhen(), "Patch Set " + psId.get() + ":" + buf, in.tag);
cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId), message);
cmUtil.addChangeMessage(ctx.getUpdate(psId), message);
return true;
}

View File

@@ -117,7 +117,7 @@ public class PutDescription
ChangeMessage cmsg =
ChangeMessagesUtil.newMessage(
psId, ctx.getUser(), ctx.getWhen(), summary, ChangeMessagesUtil.TAG_SET_DESCRIPTION);
cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
cmUtil.addChangeMessage(update, cmsg);
return true;
}
}

View File

@@ -123,7 +123,7 @@ public class PutTopic extends RetryingRestModifyView<ChangeResource, TopicInput,
ChangeMessage cmsg =
ChangeMessagesUtil.newMessage(ctx, summary, ChangeMessagesUtil.TAG_SET_TOPIC);
cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
cmUtil.addChangeMessage(update, cmsg);
return true;
}

View File

@@ -130,7 +130,7 @@ public class Restore extends RetryingRestModifyView<ChangeResource, RestoreInput
update.setStatus(change.getStatus());
message = newMessage(ctx);
cmUtil.addChangeMessage(ctx.getDb(), update, message);
cmUtil.addChangeMessage(update, message);
return true;
}

View File

@@ -318,7 +318,7 @@ public class Revert extends RetryingRestModifyView<ChangeResource, RevertInput,
}
@Override
public boolean updateChange(ChangeContext ctx) throws Exception {
public boolean updateChange(ChangeContext ctx) {
Change change = ctx.getChange();
PatchSet.Id patchSetId = change.currentPatchSetId();
ChangeMessage changeMessage =
@@ -326,7 +326,7 @@ public class Revert extends RetryingRestModifyView<ChangeResource, RevertInput,
ctx,
"Created a revert of this change as I" + computedChangeId.name(),
ChangeMessagesUtil.TAG_REVERT);
cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(patchSetId), changeMessage);
cmUtil.addChangeMessage(ctx.getUpdate(patchSetId), changeMessage);
return true;
}
}

View File

@@ -87,7 +87,7 @@ public class SetPrivateOp implements BatchUpdateOp {
privateStateChanged.fire(change, ps, ctx.getAccount(), ctx.getWhen());
}
private void addMessage(ChangeContext ctx, ChangeUpdate update) throws OrmException {
private void addMessage(ChangeContext ctx, ChangeUpdate update) {
Change c = ctx.getChange();
StringBuilder buf = new StringBuilder(c.isPrivate() ? "Set private" : "Unset private");
@@ -104,6 +104,6 @@ public class SetPrivateOp implements BatchUpdateOp {
c.isPrivate()
? ChangeMessagesUtil.TAG_SET_PRIVATE
: ChangeMessagesUtil.TAG_UNSET_PRIVATE);
cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
cmUtil.addChangeMessage(update, cmsg);
}
}

View File

@@ -911,7 +911,7 @@ public class MergeOp implements AutoCloseable {
cd.getId(),
new BatchUpdateOp() {
@Override
public boolean updateChange(ChangeContext ctx) throws OrmException {
public boolean updateChange(ChangeContext ctx) {
Change change = ctx.getChange();
if (!change.getStatus().isOpen()) {
return false;
@@ -926,8 +926,7 @@ public class MergeOp implements AutoCloseable {
change.getLastUpdatedOn(),
ChangeMessagesUtil.TAG_MERGED,
"Project was deleted.");
cmUtil.addChangeMessage(
ctx.getDb(), ctx.getUpdate(change.currentPatchSetId()), msg);
cmUtil.addChangeMessage(ctx.getUpdate(change.currentPatchSetId()), msg);
return true;
}

View File

@@ -32,7 +32,6 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
@@ -483,9 +482,8 @@ abstract class SubmitStrategyOp implements BatchUpdateOp {
psId, ctx.getUser(), ctx.getWhen(), body, ChangeMessagesUtil.TAG_MERGED);
}
private void setMerged(ChangeContext ctx, ChangeMessage msg) throws OrmException {
private void setMerged(ChangeContext ctx, ChangeMessage msg) {
Change c = ctx.getChange();
ReviewDb db = ctx.getDb();
logger.atFine().log("Setting change %s merged", c.getId());
c.setStatus(Change.Status.MERGED);
c.setSubmissionId(args.submissionId.toStringForStorage());
@@ -494,7 +492,7 @@ abstract class SubmitStrategyOp implements BatchUpdateOp {
// which is not the user from the update context. addMergedMessage was able
// to do this in the past.
if (msg != null) {
args.cmUtil.addChangeMessage(db, ctx.getUpdate(msg.getPatchSetId()), msg);
args.cmUtil.addChangeMessage(ctx.getUpdate(msg.getPatchSetId()), msg);
}
}

View File

@@ -115,7 +115,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
assertThat(psa.getRealAccountId()).isEqualTo(admin.id);
ChangeData cd = r.getChange();
ChangeMessage m = Iterables.getLast(cmUtil.byChange(db, cd.notes()));
ChangeMessage m = Iterables.getLast(cmUtil.byChange(cd.notes()));
assertThat(m.getMessage()).endsWith(in.message);
assertThat(m.getAuthor()).isEqualTo(user.id);
assertThat(m.getRealAuthor()).isEqualTo(admin.id);
@@ -517,7 +517,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
assertThat(psa.getRealAccountId()).isEqualTo(admin.id); // not user2
ChangeData cd = r.getChange();
ChangeMessage m = Iterables.getLast(cmUtil.byChange(db, cd.notes()));
ChangeMessage m = Iterables.getLast(cmUtil.byChange(cd.notes()));
assertThat(m.getMessage()).endsWith(in.message);
assertThat(m.getAuthor()).isEqualTo(user.id);
assertThat(m.getRealAuthor()).isEqualTo(admin.id); // not user2