From dd31db99c33be54ea6fabd3cfd57ee08d05c9b52 Mon Sep 17 00:00:00 2001 From: Gal Paikin Date: Fri, 6 Dec 2019 14:43:35 +0100 Subject: [PATCH] Change the structure of Submission Id Currently Submission Id is constructed by the change-id of the change that triggered the submission, the timestamp, and a random alphanumeric string. This is not pretty. Since we recently revealed the submission id via ChangeInfo in Ic557e0c94 and via queries in I337f7f8ae, it's a good idea to change the submission id to be more user friendly. Submission Id is now constructed of "-" or "" if topic doesn't exist for that change. The change id is used because it makes the submission id unique (a change can only be submitted once). The topic is used mostly since this is currently one of the main use cases for Revert Submission. Change-Id: Iec8b0db66adf9a7ff2809994acaedef09e4e885c --- Documentation/rest-api-changes.txt | 4 +++ .../google/gerrit/entities/SubmissionId.java | 34 +++++++++++++++++++ .../gerrit/server/change/ChangeInserter.java | 4 +-- .../server/change/ConsistencyChecker.java | 4 +-- .../gerrit/server/git/MergedByPushOp.java | 10 +++--- .../server/git/receive/ReceiveCommits.java | 14 ++++---- .../gerrit/server/git/receive/ReplaceOp.java | 10 ++++-- .../gerrit/server/notedb/ChangeUpdate.java | 10 +++--- .../google/gerrit/server/submit/MergeOp.java | 5 +-- .../gerrit/server/submit/SubmitStrategy.java | 8 ++--- .../server/submit/SubmitStrategyFactory.java | 4 +-- .../server/submit/SubmitStrategyOp.java | 2 +- .../server/change/ConsistencyCheckerIT.java | 6 ++-- .../gerrit/server/notedb/ChangeNotesTest.java | 20 +++++------ .../notedb/CommitMessageOutputTest.java | 14 +++----- .../gerrit/server/update/BatchUpdateTest.java | 4 +-- 16 files changed, 95 insertions(+), 58 deletions(-) create mode 100644 java/com/google/gerrit/entities/SubmissionId.java diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 0fc733abfa..67ae65c9cc 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -6017,6 +6017,10 @@ When present, change has been marked Ready at some point in time. The numeric Change-Id of the change that this change reverts. |`submission_id` |optional| ID of the submission of this change. Only set if the status is `MERGED`. +This ID is equal to the numeric ID of the change that triggered the submission. +If the change that triggered the submission also has a topic, it will be +"-" of the change that triggered the submission. +The callers must not rely on the format of the submission ID. |================================== [[change-input]] diff --git a/java/com/google/gerrit/entities/SubmissionId.java b/java/com/google/gerrit/entities/SubmissionId.java new file mode 100644 index 0000000000..eb03a5a97b --- /dev/null +++ b/java/com/google/gerrit/entities/SubmissionId.java @@ -0,0 +1,34 @@ +// Copyright (C) 2019 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.entities; + +import org.eclipse.jgit.annotations.Nullable; + +public class SubmissionId { + private final String submissionId; + + public SubmissionId(Change.Id changeId, @Nullable String topic) { + submissionId = topic != null ? String.format("%s-%s", changeId, topic) : changeId.toString(); + } + + public SubmissionId(Change change) { + this(change.getId(), change.getTopic()); + } + + @Override + public String toString() { + return submissionId; + } +} diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java index 42633731d7..d1c27d5a28 100644 --- a/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/java/com/google/gerrit/server/change/ChangeInserter.java @@ -37,6 +37,7 @@ import com.google.gerrit.entities.ChangeMessage; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.PatchSetApproval; import com.google.gerrit.entities.PatchSetInfo; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.restapi.ResourceConflictException; @@ -55,7 +56,6 @@ import com.google.gerrit.server.extensions.events.RevisionCreated; import com.google.gerrit.server.git.GroupCollector; import com.google.gerrit.server.git.validators.CommitValidationException; import com.google.gerrit.server.git.validators.CommitValidators; -import com.google.gerrit.server.logging.RequestId; import com.google.gerrit.server.mail.send.CreateChangeSender; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.patch.PatchSetInfoFactory; @@ -396,7 +396,7 @@ public class ChangeInserter implements InsertChangeOp { * instead of setting the status directly? */ if (change.getStatus() == Change.Status.MERGED) { - update.fixStatusToMerged(new RequestId(ctx.getChange().getId().toString())); + update.fixStatusToMerged(new SubmissionId(change)); } else { update.setStatus(change.getStatus()); } diff --git a/java/com/google/gerrit/server/change/ConsistencyChecker.java b/java/com/google/gerrit/server/change/ConsistencyChecker.java index 19db5eece3..513f5ca95c 100644 --- a/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -33,6 +33,7 @@ import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.api.changes.FixInput; import com.google.gerrit.extensions.common.ProblemInfo; @@ -44,7 +45,6 @@ import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.account.Accounts; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.logging.RequestId; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.PatchSetState; import com.google.gerrit.server.patch.PatchSetInfoFactory; @@ -578,7 +578,7 @@ public class ConsistencyChecker { public boolean updateChange(ChangeContext ctx) { ctx.getChange().setStatus(Change.Status.MERGED); ctx.getUpdate(ctx.getChange().currentPatchSetId()) - .fixStatusToMerged(new RequestId(ctx.getChange().getId().toString())); + .fixStatusToMerged(new SubmissionId(ctx.getChange())); p.status = Status.FIXED; p.outcome = "Marked change as merged"; return true; diff --git a/java/com/google/gerrit/server/git/MergedByPushOp.java b/java/com/google/gerrit/server/git/MergedByPushOp.java index 9aebebf57d..01d5380fc7 100644 --- a/java/com/google/gerrit/server/git/MergedByPushOp.java +++ b/java/com/google/gerrit/server/git/MergedByPushOp.java @@ -22,11 +22,11 @@ import com.google.gerrit.entities.ChangeMessage; import com.google.gerrit.entities.LabelId; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.PatchSetInfo; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.config.SendEmailExecutor; import com.google.gerrit.server.extensions.events.ChangeMerged; -import com.google.gerrit.server.logging.RequestId; import com.google.gerrit.server.mail.send.MergedSender; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.patch.PatchSetInfoFactory; @@ -52,7 +52,7 @@ public class MergedByPushOp implements BatchUpdateOp { MergedByPushOp create( RequestScopePropagator requestScopePropagator, PatchSet.Id psId, - @Assisted RequestId submissionId, + @Assisted SubmissionId submissionId, @Assisted("refName") String refName, @Assisted("mergeResultRevId") String mergeResultRevId); } @@ -66,7 +66,7 @@ public class MergedByPushOp implements BatchUpdateOp { private final ChangeMerged changeMerged; private final PatchSet.Id psId; - private final RequestId submissionId; + private final SubmissionId submissionId; private final String refName; private final String mergeResultRevId; @@ -86,7 +86,7 @@ public class MergedByPushOp implements BatchUpdateOp { ChangeMerged changeMerged, @Assisted RequestScopePropagator requestScopePropagator, @Assisted PatchSet.Id psId, - @Assisted RequestId submissionId, + @Assisted SubmissionId submissionId, @Assisted("refName") String refName, @Assisted("mergeResultRevId") String mergeResultRevId) { this.patchSetInfoFactory = patchSetInfoFactory; @@ -137,7 +137,7 @@ public class MergedByPushOp implements BatchUpdateOp { } change.setCurrentPatchSet(info); change.setStatus(Change.Status.MERGED); - change.setSubmissionId(submissionId.toStringForStorage()); + change.setSubmissionId(submissionId.toString()); // we cannot reconstruct the submit records for when this change was // submitted, this is why we must fix the status and other details. update.fixStatusToMerged(submissionId); diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index cec9e4e818..5155925ce2 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -74,6 +74,7 @@ import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.PatchSetInfo; import com.google.gerrit.entities.Project; import com.google.gerrit.entities.RefNames; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.api.changes.HashtagsInput; import com.google.gerrit.extensions.api.changes.NotifyHandling; @@ -3023,7 +3024,8 @@ class ReceiveCommits { info, groups, magicBranch, - receivePack.getPushCertificate()) + receivePack.getPushCertificate(), + notes.getChange()) .setRequestScopePropagator(requestScopePropagator); bu.addOp(notes.getChangeId(), replaceOp); if (progress != null) { @@ -3281,7 +3283,7 @@ class ReceiveCommits { int existingPatchSets = 0; int newPatchSets = 0; - RequestId submissionId = null; + SubmissionId submissionId = null; COMMIT: for (RevCommit c; (c = rw.next()) != null; ) { rw.parseBody(c); @@ -3290,10 +3292,10 @@ class ReceiveCommits { receivePackRefCache.tipsFromObjectId(c.copy(), RefNames.REFS_CHANGES)) { PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName()); Optional notes = getChangeNotes(psId.changeId()); - if (submissionId == null) { - submissionId = new RequestId(psId.changeId().toString()); - } if (notes.isPresent() && notes.get().getChange().getDest().equals(branch)) { + if (submissionId == null) { + submissionId = new SubmissionId(notes.get().getChange()); + } existingPatchSets++; bu.addOp(notes.get().getChangeId(), setPrivateOpFactory.create(false, null)); bu.addOp( @@ -3333,7 +3335,7 @@ class ReceiveCommits { continue; } if (submissionId == null) { - submissionId = new RequestId(id.toString()); + submissionId = new SubmissionId(req.notes.getChange()); } req.addOps(bu, null); bu.addOp(id, setPrivateOpFactory.create(false, null)); diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java index 6c0d5d374d..24154d605a 100644 --- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java +++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java @@ -36,6 +36,7 @@ import com.google.gerrit.entities.Comment; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.PatchSetApproval; import com.google.gerrit.entities.PatchSetInfo; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.extensions.api.changes.AddReviewerInput; import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.client.ChangeKind; @@ -61,7 +62,6 @@ import com.google.gerrit.server.extensions.events.CommentAdded; import com.google.gerrit.server.extensions.events.RevisionCreated; import com.google.gerrit.server.git.MergedByPushOp; import com.google.gerrit.server.git.receive.ReceiveCommits.MagicBranchInput; -import com.google.gerrit.server.logging.RequestId; import com.google.gerrit.server.mail.MailUtil.MailRecipients; import com.google.gerrit.server.mail.send.ReplacePatchSetSender; import com.google.gerrit.server.notedb.ChangeNotes; @@ -110,7 +110,8 @@ public class ReplaceOp implements BatchUpdateOp { PatchSetInfo info, List groups, @Nullable MagicBranchInput magicBranch, - @Nullable PushCertificate pushCertificate); + @Nullable PushCertificate pushCertificate, + Change change); } private static final String CHANGE_IS_CLOSED = "change is closed"; @@ -143,6 +144,7 @@ public class ReplaceOp implements BatchUpdateOp { private final PatchSetInfo info; private final MagicBranchInput magicBranch; private final PushCertificate pushCertificate; + private final Change change; private List groups; private final Map approvals = new HashMap<>(); @@ -177,6 +179,7 @@ public class ReplaceOp implements BatchUpdateOp { ProjectCache projectCache, @SendEmailExecutor ExecutorService sendEmailExecutor, ReviewerAdder reviewerAdder, + Change change, @Assisted ProjectState projectState, @Assisted BranchNameKey dest, @Assisted boolean checkMergedInto, @@ -218,6 +221,7 @@ public class ReplaceOp implements BatchUpdateOp { this.groups = groups; this.magicBranch = magicBranch; this.pushCertificate = pushCertificate; + this.change = change; } @Override @@ -239,7 +243,7 @@ public class ReplaceOp implements BatchUpdateOp { mergedByPushOpFactory.create( requestScopePropagator, patchSetId, - new RequestId(patchSetId.changeId().toString()), + new SubmissionId(change), mergedInto, mergeResultRevId); } diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 02a4dcc59c..6a900c0c85 100644 --- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -55,11 +55,11 @@ import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Comment; import com.google.gerrit.entities.Project; import com.google.gerrit.entities.RobotComment; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.mail.Address; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; -import com.google.gerrit.server.logging.RequestId; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.util.LabelVote; import com.google.inject.assistedinject.Assisted; @@ -220,10 +220,10 @@ public class ChangeUpdate extends AbstractChangeUpdate { this.status = status; } - public void fixStatusToMerged(RequestId submissionId) { + public void fixStatusToMerged(SubmissionId submissionId) { checkArgument(submissionId != null, "submission id must be set for merged changes"); this.status = Change.Status.MERGED; - this.submissionId = submissionId.toStringForStorage(); + this.submissionId = submissionId.toString(); } public void putApproval(String label, short value) { @@ -242,9 +242,9 @@ public class ChangeUpdate extends AbstractChangeUpdate { approvals.put(label, reviewer, Optional.empty()); } - public void merge(RequestId submissionId, Iterable submitRecords) { + public void merge(SubmissionId submissionId, Iterable submitRecords) { this.status = Change.Status.MERGED; - this.submissionId = submissionId.toStringForStorage(); + this.submissionId = submissionId.toString(); this.submitRecords = ImmutableList.copyOf(submitRecords); checkArgument(!this.submitRecords.isEmpty(), "no submit records specified at submit time"); } diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index 75ae62d605..8bdd98e2da 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java @@ -41,6 +41,7 @@ import com.google.gerrit.entities.Change; import com.google.gerrit.entities.ChangeMessage; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.api.changes.SubmitInput; @@ -237,7 +238,7 @@ public class MergeOp implements AutoCloseable { private final Map updatedChanges; private Timestamp ts; - private RequestId submissionId; + private SubmissionId submissionId; private IdentifiedUser caller; private MergeOpRepoManager orm; @@ -449,7 +450,7 @@ public class MergeOp implements AutoCloseable { this.dryrun = dryrun; this.caller = caller; this.ts = TimeUtil.nowTs(); - this.submissionId = new RequestId(change.getId().toString()); + this.submissionId = new SubmissionId(change); try (TraceContext traceContext = TraceContext.open().addTag(RequestId.Type.SUBMISSION_ID, submissionId)) { diff --git a/java/com/google/gerrit/server/submit/SubmitStrategy.java b/java/com/google/gerrit/server/submit/SubmitStrategy.java index 4c68e1b521..efb2f769c9 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategy.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategy.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Sets; import com.google.gerrit.entities.BranchNameKey; import com.google.gerrit.entities.Change; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.config.FactoryModule; @@ -42,7 +43,6 @@ import com.google.gerrit.server.git.MergeTip; import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.git.TagCache; import com.google.gerrit.server.git.validators.OnSubmitValidators; -import com.google.gerrit.server.logging.RequestId; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectConfig; @@ -94,7 +94,7 @@ public abstract class SubmitStrategy { RevFlag canMergeFlag, Set alreadyAccepted, Set incoming, - RequestId submissionId, + SubmissionId submissionId, SubmitInput submitInput, SubmoduleOp submoduleOp, boolean dryrun); @@ -125,7 +125,7 @@ public abstract class SubmitStrategy { final MergeTip mergeTip; final RevFlag canMergeFlag; final Set alreadyAccepted; - final RequestId submissionId; + final SubmissionId submissionId; final SubmitType submitType; final SubmitInput submitInput; final SubmoduleOp submoduleOp; @@ -164,7 +164,7 @@ public abstract class SubmitStrategy { @Assisted RevFlag canMergeFlag, @Assisted Set alreadyAccepted, @Assisted Set incoming, - @Assisted RequestId submissionId, + @Assisted SubmissionId submissionId, @Assisted SubmitType submitType, @Assisted SubmitInput submitInput, @Assisted SubmoduleOp submoduleOp, diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyFactory.java b/java/com/google/gerrit/server/submit/SubmitStrategyFactory.java index cba572bcb7..a015665b6f 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategyFactory.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategyFactory.java @@ -16,13 +16,13 @@ package com.google.gerrit.server.submit; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.BranchNameKey; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; import com.google.gerrit.server.git.MergeTip; -import com.google.gerrit.server.logging.RequestId; import com.google.gerrit.server.submit.MergeOp.CommitStatus; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -52,7 +52,7 @@ public class SubmitStrategyFactory { IdentifiedUser caller, MergeTip mergeTip, CommitStatus commitStatus, - RequestId submissionId, + SubmissionId submissionId, SubmitInput submitInput, SubmoduleOp submoduleOp, boolean dryrun) diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java index b7bc58a34a..6be4a91bc3 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java @@ -453,7 +453,7 @@ abstract class SubmitStrategyOp implements BatchUpdateOp { Change c = ctx.getChange(); logger.atFine().log("Setting change %s merged", c.getId()); c.setStatus(Change.Status.MERGED); - c.setSubmissionId(args.submissionId.toStringForStorage()); + c.setSubmissionId(args.submissionId.toString()); // TODO(dborowitz): We need to be able to change the author of the message, // which is not the user from the update context. addMergedMessage was able diff --git a/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index 08719d36ce..ee5f1179d1 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -29,6 +29,7 @@ import com.google.gerrit.entities.Account; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.RefNames; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.extensions.api.changes.FixInput; import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.common.ChangeInfo; @@ -39,7 +40,6 @@ import com.google.gerrit.server.change.ChangeInserter; import com.google.gerrit.server.change.ConsistencyChecker; import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.change.PatchSetInserter; -import com.google.gerrit.server.logging.RequestId; import com.google.gerrit.server.notedb.ChangeNoteUtil; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.Sequences; @@ -315,7 +315,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { public boolean updateChange(ChangeContext ctx) { ctx.getChange().setStatus(Change.Status.MERGED); ctx.getUpdate(ctx.getChange().currentPatchSetId()) - .fixStatusToMerged(new RequestId(ctx.getChange().getId().toString())); + .fixStatusToMerged(new SubmissionId(ctx.getChange())); return true; } }); @@ -865,7 +865,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { public boolean updateChange(ChangeContext ctx) { ctx.getChange().setStatus(Change.Status.MERGED); ctx.getUpdate(ctx.getChange().currentPatchSetId()) - .fixStatusToMerged(new RequestId(ctx.getChange().getId().toString())); + .fixStatusToMerged(new SubmissionId(ctx.getChange())); return true; } }); diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java index 145e914e7e..db0dec898c 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -45,6 +45,7 @@ import com.google.gerrit.entities.Comment; import com.google.gerrit.entities.CommentRange; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.PatchSetApproval; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.mail.Address; import com.google.gerrit.server.AssigneeStatusUpdate; @@ -52,7 +53,6 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.config.GerritServerId; -import com.google.gerrit.server.logging.RequestId; import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk; import com.google.gerrit.server.util.time.TimeUtil; import com.google.gerrit.testing.TestChanges; @@ -432,7 +432,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { @Test public void approvalsPostSubmit() throws Exception { Change c = newChange(); - RequestId submissionId = submissionId(c); + SubmissionId submissionId = new SubmissionId(c); ChangeUpdate update = newUpdate(c, changeOwner); update.putApproval("Code-Review", (short) 1); update.putApproval("Verified", (short) 1); @@ -467,7 +467,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { @Test public void approvalsDuringSubmit() throws Exception { Change c = newChange(); - RequestId submissionId = submissionId(c); + SubmissionId submissionId = new SubmissionId(c); ChangeUpdate update = newUpdate(c, changeOwner); update.putApproval("Code-Review", (short) 1); update.putApproval("Verified", (short) 1); @@ -604,7 +604,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { @Test public void submitRecords() throws Exception { Change c = newChange(); - RequestId submissionId = submissionId(c); + SubmissionId submissionId = new SubmissionId(c); ChangeUpdate update = newUpdate(c, changeOwner); update.setSubjectForCommit("Submit patch set 1"); @@ -640,13 +640,13 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { null, submitLabel("Verified", "OK", changeOwner.getAccountId()), submitLabel("Alternative-Code-Review", "NEED", null))); - assertThat(notes.getChange().getSubmissionId()).isEqualTo(submissionId.toStringForStorage()); + assertThat(notes.getChange().getSubmissionId()).isEqualTo(submissionId.toString()); } @Test public void latestSubmitRecordsOnly() throws Exception { Change c = newChange(); - RequestId submissionId = submissionId(c); + SubmissionId submissionId = new SubmissionId(c); ChangeUpdate update = newUpdate(c, changeOwner); update.setSubjectForCommit("Submit patch set 1"); update.merge( @@ -669,7 +669,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { assertThat(notes.getSubmitRecords()) .containsExactly( submitRecord("OK", null, submitLabel("Code-Review", "OK", changeOwner.getAccountId()))); - assertThat(notes.getChange().getSubmissionId()).isEqualTo(submissionId.toStringForStorage()); + assertThat(notes.getChange().getSubmissionId()).isEqualTo(submissionId.toString()); } @Test @@ -977,7 +977,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { // Finish off by merging the change. update = newUpdate(c, changeOwner); update.merge( - submissionId(c), + new SubmissionId(c), ImmutableList.of( submitRecord( "NOT_READY", @@ -3140,8 +3140,4 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { update.commit(); return tr.parseBody(commit); } - - private RequestId submissionId(Change c) { - return new RequestId(c.getId().toString()); - } } diff --git a/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java index 97781a4a34..5e25c131ef 100644 --- a/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java +++ b/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java @@ -21,9 +21,9 @@ import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import com.google.common.collect.ImmutableList; import com.google.gerrit.entities.Account; import com.google.gerrit.entities.Change; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.mail.Address; import com.google.gerrit.server.CurrentUser; -import com.google.gerrit.server.logging.RequestId; import com.google.gerrit.server.util.time.TimeUtil; import com.google.gerrit.testing.ConfigSuite; import com.google.gerrit.testing.TestChanges; @@ -151,7 +151,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { ChangeUpdate update = newUpdate(c, changeOwner); update.setSubjectForCommit("Submit patch set 1"); - RequestId submissionId = submissionId(c); + SubmissionId submissionId = new SubmissionId(c); update.merge( submissionId, ImmutableList.of( @@ -174,7 +174,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { + "Patch-set: 1\n" + "Status: merged\n" + "Submission-id: " - + submissionId.toStringForStorage() + + submissionId.toString() + "\n" + "Submitted-with: NOT_READY\n" + "Submitted-with: OK: Verified: Gerrit User 1 <1@gerrit>\n" @@ -223,7 +223,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { ChangeUpdate update = newUpdate(c, changeOwner); update.setSubjectForCommit("Submit patch set 1"); - RequestId submissionId = submissionId(c); + SubmissionId submissionId = new SubmissionId(c); update.merge( submissionId, ImmutableList.of(submitRecord("RULE_ERROR", "Problem with patch set:\n1"))); update.commit(); @@ -234,7 +234,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { + "Patch-set: 1\n" + "Status: merged\n" + "Submission-id: " - + submissionId.toStringForStorage() + + submissionId.toString() + "\n" + "Submitted-with: RULE_ERROR Problem with patch set: 1\n", update.getResult()); @@ -427,8 +427,4 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { RevCommit commit = parseCommit(commitId); assertThat(commit.getFullMessage()).isEqualTo(expected); } - - private RequestId submissionId(Change c) { - return new RequestId(c.getId().toString()); - } } diff --git a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java index 5860c48d60..c7ca887c1a 100644 --- a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java +++ b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java @@ -26,12 +26,12 @@ import com.google.gerrit.entities.Change; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; import com.google.gerrit.entities.RefNames; +import com.google.gerrit.entities.SubmissionId; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.change.ChangeInserter; import com.google.gerrit.server.change.PatchSetInserter; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.logging.RequestId; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.Sequences; @@ -332,7 +332,7 @@ public class BatchUpdateTest { cr.label = "Code-Review"; sr.labels = ImmutableList.of(cr); ChangeUpdate update = ctx.getUpdate(ctx.getChange().currentPatchSetId()); - update.merge(new RequestId(), ImmutableList.of(sr)); + update.merge(new SubmissionId(ctx.getChange()), ImmutableList.of(sr)); update.setChangeMessage("Submitted"); return true; }