Merge "GetCheck: Backfill checkers"
diff --git a/java/com/google/gerrit/plugins/checks/api/CheckBackfiller.java b/java/com/google/gerrit/plugins/checks/api/CheckBackfiller.java
index da98cd9..d25e8af 100644
--- a/java/com/google/gerrit/plugins/checks/api/CheckBackfiller.java
+++ b/java/com/google/gerrit/plugins/checks/api/CheckBackfiller.java
@@ -22,6 +22,8 @@
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.Checker;
+import com.google.gerrit.plugins.checks.CheckerUuid;
+import com.google.gerrit.plugins.checks.Checkers;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.index.change.ChangeField;
@@ -34,22 +36,28 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
+import java.io.IOException;
import java.util.Collection;
+import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
@Singleton
class CheckBackfiller {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final ChangeData.Factory changeDataFactory;
+ private final Checkers checkers;
private final Provider<AnonymousUser> anonymousUserProvider;
private final Provider<ChangeQueryBuilder> queryBuilderProvider;
@Inject
CheckBackfiller(
Factory changeDataFactory,
+ Checkers checkers,
Provider<AnonymousUser> anonymousUserProvider,
Provider<ChangeQueryBuilder> queryBuilderProvider) {
this.changeDataFactory = changeDataFactory;
+ this.checkers = checkers;
this.anonymousUserProvider = anonymousUserProvider;
this.queryBuilderProvider = queryBuilderProvider;
}
@@ -69,22 +77,52 @@
// NOT_STARTED, with creation time matching the patch set.
ImmutableList.Builder<Check> result = ImmutableList.builderWithExpectedSize(candidates.size());
PatchSet ps = cd.patchSet(psId);
- ChangeQueryBuilder queryBuilder =
- queryBuilderProvider.get().asUser(anonymousUserProvider.get());
+ ChangeQueryBuilder queryBuilder = newQueryBuilder();
for (Checker checker : candidates) {
if (matches(checker, cd, queryBuilder)) {
// Add synthetic check at the creation time of the patch set.
- result.add(
- Check.builder(CheckKey.create(cd.project(), ps.getId(), checker.getUuid()))
- .setState(CheckState.NOT_STARTED)
- .setCreated(ps.getCreatedOn())
- .setUpdated(ps.getCreatedOn())
- .build());
+ result.add(newBackfilledCheck(cd, ps, checker));
}
}
return result.build();
}
+ Optional<Check> getBackfilledCheckForRelevantChecker(
+ CheckerUuid candidate, ChangeNotes notes, PatchSet.Id psId) throws OrmException, IOException {
+ ChangeData cd = changeDataFactory.create(notes);
+ if (!psId.equals(cd.change().currentPatchSetId())) {
+ // The query system can only match against the current patch set; it doesn't make sense to
+ // backfill checkers for old patch sets.
+ return Optional.empty();
+ }
+
+ Optional<Checker> checker;
+ try {
+ checker = checkers.getChecker(candidate);
+ } catch (ConfigInvalidException e) {
+ // Match behavior of Checkers#checkersOf, ignoring invalid config.
+ return Optional.empty();
+ }
+ if (!checker.isPresent()
+ || checker.get().getStatus() != CheckerStatus.ENABLED
+ || !matches(checker.get(), cd, newQueryBuilder())) {
+ return Optional.empty();
+ }
+ return Optional.of(newBackfilledCheck(cd, cd.patchSet(psId), checker.get()));
+ }
+
+ private Check newBackfilledCheck(ChangeData cd, PatchSet ps, Checker checker) {
+ return Check.builder(CheckKey.create(cd.project(), ps.getId(), checker.getUuid()))
+ .setState(CheckState.NOT_STARTED)
+ .setCreated(ps.getCreatedOn())
+ .setUpdated(ps.getCreatedOn())
+ .build();
+ }
+
+ private ChangeQueryBuilder newQueryBuilder() {
+ return queryBuilderProvider.get().asUser(anonymousUserProvider.get());
+ }
+
private static boolean matches(Checker checker, ChangeData cd, ChangeQueryBuilder queryBuilder)
throws OrmException {
if (!checker.getQuery().isPresent()) {
diff --git a/java/com/google/gerrit/plugins/checks/api/ChecksCollection.java b/java/com/google/gerrit/plugins/checks/api/ChecksCollection.java
index 55772bd..37d0350 100644
--- a/java/com/google/gerrit/plugins/checks/api/ChecksCollection.java
+++ b/java/com/google/gerrit/plugins/checks/api/ChecksCollection.java
@@ -36,17 +36,21 @@
@Singleton
public class ChecksCollection implements ChildCollection<RevisionResource, CheckResource> {
-
- private final ListChecks listChecks;
+ private final CheckBackfiller checkBackfiller;
private final Checks checks;
private final DynamicMap<RestView<CheckResource>> views;
+ private final ListChecks listChecks;
@Inject
ChecksCollection(
- ListChecks listChecks, Checks checks, DynamicMap<RestView<CheckResource>> views) {
- this.listChecks = listChecks;
+ CheckBackfiller checkBackfiller,
+ Checks checks,
+ DynamicMap<RestView<CheckResource>> views,
+ ListChecks listChecks) {
+ this.checkBackfiller = checkBackfiller;
this.checks = checks;
this.views = views;
+ this.listChecks = listChecks;
}
@Override
@@ -64,8 +68,19 @@
CheckKey checkKey =
CheckKey.create(parent.getProject(), parent.getPatchSet().getId(), checkerUuid);
Optional<Check> check = checks.getCheck(checkKey);
+ if (!check.isPresent()) {
+ check =
+ checkBackfiller.getBackfilledCheckForRelevantChecker(
+ checkerUuid, parent.getNotes(), checkKey.patchSet());
+ }
return new CheckResource(
- parent, check.orElseThrow(() -> new ResourceNotFoundException("Not found: " + id.get())));
+ parent,
+ check.orElseThrow(
+ () ->
+ new ResourceNotFoundException(
+ String.format(
+ "Patch set %s in project %s doesn't have check for checker %s.",
+ checkKey.patchSet(), checkKey.project(), checkerUuid))));
}
@Override
diff --git a/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java b/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java
index dab94b4..582e580 100644
--- a/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java
+++ b/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java
@@ -17,17 +17,13 @@
import static com.google.gerrit.server.api.ApiUtil.asRestApiException;
import com.google.common.collect.ImmutableList;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.plugins.checks.Check;
-import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.CheckerUuid;
-import com.google.gerrit.plugins.checks.Checks;
import com.google.gerrit.plugins.checks.PostCheck;
import com.google.gerrit.server.change.RevisionResource;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
-import java.util.Optional;
class ChecksImpl implements com.google.gerrit.plugins.checks.api.Checks {
@@ -35,44 +31,31 @@
ChecksImpl create(RevisionResource revisionResource);
}
- private final Checks checks;
+ private final CheckApiImpl.Factory checkApiImplFactory;
+ private final ChecksCollection checksCollection;
private final ListChecks listChecks;
private final PostCheck postCheck;
- private final CheckApiImpl.Factory checkApiImplFactory;
private final RevisionResource revisionResource;
@Inject
ChecksImpl(
CheckApiImpl.Factory checkApiImplFactory,
- Checks checks,
+ ChecksCollection checksCollection,
ListChecks listChecks,
PostCheck postCheck,
@Assisted RevisionResource revisionResource) {
this.checkApiImplFactory = checkApiImplFactory;
- this.checks = checks;
- this.postCheck = postCheck;
+ this.checksCollection = checksCollection;
this.listChecks = listChecks;
+ this.postCheck = postCheck;
this.revisionResource = revisionResource;
}
@Override
public CheckApi id(CheckerUuid checkerUuid) throws RestApiException {
try {
- CheckKey checkKey =
- CheckKey.create(
- revisionResource.getProject(), revisionResource.getPatchSet().getId(), checkerUuid);
- Optional<Check> check = checks.getCheck(checkKey);
return checkApiImplFactory.create(
- new CheckResource(
- revisionResource,
- check.orElseThrow(
- () ->
- new ResourceNotFoundException(
- String.format(
- "Patch set %s in project %s doesn't have check for checker %s.",
- revisionResource.getPatchSet().getId(),
- revisionResource.getProject(),
- checkerUuid)))));
+ checksCollection.parse(revisionResource, IdString.fromDecoded(checkerUuid.toString())));
} catch (Exception e) {
throw asRestApiException("Cannot parse check", e);
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
index b36aaf7..416134b 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
@@ -14,19 +14,27 @@
package com.google.gerrit.plugins.checks.acceptance.api;
+import static com.google.common.collect.Iterables.getOnlyElement;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assert_;
+import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.plugins.checks.Check;
+import com.google.gerrit.plugins.checks.CheckJson;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
import com.google.gerrit.plugins.checks.api.CheckInfo;
import com.google.gerrit.plugins.checks.api.CheckState;
+import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.inject.Inject;
+import java.sql.Timestamp;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.RefUpdate;
@@ -37,10 +45,12 @@
public class GetCheckIT extends AbstractCheckersTest {
@Inject private RequestScopeOperations requestScopeOperations;
+ private CheckJson checkJson;
private PatchSet.Id patchSetId;
@Before
public void setUp() throws Exception {
+ checkJson = plugin.getSysInjector().getInstance(CheckJson.class);
patchSetId = createChange().getPatchSetId();
}
@@ -116,15 +126,47 @@
}
@Test
- public void getNonExistingCheck() throws Exception {
- CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ public void getNonExistingCheckBackfillsForRelevantChecker() throws Exception {
+ String topic = name("topic");
+ Change.Id changeId = patchSetId.getParentKey();
+ CheckerUuid checkerUuid =
+ checkerOperations.newChecker().repository(project).query("topic:" + topic).create();
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
- exception.expect(ResourceNotFoundException.class);
- exception.expectMessage(
- String.format(
- "Patch set %s in project %s doesn't have check for checker %s.",
- patchSetId, project, checkerUuid));
- checksApiFactory.revision(patchSetId).id(checkerUuid);
+ assertCheckNotFound(patchSetId, checkerUuid);
+
+ gApi.changes().id(changeId.get()).topic(topic);
+
+ Timestamp psCreated = getPatchSetCreated(changeId);
+ assertThat(checksApiFactory.revision(patchSetId).id(checkerUuid).get())
+ .isEqualTo(
+ checkJson.format(
+ Check.builder(checkKey)
+ .setState(CheckState.NOT_STARTED)
+ .setCreated(psCreated)
+ .setUpdated(psCreated)
+ .build()));
+ }
+
+ @Test
+ public void getNonExistingCheckDoesNotBackfillForDisabledChecker() throws Exception {
+ Change.Id changeId = patchSetId.getParentKey();
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
+
+ Timestamp psCreated = getPatchSetCreated(changeId);
+ assertThat(checksApiFactory.revision(patchSetId).id(checkerUuid).get())
+ .isEqualTo(
+ checkJson.format(
+ Check.builder(checkKey)
+ .setState(CheckState.NOT_STARTED)
+ .setCreated(psCreated)
+ .setUpdated(psCreated)
+ .build()));
+
+ checkerOperations.checker(checkerUuid).forUpdate().disable().update();
+
+ assertCheckNotFound(patchSetId, checkerUuid);
}
@Test
@@ -147,6 +189,12 @@
assertThat(checkInfo).isEqualTo(checkOperations.check(checkKey).asInfo());
}
+ private Timestamp getPatchSetCreated(Change.Id changeId) throws RestApiException {
+ return getOnlyElement(
+ gApi.changes().id(changeId.get()).get(CURRENT_REVISION).revisions.values())
+ .created;
+ }
+
private void deleteCheckerRef(CheckerUuid checkerUuid) throws Exception {
try (Repository allProjectsRepo = repoManager.openRepository(allProjects)) {
TestRepository<InMemoryRepository> testRepo =
@@ -156,4 +204,19 @@
assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED);
}
}
+
+ private void assertCheckNotFound(PatchSet.Id patchSetId, CheckerUuid checkerUuid)
+ throws Exception {
+ try {
+ checksApiFactory.revision(patchSetId).id(checkerUuid);
+ assert_().fail("expected ResourceNotFoundException");
+ } catch (ResourceNotFoundException e) {
+ assertThat(e)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "Patch set %s in project %s doesn't have check for checker %s.",
+ patchSetId, project, checkerUuid));
+ }
+ }
}
