Don't support checks for change edits
Change edits are only visible to change owners
and thus are not useful to checker systems. Thus
this commit adds additional checks to reject
if the caller tries to create/get/list checks
on a change edit.
Instead of doing like this commit, there is another
option which simply returns an empty check set
for a change edit. However, this opens the door
for the client to depend on an unuseful feature
which may break them if we decide to turn it off
in the future.
Change-Id: Icdcbca3afb070813f6cb829c1de787eff1c63782
diff --git a/java/com/google/gerrit/plugins/checks/api/ChecksCollection.java b/java/com/google/gerrit/plugins/checks/api/ChecksCollection.java
index 397d8b6..78d43e4 100644
--- a/java/com/google/gerrit/plugins/checks/api/ChecksCollection.java
+++ b/java/com/google/gerrit/plugins/checks/api/ChecksCollection.java
@@ -19,6 +19,7 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ChildCollection;
import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestReadView;
@@ -57,6 +58,10 @@
@Override
public CheckResource parse(RevisionResource parent, IdString id)
throws RestApiException, PermissionBackendException, IOException, StorageException {
+ if (parent.getEdit().isPresent()) {
+ throw new ResourceConflictException("checks are not supported on a change edit");
+ }
+
CheckerUuid checkerUuid =
CheckerUuid.tryParse(id.get())
.orElseThrow(
diff --git a/java/com/google/gerrit/plugins/checks/api/ListChecks.java b/java/com/google/gerrit/plugins/checks/api/ListChecks.java
index 6f1257d..39346e4 100644
--- a/java/com/google/gerrit/plugins/checks/api/ListChecks.java
+++ b/java/com/google/gerrit/plugins/checks/api/ListChecks.java
@@ -58,6 +58,10 @@
public ImmutableList<CheckInfo> apply(RevisionResource resource)
throws AuthException, BadRequestException, ResourceConflictException, StorageException,
IOException {
+ if (resource.getEdit().isPresent()) {
+ throw new ResourceConflictException("checks are not supported on a change edit");
+ }
+
ImmutableList.Builder<CheckInfo> result = ImmutableList.builder();
GetCheckOptions getCheckOptions = GetCheckOptions.withBackfilling();
diff --git a/java/com/google/gerrit/plugins/checks/api/PostCheck.java b/java/com/google/gerrit/plugins/checks/api/PostCheck.java
index 5de27ad..18572e9 100644
--- a/java/com/google/gerrit/plugins/checks/api/PostCheck.java
+++ b/java/com/google/gerrit/plugins/checks/api/PostCheck.java
@@ -17,6 +17,7 @@
import com.google.gerrit.exceptions.StorageException;
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.RestApiException;
import com.google.gerrit.extensions.restapi.RestCollectionModifyView;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
@@ -81,6 +82,10 @@
}
permissionBackend.currentUser().check(permission);
+ if (rsrc.getEdit().isPresent()) {
+ throw new ResourceConflictException("checks are not supported on a change edit");
+ }
+
if (input == null) {
input = new CheckInput();
}
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
index bd3dd1e..02847e8 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
@@ -103,6 +103,10 @@
// TODO(gerrit-team): Instead of reading the complete notes map, read just one note.
ChangeData changeData = changeDataFactory.create(repositoryName, psId.changeId());
PatchSet patchSet = changeData.patchSet(psId);
+ if (patchSet == null) {
+ throw new StorageException("patch set not found: " + psId);
+ }
+
CheckNotes checkNotes = checkNotesFactory.create(changeData.change());
checkNotes.load();
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
index 808027b..a8a8d19 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
@@ -18,6 +18,7 @@
import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -354,6 +355,23 @@
assertThat(thrown).hasMessageThat().contains("Authentication required");
}
+ @Test
+ public void createCheckOnChangeEditRejected() throws Exception {
+ int changeId = patchSetId.changeId().get();
+ gApi.changes().id(changeId).edit().modifyCommitMessage("new message");
+ assertThat(gApi.changes().id(changeId).edit().get()).isPresent();
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
+ CheckInput input = new CheckInput();
+ input.checkerUuid = checkerUuid.get();
+ input.state = CheckState.RUNNING;
+ RestResponse response =
+ adminRestSession.post("/changes/" + changeId + "/revisions/edit/checks~checks", input);
+
+ response.assertConflict();
+ assertThat(response.getEntityContent()).isEqualTo("checks are not supported on a change edit");
+ }
+
// TODO(gerrit-team) More tests, especially for multiple checkers and PS and how commits behave
private Check getCheck(Project.NameKey project, PatchSet.Id patchSetId, CheckerUuid checkerUuid)
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 2fbeeeb..9e2a066 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
@@ -16,6 +16,7 @@
import static com.google.common.collect.Iterables.getOnlyElement;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
@@ -27,6 +28,7 @@
import com.google.gerrit.acceptance.rest.util.RestCall;
import com.google.gerrit.acceptance.rest.util.RestCall.Method;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -46,6 +48,7 @@
import com.google.inject.Inject;
import java.sql.Timestamp;
import java.time.Instant;
+import java.util.Optional;
import java.util.concurrent.TimeUnit;
import org.junit.After;
import org.junit.Before;
@@ -517,6 +520,22 @@
.contains(String.format("change %d", patchSetId.changeId().get()));
}
+ @Test
+ public void getCheckOnChangeEditRejected() throws Exception {
+ int changeId = patchSetId.changeId().get();
+ gApi.changes().id(changeId).edit().modifyCommitMessage("new message");
+ Optional<EditInfo> editInfo = gApi.changes().id(changeId).edit().get();
+ assertThat(editInfo).isPresent();
+
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ RestResponse response =
+ adminRestSession.get(
+ "/changes/" + changeId + "/revisions/edit/checks~checks/" + checkerUuid.get());
+
+ response.assertConflict();
+ assertThat(response.getEntityContent()).isEqualTo("checks are not supported on a change edit");
+ }
+
private CheckInfo getCheckInfo(
PatchSet.Id patchSetId, CheckerUuid checkerUuid, ListChecksOption... options)
throws RestApiException {
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 bf12827..9650784 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
@@ -23,6 +23,7 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.CheckerUuid;
@@ -327,6 +328,18 @@
assertThat(checksApiFactory.revision(patchSetId).list()).hasSize(1);
}
+ @Test
+ public void listAllOnChangeEditRejected() throws Exception {
+ gApi.changes().id(changeId).edit().modifyCommitMessage("new message");
+ Optional<EditInfo> editInfo = gApi.changes().id(changeId).edit().get();
+ assertThat(editInfo).isPresent();
+
+ RestResponse response =
+ adminRestSession.get("/changes/" + changeId + "/revisions/edit/checks?o=CHECKER");
+ response.assertConflict();
+ assertThat(response.getEntityContent()).isEqualTo("checks are not supported on a change edit");
+ }
+
private Timestamp getPatchSetCreated(Change.Id changeId) throws RestApiException {
return getOnlyElement(
gApi.changes().id(changeId.get()).get(CURRENT_REVISION).revisions.values())
