Support fixing corrupt changes in ConsistencyChecker

To fix changes, pass an appropriate input to the checker; for now this
is empty, but may include things like a strategy to choose when there
are multiple options. (For example, if multiple patch sets have the
same SHA-1, we could delete all but the newest, or all but the
oldest.)

Change-Id: I016a94e27b8f6b63b3662b46271c01ed86110ec3

Documentation/rest-api-changes.txt

@@ -1192,6 +1192,60 @@ missing from the result. At least `id`, `project`, `branch`, and
   }
 ----
 
+[[fix-change]]
+=== Fix change
+--
+'POST /changes/link:#change-id[\{change-id\}]/check'
+--
+
+Performs consistency checks on the change as with link:#check-change[GET
+/check], and additionally fixes any problems that can be fixed
+automatically. The returned field values reflect any fixes.
+
+Only the change owner, a project owner, or an administrator may fix changes.
+
+.Request
+----
+  POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/check HTTP/1.0
+----
+
+.Response
+----
+  HTTP/1.1 200 OK
+  Content-Disposition: attachment
+  Content-Type: application/json;charset=UTF-8
+
+  )]}'
+  {
+    "id": "myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940",
+    "project": "myProject",
+    "branch": "master",
+    "change_id": "I8473b95934b5732ac55d26311a706c9c2bde9940",
+    "subject": "Implementing Feature X",
+    "status": "MERGED",
+    "created": "2013-02-01 09:59:32.126000000",
+    "updated": "2013-02-21 11:16:36.775000000",
+    "mergeable": true,
+    "insertions": 34,
+    "deletions": 101,
+    "_sortkey": "0023412400000f7d",
+    "_number": 3965,
+    "owner": {
+      "name": "John Doe"
+    },
+    "problems": [
+      {
+        "message": "Current patch set 2 not found"
+      },
+      {
+        "message": "Patch set 1 (1eee2c9d8f352483781e772f35dc586a69ff5646) is merged into destination ref master (1eee2c9d8f352483781e772f35dc586a69ff5646), but change status is NEW",
+        "status": FIXED,
+        "outcome": "Marked change as merged"
+      }
+    ]
+  }
+----
+
 [[edit-endpoints]]
 == Change Edit Endpoints
 
@@ -3998,10 +4052,16 @@ with a change. These are not related to the code review process, but rather
 indicate some inconsistency in Gerrit's database or repository metadata related
 to the enclosing change.
 
-[options="header",cols="1,6"]
+[options="header",cols="1,^1,5"]
 |===========================
-|Field Name|Description
+|Field Name||Description
-|`message` |Plaintext message describing the problem with the change.
+|`message` ||Plaintext message describing the problem with the change.
+|`status`  |optional|
+The status of fixing the problem (`FIXED`, `FIX_FAILED`). Only set if a
+fix was attempted.
+|`outcome` |optional|
+If `status` is set, an additional plaintext message describing the
+outcome of the fix.
 |===========================
 
 GERRIT

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/CheckIT.java

@@ -19,6 +19,10 @@ import static com.google.common.truth.Truth.assertThat;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.extensions.api.changes.FixInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ChangeStatus;
 import com.google.gerrit.extensions.common.ProblemInfo;
 import com.google.gerrit.reviewdb.client.Change;
 
@@ -46,6 +50,37 @@ public class CheckIT extends AbstractDaemonTest {
         .isEqualTo("Current patch set 1 not found");
   }
 
