Check consistency with a ListChangesOption

This allows for bulk consistency checking using the normal change
query interface. We already know how to detect and omit corrupt
changes; when consistency checking is enabled, we can do something
more useful.

One subtlety is that depending on the call path, we might fail to even
resolve a Change.Id to a Change. Teach ConsistencyChecker how to
handle this case by returning a single problem for this error, and
encapsulating the problem list in a Result containing the change as
well. We need to return the change in this way because the failure to
look up the change might have been transient; it would be confusing to
return "Error looking up change" and also return the change info
because the second lookup to populate the ChangeInfo succeeded.

Check can now be a thin wrapper around GetChange, at least for the GET
path.

Change-Id: I2b2e2b64d6c4e294e28640df271a9bd64a2f0b5f

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
@@ -3266,8 +3271,7 @@ 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 by the link:#check-change[/check]
-handler.
+problems with this change. Only set if link:#check[CHECK] is set.
 |==================================
 
 [[related-changes-info]]

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-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;
 
   /**

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-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java

@@ -206,7 +206,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

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;
@@ -141,6 +142,7 @@ public class ChangeJson {
   private final EnumSet<ListChangesOption> options;
   private final ChangeMessagesUtil cmUtil;
   private final PatchLineCommentsUtil plcUtil;
+  private final Provider<ConsistencyChecker> checkerProvider;
 
   private AccountLoader accountLoader;
 
@@ -161,7 +163,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 +181,7 @@ public class ChangeJson {
     this.webLinks = webLinks;
     this.cmUtil = cmUtil;
     this.plcUtil = plcUtil;
+    this.checkerProvider = checkerProvider;
     options = EnumSet.noneOf(ListChangesOption.class);
   }
 
@@ -200,7 +204,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 +222,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 +231,12 @@ public class ChangeJson {
       ChangeInfo res = toChangeInfo(cd, reviewed, limitToPsId);
       accountLoader.fill();
       return res;
+    } catch (OrmException e) {
+      if (!has(CHECK)) {
+        throw e;
+      }
+      return checkOnly(cd);
+    }
   }
 
   public ChangeInfo format(RevisionResource rsrc) throws OrmException {
@@ -262,10 +282,14 @@ public class ChangeJson {
         try {
           i = toChangeInfo(cd, reviewed, Optional.<PatchSet.Id> absent());
         } catch (OrmException 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,6 +297,32 @@ public class ChangeJson {
     return info;
   }
 
+  private ChangeInfo checkOnly(ChangeData cd) {
+    ConsistencyChecker.Result result = checkerProvider.get().check(cd);
+    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());
@@ -355,6 +405,11 @@ public class ChangeJson {
         out.actions.put(descr.getId(), new ActionInfo(descr));
       }
     }
+
+    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,68 +14,26 @@
 
 package com.google.gerrit.server.change;
 
-import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
-import com.google.gerrit.extensions.common.ProblemInfo;
+import com.google.gerrit.extensions.common.ListChangesOption;
+import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestReadView;
-import com.google.gerrit.reviewdb.client.Change;
 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;
-import org.slf4j.LoggerFactory;
-
-import java.util.List;
-
 @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;
+  private final GetChange delegate;
 
   @Inject
-  Check(Provider<ConsistencyChecker> checkerProvider,
-      ChangeJson json) {
-    this.checkerProvider = checkerProvider;
-    this.json = json;
+  Check(GetChange delegate) {
+    this.delegate = delegate;
+    delegate.addOption(ListChangesOption.CHECK);
   }
 
   @Override
-  public ChangeInfo apply(ChangeResource rsrc) {
-    List<ProblemInfo> problems = checkerProvider.get().check(rsrc.getChange());
-    ChangeInfo result;
-    try {
-      result = 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.
-      ProblemInfo p = new ProblemInfo();
-      p.message = "Error rendering final ChangeInfo";
-      log.warn(p.message, e);
-      problems.add(p);
-      result = basicChangeInfo(rsrc.getChange());
-    }
-    result.problems = problems;
-    return result;
-  }
-
-  private static ChangeInfo basicChangeInfo(Change c) {
-    ChangeInfo 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();
-    ChangeJson.finish(info);
-    return info;
+  public Response<ChangeInfo> apply(ChangeResource rsrc) throws OrmException {
+    return delegate.apply(rsrc);
   }
 }

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

@@ -14,17 +14,20 @@
 
 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.common.ProblemInfo;
 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.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -56,6 +59,24 @@ 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;
 
@@ -83,12 +104,22 @@ public class ConsistencyChecker {
     problems = new ArrayList<>();
   }
 
-  public List<ProblemInfo> check(Change c) {
+  public Result check(ChangeData cd) {
+    reset();
+    try {
+      return check(cd.change());
+    } catch (OrmException e) {
+      error("Error looking up change", e);
+      return Result.create(cd.getId(), problems);
+    }
+  }
+
+  public Result check(Change c) {
     reset();
     change = c;
     try {
       checkImpl();
-      return problems;
+      return Result.create(c, problems);
     } finally {
       if (rw != null) {
         rw.release();

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

@@ -241,7 +241,7 @@ public class ConsistencyCheckerTest {
   }
 
   private void assertProblems(Change c, String... expected) {
-    assertThat(Lists.transform(checker.check(c),
+    assertThat(Lists.transform(checker.check(c).problems(),
           new Function<ProblemInfo, String>() {
             @Override
             public String apply(ProblemInfo in) {