ListChecks: Include implicit NOT_STARTED relevant checkers
Checkers that are relevant for the change should have their checks
reported to users even if the external system has not yet reported any
check status. Keep track of which checkers associated with the repo are
not found in the check map, and backfill the results with synthetic
entries for any relevant ones.
This backfilling logic only takes effect in the API layer, not the
underlying NoteDb layer. The NoteDb layer is about reporting exactly
what is being stored, and shouldn't need to have a dependency on the
whole change query subsystem to do so.
This change only backfills entries when listing checkers. Backfilling
individual checks when looking up by key will be implemented in a
followup.
Change-Id: I2d105efecb83c0c6c29fa0c8a6eee323d39bdb2f
diff --git a/java/com/google/gerrit/plugins/checks/api/CheckBackfiller.java b/java/com/google/gerrit/plugins/checks/api/CheckBackfiller.java
new file mode 100644
index 0000000..da98cd9
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/api/CheckBackfiller.java
@@ -0,0 +1,124 @@
+// Copyright (C) 2019 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.plugins.checks.api;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.index.query.IndexPredicate;
+import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.index.query.QueryParseException;
+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.reviewdb.client.PatchSet;
+import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.index.change.ChangeField;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeData.Factory;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
+import com.google.gerrit.server.query.change.ChangeStatusPredicate;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import java.util.Collection;
+
+@Singleton
+class CheckBackfiller {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final ChangeData.Factory changeDataFactory;
+ private final Provider<AnonymousUser> anonymousUserProvider;
+ private final Provider<ChangeQueryBuilder> queryBuilderProvider;
+
+ @Inject
+ CheckBackfiller(
+ Factory changeDataFactory,
+ Provider<AnonymousUser> anonymousUserProvider,
+ Provider<ChangeQueryBuilder> queryBuilderProvider) {
+ this.changeDataFactory = changeDataFactory;
+ this.anonymousUserProvider = anonymousUserProvider;
+ this.queryBuilderProvider = queryBuilderProvider;
+ }
+
+ ImmutableList<Check> getBackfilledChecksForRelevantCheckers(
+ Collection<Checker> candidates, ChangeNotes notes, PatchSet.Id psId) throws OrmException {
+ if (candidates.isEmpty()) {
+ return ImmutableList.of();
+ }
+ 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 ImmutableList.of();
+ }
+ // All candidates need to be checked for relevance. Any relevant checkers are reported as
+ // 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());
+ 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());
+ }
+ }
+ return result.build();
+ }
+
+ private static boolean matches(Checker checker, ChangeData cd, ChangeQueryBuilder queryBuilder)
+ throws OrmException {
+ if (!checker.getQuery().isPresent()) {
+ return cd.change().isNew();
+ }
+ String query = checker.getQuery().get();
+ Predicate<ChangeData> predicate;
+ try {
+ predicate = queryBuilder.parse(query);
+ } catch (QueryParseException e) {
+ logger.atWarning().withCause(e).log(
+ "skipping invalid query for checker %s: %s", checker.getUuid(), query);
+ return false;
+ }
+ if (!predicate.isMatchable()) {
+ // Assuming nobody modified the query behind Gerrit's back, this is programmer error:
+ // CheckerQuery should not be able to produce non-matchable queries.
+ logger.atWarning().log(
+ "skipping non-matchable query for checker %s: %s", checker.getUuid(), query);
+ return false;
+ }
+ if (!hasStatusPredicate(predicate)) {
+ predicate = Predicate.and(ChangeStatusPredicate.open(), predicate);
+ }
+ return predicate.asMatchable().match(cd);
+ }
+
+ private static boolean hasStatusPredicate(Predicate<ChangeData> predicate) {
+ if (predicate instanceof IndexPredicate) {
+ return ((IndexPredicate<ChangeData>) predicate)
+ .getField()
+ .getName()
+ .equals(ChangeField.STATUS.getName());
+ }
+ return predicate.getChildren().stream().anyMatch(CheckBackfiller::hasStatusPredicate);
+ }
+}
diff --git a/java/com/google/gerrit/plugins/checks/api/ListChecks.java b/java/com/google/gerrit/plugins/checks/api/ListChecks.java
index 11ae7ad..6cfa6fc 100644
--- a/java/com/google/gerrit/plugins/checks/api/ListChecks.java
+++ b/java/com/google/gerrit/plugins/checks/api/ListChecks.java
@@ -14,41 +14,66 @@
package com.google.gerrit.plugins.checks.api;
-import static com.google.common.collect.ImmutableList.toImmutableList;
+import static java.util.stream.Collectors.toMap;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestReadView;
+import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckJson;
+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.plugins.checks.Checks;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
+import java.util.Map;
@Singleton
public class ListChecks implements RestReadView<RevisionResource> {
-
- private final Checks checks;
+ private final CheckBackfiller checkBackfiller;
private final CheckJson checkJson;
+ private final Checkers checkers;
+ private final Checks checks;
@Inject
- ListChecks(Checks checks, CheckJson checkJson) {
- this.checks = checks;
+ ListChecks(
+ CheckBackfiller checkBackfiller, CheckJson checkJson, Checkers checkers, Checks checks) {
+ this.checkBackfiller = checkBackfiller;
this.checkJson = checkJson;
+ this.checkers = checkers;
+ this.checks = checks;
}
@Override
public ImmutableList<CheckInfo> apply(RevisionResource resource)
throws AuthException, BadRequestException, ResourceConflictException, OrmException,
IOException {
- return checks
- .getChecks(resource.getProject(), resource.getPatchSet().getId())
+ Map<CheckerUuid, Checker> checkersByUuid =
+ checkers
+ .checkersOf(resource.getProject())
+ .stream()
+ .collect(toMap(Checker::getUuid, c -> c));
+
+ ImmutableList.Builder<CheckInfo> result =
+ ImmutableList.builderWithExpectedSize(checkersByUuid.size());
+ for (Check check : checks.getChecks(resource.getProject(), resource.getPatchSet().getId())) {
+ checkersByUuid.remove(check.key().checkerUuid());
+ result.add(checkJson.format(check));
+ }
+
+ checkBackfiller
+ .getBackfilledChecksForRelevantCheckers(
+ checkersByUuid.values(), resource.getNotes(), resource.getPatchSet().getId())
.stream()
.map(checkJson::format)
- .collect(toImmutableList());
+ .forEach(result::add);
+
+ return result.build();
}
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
index 64f8936..e085d91 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
@@ -14,15 +14,22 @@
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.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
+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 java.sql.Timestamp;
import java.util.Collection;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
@@ -32,12 +39,14 @@
import org.junit.Test;
public class ListChecksIT extends AbstractCheckersTest {
+ private CheckJson checkJson;
private PatchSet.Id patchSetId;
private CheckKey checkKey1;
private CheckKey checkKey2;
@Before
public void setUp() throws Exception {
+ checkJson = plugin.getSysInjector().getInstance(CheckJson.class);
patchSetId = createChange().getPatchSetId();
CheckerUuid checker1Uuid = checkerOperations.newChecker().repository(project).create();
@@ -117,6 +126,70 @@
checkOperations.check(checkKey1).asInfo(), checkOperations.check(checkKey2).asInfo());
}
+ @Test
+ public void listBackfillsForRelevantChecker() throws Exception {
+ String topic = name("topic");
+ Change.Id changeId = patchSetId.getParentKey();
+ CheckerUuid checker3Uuid =
+ checkerOperations.newChecker().repository(project).query("topic:" + topic).create();
+ CheckKey checkKey3 = CheckKey.create(project, patchSetId, checker3Uuid);
+
+ CheckInfo checkInfo1 = checkOperations.check(checkKey1).asInfo();
+ CheckInfo checkInfo2 = checkOperations.check(checkKey2).asInfo();
+ assertThat(checksApiFactory.revision(patchSetId).list())
+ .containsExactly(checkInfo1, checkInfo2);
+
+ // Update change to match checker3's query.
+ gApi.changes().id(changeId.get()).topic(topic);
+
+ // TODO(dborowitz): Get from checkOperations once we backfill the get API as well.
+ Timestamp psCreated = getPatchSetCreated(changeId);
+ CheckInfo checkInfo3 =
+ checkJson.format(
+ Check.builder(checkKey3)
+ .setState(CheckState.NOT_STARTED)
+ .setCreated(psCreated)
+ .setUpdated(psCreated)
+ .build());
+ assertThat(checksApiFactory.revision(patchSetId).list())
+ .containsExactly(checkInfo1, checkInfo2, checkInfo3);
+ }
+
+ @Test
+ public void listDoesntBackfillForDisabledChecker() throws Exception {
+ Change.Id changeId = patchSetId.getParentKey();
+ CheckerUuid checker3Uuid = checkerOperations.newChecker().repository(project).create();
+ CheckKey checkKey3 = CheckKey.create(project, patchSetId, checker3Uuid);
+
+ CheckInfo checkInfo1 = checkOperations.check(checkKey1).asInfo();
+ CheckInfo checkInfo2 = checkOperations.check(checkKey2).asInfo();
+
+ // TODO(dborowitz): Get from checkOperations once we backfill the get API as well.
+ Timestamp psCreated = getPatchSetCreated(changeId);
+ CheckInfo checkInfo3 =
+ checkJson.format(
+ Check.builder(checkKey3)
+ .setState(CheckState.NOT_STARTED)
+ .setCreated(psCreated)
+ .setUpdated(psCreated)
+ .build());
+
+ assertThat(checksApiFactory.revision(patchSetId).list())
+ .containsExactly(checkInfo1, checkInfo2, checkInfo3);
+
+ // Disable checker3.
+ checkerOperations.checker(checker3Uuid).forUpdate().disable().update();
+
+ assertThat(checksApiFactory.revision(patchSetId).list())
+ .containsExactly(checkInfo1, checkInfo2);
+ }
+
+ 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 =
