commit 888ff4fdff16872b1d2791f327cf412182eefa39
author Edwin Kempin <ekempin@google.com> Mon Mar 11 16:14:52 2019 +0100
committer Edwin Kempin <ekempin@google.com> Tue Mar 12 17:12:32 2019 +0100
tree 377e316d2bee6f8f0def24f568eeb7a94640c5dc
parent 08a3a9707819c498e3d6537c12eac56d3657e628

Allow to update checks of non-existing and invalid checkers

We prevent creation of checks for non-existing and invalid checkers
(invalid checker = checker with non-parseable config). However if a
checker gets deleted/invalid after a check for it was created we allow
further updates to this check.

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Idc4d21dc28becc6e4a457dbeeabefd89e114028f

java/com/google/gerrit/plugins/checks/PostCheck.java

diff --git a/java/com/google/gerrit/plugins/checks/PostCheck.java b/java/com/google/gerrit/plugins/checks/PostCheck.java
index 9b78e07..5c69330 100644
--- a/java/com/google/gerrit/plugins/checks/PostCheck.java
+++ b/java/com/google/gerrit/plugins/checks/PostCheck.java
@@ -76,16 +76,17 @@
     permissionBackend.currentUser().check(permission);
 
     CheckerUuid checkerUuid = CheckerUuid.parse(input.checkerUuid);
-    checkers
-        .getChecker(checkerUuid)
-        .orElseThrow(
-            () ->
-                new UnprocessableEntityException(
-                    String.format("checker %s not found", checkerUuid)));
 
     CheckKey key = CheckKey.create(rsrc.getProject(), rsrc.getPatchSet().getId(), checkerUuid);
     Optional<Check> check = checks.getCheck(key);
     if (!check.isPresent()) {
+      checkers
+          .getChecker(checkerUuid)
+          .orElseThrow(
+              () ->
+                  new UnprocessableEntityException(
+                      String.format("checker %s not found", checkerUuid)));
+
       if (input.state == null) {
         throw new BadRequestException("state is required on creation");
       }

java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java

diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java b/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java
index f39802f..3384bad 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java
@@ -156,7 +156,9 @@
 
   private Check upsertCheckInNoteDb(CheckKey checkKey, CheckUpdate checkUpdate, Operation operation)
       throws IOException, ConfigInvalidException, OrmDuplicateKeyException {
-    assertCheckerIsPresent(checkKey.checkerUuid());
+    if (operation == Operation.CREATE) {
+      assertCheckerIsPresent(checkKey.checkerUuid());
+    }
 
     try (Repository repo = repoManager.openRepository(checkKey.project());
         ObjectInserter objectInserter = repo.newObjectInserter();

javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java

diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
index 1ed9ca8..6446160 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
@@ -27,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;
 
@@ -93,6 +97,28 @@
   }
 
   @Test
+  public void canUpdateCheckForNonExistingChecker() throws Exception {
+    deleteCheckerRef(checkKey.checkerUuid());
+
+    CheckInput input = new CheckInput();
+    input.state = CheckState.SUCCESSFUL;
+
+    CheckInfo info = checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+    assertThat(info.state).isEqualTo(CheckState.SUCCESSFUL);
+  }
+
+  @Test
+  public void canUpdateCheckForInvalidChecker() throws Exception {
+    checkerOperations.checker(checkKey.checkerUuid()).forUpdate().forceInvalidConfig().update();
+
+    CheckInput input = new CheckInput();
+    input.state = CheckState.SUCCESSFUL;
+
+    CheckInfo info = checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+    assertThat(info.state).isEqualTo(CheckState.SUCCESSFUL);
+  }
+
+  @Test
   public void cannotUpdateCheckWithoutAdministrateCheckers() throws Exception {
     requestScopeOperations.setApiUser(user.getId());
 
@@ -100,4 +126,14 @@
     exception.expectMessage("not permitted");
     checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(new CheckInput());
   }
+
+  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);
+    }
+  }
 }