+  @Test
+  public void fixReturnsUpdatedValue() throws Exception {
+    PushOneCommit.Result r = createChange();
+    gApi.changes()
+        .id(r.getChangeId())
+        .revision(r.getCommit().name())
+        .review(ReviewInput.approve());
+    gApi.changes()
+        .id(r.getChangeId())
+        .revision(r.getCommit().name())
+        .submit();
+
+    Change c = getChange(r);
+    c.setStatus(Change.Status.NEW);
+    db.changes().update(Collections.singleton(c));
+
+    ChangeInfo info = gApi.changes()
+        .id(r.getChangeId())
+        .check();
+    assertThat(info.problems).hasSize(1);
+    assertThat(info.problems.get(0).status).isNull();
+    assertThat(info.status).isEqualTo(ChangeStatus.NEW);
+
+    info = gApi.changes()
+        .id(r.getChangeId())
+        .check(new FixInput());
+    assertThat(info.problems).hasSize(1);
+    assertThat(info.problems.get(0).status).isEqualTo(ProblemInfo.Status.FIXED);
+    assertThat(info.status).isEqualTo(ChangeStatus.MERGED);
+  }
+
   private Change getChange(PushOneCommit.Result r) throws Exception {
     return db.changes().get(new Change.Id(
         gApi.changes().id(r.getChangeId()).get()._number));

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java

@@ -99,6 +99,7 @@ public interface ChangeApi {
   Set<String> getHashtags() throws RestApiException;
 
   ChangeInfo check() throws RestApiException;
+  ChangeInfo check(FixInput fix) throws RestApiException;
 
   /**
    * A default implementation which allows source compatibility
@@ -204,5 +205,10 @@ public interface ChangeApi {
     public ChangeInfo check() throws RestApiException {
       throw new NotImplementedException();
     }
+
+    @Override
+    public ChangeInfo check(FixInput fix) throws RestApiException {
+      throw new NotImplementedException();
+    }
   }
 }

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FixInput.java

@@ -0,0 +1,18 @@
+// Copyright (C) 2014 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.extensions.api.changes;
+
+public class FixInput {
+}

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java

@@ -15,5 +15,11 @@
 package com.google.gerrit.extensions.common;
 
 public class ProblemInfo {
+  public static enum Status {
+    FIXED, FIX_FAILED;
+  }
+
   public String message;
+  public Status status;
+  public String outcome;
 }

gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java

@@ -19,6 +19,7 @@ import com.google.gerrit.extensions.api.changes.AbandonInput;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
 import com.google.gerrit.extensions.api.changes.ChangeApi;
 import com.google.gerrit.extensions.api.changes.Changes;
+import com.google.gerrit.extensions.api.changes.FixInput;
 import com.google.gerrit.extensions.api.changes.HashtagsInput;
 import com.google.gerrit.extensions.api.changes.RestoreInput;
 import com.google.gerrit.extensions.api.changes.RevertInput;
@@ -244,4 +245,13 @@ class ChangeApiImpl extends ChangeApi.NotImplemented implements ChangeApi {
       throw new RestApiException("Cannot check change", e);
     }
   }
+
+  @Override
+  public ChangeInfo check(FixInput fix) throws RestApiException {
+    try {
+      return check.apply(change, fix).value();
+    } catch (OrmException e) {
+      throw new RestApiException("Cannot check change", e);
+    }
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -56,6 +56,7 @@ import com.google.gerrit.common.data.LabelValue;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.common.data.PermissionRange;
 import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.extensions.api.changes.FixInput;
 import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.common.ActionInfo;
 import com.google.gerrit.extensions.common.ApprovalInfo;
@@ -66,6 +67,7 @@ import com.google.gerrit.extensions.common.FetchInfo;
 import com.google.gerrit.extensions.common.GitPerson;
 import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.extensions.common.ListChangesOption;
+import com.google.gerrit.extensions.common.ProblemInfo;
 import com.google.gerrit.extensions.common.RevisionInfo;
 import com.google.gerrit.extensions.common.WebLinkInfo;
 import com.google.gerrit.extensions.config.DownloadCommand;
@@ -145,6 +147,7 @@ public class ChangeJson {
   private final Provider<ConsistencyChecker> checkerProvider;
 
   private AccountLoader accountLoader;
+  private FixInput fix;
 
   @Inject
   ChangeJson(
@@ -195,6 +198,11 @@ public class ChangeJson {
     return this;
   }
 
+  public ChangeJson fix(FixInput fix) {
+    this.fix = fix;
+    return this;
+  }
+
   public ChangeInfo format(ChangeResource rsrc) throws OrmException {
     return format(changeDataFactory.create(db.get(), rsrc.getControl()));
   }
@@ -298,7 +306,7 @@ public class ChangeJson {
   }
 
   private ChangeInfo checkOnly(ChangeData cd) {
-    ConsistencyChecker.Result result = checkerProvider.get().check(cd);
+    ConsistencyChecker.Result result = checkerProvider.get().check(cd, fix);
     ChangeInfo info;
     Change c = result.change();
     if (c != null) {
@@ -325,9 +333,21 @@ public class ChangeJson {
 
   private ChangeInfo toChangeInfo(ChangeData cd, Set<Change.Id> reviewed,
       Optional<PatchSet.Id> limitToPsId) throws OrmException {
-    ChangeControl ctl = cd.changeControl().forUser(userProvider.get());
     ChangeInfo out = new ChangeInfo();
+
+    if (has(CHECK)) {
+      out.problems = checkerProvider.get().check(cd.change(), fix).problems();
+      // 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());
+          break;
+        }
+      }
+    }
+
     Change in = cd.change();
+    ChangeControl ctl = cd.changeControl().forUser(userProvider.get());
     out.project = in.getProject().get();
     out.branch = in.getDest().getShortName();
     out.topic = in.getTopic();
@@ -406,10 +426,6 @@ public class ChangeJson {
       }
     }
 
-    if (has(CHECK)) {
-      out.problems = checkerProvider.get().check(in).problems();
-    }
-
     return out;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java

@@ -14,26 +14,41 @@
 
 package com.google.gerrit.server.change;
 
+import com.google.gerrit.extensions.api.changes.FixInput;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.ListChangesOption;
+import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.Response;
+import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.extensions.restapi.RestReadView;
+import com.google.gerrit.server.project.ChangeControl;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
-import com.google.inject.Singleton;
 
-@Singleton
+public class Check implements RestReadView<ChangeResource>,
-public class Check implements RestReadView<ChangeResource> {
+    RestModifyView<ChangeResource, FixInput> {
-  private final GetChange delegate;
+  private final ChangeJson json;
 
   @Inject
-  Check(GetChange delegate) {
+  Check(ChangeJson json) {
-    this.delegate = delegate;
+    this.json = json;
-    delegate.addOption(ListChangesOption.CHECK);
+    json.addOption(ListChangesOption.CHECK);
   }
 
   @Override
   public Response<ChangeInfo> apply(ChangeResource rsrc) throws OrmException {
-    return delegate.apply(rsrc);
+    return GetChange.cache(json.format(rsrc));
+  }
+
+  @Override
+  public Response<ChangeInfo> apply(ChangeResource rsrc, FixInput input)
+      throws AuthException, OrmException {
+    ChangeControl ctl = rsrc.getControl();
+    if (!ctl.isOwner()
+        && !ctl.getProjectControl().isOwner()
+        && !ctl.getCurrentUser().getCapabilities().canAdministrateServer()) {
+      throw new AuthException("Not owner");
+    }
+    return GetChange.cache(json.fix(input).format(rsrc));
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java

@@ -21,13 +21,16 @@ import com.google.common.collect.Multimap;
 import com.google.common.collect.MultimapBuilder;
 import com.google.common.collect.Ordering;
 import com.google.gerrit.common.Nullable;
+import com.google.gerrit.extensions.api.changes.FixInput;
 import com.google.gerrit.extensions.common.ProblemInfo;
+import com.google.gerrit.extensions.common.ProblemInfo.Status;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gwtorm.server.AtomicUpdate;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -80,6 +83,7 @@ public class ConsistencyChecker {
   private final Provider<ReviewDb> db;
   private final GitRepositoryManager repoManager;
 
+  private FixInput fix;
   private Change change;
   private Repository repo;
   private RevWalk rw;
@@ -105,9 +109,13 @@ public class ConsistencyChecker {
   }
 
   public Result check(ChangeData cd) {
+    return check(cd, null);
+  }
+
+  public Result check(ChangeData cd, @Nullable FixInput f) {
     reset();
     try {
-      return check(cd.change());
+      return check(cd.change(), f);
     } catch (OrmException e) {
       error("Error looking up change", e);
       return Result.create(cd.getId(), problems);
@@ -115,7 +123,12 @@ public class ConsistencyChecker {
   }
 
   public Result check(Change c) {
+    return check(c, null);
+  }
+
+  public Result check(Change c, @Nullable FixInput f) {
     reset();
+    fix = f;
     change = c;
     try {
       checkImpl();
@@ -258,10 +271,13 @@ public class ConsistencyChecker {
       return;
     }
     if (merged && change.getStatus() != Change.Status.MERGED) {
-      problem(String.format("Patch set %d (%s) is merged into destination"
+      ProblemInfo p = problem(String.format(
-            + " ref %s (%s), but change status is %s", currPs.getId().get(),
+          "Patch set %d (%s) is merged into destination ref %s (%s), but change"
-            currPsCommit.name(), refName, tip.name(), change.getStatus()));
+          + " status is %s", currPs.getId().get(), currPsCommit.name(),
-      // TODO(dborowitz): Just fix it.
+          refName, tip.name(), change.getStatus()));
+      if (fix != null) {
+        fixMerged(p);
+      }
     } else if (!merged && change.getStatus() == Change.Status.MERGED) {
       problem(String.format("Patch set %d (%s) is not merged into"
             + " destination ref %s (%s), but change status is %s",
@@ -270,6 +286,25 @@ public class ConsistencyChecker {
     }
   }
 
+  private void fixMerged(ProblemInfo p) {
+    try {
+      change = db.get().changes().atomicUpdate(change.getId(),
+          new AtomicUpdate<Change>() {
+            @Override
+            public Change update(Change c) {
+              c.setStatus(Change.Status.MERGED);
+              return c;
+            }
+          });
+      p.status = Status.FIXED;
+      p.outcome = "Marked change as merged";
+    } catch (OrmException e) {
+      log.warn("Error marking " + change.getId() + "as merged", e);
+      p.status = Status.FIX_FAILED;
+      p.outcome = "Error updating status to merged";
+    }
+  }
+
   private RevCommit parseCommit(ObjectId objId, String desc) {
     try {
       return rw.parseCommit(objId);
@@ -283,10 +318,11 @@ public class ConsistencyChecker {
     return null;
   }
 
-  private void problem(String msg) {
+  private ProblemInfo problem(String msg) {
     ProblemInfo p = new ProblemInfo();
     p.message = msg;
     problems.add(p);
+    return p;
   }
 
   private boolean error(String msg, Throwable t) {

gerrit-server/src/main/java/com/google/gerrit/server/change/GetChange.java

@@ -53,7 +53,7 @@ public class GetChange implements RestReadView<ChangeResource> {
     return cache(json.format(rsrc));
   }
 
-  private Response<ChangeInfo> cache(ChangeInfo res) {
+  static Response<ChangeInfo> cache(ChangeInfo res) {
     return Response.ok(res)
         .caching(CacheControl.PRIVATE(0, TimeUnit.SECONDS).setMustRevalidate());
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java

@@ -53,6 +53,7 @@ public class Module extends RestApiModule {
     get(CHANGE_KIND, "in").to(IncludedIn.class);
     get(CHANGE_KIND, "hashtags").to(GetHashtags.class);
     get(CHANGE_KIND, "check").to(Check.class);
+    post(CHANGE_KIND, "check").to(Check.class);
     put(CHANGE_KIND, "topic").to(PutTopic.class);
     delete(CHANGE_KIND, "topic").to(PutTopic.class);
     delete(CHANGE_KIND).to(DeleteDraftChange.class);

gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java

@@ -23,6 +23,7 @@ import static java.util.Collections.singleton;
 import com.google.common.base.Function;
 import com.google.common.collect.Lists;
 import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.extensions.api.changes.FixInput;
 import com.google.gerrit.extensions.common.ProblemInfo;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
@@ -43,6 +44,8 @@ import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.util.List;
+
 public class ConsistencyCheckerTest {
   private InMemoryDatabase schemaFactory;
   private ReviewDb db;
@@ -240,6 +243,30 @@ public class ConsistencyCheckerTest {
         + " master (" + commit.name() + "), but change status is NEW");
   }
 
+  @Test
+  public void newChangeIsMergedWithFix() throws Exception {
+    Change c = newChange(project, userId);
+    db.changes().insert(singleton(c));
+    RevCommit commit = repo.branch(c.currentPatchSetId().toRefName()).commit()
+        .parent(tip).create();
+    PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId);
+    db.patchSets().insert(singleton(ps));
+    repo.branch(c.getDest().get()).update(commit);
+
+    List<ProblemInfo> problems = checker.check(c, new FixInput()).problems();
+    assertThat(problems).hasSize(1);
+    ProblemInfo p = problems.get(0);
+    assertThat(p.message).isEqualTo(
+        "Patch set 1 (" + commit.name() + ") is merged into destination ref"
+        + " master (" + commit.name() + "), but change status is NEW");
+    assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED);
+    assertThat(p.outcome).isEqualTo("Marked change as merged");
+
+    c = db.changes().get(c.getId());
+    assertThat(c.getStatus()).isEqualTo(Change.Status.MERGED);
+    assertProblems(c);
+  }
+
   private void assertProblems(Change c, String... expected) {
     assertThat(Lists.transform(checker.check(c).problems(),
           new Function<ProblemInfo, String>() {