Merge changes from topic 'check'

* changes:
  ChangeJson: Catch RuntimeException in fallback check handlers
  Support fixing corrupt changes in ConsistencyChecker
  Add API method for checking changes
  Check consistency with a ListChangesOption
  Embed consistency check results in ChangeInfo result
  ChangeData: Handle null PatchSet in isMergeable

Documentation/rest-api-changes.txt

@@ -300,6 +300,11 @@ default. Optional fields are:
 * `WEB_LINKS`: include the `web_links` field.
 --
 
+[[check]]
+--
+* `CHECK`: include potential problems with the change.
+--
+
 .Request
 ----
   GET /changes/?q=97&o=CURRENT_REVISION&o=CURRENT_COMMIT&o=CURRENT_FILES&o=DOWNLOAD_COMMANDS HTTP/1.0
@@ -1143,7 +1148,12 @@ Adds or updates the change in the secondary index.
 --
 
 Performs consistency checks on the change, and returns a
-link:#check-result[CheckResult] entity.
+link:#change-info[ChangeInfo] entity with the `problems` field set to a
+list of link:#problem-info[ProblemInfo] entities.
+
+Depending on the type of problem, some fields not marked optional may be
+missing from the result. At least `id`, `project`, `branch`, and
+`_number` will be present.
 
 .Request
 ----
@@ -1158,7 +1168,6 @@ link:#check-result[CheckResult] entity.
 
   )]}'
   {
-    "change": {
     "id": "myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940",
     "project": "myProject",
     "branch": "master",
@@ -1174,10 +1183,65 @@ link:#check-result[CheckResult] entity.
     "_number": 3965,
     "owner": {
       "name": "John Doe"
-      }
     },
-    "messages": [
+    "problems": [
-      "Current patch set 1 not found"
+      {
+        "message": "Current patch set 1 not found"
+      }
+    ]
+  }
+----
+
+[[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"
+      }
     ]
   }
 ----
@@ -3259,6 +3323,9 @@ if link:#all-revisions[all revisions] are requested.
 |`_more_changes`      |optional, not set if `false`|
 Whether the query would deliver more results if not limited. +
 Only set on either the last or the first change that is returned.
+|`problems`           |optional|
+A list of link:#problem-info[ProblemInfo] entities describing potential
+problems with this change. Only set if link:#check[CHECK] is set.
 |==================================
 
 [[related-changes-info]]
@@ -3978,22 +4045,23 @@ the commit message within a change edit.
 |`message`     ||New commit message.
 |===========================
 
-[[check-result]]
+[[problem-info]]
-=== CheckResult
+=== ProblemInfo
-The `CheckResult` entity contains the results of a consistency check on
+The `ProblemInfo` entity contains a description of a potential consistency problem
-a change.
+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
-|`change`|
+|`message` ||Plaintext message describing the problem with the change.
-link:#change-info[ChangeInfo] entity containing information about the change,
+|`status`  |optional|
-as in link:#get-change[Get Change] with no options. Some fields not marked
+The status of fixing the problem (`FIXED`, `FIX_FAILED`). Only set if a
-optional may be missing if a consistency check failed, but at least
+fix was attempted.
-`id`, `project`, `branch`, and `_number` will be present.
+|`outcome` |optional|
-|`messages`|
+If `status` is set, an additional plaintext message describing the
-List of messages describing potential problems with the change. May be
+outcome of the fix.
-empty if no problems were found.
 |===========================
 
 GERRIT

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

@@ -301,4 +301,17 @@ public class ChangeIT extends AbstractDaemonTest {
         .id(r.getChangeId())
         .topic()).isEqualTo("");
   }
+
+  @Test
+  public void check() throws Exception {
+    PushOneCommit.Result r = createChange();
+    assertThat(gApi.changes()
+        .id(r.getChangeId())
+        .get()
+        .problems).isNull();
+    assertThat(gApi.changes()
+        .id(r.getChangeId())
+        .get(EnumSet.of(ListChangesOption.CHECK))
+        .problems).isEmpty();
+  }
 }

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

