Consistently return all checks from NoteDb for Get Check and List Checks
In our recent discussions we decided that for the Get Check and List
Checks REST endpoints we always want to return all checks that exist in
NoteDb, regardless whether the corresponding checker is diabled, no
longer applying (repo or query changed), not-existing or invalid (not
parseable).
Reasons for returning those checks:
1. better UX as checks that have been once shown on the screen do not
magically disappear because the checker no loger applies
2. the UI has full control over how to visiulize these checks (the UI
knows the checks and the states of the corresponding checkers)
Before this change the behaviour was not consistent, e.g. it was
possible to get checks of a checker that no longer applies because the
query was changed, but it was not possible to get checks of a checker
that no longer applies because the repository was changed.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I523254403375d87af6f7c23c21e72417e39e0bd7
diff --git a/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java b/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java
index c75a4d7..dab94b4 100644
--- a/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java
+++ b/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java
@@ -22,7 +22,6 @@
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.Checkers;
import com.google.gerrit.plugins.checks.Checks;
import com.google.gerrit.plugins.checks.PostCheck;
import com.google.gerrit.server.change.RevisionResource;
@@ -36,7 +35,6 @@
ChecksImpl create(RevisionResource revisionResource);
}
- private final Checkers checkers;
private final Checks checks;
private final ListChecks listChecks;
private final PostCheck postCheck;
@@ -47,36 +45,18 @@
ChecksImpl(
CheckApiImpl.Factory checkApiImplFactory,
Checks checks,
- Checkers checkers,
ListChecks listChecks,
PostCheck postCheck,
@Assisted RevisionResource revisionResource) {
this.checkApiImplFactory = checkApiImplFactory;
this.checks = checks;
this.postCheck = postCheck;
- this.checkers = checkers;
this.listChecks = listChecks;
this.revisionResource = revisionResource;
}
@Override
public CheckApi id(CheckerUuid checkerUuid) throws RestApiException {
- // Ensure that the checker exists.
- // TODO(gerrit-team): We are not asking for any permission for resolving the checker. This means
- // users can probe which checkers exist. That's okay for now. The only permission that we have
- // and that we could check is the 'Administrate Checkers' permission, but this permission should
- // not be required for viewing checks on a change that is visible to the user.
- try {
- checkers
- .getChecker(checkerUuid)
- .orElseThrow(
- () ->
- new ResourceNotFoundException(
- String.format("Checker %s not found", checkerUuid)));
- } catch (Exception e) {
- throw asRestApiException("Cannot retrieve checker", e);
- }
-
try {
CheckKey checkKey =
CheckKey.create(
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
index bced9f3..9b0b085 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
@@ -15,15 +15,11 @@
package com.google.gerrit.plugins.checks.db;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
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.plugins.checks.Checks;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
@@ -34,7 +30,6 @@
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Optional;
-import java.util.Set;
import java.util.stream.Stream;
/** Class to read checks from NoteDb. */
@@ -43,18 +38,15 @@
private final ChangeNotes.Factory changeNotesFactory;
private final PatchSetUtil psUtil;
private final CheckNotes.Factory checkNotesFactory;
- private final Checkers checkers;
@Inject
NoteDbChecks(
ChangeNotes.Factory changeNotesFactory,
PatchSetUtil psUtil,
- CheckNotes.Factory checkNotesFactory,
- Checkers checkers) {
+ CheckNotes.Factory checkNotesFactory) {
this.changeNotesFactory = changeNotesFactory;
this.psUtil = psUtil;
this.checkNotesFactory = checkNotesFactory;
- this.checkers = checkers;
}
@Override
@@ -72,31 +64,19 @@
}
private Stream<Check> getChecksAsStream(Project.NameKey projectName, PatchSet.Id psId)
- throws OrmException, IOException {
+ throws OrmException {
// TODO(gerrit-team): Instead of reading the complete notes map, read just one note.
ChangeNotes notes = changeNotesFactory.create(projectName, psId.getParentKey());
PatchSet patchSet = psUtil.get(notes, psId);
CheckNotes checkNotes = checkNotesFactory.create(notes.getChange());
checkNotes.load();
- Set<CheckerUuid> checkerUuids = activeAndValidCheckersForProject(projectName);
return checkNotes
.getChecks()
.getOrDefault(patchSet.getRevision(), NoteDbCheckMap.empty())
.checks
.entrySet()
.stream()
- .map(e -> e.getValue().toCheck(projectName, psId, CheckerUuid.parse(e.getKey())))
- .filter(c -> checkerUuids.contains(c.key().checkerUuid()));
- }
-
- /** Get all checkers that apply to a project. Might return a superset of checkers that apply. */
- private ImmutableSet<CheckerUuid> activeAndValidCheckersForProject(Project.NameKey projectName)
- throws IOException {
- return checkers
- .checkersOf(projectName)
- .stream()
- .map(Checker::getUuid)
- .collect(toImmutableSet());
+ .map(e -> e.getValue().toCheck(projectName, psId, CheckerUuid.parse(e.getKey())));
}
}
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 c3f8edd..b36aaf7 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
@@ -19,7 +19,6 @@
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.CheckKey;
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
@@ -28,6 +27,10 @@
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.inject.Inject;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.lib.Repository;
import org.junit.Before;
import org.junit.Test;
@@ -60,16 +63,22 @@
CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
checkOperations.newCheck(checkKey).setState(CheckState.RUNNING).upsert();
- 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);
+ CheckInfo checkInfo = checksApiFactory.revision(patchSetId).id(checkerUuid).get();
+ assertThat(checkInfo).isEqualTo(checkOperations.check(checkKey).asInfo());
}
- // TODO(gerrit-team): I think according to our latest discussions it should be possible to
- // retrieve checks of disabled checkers, when we implement this, this test needs to be adapted
+ @Test
+ public void getCheckForCheckerThatDoesNotApplyToTheChange() throws Exception {
+ CheckerUuid checkerUuid =
+ checkerOperations.newChecker().repository(project).query("message:not-matching").create();
+
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
+ checkOperations.newCheck(checkKey).setState(CheckState.RUNNING).upsert();
+
+ CheckInfo checkInfo = checksApiFactory.revision(patchSetId).id(checkerUuid).get();
+ assertThat(checkInfo).isEqualTo(checkOperations.check(checkKey).asInfo());
+ }
+
@Test
public void getCheckForDisabledChecker() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
@@ -79,6 +88,37 @@
checkerOperations.checker(checkerUuid).forUpdate().disable().update();
+ CheckInfo checkInfo = checksApiFactory.revision(patchSetId).id(checkerUuid).get();
+ assertThat(checkInfo).isEqualTo(checkOperations.check(checkKey).asInfo());
+ }
+
+ @Test
+ public void getCheckForInvalidChecker() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
+ checkOperations.newCheck(checkKey).setState(CheckState.RUNNING).upsert();
+
+ checkerOperations.checker(checkerUuid).forUpdate().forceInvalidConfig().update();
+
+ CheckInfo checkInfo = checksApiFactory.revision(patchSetId).id(checkerUuid).get();
+ assertThat(checkInfo).isEqualTo(checkOperations.check(checkKey).asInfo());
+ }
+
+ @Test
+ public void getCheckForNonExistingChecker() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
+ checkOperations.newCheck(checkKey).setState(CheckState.RUNNING).upsert();
+ deleteCheckerRef(checkerUuid);
+
+ CheckInfo checkInfo = checksApiFactory.revision(patchSetId).id(checkerUuid).get();
+ assertThat(checkInfo).isEqualTo(checkOperations.check(checkKey).asInfo());
+ }
+
+ @Test
+ public void getNonExistingCheck() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
exception.expect(ResourceNotFoundException.class);
exception.expectMessage(
String.format(
@@ -88,26 +128,6 @@
}
@Test
- public void getCheckForInvalidChecker() throws Exception {
- CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
- CheckKey key = CheckKey.create(project, patchSetId, checkerUuid);
- checkOperations.newCheck(key).setState(CheckState.RUNNING).upsert();
-
- checkerOperations.checker(checkerUuid).forUpdate().forceInvalidConfig().update();
-
- exception.expect(RestApiException.class);
- exception.expectMessage("Cannot retrieve checker");
- checksApiFactory.revision(patchSetId).id(checkerUuid.toString());
- }
-
- @Test
- public void getCheckForNonExistingChecker() throws Exception {
- exception.expect(ResourceNotFoundException.class);
- exception.expectMessage("Checker test:non-existing not found");
- checksApiFactory.revision(patchSetId).id("test:non-existing");
- }
-
- @Test
public void getCheckForInvalidCheckerUuid() throws Exception {
exception.expect(BadRequestException.class);
exception.expectMessage("invalid checker UUID: malformed::checker*UUID");
@@ -126,4 +146,14 @@
CheckInfo checkInfo = checksApiFactory.revision(patchSetId).id(checkerUuid).get();
assertThat(checkInfo).isEqualTo(checkOperations.check(checkKey).asInfo());
}
+
+ private void deleteCheckerRef(CheckerUuid checkerUuid) throws Exception {
+ try (Repository allProjectsRepo = repoManager.openRepository(allProjects)) {
+ TestRepository<InMemoryRepository> testRepo =
+ new TestRepository<>((InMemoryRepository) allProjectsRepo);
+ RefUpdate ru = testRepo.getRepository().updateRef(checkerUuid.toRefName(), true);
+ ru.setForceUpdate(true);
+ assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED);
+ }
+ }
}
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 b38865e..64f8936 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
@@ -22,7 +22,12 @@
import com.google.gerrit.plugins.checks.api.CheckInfo;
import com.google.gerrit.plugins.checks.api.CheckState;
import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.Project;
import java.util.Collection;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.lib.Repository;
import org.junit.Before;
import org.junit.Test;
@@ -54,18 +59,71 @@
}
@Test
- public void listExcludesCheckFromDisabledChecker() throws Exception {
- checkerOperations.checker(checkKey2.checkerUuid()).forUpdate().disable().update();
+ public void listIncludesCheckFromCheckerThatDoesNotApplyToTheProject() throws Exception {
+ Project.NameKey otherProject = createProjectOverAPI("other", null, true, null);
+ checkerOperations
+ .checker(checkKey2.checkerUuid())
+ .forUpdate()
+ .repository(otherProject)
+ .update();
Collection<CheckInfo> info = checksApiFactory.revision(patchSetId).list();
- assertThat(info).containsExactly(checkOperations.check(checkKey1).asInfo());
+ assertThat(info)
+ .containsExactly(
+ checkOperations.check(checkKey1).asInfo(), checkOperations.check(checkKey2).asInfo());
}
@Test
- public void listExcludesCheckFromInvalidChecker() throws Exception {
+ public void listIncludesCheckFromCheckerThatDoesNotApplyToTheChange() throws Exception {
+ checkerOperations
+ .checker(checkKey2.checkerUuid())
+ .forUpdate()
+ .query("message:not-matching")
+ .update();
+
+ Collection<CheckInfo> info = checksApiFactory.revision(patchSetId).list();
+ assertThat(info)
+ .containsExactly(
+ checkOperations.check(checkKey1).asInfo(), checkOperations.check(checkKey2).asInfo());
+ }
+
+ @Test
+ public void listIncludesCheckFromDisabledChecker() throws Exception {
+ checkerOperations.checker(checkKey2.checkerUuid()).forUpdate().disable().update();
+
+ Collection<CheckInfo> info = checksApiFactory.revision(patchSetId).list();
+ assertThat(info)
+ .containsExactly(
+ checkOperations.check(checkKey1).asInfo(), checkOperations.check(checkKey2).asInfo());
+ }
+
+ @Test
+ public void listIncludesCheckFromInvalidChecker() throws Exception {
checkerOperations.checker(checkKey2.checkerUuid()).forUpdate().forceInvalidConfig().update();
Collection<CheckInfo> info = checksApiFactory.revision(patchSetId).list();
- assertThat(info).containsExactly(checkOperations.check(checkKey1).asInfo());
+ assertThat(info)
+ .containsExactly(
+ checkOperations.check(checkKey1).asInfo(), checkOperations.check(checkKey2).asInfo());
+ }
+
+ @Test
+ public void listIncludesCheckFromNonExistingChecker() throws Exception {
+ deleteCheckerRef(checkKey2.checkerUuid());
+
+ Collection<CheckInfo> info = checksApiFactory.revision(patchSetId).list();
+ assertThat(info)
+ .containsExactly(
+ checkOperations.check(checkKey1).asInfo(), checkOperations.check(checkKey2).asInfo());
+ }
+
+ private void deleteCheckerRef(CheckerUuid checkerUuid) throws Exception {
+ try (Repository allProjectsRepo = repoManager.openRepository(allProjects)) {
+ TestRepository<InMemoryRepository> testRepo =
+ new TestRepository<>((InMemoryRepository) allProjectsRepo);
+ RefUpdate ru = testRepo.getRepository().updateRef(checkerUuid.toRefName(), true);
+ ru.setForceUpdate(true);
+ assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED);
+ }
}
}
