diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index 06639feea6..ed64ce0648 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -49,7 +49,6 @@ import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.notedb.ChangeNoteUtil; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; -import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; @@ -75,8 +74,6 @@ import org.junit.Test; @NoHttpd public class ConsistencyCheckerIT extends AbstractDaemonTest { - @Inject private ChangeControl.GenericFactory changeControlFactory; - @Inject private ChangeNotes.Factory changeNotesFactory; @Inject private Provider checkerProvider; @@ -141,11 +138,8 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { ChangeNotes notes = insertChange(); Project.NameKey name = notes.getProjectName(); - // Create control before deleting repo to avoid NoSuchProjectException - ChangeControl ctl = controlForNotes(notes); ((InMemoryRepositoryManager) repoManager).deleteRepository(name); - - assertThat(checker.check(ctl, null).problems()) + assertThat(checker.check(notes, null).problems()) .containsExactly(problem("Destination repository not found: " + name)); } @@ -961,17 +955,10 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { List expected = new ArrayList<>(1 + rest.length); expected.add(first); expected.addAll(Arrays.asList(rest)); - assertThat(checker.check(controlForNotes(notes), fix).problems()) - .containsExactlyElementsIn(expected) - .inOrder(); + assertThat(checker.check(notes, fix).problems()).containsExactlyElementsIn(expected).inOrder(); } private void assertNoProblems(ChangeNotes notes, @Nullable FixInput fix) throws Exception { - assertThat(checker.check(controlForNotes(notes), fix).problems()).isEmpty(); - } - - /** @return {@link ChangeControl} for notes and admin regardless of owner. */ - private ChangeControl controlForNotes(ChangeNotes notes) throws Exception { - return changeControlFactory.controlFor(notes, userFactory.create(admin.id)); + assertThat(checker.check(notes, fix).problems()).isEmpty(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ActionJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ActionJson.java index f9c4808003..df7a451c00 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ActionJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ActionJson.java @@ -30,10 +30,12 @@ import com.google.gerrit.extensions.restapi.RestView; import com.google.gerrit.extensions.webui.PrivateInternals_UiActionDescription; import com.google.gerrit.extensions.webui.UiAction; import com.google.gerrit.reviewdb.client.Change.Status; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.extensions.webui.UiActions; -import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.Singleton; import java.util.ArrayList; import java.util.Collections; @@ -49,6 +51,7 @@ public class ActionJson { private final UiActions uiActions; private final DynamicMap> changeViews; private final DynamicSet visitorSet; + private final Provider userProvider; @Inject ActionJson( @@ -57,13 +60,15 @@ public class ActionJson { ChangeResource.Factory changeResourceFactory, UiActions uiActions, DynamicMap> changeViews, - DynamicSet visitorSet) { + DynamicSet visitorSet, + Provider userProvider) { this.revisions = revisions; this.changeJsonFactory = changeJsonFactory; this.changeResourceFactory = changeResourceFactory; this.uiActions = uiActions; this.changeViews = changeViews; this.visitorSet = visitorSet; + this.userProvider = userProvider; } public Map format(RevisionResource rsrc) throws OrmException { @@ -86,9 +91,9 @@ public class ActionJson { return Lists.newArrayList(visitorSet); } - public ChangeInfo addChangeActions(ChangeInfo to, ChangeControl ctl) { + public ChangeInfo addChangeActions(ChangeInfo to, ChangeNotes notes) { List visitors = visitors(); - to.actions = toActionMap(ctl, visitors, copy(visitors, to)); + to.actions = toActionMap(notes, visitors, copy(visitors, to)); return to; } @@ -158,19 +163,20 @@ public class ActionJson { } private Map toActionMap( - ChangeControl ctl, List visitors, ChangeInfo changeInfo) { + ChangeNotes notes, List visitors, ChangeInfo changeInfo) { + CurrentUser user = userProvider.get(); Map out = new LinkedHashMap<>(); - if (!ctl.getUser().isIdentifiedUser()) { + if (!user.isIdentifiedUser()) { return out; } Iterable descs = - uiActions.from(changeViews, changeResourceFactory.create(ctl.getNotes(), ctl.getUser())); + uiActions.from(changeViews, changeResourceFactory.create(notes, user)); // The followup action is a client-side only operation that does not // have a server side handler. It must be manually registered into the // resulting action map. - Status status = ctl.getChange().getStatus(); + Status status = notes.getChange().getStatus(); if (status.isOpen() || status.equals(Status.MERGED)) { UiAction.Description descr = new UiAction.Description(); PrivateInternals_UiActionDescription.setId(descr, "followup"); 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 0a7738d196..ffea6048ad 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 @@ -444,7 +444,7 @@ public class ChangeJson { return info; } - ConsistencyChecker.Result result = checkerProvider.get().check(ctl, fix); + ConsistencyChecker.Result result = checkerProvider.get().check(ctl.getNotes(), fix); ChangeInfo info; Change c = result.change(); if (c != null) { @@ -480,7 +480,7 @@ public class ChangeJson { ChangeControl ctl = cd.changeControl().forUser(user); if (has(CHECK)) { - out.problems = checkerProvider.get().check(ctl, fix).problems(); + out.problems = checkerProvider.get().check(ctl.getNotes(), fix).problems(); // If any problems were fixed, the ChangeData needs to be reloaded. for (ProblemInfo p : out.problems) { if (p.status == ProblemInfo.Status.FIXED) { @@ -604,7 +604,7 @@ public class ChangeJson { } if (has(CURRENT_ACTIONS) || has(CHANGE_ACTIONS)) { - actionJson.addChangeActions(out, ctl); + actionJson.addChangeActions(out, ctl.getNotes()); } if (has(TRACKING_IDS)) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java index 76d55508b1..a149935fcc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -50,7 +50,6 @@ import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.PatchSetState; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; -import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; @@ -93,8 +92,9 @@ public class ConsistencyChecker { @AutoValue public abstract static class Result { - private static Result create(ChangeControl ctl, List problems) { - return new AutoValue_ConsistencyChecker_Result(ctl.getId(), ctl.getChange(), problems); + private static Result create(ChangeNotes notes, List problems) { + return new AutoValue_ConsistencyChecker_Result( + notes.getChangeId(), notes.getChange(), problems); } public abstract Change.Id id(); @@ -105,7 +105,6 @@ public class ConsistencyChecker { public abstract List problems(); } - private final ChangeControl.GenericFactory changeControlFactory; private final ChangeNotes.Factory notesFactory; private final Accounts accounts; private final DynamicItem accountPatchReviewStore; @@ -120,7 +119,7 @@ public class ConsistencyChecker { private BatchUpdate.Factory updateFactory; private FixInput fix; - private ChangeControl ctl; + private ChangeNotes notes; private Repository repo; private RevWalk rw; private ObjectInserter oi; @@ -135,7 +134,6 @@ public class ConsistencyChecker { @Inject ConsistencyChecker( @GerritPersonIdent Provider serverIdent, - ChangeControl.GenericFactory changeControlFactory, ChangeNotes.Factory notesFactory, Accounts accounts, DynamicItem accountPatchReviewStore, @@ -148,7 +146,6 @@ public class ConsistencyChecker { RetryHelper retryHelper) { this.accounts = accounts; this.accountPatchReviewStore = accountPatchReviewStore; - this.changeControlFactory = changeControlFactory; this.db = db; this.notesFactory = notesFactory; this.patchSetInfoFactory = patchSetInfoFactory; @@ -163,25 +160,25 @@ public class ConsistencyChecker { private void reset() { updateFactory = null; - ctl = null; + notes = null; repo = null; rw = null; problems = new ArrayList<>(); } private Change change() { - return ctl.getChange(); + return notes.getChange(); } - public Result check(ChangeControl cc, @Nullable FixInput f) { - checkNotNull(cc); + public Result check(ChangeNotes notes, @Nullable FixInput f) { + checkNotNull(notes); try { return retryHelper.execute( buf -> { try { reset(); this.updateFactory = buf; - ctl = cc; + this.notes = notes; fix = f; checkImpl(); return result(); @@ -197,15 +194,15 @@ public class ConsistencyChecker { } }); } catch (RestApiException e) { - return logAndReturnOneProblem(e, cc, "Error checking change: " + e.getMessage()); + return logAndReturnOneProblem(e, notes, "Error checking change: " + e.getMessage()); } catch (UpdateException e) { - return logAndReturnOneProblem(e, cc, "Error checking change"); + return logAndReturnOneProblem(e, notes, "Error checking change"); } } - private Result logAndReturnOneProblem(Exception e, ChangeControl cc, String problem) { - log.warn("Error checking change " + cc.getId(), e); - return Result.create(cc, ImmutableList.of(problem(problem))); + private Result logAndReturnOneProblem(Exception e, ChangeNotes notes, String problem) { + log.warn("Error checking change " + notes.getChangeId(), e); + return Result.create(notes, ImmutableList.of(problem(problem))); } private void checkImpl() { @@ -234,7 +231,7 @@ public class ConsistencyChecker { private void checkCurrentPatchSetEntity() { try { - currPs = psUtil.current(db.get(), ctl.getNotes()); + currPs = psUtil.current(db.get(), notes); if (currPs == null) { problem( String.format("Current patch set %d not found", change().currentPatchSetId().get())); @@ -262,7 +259,7 @@ public class ConsistencyChecker { List all; try { // Iterate in descending order. - all = PS_ID_ORDER.sortedCopy(psUtil.byChange(db.get(), ctl.getNotes())); + all = PS_ID_ORDER.sortedCopy(psUtil.byChange(db.get(), notes)); } catch (OrmException e) { return error("Failed to look up patch sets", e); } @@ -515,7 +512,7 @@ public class ConsistencyChecker { (psIdToDelete != null && reuseOldPsId) ? psIdToDelete : ChangeUtil.nextPatchSetId(repo, change().currentPatchSetId()); - PatchSetInserter inserter = patchSetInserterFactory.create(ctl.getNotes(), psId, commit); + PatchSetInserter inserter = patchSetInserterFactory.create(notes, psId, commit); try (BatchUpdate bu = newBatchUpdate()) { bu.setRepository(repo, rw, oi); @@ -523,7 +520,7 @@ public class ConsistencyChecker { // Delete the given patch set ref. If reuseOldPsId is true, // PatchSetInserter will reinsert the same ref, making it a no-op. bu.addOp( - ctl.getId(), + notes.getChangeId(), new BatchUpdateOp() { @Override public void updateRepo(RepoContext ctx) throws IOException { @@ -532,23 +529,23 @@ public class ConsistencyChecker { }); if (!reuseOldPsId) { bu.addOp( - ctl.getId(), + notes.getChangeId(), new DeletePatchSetFromDbOp(checkNotNull(deleteOldPatchSetProblem), psIdToDelete)); } } bu.addOp( - ctl.getId(), + notes.getChangeId(), inserter .setValidate(false) .setFireRevisionCreated(false) .setNotify(NotifyHandling.NONE) .setAllowClosed(true) .setMessage("Patch set for merged commit inserted by consistency checker")); - bu.addOp(ctl.getId(), new FixMergedOp(notFound)); + bu.addOp(notes.getChangeId(), new FixMergedOp(notFound)); bu.execute(); } - ctl = changeControlFactory.controlFor(db.get(), inserter.getChange(), ctl.getUser()); + notes = notesFactory.createChecked(db.get(), inserter.getChange()); insertPatchSetProblem.status = Status.FIXED; insertPatchSetProblem.outcome = "Inserted as patch set " + psId.get(); } catch (OrmException | IOException | UpdateException | RestApiException e) { @@ -581,17 +578,17 @@ public class ConsistencyChecker { private void fixMerged(ProblemInfo p) { try (BatchUpdate bu = newBatchUpdate()) { bu.setRepository(repo, rw, oi); - bu.addOp(ctl.getId(), new FixMergedOp(p)); + bu.addOp(notes.getChangeId(), new FixMergedOp(p)); bu.execute(); } catch (UpdateException | RestApiException e) { - log.warn("Error marking " + ctl.getId() + "as merged", e); + log.warn("Error marking " + notes.getChangeId() + "as merged", e); p.status = Status.FIX_FAILED; p.outcome = "Error updating status to merged"; } } private BatchUpdate newBatchUpdate() { - return updateFactory.create(db.get(), change().getProject(), ctl.getUser(), TimeUtil.nowTs()); + return updateFactory.create(db.get(), change().getProject(), user.get(), TimeUtil.nowTs()); } private void fixPatchSetRef(ProblemInfo p, PatchSet ps) { @@ -635,10 +632,10 @@ public class ConsistencyChecker { try (BatchUpdate bu = newBatchUpdate()) { bu.setRepository(repo, rw, oi); for (DeletePatchSetFromDbOp op : ops) { - checkArgument(op.psId.getParentKey().equals(ctl.getId())); - bu.addOp(ctl.getId(), op); + checkArgument(op.psId.getParentKey().equals(notes.getChangeId())); + bu.addOp(notes.getChangeId(), op); } - bu.addOp(ctl.getId(), new UpdateCurrentPatchSetOp(ops)); + bu.addOp(notes.getChangeId(), new UpdateCurrentPatchSetOp(ops)); bu.execute(); } catch (NoPatchSetsWouldRemainException e) { for (DeletePatchSetFromDbOp op : ops) { @@ -779,10 +776,10 @@ public class ConsistencyChecker { } private void warn(Throwable t) { - log.warn("Error in consistency check of change " + ctl.getId(), t); + log.warn("Error in consistency check of change " + notes.getChangeId(), t); } private Result result() { - return Result.create(ctl, problems); + return Result.create(notes, problems); } }