diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 7a9893c500..7e6f3ad77a 100644 --- a/Documentation/rest-api-changes.txt +++ b/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]] diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index ae7e94be6c..ec213289a2 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/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(); + } } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java index 6dd6b0607b..2563de494e 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java @@ -81,9 +81,9 @@ public interface ChangeApi { ChangeInfo get(EnumSet 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; /** diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ListChangesOption.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ListChangesOption.java index f9f8b62b19..dd3f07589d 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ListChangesOption.java +++ b/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; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java index fcd669772c..fa97f39be8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java +++ b/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 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 8b1fd5f733..1c4f896269 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 @@ -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 options; private final ChangeMessagesUtil cmUtil; private final PatchLineCommentsUtil plcUtil; + private final Provider checkerProvider; private AccountLoader accountLoader; @@ -161,7 +163,8 @@ public class ChangeJson { Revisions revisions, WebLinks webLinks, ChangeMessagesUtil cmUtil, - PatchLineCommentsUtil plcUtil) { + PatchLineCommentsUtil plcUtil, + Provider 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,14 +222,21 @@ public class ChangeJson { private ChangeInfo format(ChangeData cd, Optional limitToPsId) throws OrmException { - accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS)); - Set reviewed = Sets.newHashSet(); - if (has(REVIEWED)) { - reviewed = loadReviewed(Collections.singleton(cd)); + try { + accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS)); + Set reviewed = Sets.newHashSet(); + if (has(REVIEWED)) { + reviewed = loadReviewed(Collections.singleton(cd)); + } + ChangeInfo res = toChangeInfo(cd, reviewed, limitToPsId); + accountLoader.fill(); + return res; + } catch (OrmException e) { + if (!has(CHECK)) { + throw e; + } + return checkOnly(cd); } - ChangeInfo res = toChangeInfo(cd, reviewed, limitToPsId); - accountLoader.fill(); - return res; } public ChangeInfo format(RevisionResource rsrc) throws OrmException { @@ -262,9 +282,13 @@ public class ChangeJson { try { i = toChangeInfo(cd, reviewed, Optional. absent()); } catch (OrmException e) { - log.warn( - "Omitting corrupt change " + cd.getId() + " from results", e); - continue; + if (has(CHECK)) { + i = checkOnly(cd); + } else { + log.warn( + "Omitting corrupt change " + cd.getId() + " from results", e); + continue; + } } out.put(cd.getId(), 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 reviewed, Optional 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; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java index 075d055f4f..60a6325e5f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java +++ b/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 { - private static final Logger log = LoggerFactory.getLogger(Check.class); - - private final Provider checkerProvider; - private final ChangeJson json; + private final GetChange delegate; @Inject - Check(Provider 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 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 apply(ChangeResource rsrc) throws OrmException { + return delegate.apply(rsrc); } } 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 eece1c2371..14cd238b41 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 @@ -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 problems) { + return new AutoValue_ConsistencyChecker_Result(id, null, problems); + } + + private static Result create(Change c, List problems) { + return new AutoValue_ConsistencyChecker_Result(c.getId(), c, problems); + } + + public abstract Change.Id id(); + + @Nullable + public abstract Change change(); + + public abstract List problems(); + } + private final Provider db; private final GitRepositoryManager repoManager; @@ -83,12 +104,22 @@ public class ConsistencyChecker { problems = new ArrayList<>(); } - public List 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(); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java index bf2c7e6793..9bc9ec0d49 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java +++ b/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() { @Override public String apply(ProblemInfo in) {