From 341baf22a0756b73ba36caec87fcb740061e332f Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 4 Feb 2016 11:52:09 +0100 Subject: [PATCH] Pass project to ChangeData so that it can load the change from notes ChangeData currently loads the change directly from the database, but now when we support notedb it should load the change via the change notes factory. Loading the change via the change notes factory requires the project name in addition to the change ID, hence ChangeData must know it. The only caller of ChangeData that cannot provide the project name is HasDraftByLegacyPredicate. Due to this HasDraftByLegacyPredicate loads the changes on its own from the database for now. We may remove this deprecated predicate before migrating to notedb or find another solution later. When the change is now loaded from notedb its last modified timestamp is only precise to seconds, while when its loaded from the database it is precise to milliseconds. Due to this different changes got the same last modified timestamp when notedb was enabled and as result the GetRelatedIT#getRelatedReworkThenExtendInTheMiddleOfSeries test started to fail. Defining a 1s time clock step for this test fixes this issue. Change-Id: Iee0dce795766f543c996d75fde4067cb23a357dd Signed-off-by: Edwin Kempin --- .../gerrit/acceptance/AbstractDaemonTest.java | 3 +- .../server/change/GetRelatedIT.java | 21 ++++++++++- .../gerrit/lucene/LuceneChangeIndex.java | 4 +-- .../com/google/gerrit/server/ChangeUtil.java | 2 +- .../google/gerrit/server/change/Abandon.java | 3 +- .../gerrit/server/change/ChangeInserter.java | 4 +-- .../gerrit/server/change/ChangeJson.java | 4 +-- .../server/change/EmailReviewComments.java | 3 +- .../server/change/PatchSetInserter.java | 2 +- .../gerrit/server/change/PostReviewers.java | 5 +-- .../server/change/PublishDraftPatchSet.java | 4 +-- .../google/gerrit/server/change/Restore.java | 3 +- .../google/gerrit/server/git/BatchUpdate.java | 2 +- .../google/gerrit/server/git/EmailMerge.java | 9 +++-- .../gerrit/server/git/MergeSuperSet.java | 5 +-- .../gerrit/server/git/ReceiveCommits.java | 8 ++--- .../server/git/strategy/SubmitStrategyOp.java | 3 +- .../gerrit/server/index/ChangeIndexer.java | 19 ++++++---- .../gerrit/server/mail/AbandonedSender.java | 9 +++-- .../gerrit/server/mail/AddReviewerSender.java | 9 +++-- .../gerrit/server/mail/ChangeEmail.java | 6 ++-- .../gerrit/server/mail/CommentSender.java | 6 ++-- .../server/mail/CreateChangeSender.java | 9 +++-- .../gerrit/server/mail/MergeFailSender.java | 9 +++-- .../gerrit/server/mail/MergedSender.java | 9 +++-- .../server/mail/ReplacePatchSetSender.java | 9 +++-- .../server/mail/ReplyToChangeSender.java | 3 +- .../gerrit/server/mail/RestoredSender.java | 9 +++-- .../gerrit/server/mail/RevertedSender.java | 9 +++-- .../server/query/change/ChangeData.java | 36 +++++++++++++------ .../change/HasDraftByLegacyPredicate.java | 6 ++-- .../gerrit/server/change/WalkSorterTest.java | 2 +- .../server/query/change/ChangeDataTest.java | 6 ++-- .../query/change/RegexPathPredicateTest.java | 4 ++- 34 files changed, 163 insertions(+), 82 deletions(-) diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 78fa0a49c5..783af770b9 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -640,7 +640,8 @@ public abstract class AbstractDaemonTest { } protected PatchSet getPatchSet(PatchSet.Id psId) throws OrmException { - return changeDataFactory.create(db, psId.getParentKey()).patchSet(psId); + return changeDataFactory.create(db, project, psId.getParentKey()) + .patchSet(psId); } protected IdentifiedUser user(TestAccount testAccount) { diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java index 3918904f7b..27f8bbef4c 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java @@ -16,6 +16,7 @@ package com.google.gerrit.acceptance.server.change; import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.acceptance.GitUtil.pushHead; +import static java.util.concurrent.TimeUnit.SECONDS; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; @@ -33,17 +34,34 @@ import com.google.gerrit.server.edit.ChangeEditUtil; import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.testutil.TestTimeUtil; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.revwalk.RevCommit; +import org.junit.After; +import org.junit.Before; import org.junit.Test; import java.io.IOException; import java.util.List; public class GetRelatedIT extends AbstractDaemonTest { + private String systemTimeZone; + + @Before + public void setTimeForTesting() { + systemTimeZone = System.setProperty("user.timezone", "US/Eastern"); + TestTimeUtil.resetWithClockStep(1, SECONDS); + } + + @After + public void resetTime() { + TestTimeUtil.useSystemTime(); + System.setProperty("user.timezone", systemTimeZone); + } + @Inject private ChangeEditUtil editUtil; @@ -573,7 +591,8 @@ public class GetRelatedIT extends AbstractDaemonTest { // Pretend PS1,1 was pushed before the groups field was added. clearGroups(psId1_1); - indexer.index(changeDataFactory.create(db, psId1_1.getParentKey())); + indexer.index( + changeDataFactory.create(db, project, psId1_1.getParentKey())); // PS1,1 has no groups, so disappeared from related changes. assertRelated(psId2_1); diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java index 3a03959953..d80dcf3692 100644 --- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -456,11 +456,9 @@ public class LuceneChangeIndex implements ChangeIndex { ChangeProtoField.CODEC.decode(cb.bytes, cb.offset, cb.length)); } else { int id = doc.getField(idFieldName).numericValue().intValue(); - // TODO(ekempin): Pass project to changeDataFactory - @SuppressWarnings("unused") Project.NameKey project = new Project.NameKey(doc.getField(PROJECT.getName()).stringValue()); - cd = changeDataFactory.create(db.get(), new Change.Id(id)); + cd = changeDataFactory.create(db.get(), project, new Change.Id(id)); } if (fields.contains(PATCH_SET_FIELD)) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index d256346a03..7c76644547 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java @@ -281,7 +281,7 @@ public class ChangeUtil { } try { - RevertedSender cm = revertedSenderFactory.create(changeId); + RevertedSender cm = revertedSenderFactory.create(project, changeId); cm.setFrom(user.get().getAccountId()); cm.setChangeMessage(ins.getChangeMessage()); cm.send(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java index 6bddf22e10..def4591865 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java @@ -162,7 +162,8 @@ public class Abandon implements RestModifyView, @Override public void postUpdate(Context ctx) throws OrmException { try { - ReplyToChangeSender cm = abandonedSenderFactory.create(change.getId()); + ReplyToChangeSender cm = + abandonedSenderFactory.create(ctx.getProject(), change.getId()); if (account != null) { cm.setFrom(account.getId()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java index 3b36f3af04..562d8814a7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java @@ -361,8 +361,8 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp { @Override public void run() { try { - CreateChangeSender cm = - createChangeSenderFactory.create(change.getId()); + CreateChangeSender cm = createChangeSenderFactory + .create(change.getProject(), change.getId()); cm.setFrom(change.getOwner()); cm.setPatchSet(patchSet, patchSetInfo); cm.setNotify(notify); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 138377d7a2..4cef85aaa9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -231,7 +231,7 @@ public class ChangeJson { if (!has(CHECK)) { throw e; } - return checkOnly(changeDataFactory.create(db.get(), id)); + return checkOnly(changeDataFactory.create(db.get(), project, id)); } return format(changeDataFactory.create(db.get(), notes.getChange())); } @@ -381,7 +381,7 @@ public class ChangeJson { // If any problems were fixed, the ChangeData needs to be reloaded. for (ProblemInfo p : out.problems) { if (p.status == ProblemInfo.Status.FIXED) { - cd = changeDataFactory.create(cd.db(), cd.getId()); + cd = changeDataFactory.create(cd.db(), cd.getProject(), cd.getId()); break; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java index 243e8553eb..dd499ec76b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java @@ -105,7 +105,8 @@ public class EmailReviewComments implements Runnable, RequestContext { RequestContext old = requestContext.setContext(this); try { - CommentSender cm = commentSenderFactory.create(notes.getChangeId()); + CommentSender cm = commentSenderFactory.create(notes.getProjectName(), + notes.getChangeId()); cm.setFrom(authorId); cm.setPatchSet(patchSet, patchSetInfoFactory.get(notes.getProjectName(), patchSet)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java index 822a12f83c..39c27cd93e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java @@ -259,7 +259,7 @@ public class PatchSetInserter extends BatchUpdate.Op { if (sendMail) { try { ReplacePatchSetSender cm = replacePatchSetFactory.create( - change.getId()); + ctx.getProject(), change.getId()); cm.setFrom(ctx.getUser().getAccountId()); cm.setPatchSet(patchSet, patchSetInfo); cm.setChangeMessage(changeMessage); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java index bf0c903ccc..a15297d100 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java @@ -248,7 +248,7 @@ public class PostReviewers implements RestModifyView indexFuture = - indexer.indexAsync(rsrc.getId()); + indexer.indexAsync(rsrc.getProject(), rsrc.getId()); result.reviewers = Lists.newArrayListWithCapacity(added.size()); for (PatchSetApproval psa : added) { // New reviewers have value 0, don't bother normalizing. @@ -286,7 +286,8 @@ public class PostReviewers implements RestModifyView, @Override public void postUpdate(Context ctx) throws OrmException { try { - ReplyToChangeSender cm = restoredSenderFactory.create(change.getId()); + ReplyToChangeSender cm = + restoredSenderFactory.create(ctx.getProject(), change.getId()); cm.setFrom(caller.getAccountId()); cm.setChangeMessage(message); cm.send(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java index c3d0ecea8e..57c2ae276d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java @@ -615,7 +615,7 @@ public class BatchUpdate implements AutoCloseable { bmdu.commit(); } } - indexFutures.add(indexer.indexAsync(id)); + indexFutures.add(indexer.indexAsync(ctx.getProject(), id)); } } } catch (Exception e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/EmailMerge.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/EmailMerge.java index b281c5b139..2e993bd9f9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/EmailMerge.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/EmailMerge.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.git; 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.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.mail.MergedSender; @@ -39,7 +40,8 @@ public class EmailMerge implements Runnable, RequestContext { private static final Logger log = LoggerFactory.getLogger(EmailMerge.class); public interface Factory { - EmailMerge create(Change.Id changeId, Account.Id submitter); + EmailMerge create(Project.NameKey project, Change.Id changeId, + Account.Id submitter); } private final ExecutorService sendEmailsExecutor; @@ -47,6 +49,7 @@ public class EmailMerge implements Runnable, RequestContext { private final SchemaFactory schemaFactory; private final ThreadLocalRequestContext requestContext; + private final Project.NameKey project; private final Change.Id changeId; private final Account.Id submitter; private ReviewDb db; @@ -56,12 +59,14 @@ public class EmailMerge implements Runnable, RequestContext { MergedSender.Factory mergedSenderFactory, SchemaFactory schemaFactory, ThreadLocalRequestContext requestContext, + @Assisted Project.NameKey project, @Assisted Change.Id changeId, @Assisted @Nullable Account.Id submitter) { this.sendEmailsExecutor = executor; this.mergedSenderFactory = mergedSenderFactory; this.schemaFactory = schemaFactory; this.requestContext = requestContext; + this.project = project; this.changeId = changeId; this.submitter = submitter; } @@ -74,7 +79,7 @@ public class EmailMerge implements Runnable, RequestContext { public void run() { RequestContext old = requestContext.setContext(this); try { - MergedSender cm = mergedSenderFactory.create(changeId); + MergedSender cm = mergedSenderFactory.create(project, changeId); if (submitter != null) { cm.setFrom(submitter); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java index b38304227f..b3538b7280 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java @@ -83,7 +83,8 @@ public class MergeSuperSet { public ChangeSet completeChangeSet(ReviewDb db, Change change) throws MissingObjectException, IncorrectObjectTypeException, IOException, OrmException { - ChangeData cd = changeDataFactory.create(db, change.getId()); + ChangeData cd = + changeDataFactory.create(db, change.getProject(), change.getId()); if (Submit.wholeTopicEnabled(cfg)) { return completeChangeSetIncludingTopics(db, new ChangeSet(cd)); } else { @@ -101,7 +102,7 @@ public class MergeSuperSet { try (Repository repo = repoManager.openRepository(project); RevWalk rw = CodeReviewCommit.newRevWalk(repo)) { for (Change.Id cId : pc.get(project)) { - ChangeData cd = changeDataFactory.create(db, cId); + ChangeData cd = changeDataFactory.create(db, project, cId); SubmitTypeRecord str = cd.submitTypeRecord(); if (!str.isOk()) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 89eb49fc3c..a04b06ac7d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -2379,8 +2379,8 @@ public class ReceiveCommits { @Override public void run() { try { - ReplacePatchSetSender cm = - replacePatchSetFactory.create(change.getId()); + ReplacePatchSetSender cm = replacePatchSetFactory + .create(project.getNameKey(), change.getId()); cm.setFrom(me); cm.setPatchSet(newPatchSet, info); cm.setChangeMessage(msg); @@ -2819,8 +2819,8 @@ public class ReceiveCommits { @Override public void run() { try { - MergedSender cm = - mergedSenderFactory.create(ps.getId().getParentKey()); + MergedSender cm = mergedSenderFactory.create(project.getNameKey(), + ps.getId().getParentKey()); cm.setFrom(user.getAccountId()); cm.setPatchSet(ps, info); cm.send(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java index e96453d5ca..e5bc614046 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java @@ -481,7 +481,8 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op { // Assume the change must have been merged at this point, otherwise we would // have failed fast in one of the other steps. try { - args.mergedSenderFactory.create(getId(), submitter.getAccountId()) + args.mergedSenderFactory + .create(ctx.getProject(), getId(), submitter.getAccountId()) .sendAsync(); } catch (Exception e) { log.error("Cannot email merged notification for " + getId(), e); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndexer.java index 84edcbff71..a0860c7afc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeIndexer.java @@ -21,6 +21,7 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.query.change.ChangeData; @@ -128,9 +129,10 @@ public class ChangeIndexer { * @param id change to index. * @return future for the indexing task. */ - public CheckedFuture indexAsync(Change.Id id) { + public CheckedFuture indexAsync(Project.NameKey project, + Change.Id id) { return executor != null - ? submit(new IndexTask(id)) + ? submit(new IndexTask(project, id)) : Futures. immediateCheckedFuture(null); } @@ -140,10 +142,11 @@ public class ChangeIndexer { * @param ids changes to index. * @return future for completing indexing of all changes. */ - public CheckedFuture indexAsync(Collection ids) { + public CheckedFuture indexAsync(Project.NameKey project, + Collection ids) { List> futures = new ArrayList<>(ids.size()); for (Change.Id id : ids) { - futures.add(indexAsync(id)); + futures.add(indexAsync(project, id)); } return allAsList(futures); } @@ -201,9 +204,11 @@ public class ChangeIndexer { } private class IndexTask implements Callable { + private final Project.NameKey project; private final Change.Id id; - private IndexTask(Change.Id id) { + private IndexTask(Project.NameKey project, Change.Id id) { + this.project = project; this.id = id; } @@ -237,8 +242,8 @@ public class ChangeIndexer { }; RequestContext oldCtx = context.setContext(newCtx); try { - ChangeData cd = changeDataFactory.create( - newCtx.getReviewDbProvider().get(), id); + ChangeData cd = changeDataFactory + .create(newCtx.getReviewDbProvider().get(), project, id); index(cd); return null; } finally { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/AbandonedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/AbandonedSender.java index 409c1559cf..dbd0438483 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/AbandonedSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/AbandonedSender.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.mail; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -26,13 +27,15 @@ public class AbandonedSender extends ReplyToChangeSender { public static interface Factory extends ReplyToChangeSender.Factory { @Override - AbandonedSender create(Change.Id change); + AbandonedSender create(Project.NameKey project, Change.Id change); } @Inject - public AbandonedSender(EmailArguments ea, @Assisted Change.Id id) + public AbandonedSender(EmailArguments ea, + @Assisted Project.NameKey project, + @Assisted Change.Id id) throws OrmException { - super(ea, "abandon", newChangeData(ea, id)); + super(ea, "abandon", newChangeData(ea, project, id)); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/AddReviewerSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/AddReviewerSender.java index 7a6d204df2..98fc4a023e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/AddReviewerSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/AddReviewerSender.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.mail; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -23,13 +24,15 @@ import com.google.inject.assistedinject.Assisted; /** Asks a user to review a change. */ public class AddReviewerSender extends NewChangeSender { public static interface Factory { - AddReviewerSender create(Change.Id id); + AddReviewerSender create(Project.NameKey project, Change.Id id); } @Inject - public AddReviewerSender(EmailArguments ea, @Assisted Change.Id id) + public AddReviewerSender(EmailArguments ea, + @Assisted Project.NameKey project, + @Assisted Change.Id id) throws OrmException { - super(ea, newChangeData(ea, id)); + super(ea, newChangeData(ea, project, id)); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java index 3da6605760..5c888992bb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java @@ -26,6 +26,7 @@ import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.Patch; 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.server.IdentifiedUser; import com.google.gerrit.server.mail.ProjectWatch.Watchers; import com.google.gerrit.server.patch.PatchList; @@ -56,8 +57,9 @@ import java.util.TreeSet; public abstract class ChangeEmail extends NotificationEmail { private static final Logger log = LoggerFactory.getLogger(ChangeEmail.class); - protected static ChangeData newChangeData(EmailArguments ea, Change.Id id) { - return ea.changeDataFactory.create(ea.db.get(), id); + protected static ChangeData newChangeData(EmailArguments ea, + Project.NameKey project, Change.Id id) { + return ea.changeDataFactory.create(ea.db.get(), project, id); } protected final Change change; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java index 83494cc6a2..c845c28a47 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java @@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.client.CommentRange; import com.google.gerrit.reviewdb.client.Patch; 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.server.PatchLineCommentsUtil; import com.google.gerrit.server.patch.PatchFile; import com.google.gerrit.server.patch.PatchList; @@ -52,7 +53,7 @@ public class CommentSender extends ReplyToChangeSender { .getLogger(CommentSender.class); public static interface Factory { - CommentSender create(Change.Id id); + CommentSender create(Project.NameKey project, Change.Id id); } private List inlineComments = Collections.emptyList(); @@ -61,8 +62,9 @@ public class CommentSender extends ReplyToChangeSender { @Inject public CommentSender(EmailArguments ea, PatchLineCommentsUtil plcUtil, + @Assisted Project.NameKey project, @Assisted Change.Id id) throws OrmException { - super(ea, "comment", newChangeData(ea, id)); + super(ea, "comment", newChangeData(ea, project, id)); this.plcUtil = plcUtil; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CreateChangeSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CreateChangeSender.java index e67b8ce2aa..87bfa375d6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CreateChangeSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CreateChangeSender.java @@ -19,6 +19,7 @@ import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.mail.ProjectWatch.Watchers; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -33,13 +34,15 @@ public class CreateChangeSender extends NewChangeSender { LoggerFactory.getLogger(CreateChangeSender.class); public static interface Factory { - CreateChangeSender create(Change.Id id); + CreateChangeSender create(Project.NameKey project, Change.Id id); } @Inject - public CreateChangeSender(EmailArguments ea, @Assisted Change.Id id) + public CreateChangeSender(EmailArguments ea, + @Assisted Project.NameKey project, + @Assisted Change.Id id) throws OrmException { - super(ea, newChangeData(ea, id)); + super(ea, newChangeData(ea, project, id)); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergeFailSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergeFailSender.java index f91b0fd9c0..3dfbcda31f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergeFailSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergeFailSender.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.mail; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -23,13 +24,15 @@ import com.google.inject.assistedinject.Assisted; /** Send notice about a change failing to merged. */ public class MergeFailSender extends ReplyToChangeSender { public static interface Factory { - MergeFailSender create(Change.Id id); + MergeFailSender create(Project.NameKey project, Change.Id id); } @Inject - public MergeFailSender(EmailArguments ea, @Assisted Change.Id id) + public MergeFailSender(EmailArguments ea, + @Assisted Project.NameKey project, + @Assisted Change.Id id) throws OrmException { - super(ea, "merge-failed", newChangeData(ea, id)); + super(ea, "merge-failed", newChangeData(ea, project, id)); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java index 9725e7f34e..7271c181e7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java @@ -24,6 +24,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSetApproval; +import com.google.gerrit.reviewdb.client.Project; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -31,15 +32,17 @@ import com.google.inject.assistedinject.Assisted; /** Send notice about a change successfully merged. */ public class MergedSender extends ReplyToChangeSender { public static interface Factory { - MergedSender create(Change.Id id); + MergedSender create(Project.NameKey project, Change.Id id); } private final LabelTypes labelTypes; @Inject - public MergedSender(EmailArguments ea, @Assisted Change.Id id) + public MergedSender(EmailArguments ea, + @Assisted Project.NameKey project, + @Assisted Change.Id id) throws OrmException { - super(ea, "merged", newChangeData(ea, id)); + super(ea, "merged", newChangeData(ea, project, id)); labelTypes = changeData.changeControl().getLabelTypes(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ReplacePatchSetSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ReplacePatchSetSender.java index 7980845fca..6bee3e8a82 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ReplacePatchSetSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ReplacePatchSetSender.java @@ -18,6 +18,7 @@ import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -31,16 +32,18 @@ import java.util.Set; /** Send notice of new patch sets for reviewers. */ public class ReplacePatchSetSender extends ReplyToChangeSender { public static interface Factory { - ReplacePatchSetSender create(Change.Id id); + ReplacePatchSetSender create(Project.NameKey project, Change.Id id); } private final Set reviewers = new HashSet<>(); private final Set extraCC = new HashSet<>(); @Inject - public ReplacePatchSetSender(EmailArguments ea, @Assisted Change.Id id) + public ReplacePatchSetSender(EmailArguments ea, + @Assisted Project.NameKey project, + @Assisted Change.Id id) throws OrmException { - super(ea, "newpatchset", newChangeData(ea, id)); + super(ea, "newpatchset", newChangeData(ea, project, id)); } public void addReviewers(final Collection cc) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ReplyToChangeSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ReplyToChangeSender.java index fbfbe61212..8eadeefa19 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ReplyToChangeSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ReplyToChangeSender.java @@ -16,13 +16,14 @@ package com.google.gerrit.server.mail; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; /** Alert a user to a reply to a change, usually commentary made during review. */ public abstract class ReplyToChangeSender extends ChangeEmail { public static interface Factory { - T create(Change.Id id); + T create(Project.NameKey project, Change.Id id); } protected ReplyToChangeSender(EmailArguments ea, String mc, ChangeData cd) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/RestoredSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/RestoredSender.java index a43c7b665c..0808090245 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/RestoredSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/RestoredSender.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.mail; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -26,13 +27,15 @@ public class RestoredSender extends ReplyToChangeSender { public static interface Factory extends ReplyToChangeSender.Factory { @Override - RestoredSender create(Change.Id id); + RestoredSender create(Project.NameKey project, Change.Id id); } @Inject - public RestoredSender(EmailArguments ea, @Assisted Change.Id id) + public RestoredSender(EmailArguments ea, + @Assisted Project.NameKey project, + @Assisted Change.Id id) throws OrmException { - super(ea, "restore", newChangeData(ea, id)); + super(ea, "restore", newChangeData(ea, project, id)); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/RevertedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/RevertedSender.java index dda68ab549..55438fea29 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/RevertedSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/RevertedSender.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.mail; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -24,13 +25,15 @@ import com.google.inject.assistedinject.Assisted; /** Send notice about a change being reverted. */ public class RevertedSender extends ReplyToChangeSender { public static interface Factory { - RevertedSender create(Change.Id id); + RevertedSender create(Project.NameKey project, Change.Id id); } @Inject - public RevertedSender(EmailArguments ea, @Assisted Change.Id id) + public RevertedSender(EmailArguments ea, + @Assisted Project.NameKey project, + @Assisted Change.Id id) throws OrmException { - super(ea, "revert", newChangeData(ea, id)); + super(ea, "revert", newChangeData(ea, project, id)); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index 431852d6b2..51292c973e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -33,6 +33,7 @@ import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.PatchLineComment; 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.server.ApprovalsUtil; @@ -269,7 +270,7 @@ public class ChangeData { } public interface Factory { - ChangeData create(ReviewDb db, Change.Id id); + ChangeData create(ReviewDb db, Project.NameKey project, Change.Id id); ChangeData create(ReviewDb db, Change c); ChangeData create(ReviewDb db, ChangeControl c); } @@ -283,9 +284,10 @@ public class ChangeData { * @param id change ID * @return instance for testing. */ - public static ChangeData createForTest(Change.Id id, int currentPatchSetId) { + public static ChangeData createForTest(Project.NameKey project, Change.Id id, + int currentPatchSetId) { ChangeData cd = new ChangeData(null, null, null, null, null, null, null, - null, null, null, null, null, null, null, id); + null, null, null, null, null, null, null, project, id); cd.currentPatchSet = new PatchSet(new PatchSet.Id(id, currentPatchSetId)); return cd; } @@ -305,6 +307,7 @@ public class ChangeData { private final NotesMigration notesMigration; private final MergeabilityCache mergeabilityCache; private final Change.Id legacyId; + private final Project.NameKey project; private ChangeDataSource returnedBySource; private Change change; private ChangeNotes notes; @@ -345,6 +348,7 @@ public class ChangeData { NotesMigration notesMigration, MergeabilityCache mergeabilityCache, @Assisted ReviewDb db, + @Assisted Project.NameKey project, @Assisted Change.Id id) { this.db = db; this.repoManager = repoManager; @@ -360,7 +364,8 @@ public class ChangeData { this.patchListCache = patchListCache; this.notesMigration = notesMigration; this.mergeabilityCache = mergeabilityCache; - legacyId = id; + this.project = project; + this.legacyId = id; } @AssistedInject @@ -396,6 +401,7 @@ public class ChangeData { this.mergeabilityCache = mergeabilityCache; legacyId = c.getId(); change = c; + project = c.getProject(); } @AssistedInject @@ -433,6 +439,7 @@ public class ChangeData { change = c.getChange(); changeControl = c; notes = c.getNotes(); + project = notes.getProjectName(); } public ReviewDb db() { @@ -536,6 +543,10 @@ public class ChangeData { return legacyId; } + public Project.NameKey getProject() { + return project; + } + boolean fastIsVisibleTo(CurrentUser user) { return visibleTo == user; } @@ -591,14 +602,17 @@ public class ChangeData { } public Change reloadChange() throws OrmException { - change = db.changes().get(legacyId); + notes = notesFactory.create(db, project, legacyId); + change = notes.getChange(); + if (change == null) { + throw new OrmException("Unable to load change " + legacyId); + } return change; } public ChangeNotes notes() throws OrmException { if (notes == null) { - Change c = change(); - notes = notesFactory.create(db, c.getProject(), c.getId()); + notes = notesFactory.create(db, project, legacyId); } return notes; } @@ -681,7 +695,7 @@ public class ChangeData { return false; } String sha1 = ps.getRevision().get(); - try (Repository repo = repoManager.openRepository(change().getProject()); + try (Repository repo = repoManager.openRepository(project); RevWalk walk = new RevWalk(repo)) { RevCommit c = walk.parseCommit(ObjectId.fromString(sha1)); commitMessage = c.getFullMessage(); @@ -790,7 +804,7 @@ public class ChangeData { if (ps == null || !changeControl().isPatchVisible(ps, db)) { return null; } - try (Repository repo = repoManager.openRepository(c.getProject())) { + try (Repository repo = repoManager.openRepository(project)) { Ref ref = repo.getRefDatabase().exactRef(c.getDest().get()); SubmitTypeRecord str = submitTypeRecord(); if (!str.isOk()) { @@ -799,7 +813,7 @@ public class ChangeData { return false; } String mergeStrategy = mergeUtilFactory - .create(projectCache.get(c.getProject())) + .create(projectCache.get(project)) .mergeStrategyName(); mergeable = mergeabilityCache.get( ObjectId.fromString(ps.getRevision().get()), @@ -820,7 +834,7 @@ public class ChangeData { } editsByUser = new HashSet<>(); Change.Id id = change.getId(); - try (Repository repo = repoManager.openRepository(change.getProject())) { + try (Repository repo = repoManager.openRepository(project)) { for (String ref : repo.getRefDatabase().getRefs(RefNames.REFS_USERS).keySet()) { if (Change.Id.fromEditRefPart(ref).equals(id)) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/HasDraftByLegacyPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/HasDraftByLegacyPredicate.java index 578a9f75d6..f85a9ed765 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/HasDraftByLegacyPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/HasDraftByLegacyPredicate.java @@ -57,8 +57,10 @@ class HasDraftByLegacyPredicate extends OperatorPredicate implements } List r = new ArrayList<>(ids.size()); - for (Change.Id id : ids) { - r.add(args.changeDataFactory.create(args.db.get(), id)); + // TODO Don't load the changes directly from the database, but provide + // project name + change ID to changeDataFactory, or delete this predicate. + for (Change c : args.db.get().changes().get(ids)) { + r.add(args.changeDataFactory.create(args.db.get(), c)); } return new ListResultSet<>(r); } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/change/WalkSorterTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/change/WalkSorterTest.java index cb2d5a1244..4f2166d1b8 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/change/WalkSorterTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/change/WalkSorterTest.java @@ -355,7 +355,7 @@ public class WalkSorterTest { throws Exception { Project.NameKey project = tr.getRepository().getDescription().getProject(); Change c = TestChanges.newChange(project, userId); - ChangeData cd = ChangeData.createForTest(c.getId(), 1); + ChangeData cd = ChangeData.createForTest(project, c.getId(), 1); cd.setChange(c); cd.currentPatchSet().setRevision(new RevId(id.name())); cd.setPatchSets(ImmutableList.of(cd.currentPatchSet())); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/ChangeDataTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/ChangeDataTest.java index ca1e2b133b..eb19ebe497 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/ChangeDataTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/ChangeDataTest.java @@ -28,9 +28,9 @@ import org.junit.Test; public class ChangeDataTest { @Test public void setPatchSetsClearsCurrentPatchSet() throws Exception { - ChangeData cd = ChangeData.createForTest(new Change.Id(1), 1); - cd.setChange(TestChanges.newChange( - new Project.NameKey("project"), new Account.Id(1000))); + Project.NameKey project = new Project.NameKey("project"); + ChangeData cd = ChangeData.createForTest(project, new Change.Id(1), 1); + cd.setChange(TestChanges.newChange(project, new Account.Id(1000))); PatchSet curr1 = cd.currentPatchSet(); int currId = curr1.getId().get(); PatchSet ps1 = new PatchSet(new PatchSet.Id(cd.getId(), currId + 1)); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexPathPredicateTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexPathPredicateTest.java index 55d0f38c10..5532108534 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexPathPredicateTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/RegexPathPredicateTest.java @@ -18,6 +18,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gwtorm.server.OrmException; import org.junit.Test; @@ -84,7 +85,8 @@ public class RegexPathPredicateTest { private static ChangeData change(String... files) throws OrmException { Arrays.sort(files); - ChangeData cd = ChangeData.createForTest(new Change.Id(1), 1); + ChangeData cd = ChangeData.createForTest(new Project.NameKey("project"), + new Change.Id(1), 1); cd.setCurrentFilePaths(Arrays.asList(files)); return cd; }