Validate checker properties in CheckerCreation and CheckerUpdate
It's best to validate the properties as early as possible. Also we don't
want to rely on other layers to do the validation.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I92fa8ce18a1b243c5e7af57f9935a42275306284
diff --git a/java/com/google/gerrit/plugins/checks/CheckerCreation.java b/java/com/google/gerrit/plugins/checks/CheckerCreation.java
index 1870b18..294f354 100644
--- a/java/com/google/gerrit/plugins/checks/CheckerCreation.java
+++ b/java/com/google/gerrit/plugins/checks/CheckerCreation.java
@@ -14,7 +14,10 @@
package com.google.gerrit.plugins.checks;
+import static com.google.common.base.Preconditions.checkState;
+
import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
import com.google.gerrit.reviewdb.client.Project;
@AutoValue
@@ -48,6 +51,20 @@
public abstract Builder setRepository(Project.NameKey repository);
- public abstract CheckerCreation build();
+ abstract CheckerCreation autoBuild();
+
+ public CheckerCreation build() {
+ CheckerCreation checkerCreation = autoBuild();
+ checkState(!checkerCreation.getName().trim().isEmpty(), "checker name cannot be empty");
+ checkState(
+ !checkerCreation.getRepository().get().trim().isEmpty(),
+ "repository name cannot be empty");
+ return checkerCreation;
+ }
+
+ @VisibleForTesting
+ public CheckerCreation buildWithoutValidationForTesting() {
+ return autoBuild();
+ }
}
}
diff --git a/java/com/google/gerrit/plugins/checks/CheckerUpdate.java b/java/com/google/gerrit/plugins/checks/CheckerUpdate.java
index 97165f6..463cb1d 100644
--- a/java/com/google/gerrit/plugins/checks/CheckerUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/CheckerUpdate.java
@@ -14,7 +14,10 @@
package com.google.gerrit.plugins.checks;
+import static com.google.common.base.Preconditions.checkState;
+
import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableSortedSet;
import com.google.gerrit.plugins.checks.api.BlockingCondition;
import com.google.gerrit.plugins.checks.api.CheckerStatus;
@@ -82,6 +85,27 @@
public abstract Builder setQuery(String query);
- public abstract CheckerUpdate build();
+ abstract CheckerUpdate autoBuild();
+
+ public CheckerUpdate build() {
+ CheckerUpdate checkerUpdate = autoBuild();
+
+ if (checkerUpdate.getName().isPresent()) {
+ checkState(!checkerUpdate.getName().get().trim().isEmpty(), "checker name cannot be empty");
+ }
+
+ if (checkerUpdate.getRepository().isPresent()) {
+ checkState(
+ !checkerUpdate.getRepository().get().get().trim().isEmpty(),
+ "repository name cannot be empty");
+ }
+
+ return checkerUpdate;
+ }
+
+ @VisibleForTesting
+ public CheckerUpdate buildWithoutValidationForTesting() {
+ return autoBuild();
+ }
}
}
diff --git a/javatests/com/google/gerrit/plugins/checks/CheckerCreationTest.java b/javatests/com/google/gerrit/plugins/checks/CheckerCreationTest.java
new file mode 100644
index 0000000..36968d3
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/CheckerCreationTest.java
@@ -0,0 +1,83 @@
+// 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;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.gerrit.reviewdb.client.Project;
+import org.junit.Test;
+
+public class CheckerCreationTest {
+ private final CheckerUuid checkerUuid = CheckerUuid.parse("test:my-checker");
+ private final String checkerName = "My Checker";
+ private final Project.NameKey checkerRepository = Project.nameKey("my-repo");
+
+ @Test
+ public void nameCannotBeEmpty() {
+ IllegalStateException thrown =
+ assertThrows(
+ IllegalStateException.class,
+ () ->
+ CheckerCreation.builder()
+ .setCheckerUuid(checkerUuid)
+ .setName("")
+ .setRepository(checkerRepository)
+ .build());
+ assertThat(thrown).hasMessageThat().contains(String.format("checker name cannot be empty"));
+ }
+
+ @Test
+ public void nameCannotBeEmptyAfterTrim() {
+ IllegalStateException thrown =
+ assertThrows(
+ IllegalStateException.class,
+ () ->
+ CheckerCreation.builder()
+ .setCheckerUuid(checkerUuid)
+ .setName(" ")
+ .setRepository(checkerRepository)
+ .build());
+ assertThat(thrown).hasMessageThat().contains(String.format("checker name cannot be empty"));
+ }
+
+ @Test
+ public void repositoryCannotBeEmpty() {
+ IllegalStateException thrown =
+ assertThrows(
+ IllegalStateException.class,
+ () ->
+ CheckerCreation.builder()
+ .setCheckerUuid(checkerUuid)
+ .setName(checkerName)
+ .setRepository(Project.nameKey(""))
+ .build());
+ assertThat(thrown).hasMessageThat().contains(String.format("repository name cannot be empty"));
+ }
+
+ @Test
+ public void repositoryCannotBeEmptyAfterTrim() {
+ IllegalStateException thrown =
+ assertThrows(
+ IllegalStateException.class,
+ () ->
+ CheckerCreation.builder()
+ .setCheckerUuid(checkerUuid)
+ .setName(checkerName)
+ .setRepository(Project.nameKey(" "))
+ .build());
+ assertThat(thrown).hasMessageThat().contains(String.format("repository name cannot be empty"));
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/checks/CheckerUpdateTest.java b/javatests/com/google/gerrit/plugins/checks/CheckerUpdateTest.java
new file mode 100644
index 0000000..ae2ed4d
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/CheckerUpdateTest.java
@@ -0,0 +1,57 @@
+// 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;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.gerrit.reviewdb.client.Project;
+import org.junit.Test;
+
+public class CheckerUpdateTest {
+ @Test
+ public void nameCannotBeEmpty() {
+ IllegalStateException thrown =
+ assertThrows(
+ IllegalStateException.class, () -> CheckerUpdate.builder().setName("").build());
+ assertThat(thrown).hasMessageThat().contains(String.format("checker name cannot be empty"));
+ }
+
+ @Test
+ public void nameCannotBeEmptyAfterTrim() {
+ IllegalStateException thrown =
+ assertThrows(
+ IllegalStateException.class, () -> CheckerUpdate.builder().setName(" ").build());
+ assertThat(thrown).hasMessageThat().contains(String.format("checker name cannot be empty"));
+ }
+
+ @Test
+ public void repositoryCannotBeEmpty() {
+ IllegalStateException thrown =
+ assertThrows(
+ IllegalStateException.class,
+ () -> CheckerUpdate.builder().setRepository(Project.nameKey("")).build());
+ assertThat(thrown).hasMessageThat().contains(String.format("repository name cannot be empty"));
+ }
+
+ @Test
+ public void repositoryCannotBeEmptyAfterTrim() {
+ IllegalStateException thrown =
+ assertThrows(
+ IllegalStateException.class,
+ () -> CheckerUpdate.builder().setRepository(Project.nameKey(" ")).build());
+ assertThat(thrown).hasMessageThat().contains(String.format("repository name cannot be empty"));
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/checks/db/CheckerConfigTest.java b/javatests/com/google/gerrit/plugins/checks/db/CheckerConfigTest.java
index 35c741f..a4d02b1 100644
--- a/javatests/com/google/gerrit/plugins/checks/db/CheckerConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/db/CheckerConfigTest.java
@@ -109,7 +109,8 @@
@Test
public void nameOfNewCheckerMustNotBeEmpty() throws Exception {
- CheckerCreation checkerCreation = getPrefilledCheckerCreationBuilder().setName("").build();
+ CheckerCreation checkerCreation =
+ getPrefilledCheckerCreationBuilder().setName("").buildWithoutValidationForTesting();
CheckerConfig checkerConfig =
CheckerConfig.createForNewChecker(projectName, repository, checkerCreation);
@@ -232,7 +233,9 @@
@Test
public void repositoryOfNewCheckerMustNotBeEmpty() throws Exception {
CheckerCreation checkerCreation =
- getPrefilledCheckerCreationBuilder().setRepository(Project.nameKey("")).build();
+ getPrefilledCheckerCreationBuilder()
+ .setRepository(Project.nameKey(""))
+ .buildWithoutValidationForTesting();
CheckerConfig checkerConfig =
CheckerConfig.createForNewChecker(projectName, repository, checkerCreation);
@@ -319,7 +322,8 @@
public void nameCannotBeRemoved() throws Exception {
createArbitraryChecker(checkerUuid);
- CheckerUpdate checkerUpdate = CheckerUpdate.builder().setName("").build();
+ CheckerUpdate checkerUpdate =
+ CheckerUpdate.builder().setName("").buildWithoutValidationForTesting();
IOException thrown =
assertThrows(IOException.class, () -> updateChecker(checkerUuid, checkerUpdate));
@@ -400,7 +404,9 @@
createArbitraryChecker(checkerUuid);
CheckerUpdate checkerUpdate =
- CheckerUpdate.builder().setRepository(Project.nameKey("")).build();
+ CheckerUpdate.builder()
+ .setRepository(Project.nameKey(""))
+ .buildWithoutValidationForTesting();
IOException thrown =
assertThrows(IOException.class, () -> updateChecker(checkerUuid, checkerUpdate));