@@ -0,0 +1,88 @@
+// 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.acceptance.api.change;
+
+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;
+
+import org.junit.Test;
+
+import java.util.Collections;
+import java.util.List;
+
+@NoHttpd
+public class CheckIT extends AbstractDaemonTest {
+  // Most types of tests belong in ConsistencyCheckerTest; these mostly just
+  // test paths outside of ConsistencyChecker, like API wiring.
+  @Test
+  public void currentPatchSetMissing() throws Exception {
+    PushOneCommit.Result r = createChange();
+    Change c = getChange(r);
+    db.patchSets().deleteKeys(Collections.singleton(c.currentPatchSetId()));
+
+    List<ProblemInfo> problems = gApi.changes()
+        .id(r.getChangeId())
+        .check()
+        .problems;
+    assertThat(problems).hasSize(1);
+    assertThat(problems.get(0).message)
+        .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

@@ -81,9 +81,9 @@ public interface ChangeApi {
 
   ChangeInfo get(EnumSet<ListChangesOption> options) throws RestApiException;
 
-  /** {@code get} with {@link ListChangesOption} set to ALL. */
+  /** {@code get} with {@link ListChangesOption} set to all except CHECK. */
   ChangeInfo get() throws RestApiException;
-  /** {@code get} with {@link ListChangesOption} set to NONE. */
+  /** {@code get} with {@link ListChangesOption} set to none. */
   ChangeInfo info() throws RestApiException;
 
   /**
@@ -98,6 +98,9 @@ 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
    * when adding new methods to the interface.
@@ -197,5 +200,15 @@ public interface ChangeApi {
     public Set<String> getHashtags() throws RestApiException {
       throw new NotImplementedException();
     }
+
+    @Override
+    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/ChangeInfo.java

@@ -16,6 +16,7 @@ package com.google.gerrit.extensions.common;
 
 import java.sql.Timestamp;
 import java.util.Collection;
+import java.util.List;
 import java.util.Map;
 
 public class ChangeInfo {
@@ -50,4 +51,6 @@ public class ChangeInfo {
   public String currentRevision;
   public Map<String, RevisionInfo> revisions;
   public Boolean _moreChanges;
+
+  public List<ProblemInfo> problems;
 }

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

@@ -52,7 +52,10 @@ public enum ListChangesOption {
   DOWNLOAD_COMMANDS(13),
 
   /** Include patch set weblinks. */
-  WEB_LINKS(14);
+  WEB_LINKS(14),
+
+  /** Include consistency check results. */
+  CHECK(15);
 
   private final int value;
 

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

@@ -12,13 +12,14 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.gerrit.server.change;
+package com.google.gerrit.extensions.common;
 
-import com.google.gerrit.extensions.common.ChangeInfo;
+public class ProblemInfo {
+  public static enum Status {
+    FIXED, FIX_FAILED;
+  }
 
-import java.util.List;
+  public String message;
-
+  public Status status;
-public class CheckResult {
+  public String outcome;
-  public ChangeInfo change;
-  public List<String> messages;
 }

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;
@@ -30,6 +31,7 @@ import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.server.change.Abandon;
 import com.google.gerrit.server.change.ChangeJson;
 import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.Check;
 import com.google.gerrit.server.change.GetHashtags;
 import com.google.gerrit.server.change.GetTopic;
 import com.google.gerrit.server.change.PostHashtags;
@@ -65,6 +67,7 @@ class ChangeApiImpl extends ChangeApi.NotImplemented implements ChangeApi {
   private final Provider<ChangeJson> changeJson;
   private final PostHashtags postHashtags;
   private final GetHashtags getHashtags;
+  private final Check check;
 
   @Inject
   ChangeApiImpl(Changes changeApi,
@@ -79,6 +82,7 @@ class ChangeApiImpl extends ChangeApi.NotImplemented implements ChangeApi {
       Provider<ChangeJson> changeJson,
       PostHashtags postHashtags,
       GetHashtags getHashtags,
+      Check check,
       @Assisted ChangeResource change) {
     this.changeApi = changeApi;
     this.revert = revert;
@@ -92,6 +96,7 @@ class ChangeApiImpl extends ChangeApi.NotImplemented implements ChangeApi {
     this.changeJson = changeJson;
     this.postHashtags = postHashtags;
     this.getHashtags = getHashtags;
+    this.check = check;
     this.change = change;
   }
 
@@ -206,7 +211,7 @@ class ChangeApiImpl extends ChangeApi.NotImplemented implements ChangeApi {
 
   @Override
   public ChangeInfo get() throws RestApiException {
-    return get(EnumSet.allOf(ListChangesOption.class));
+    return get(EnumSet.complementOf(EnumSet.of(ListChangesOption.CHECK)));
   }
 
   @Override
@@ -231,4 +236,22 @@ class ChangeApiImpl extends ChangeApi.NotImplemented implements ChangeApi {
       throw new RestApiException("Cannot get hashtags", e);
     }
   }
+
+  @Override
+  public ChangeInfo check() throws RestApiException {
+    try {
+      return check.apply(change).value();
+    } catch (OrmException e) {
+      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

@@ -17,6 +17,7 @@ package com.google.gerrit.server.change;
 import static com.google.gerrit.extensions.common.ListChangesOption.ALL_COMMITS;
 import static com.google.gerrit.extensions.common.ListChangesOption.ALL_FILES;
 import static com.google.gerrit.extensions.common.ListChangesOption.ALL_REVISIONS;
+import static com.google.gerrit.extensions.common.ListChangesOption.CHECK;
 import static com.google.gerrit.extensions.common.ListChangesOption.CURRENT_ACTIONS;
 import static com.google.gerrit.extensions.common.ListChangesOption.CURRENT_COMMIT;
 import static com.google.gerrit.extensions.common.ListChangesOption.CURRENT_FILES;
@@ -55,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;
@@ -65,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;
@@ -141,8 +144,10 @@ public class ChangeJson {
   private final EnumSet<ListChangesOption> options;
   private final ChangeMessagesUtil cmUtil;
   private final PatchLineCommentsUtil plcUtil;
+  private final Provider<ConsistencyChecker> checkerProvider;
 
   private AccountLoader accountLoader;
+  private FixInput fix;
 
   @Inject
   ChangeJson(
@@ -161,7 +166,8 @@ public class ChangeJson {
       Revisions revisions,
       WebLinks webLinks,
       ChangeMessagesUtil cmUtil,
-      PatchLineCommentsUtil plcUtil) {
+      PatchLineCommentsUtil plcUtil,
+      Provider<ConsistencyChecker> checkerProvider) {
     this.db = db;
     this.labelNormalizer = ln;
     this.userProvider = user;
@@ -178,6 +184,7 @@ public class ChangeJson {
     this.webLinks = webLinks;
     this.cmUtil = cmUtil;
     this.plcUtil = plcUtil;
+    this.checkerProvider = checkerProvider;
     options = EnumSet.noneOf(ListChangesOption.class);
   }
 
@@ -191,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()));
   }
@@ -200,7 +212,16 @@ public class ChangeJson {
   }
 
   public ChangeInfo format(Change.Id id) throws OrmException {
-    return format(changeDataFactory.create(db.get(), id));
+    Change c;
+    try {
+      c = db.get().changes().get(id);
+    } catch (OrmException e) {
+      if (!has(CHECK)) {
+        throw e;
+      }
+      return checkOnly(changeDataFactory.create(db.get(), id));
+    }
+    return format(changeDataFactory.create(db.get(), c));
   }
 
   public ChangeInfo format(ChangeData cd) throws OrmException {
@@ -209,6 +230,7 @@ public class ChangeJson {
 
   private ChangeInfo format(ChangeData cd, Optional<PatchSet.Id> limitToPsId)
       throws OrmException {
+    try {
       accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS));
       Set<Change.Id> reviewed = Sets.newHashSet();
       if (has(REVIEWED)) {
@@ -217,6 +239,12 @@ public class ChangeJson {
       ChangeInfo res = toChangeInfo(cd, reviewed, limitToPsId);
       accountLoader.fill();
       return res;
+    } catch (OrmException | RuntimeException e) {
+      if (!has(CHECK)) {
+        throw e;
+      }
+      return checkOnly(cd);
+    }
   }
 
   public ChangeInfo format(RevisionResource rsrc) throws OrmException {
@@ -261,11 +289,15 @@ public class ChangeJson {
       if (i == null) {
         try {
           i = toChangeInfo(cd, reviewed, Optional.<PatchSet.Id> absent());
-        } catch (OrmException e) {
+        } catch (OrmException | RuntimeException e) {
+          if (has(CHECK)) {
+            i = checkOnly(cd);
+          } else {
             log.warn(
                 "Omitting corrupt change " + cd.getId() + " from results", e);
             continue;
           }
+        }
         out.put(cd.getId(), i);
       }
       info.add(i);
@@ -273,11 +305,49 @@ public class ChangeJson {
     return info;
   }
 
+  private ChangeInfo checkOnly(ChangeData cd) {
+    ConsistencyChecker.Result result = checkerProvider.get().check(cd, fix);
+    ChangeInfo info;
+    Change c = result.change();
+    if (c != null) {
+      info = new ChangeInfo();
+      info.project = c.getProject().get();
+      info.branch = c.getDest().getShortName();
+      info.topic = c.getTopic();
+      info.changeId = c.getKey().get();
+      info.subject = c.getSubject();
+      info.status = c.getStatus().asChangeStatus();
+      info.owner = new AccountInfo(c.getOwner().get());
+      info.created = c.getCreatedOn();
+      info.updated = c.getLastUpdatedOn();
+      info._number = c.getId().get();
+      info.problems = result.problems();
+      finish(info);
+    } else {
+      info = new ChangeInfo();
+      info._number = result.id().get();
+      info.problems = result.problems();
+    }
+    return info;
+  }
+
   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();
@@ -355,6 +425,7 @@ public class ChangeJson {
         out.actions.put(descr.getId(), new ActionInfo(descr));
       }
     }
+
     return out;
   }
 

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

@@ -14,63 +14,41 @@
 
 package com.google.gerrit.server.change;
 
-import com.google.gerrit.extensions.common.AccountInfo;
+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.reviewdb.client.Change;
+import com.google.gerrit.server.project.ChangeControl;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
-import com.google.inject.Provider;
-import com.google.inject.Singleton;
 
-import org.slf4j.Logger;
+public class Check implements RestReadView<ChangeResource>,
-import org.slf4j.LoggerFactory;
+    RestModifyView<ChangeResource, FixInput> {
-
-@Singleton
-public class Check implements RestReadView<ChangeResource> {
-  private static final Logger log = LoggerFactory.getLogger(Check.class);
-
-  private final Provider<ConsistencyChecker> checkerProvider;
   private final ChangeJson json;
 
   @Inject
-  Check(Provider<ConsistencyChecker> checkerProvider,
+  Check(ChangeJson json) {
-      ChangeJson json) {
-    this.checkerProvider = checkerProvider;
     this.json = json;
+    json.addOption(ListChangesOption.CHECK);
   }
 
   @Override
-  public CheckResult apply(ChangeResource rsrc) {
+  public Response<ChangeInfo> apply(ChangeResource rsrc) throws OrmException {
-    CheckResult result = new CheckResult();
+    return GetChange.cache(json.format(rsrc));
-    result.messages = checkerProvider.get().check(rsrc.getChange());
-    try {
-      result.change = json.format(rsrc);
-    } catch (OrmException e) {
-      // Even with no options there are a surprising number of dependencies in
-      // ChangeJson. Fall back to a very basic implementation with no
-      // dependencies if this fails.
-      String msg = "Error rendering final ChangeInfo";
-      log.warn(msg, e);
-      result.messages.add(msg);
-      result.change = basicChangeInfo(rsrc.getChange());
-    }
-    return result;
   }
 
-  private static ChangeInfo basicChangeInfo(Change c) {
+  @Override
-    ChangeInfo info = new ChangeInfo();
+  public Response<ChangeInfo> apply(ChangeResource rsrc, FixInput input)
-    info.project = c.getProject().get();
+      throws AuthException, OrmException {
-    info.branch = c.getDest().getShortName();
+    ChangeControl ctl = rsrc.getControl();
-    info.topic = c.getTopic();
+    if (!ctl.isOwner()
-    info.changeId = c.getKey().get();
+        && !ctl.getProjectControl().isOwner()
-    info.subject = c.getSubject();
+        && !ctl.getCurrentUser().getCapabilities().canAdministrateServer()) {
-    info.status = c.getStatus().asChangeStatus();
+      throw new AuthException("Not owner");
-    info.owner = new AccountInfo(c.getOwner().get());
+    }
-    info.created = c.getCreatedOn();
+    return GetChange.cache(json.fix(input).format(rsrc));
-    info.updated = c.getLastUpdatedOn();
-    info._number = c.getId().get();
-    ChangeJson.finish(info);
-    return info;
   }
 }

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

@@ -14,16 +14,23 @@
 
 package com.google.gerrit.server.change;
 
+import com.google.auto.value.AutoValue;
 import com.google.common.base.Function;
 import com.google.common.collect.Collections2;
 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;
@@ -55,9 +62,28 @@ public class ConsistencyChecker {
   private static final Logger log =
       LoggerFactory.getLogger(ConsistencyChecker.class);
 
+  @AutoValue
+  public static abstract class Result {
+    private static Result create(Change.Id id, List<ProblemInfo> problems) {
+      return new AutoValue_ConsistencyChecker_Result(id, null, problems);
+    }
+
+    private static Result create(Change c, List<ProblemInfo> problems) {
+      return new AutoValue_ConsistencyChecker_Result(c.getId(), c, problems);
+    }
+
+    public abstract Change.Id id();
+
+    @Nullable
+    public abstract Change change();
+
+    public abstract List<ProblemInfo> problems();
+  }
+
   private final Provider<ReviewDb> db;
   private final GitRepositoryManager repoManager;
 
+  private FixInput fix;
   private Change change;
   private Repository repo;
   private RevWalk rw;
@@ -65,7 +91,7 @@ public class ConsistencyChecker {
   private PatchSet currPs;
   private RevCommit currPsCommit;
 
-  private List<String> messages;
+  private List<ProblemInfo> problems;
 
   @Inject
   ConsistencyChecker(Provider<ReviewDb> db,
@@ -79,15 +105,34 @@ public class ConsistencyChecker {
     change = null;
     repo = null;
     rw = null;
-    messages = new ArrayList<>();
+    problems = new ArrayList<>();
   }
 
-  public List<String> check(Change c) {
+  public Result check(ChangeData cd) {
+    return check(cd, null);
+  }
+
+  public Result check(ChangeData cd, @Nullable FixInput f) {
     reset();
+    try {
+      return check(cd.change(), f);
+    } catch (OrmException e) {
+      error("Error looking up change", e);
+      return Result.create(cd.getId(), problems);
+    }
+  }
+
+  public Result check(Change c) {
+    return check(c, null);
+  }
+
+  public Result check(Change c, @Nullable FixInput f) {
+    reset();
+    fix = f;
     change = c;
     try {
       checkImpl();
-      return messages;
+      return Result.create(c, problems);
     } finally {
       if (rw != null) {
         rw.release();
@@ -115,7 +160,7 @@ public class ConsistencyChecker {
   private void checkOwner() {
     try {
       if (db.get().accounts().get(change.getOwner()) == null) {
-        messages.add("Missing change owner: " + change.getOwner());
+        problem("Missing change owner: " + change.getOwner());
       }
     } catch (OrmException e) {
       error("Failed to look up owner", e);
@@ -127,7 +172,7 @@ public class ConsistencyChecker {
       PatchSet.Id psId = change.currentPatchSetId();
       currPs = db.get().patchSets().get(psId);
       if (currPs == null) {
-        messages.add(String.format("Current patch set %d not found", psId.get()));
+        problem(String.format("Current patch set %d not found", psId.get()));
       }
     } catch (OrmException e) {
       error("Failed to look up current patch set", e);
@@ -189,7 +234,7 @@ public class ConsistencyChecker {
     for (Map.Entry<ObjectId, Collection<PatchSet>> e
         : bySha.asMap().entrySet()) {
       if (e.getValue().size() > 1) {
-        messages.add(String.format("Multiple patch sets pointing to %s: %s",
+        problem(String.format("Multiple patch sets pointing to %s: %s",
             e.getKey().name(),
             Collections2.transform(e.getValue(), toPsId)));
       }
@@ -204,11 +249,11 @@ public class ConsistencyChecker {
     try {
       dest = repo.getRef(refName);
     } catch (IOException e) {
-      messages.add("Failed to look up destination ref: " + refName);
+      problem("Failed to look up destination ref: " + refName);
       return;
     }
     if (dest == null) {
-      messages.add("Destination ref not found (may be new branch): "
+      problem("Destination ref not found (may be new branch): "
           + change.getDest().get());
       return;
     }
@@ -221,38 +266,67 @@ public class ConsistencyChecker {
     try {
       merged = rw.isMergedInto(currPsCommit, tip);
     } catch (IOException e) {
-      messages.add("Error checking whether patch set " + currPs.getId().get()
+      problem("Error checking whether patch set " + currPs.getId().get()
           + " is merged");
       return;
     }
     if (merged && change.getStatus() != Change.Status.MERGED) {
-      messages.add(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) {
-      messages.add(String.format("Patch set %d (%s) is not merged into"
+      problem(String.format("Patch set %d (%s) is not merged into"
             + " destination ref %s (%s), but change status is %s",
             currPs.getId().get(), currPsCommit.name(), refName, tip.name(),
             change.getStatus()));
     }
   }
 
+  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);
     } catch (MissingObjectException e) {
-      messages.add(String.format("Object missing: %s: %s", desc, objId.name()));
+      problem(String.format("Object missing: %s: %s", desc, objId.name()));
     } catch (IncorrectObjectTypeException e) {
-      messages.add(String.format("Not a commit: %s: %s", desc, objId.name()));
+      problem(String.format("Not a commit: %s: %s", desc, objId.name()));
     } catch (IOException e) {
-      messages.add(String.format("Failed to look up: %s: %s", desc, objId.name()));
+      problem(String.format("Failed to look up: %s: %s", desc, objId.name()));
     }
     return null;
   }
 
+  private ProblemInfo problem(String msg) {
+    ProblemInfo p = new ProblemInfo();
+    p.message = msg;
+    problems.add(p);
+    return p;
+  }
+
   private boolean error(String msg, Throwable t) {
-    messages.add(msg);
+    problem(msg);
     // TODO(dborowitz): Expose stack trace to administrators.
     log.warn("Error in consistency check of change " + change.getId(), t);
     return false;

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/main/java/com/google/gerrit/server/query/change/ChangeData.java

@@ -594,6 +594,9 @@ public class ChangeData {
         mergeable = true;
       } else {
         PatchSet ps = currentPatchSet();
+        if (ps == null) {
+          throw new OrmException("Missing patch set for mergeability check");
+        }
         Repository repo = null;
         try {
           repo = repoManager.openRepository(c.getProject());

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

@@ -20,7 +20,11 @@ import static com.google.gerrit.testutil.TestChanges.newChange;
 import static com.google.gerrit.testutil.TestChanges.newPatchSet;
 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;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -40,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;
@@ -90,7 +96,7 @@ public class ConsistencyCheckerTest {
     PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit2, userId);
     db.patchSets().insert(singleton(ps2));
 
-    assertThat(checker.check(c)).isEmpty();
+    assertProblems(c);
   }
 
   @Test
@@ -110,7 +116,7 @@ public class ConsistencyCheckerTest {
     db.patchSets().insert(singleton(ps2));
 
     repo.branch(c.getDest().get()).update(commit2);
-    assertThat(checker.check(c)).isEmpty();
+    assertProblems(c);
   }
 
   @Test
@@ -122,7 +128,7 @@ public class ConsistencyCheckerTest {
     PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId);
     db.patchSets().insert(singleton(ps));
 
-    assertThat(checker.check(c)).containsExactly("Missing change owner: 2");
+    assertProblems(c, "Missing change owner: 2");
   }
 
   @Test
@@ -132,8 +138,7 @@ public class ConsistencyCheckerTest {
     PatchSet ps = newPatchSet(c.currentPatchSetId(),
         ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), userId);
     db.patchSets().insert(singleton(ps));
-    assertThat(checker.check(c))
+    assertProblems(c, "Destination repository not found: otherproject");
-        .containsExactly("Destination repository not found: otherproject");
   }
 
   @Test
@@ -153,7 +158,7 @@ public class ConsistencyCheckerTest {
     PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit2, userId);
     db.patchSets().insert(singleton(ps2));
 
-    assertThat(checker.check(c)).containsExactly(
+    assertProblems(c,
         "Invalid revision on patch set 1:"
         + " fooooooooooooooooooooooooooooooooooooooo");
   }
@@ -166,7 +171,7 @@ public class ConsistencyCheckerTest {
         ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), userId);
     db.patchSets().insert(singleton(ps));
 
-    assertThat(checker.check(c)).containsExactly(
+    assertProblems(c,
         "Object missing: patch set 1: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
   }
 
@@ -174,8 +179,7 @@ public class ConsistencyCheckerTest {
   public void currentPatchSetMissing() throws Exception {
     Change c = newChange(project, userId);
     db.changes().insert(singleton(c));
-    assertThat(checker.check(c))
+    assertProblems(c, "Current patch set 1 not found");
-      .containsExactly("Current patch set 1 not found");
   }
 
   @Test
@@ -191,8 +195,8 @@ public class ConsistencyCheckerTest {
     PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit1, userId);
     db.patchSets().insert(singleton(ps2));
 
-    assertThat(checker.check(c)).containsExactly("Multiple patch sets pointing to "
+    assertProblems(c,
-        + commit1.name() + ": [1, 2]");
+        "Multiple patch sets pointing to " + commit1.name() + ": [1, 2]");
   }
 
   @Test
@@ -206,8 +210,7 @@ public class ConsistencyCheckerTest {
     PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId);
     db.patchSets().insert(singleton(ps));
 
-    assertThat(checker.check(c)).containsExactly(
+    assertProblems(c, "Destination ref not found (may be new branch): master");
-        "Destination ref not found (may be new branch): master");
   }
 
   @Test
@@ -220,7 +223,7 @@ public class ConsistencyCheckerTest {
     PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId);
     db.patchSets().insert(singleton(ps));
 
-    assertThat(checker.check(c)).containsExactly(
+    assertProblems(c,
         "Patch set 1 (" + commit.name() + ") is not merged into destination ref"
         + " master (" + tip.name() + "), but change status is MERGED");
   }
@@ -235,8 +238,42 @@ public class ConsistencyCheckerTest {
     db.patchSets().insert(singleton(ps));
     repo.branch(c.getDest().get()).update(commit);
 
-    assertThat(checker.check(c)).containsExactly(
+    assertProblems(c,
         "Patch set 1 (" + commit.name() + ") is merged into destination ref"
         + " 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>() {
+            @Override
+            public String apply(ProblemInfo in) {
+              return in.message;
+            }
+          })).containsExactly((Object[]) expected);
+  }
 }