Make name of checker mandatory
So far it was possible to create checkers without name, but unsetting a
name of an existing checker was not allowed. Make this consistent and
require a name also on checker creation. The documentation already
claimed that it was mandatory.
The same as for all other mandatory checker properties, a missing name
makes a checker invalid and it cannot be accessed through the API. This
means existing checkers without name have to be fixed manually to work
after this change. This is OK since the checks plugin is not widely used
yet.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I08822edb6d068d750b3905bc02e01d44914dfc94
diff --git a/java/com/google/gerrit/plugins/checks/CheckJson.java b/java/com/google/gerrit/plugins/checks/CheckJson.java
index e4199e9..f36c79f 100644
--- a/java/com/google/gerrit/plugins/checks/CheckJson.java
+++ b/java/com/google/gerrit/plugins/checks/CheckJson.java
@@ -86,7 +86,7 @@
.getChecker(checkerUuid)
.ifPresent(
checker -> {
- info.checkerName = checker.getName().orElse(null);
+ info.checkerName = checker.getName();
info.checkerStatus = checker.getStatus();
info.blocking = checker.getBlockingConditions();
});
diff --git a/java/com/google/gerrit/plugins/checks/Checker.java b/java/com/google/gerrit/plugins/checks/Checker.java
index 075e2c9..348bd5e 100644
--- a/java/com/google/gerrit/plugins/checks/Checker.java
+++ b/java/com/google/gerrit/plugins/checks/Checker.java
@@ -39,11 +39,11 @@
/**
* Returns the display name of the checker.
*
- * <p>Checkers may not have a name, in this case {@link Optional#empty()} is returned.
+ * <p>The same name may be used by multiple checkers.
*
* @return display name of the checker
*/
- public abstract Optional<String> getName();
+ public abstract String getName();
/**
* Returns the description of the checker.
diff --git a/java/com/google/gerrit/plugins/checks/CheckerCreation.java b/java/com/google/gerrit/plugins/checks/CheckerCreation.java
index f814ce4..1870b18 100644
--- a/java/com/google/gerrit/plugins/checks/CheckerCreation.java
+++ b/java/com/google/gerrit/plugins/checks/CheckerCreation.java
@@ -26,6 +26,13 @@
*/
public abstract CheckerUuid getCheckerUuid();
+ /**
+ * Defines the name the checker should have.
+ *
+ * <p>The same name may be used by multiple checkers.
+ */
+ public abstract String getName();
+
/** Defines the repository for which the checker applies. */
public abstract Project.NameKey getRepository();
@@ -37,6 +44,8 @@
public abstract static class Builder {
public abstract Builder setCheckerUuid(CheckerUuid checkerUuid);
+ public abstract Builder setName(String name);
+
public abstract Builder setRepository(Project.NameKey repository);
public abstract CheckerCreation build();
diff --git a/java/com/google/gerrit/plugins/checks/CheckerJson.java b/java/com/google/gerrit/plugins/checks/CheckerJson.java
index 701a169..55fdf91 100644
--- a/java/com/google/gerrit/plugins/checks/CheckerJson.java
+++ b/java/com/google/gerrit/plugins/checks/CheckerJson.java
@@ -23,7 +23,7 @@
public CheckerInfo format(Checker checker) {
CheckerInfo info = new CheckerInfo();
info.uuid = checker.getUuid().get();
- info.name = checker.getName().orElse(null);
+ info.name = checker.getName();
info.description = checker.getDescription().orElse(null);
info.url = checker.getUrl().orElse(null);
info.repository = checker.getRepository().get();
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java
index 984cd31..e96f0c2 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java
@@ -115,8 +115,13 @@
checkerCreation
.uuid()
.orElseGet(() -> CheckerUuid.parse("test:checker-" + checkerCounter.incrementAndGet()));
+ String checkerName = checkerCreation.name().orElse("Test Checker");
Project.NameKey repository = checkerCreation.repository().orElse(allProjectsName);
- return CheckerCreation.builder().setCheckerUuid(checkerUuid).setRepository(repository).build();
+ return CheckerCreation.builder()
+ .setCheckerUuid(checkerUuid)
+ .setName(checkerName)
+ .setRepository(repository)
+ .build();
}
private static CheckerUpdate toCheckerUpdate(TestCheckerCreation checkerCreation) {
@@ -268,6 +273,10 @@
unsetValueInCheckerConfig("uuid");
}
+ if (testCheckerInvalidation.unsetName()) {
+ unsetValueInCheckerConfig("name");
+ }
+
if (testCheckerInvalidation.unsetRepository()) {
unsetValueInCheckerConfig("repository");
}
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckerInvalidation.java b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckerInvalidation.java
index f845ddd..850d7c8 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckerInvalidation.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckerInvalidation.java
@@ -29,6 +29,8 @@
public abstract boolean unsetUuid();
+ public abstract boolean unsetName();
+
public abstract boolean unsetRepository();
public abstract boolean unsetStatus();
@@ -45,6 +47,7 @@
.invalidBlockingCondition(false)
.invalidStatus(false)
.unsetUuid(false)
+ .unsetName(false)
.unsetRepository(false)
.unsetStatus(false)
.deleteRef(false);
@@ -80,7 +83,13 @@
return unsetUuid(true);
}
- abstract Builder unsetUuid(boolean unsetUuid);
+ abstract Builder unsetUuid(boolean unsetName);
+
+ public Builder unsetName() {
+ return unsetName(true);
+ }
+
+ abstract Builder unsetName(boolean unsetName);
public Builder unsetRepository() {
return unsetRepository(true);
diff --git a/java/com/google/gerrit/plugins/checks/api/CreateChecker.java b/java/com/google/gerrit/plugins/checks/api/CreateChecker.java
index ff533a7..8ae0513 100644
--- a/java/com/google/gerrit/plugins/checks/api/CreateChecker.java
+++ b/java/com/google/gerrit/plugins/checks/api/CreateChecker.java
@@ -99,15 +99,20 @@
CheckerUuid.tryParse(input.uuid)
.orElseThrow(() -> new BadRequestException("invalid uuid: " + uuidStr));
+ String name = CheckerName.clean(input.name);
+ if (name.isEmpty()) {
+ throw new BadRequestException("name is required");
+ }
+
Project.NameKey repository = resolveRepository(input.repository);
CheckerCreation.Builder checkerCreationBuilder =
- CheckerCreation.builder().setCheckerUuid(checkerUuid).setRepository(repository);
+ CheckerCreation.builder()
+ .setCheckerUuid(checkerUuid)
+ .setName(name)
+ .setRepository(repository);
CheckerUpdate.Builder checkerUpdateBuilder = CheckerUpdate.builder();
- String name = CheckerName.clean(input.name);
- if (!name.isEmpty()) {
- checkerUpdateBuilder.setName(name);
- }
+
if (input.description != null && !input.description.trim().isEmpty()) {
checkerUpdateBuilder.setDescription(input.description.trim());
}
diff --git a/java/com/google/gerrit/plugins/checks/db/CheckerConfig.java b/java/com/google/gerrit/plugins/checks/db/CheckerConfig.java
index 61bd436..57fda1e 100644
--- a/java/com/google/gerrit/plugins/checks/db/CheckerConfig.java
+++ b/java/com/google/gerrit/plugins/checks/db/CheckerConfig.java
@@ -311,6 +311,11 @@
}
private void ensureThatMandatoryPropertiesAreSet() throws ConfigInvalidException {
+ if (getNewName().equals(Optional.of(""))) {
+ throw new ConfigInvalidException(
+ String.format("Name of the checker %s must be defined", describeForError()));
+ }
+
if (getNewRepository().equals(Optional.of(""))) {
throw new ConfigInvalidException(
String.format("Repository of the checker %s must be defined", describeForError()));
@@ -321,6 +326,16 @@
checkState(isLoaded, "Checker %s not loaded yet", describeForError());
}
+ private Optional<String> getNewName() {
+ if (checkerUpdate.isPresent()) {
+ return checkerUpdate.get().getName().map(Strings::nullToEmpty).map(String::trim);
+ }
+ if (checkerCreation.isPresent()) {
+ return Optional.of(Strings.nullToEmpty(checkerCreation.get().getName()).trim());
+ }
+ return Optional.empty();
+ }
+
private Optional<String> getNewRepository() {
if (checkerUpdate.isPresent()) {
return checkerUpdate
diff --git a/java/com/google/gerrit/plugins/checks/db/CheckerConfigEntry.java b/java/com/google/gerrit/plugins/checks/db/CheckerConfigEntry.java
index 0b2c5ab..eabcfb2 100644
--- a/java/com/google/gerrit/plugins/checks/db/CheckerConfigEntry.java
+++ b/java/com/google/gerrit/plugins/checks/db/CheckerConfigEntry.java
@@ -91,16 +91,22 @@
*/
NAME("name") {
@Override
- void readFromConfig(CheckerUuid checkerUuid, Checker.Builder checker, Config config) {
+ void readFromConfig(CheckerUuid checkerUuid, Checker.Builder checker, Config config)
+ throws ConfigInvalidException {
String name = config.getString(SECTION_NAME, null, super.keyName);
- if (name != null) {
- checker.setName(name);
+ if (name == null) {
+ throw new ConfigInvalidException(
+ String.format(
+ "%s.%s is not set in config file for checker %s",
+ SECTION_NAME, super.keyName, checkerUuid));
}
+ checker.setName(name);
}
@Override
void initNewConfig(Config config, CheckerCreation checkerCreation) {
- // Do nothing. Name key will be set by updateConfigValue.
+ String checkerName = checkerCreation.getName();
+ config.setString(SECTION_NAME, null, super.keyName, checkerName);
}
@Override
diff --git a/java/com/google/gerrit/plugins/checks/testing/CheckerConfigSubject.java b/java/com/google/gerrit/plugins/checks/testing/CheckerConfigSubject.java
index 8e5bb2c..2b4e356 100644
--- a/java/com/google/gerrit/plugins/checks/testing/CheckerConfigSubject.java
+++ b/java/com/google/gerrit/plugins/checks/testing/CheckerConfigSubject.java
@@ -52,10 +52,8 @@
check("uuid()").that(checker().getUuid()).isEqualTo(expectedUuid);
}
- public OptionalSubject<StringSubject, String> hasNameThat() {
- return check("name()")
- .about(optionals())
- .that(checker().getName(), StandardSubjectBuilder::that);
+ public void hasName(String expectedName) {
+ check("name()").that(checker().getName()).isEqualTo(expectedName);
}
public OptionalSubject<StringSubject, String> hasDescriptionThat() {
diff --git a/javatests/com/google/gerrit/plugins/checks/CheckerDefinitionTest.java b/javatests/com/google/gerrit/plugins/checks/CheckerDefinitionTest.java
index 9d090ce..f0e967a 100644
--- a/javatests/com/google/gerrit/plugins/checks/CheckerDefinitionTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/CheckerDefinitionTest.java
@@ -50,6 +50,7 @@
private Checker.Builder newChecker() {
return Checker.builder()
+ .setName("My Checker")
.setRepository(Project.nameKey("test-repo"))
.setStatus(CheckerStatus.ENABLED)
.setUuid(CheckerUuid.parse("schema:any-id"))
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java
index 7514878..fa36005 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java
@@ -78,10 +78,11 @@
Timestamp expectedCreationTimestamp = TestTimeUtil.getCurrentTimestamp();
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = repositoryName.get();
CheckerInfo info = checkersApi.create(input).get();
assertThat(info.uuid).isEqualTo("test:my-checker");
- assertThat(info.name).isNull();
+ assertThat(info.name).isEqualTo("My Checker");
assertThat(info.description).isNull();
assertThat(info.url).isNull();
assertThat(info.repository).isEqualTo(input.repository);
@@ -104,6 +105,7 @@
public void createCheckerWithDescription() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.description = "some description";
input.repository = allProjects.get();
CheckerInfo info = checkersApi.create(input).get();
@@ -118,6 +120,7 @@
public void createCheckerWithUrl() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.url = "http://example.com/my-checker";
input.repository = allProjects.get();
CheckerInfo info = checkersApi.create(input).get();
@@ -129,27 +132,24 @@
}
@Test
- public void createCheckerWithName() throws Exception {
+ public void createCheckerWithoutNameFails() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
- input.name = "my-checker";
input.repository = allProjects.get();
- CheckerInfo info = checkersApi.create(input).get();
- assertThat(info.name).isEqualTo("my-checker");
- PerCheckerOperations perCheckerOps = checkerOperations.checker(info.uuid);
- assertCommit(
- perCheckerOps.commit(), "Create checker", info.created, perCheckerOps.get().getRefState());
+ BadRequestException thrown =
+ assertThrows(BadRequestException.class, () -> checkersApi.create(input));
+ assertThat(thrown).hasMessageThat().contains("name is required");
}
@Test
public void createCheckerNameIsTrimmed() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
- input.name = " my-checker ";
+ input.name = " My Checker ";
input.repository = allProjects.get();
CheckerInfo info = checkersApi.create(input).get();
- assertThat(info.name).isEqualTo("my-checker");
+ assertThat(info.name).isEqualTo("My Checker");
PerCheckerOperations perCheckerOps = checkerOperations.checker(info.uuid);
assertCommit(
@@ -160,6 +160,7 @@
public void createCheckerDescriptionIsTrimmed() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.description = " some description ";
input.repository = allProjects.get();
CheckerInfo info = checkersApi.create(input).get();
@@ -174,6 +175,7 @@
public void createCheckerUrlIsTrimmed() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.url = " http://example.com/my-checker ";
input.repository = allProjects.get();
CheckerInfo info = checkersApi.create(input).get();
@@ -188,6 +190,7 @@
public void createCheckerRepositoryIsTrimmed() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = " " + allProjects.get() + " ";
CheckerInfo info = checkersApi.create(input).get();
assertThat(info.repository).isEqualTo(allProjects.get());
@@ -212,7 +215,7 @@
public void createCheckersWithSameName() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
- input.name = "my-checker";
+ input.name = "My Checker";
input.repository = allProjects.get();
CheckerInfo info1 = checkersApi.create(input).get();
assertThat(info1.name).isEqualTo(input.name);
@@ -228,6 +231,7 @@
public void createCheckerWithExistingUuidFails() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = allProjects.get();
checkersApi.create(input).get();
@@ -239,6 +243,7 @@
@Test
public void createCheckerWithoutUuidFails() throws Exception {
CheckerInput input = new CheckerInput();
+ input.name = "My Checker";
input.repository = allProjects.get();
BadRequestException thrown =
@@ -250,6 +255,7 @@
public void createCheckerWithEmptyUuidFails() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "";
+ input.name = "My Checker";
input.repository = allProjects.get();
BadRequestException thrown =
@@ -261,6 +267,7 @@
public void createCheckerWithEmptyUuidAfterTrimFails() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = " ";
+ input.name = "My Checker";
input.repository = allProjects.get();
BadRequestException thrown =
@@ -272,6 +279,7 @@
public void createCheckerWithInvalidUuidFails() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = CheckerTestData.INVALID_UUID;
+ input.name = "My Checker";
input.repository = allProjects.get();
BadRequestException thrown =
@@ -283,6 +291,7 @@
public void createCheckerWithoutRepositoryFails() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
BadRequestException thrown =
assertThrows(BadRequestException.class, () -> checkersApi.create(input));
@@ -293,6 +302,7 @@
public void createCheckerWithEmptyRepositoryFails() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = "";
BadRequestException thrown =
@@ -304,6 +314,7 @@
public void createCheckerWithEmptyRepositoryAfterTrimFails() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = " ";
BadRequestException thrown =
@@ -315,6 +326,7 @@
public void createCheckerWithNonExistingRepositoryFails() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = "non-existing";
UnprocessableEntityException thrown =
@@ -326,6 +338,7 @@
public void createDisabledChecker() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = allProjects.get();
input.status = CheckerStatus.DISABLED;
@@ -337,6 +350,7 @@
public void createCheckerWithBlockingConditions() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = allProjects.get();
input.blocking = ImmutableSet.of(BlockingCondition.STATE_NOT_PASSING);
@@ -348,6 +362,7 @@
public void createCheckerWithQuery() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = allProjects.get();
input.query = "f:foo";
@@ -359,6 +374,7 @@
public void createCheckerWithEmptyQuery() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = allProjects.get();
input.query = "";
@@ -370,6 +386,7 @@
public void createCheckerWithEmptyQueryAfterTrim() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = allProjects.get();
input.query = " ";
@@ -381,6 +398,7 @@
public void createCheckerWithUnsupportedOperatorInQueryFails() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = allProjects.get();
input.query = CheckerTestData.QUERY_WITH_UNSUPPORTED_OPERATOR;
@@ -395,6 +413,7 @@
public void createCheckerWithInvalidQueryFails() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = allProjects.get();
input.query = CheckerTestData.INVALID_QUERY;
@@ -407,6 +426,7 @@
public void createCheckerWithTooLongQueryFails() throws Exception {
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
+ input.name = "My Checker";
input.repository = allProjects.get();
input.query = CheckerTestData.longQueryWithSupportedOperators(MAX_INDEX_TERMS * 2);
assertThat(CheckerQuery.clean(input.query)).isEqualTo(input.query);
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java
index 84463c6..a19dee2 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java
@@ -255,6 +255,16 @@
}
@Test
+ public void getCheckerWithMissingNameFails() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().create();
+ checkerOperations.checker(checkerUuid).forInvalidation().unsetName().invalidate();
+
+ RestApiException thrown =
+ assertThrows(RestApiException.class, () -> getCheckerInfo(checkerUuid));
+ assertThat(thrown).hasMessageThat().contains("Cannot retrieve checker " + checkerUuid);
+ }
+
+ @Test
public void getCheckerWithMissingRepositoryFails() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().create();
checkerOperations.checker(checkerUuid).forInvalidation().unsetRepository().invalidate();
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
index 590f82f..b41d9e8 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
@@ -89,6 +89,7 @@
CheckerUuid invalidCheckerUuid5 = checkerOperations.newChecker().create();
CheckerUuid invalidCheckerUuid6 = checkerOperations.newChecker().create();
CheckerUuid invalidCheckerUuid7 = checkerOperations.newChecker().create();
+ CheckerUuid invalidCheckerUuid8 = checkerOperations.newChecker().create();
checkerOperations
.checker(invalidCheckerUuid1)
.forInvalidation()
@@ -102,8 +103,9 @@
.invalidate();
checkerOperations.checker(invalidCheckerUuid4).forInvalidation().invalidStatus().invalidate();
checkerOperations.checker(invalidCheckerUuid5).forInvalidation().unsetUuid().invalidate();
- checkerOperations.checker(invalidCheckerUuid6).forInvalidation().unsetRepository().invalidate();
- checkerOperations.checker(invalidCheckerUuid7).forInvalidation().unsetStatus().invalidate();
+ checkerOperations.checker(invalidCheckerUuid6).forInvalidation().unsetName().invalidate();
+ checkerOperations.checker(invalidCheckerUuid7).forInvalidation().unsetRepository().invalidate();
+ checkerOperations.checker(invalidCheckerUuid8).forInvalidation().unsetStatus().invalidate();
List<CheckerInfo> allCheckers = checkersApi.all();
assertThat(allCheckers).containsExactly(checkerOperations.checker(checkerUuid).asInfo());
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 9650784..816ff5c 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
@@ -77,7 +77,10 @@
String checkerName1 = "Checker One";
CheckerUuid checkerUuid1 =
checkerOperations.newChecker().name(checkerName1).repository(project).create();
- CheckerUuid checkerUuid2 = checkerOperations.newChecker().repository(project).create();
+
+ String checkerName2 = "Checker Two";
+ CheckerUuid checkerUuid2 =
+ checkerOperations.newChecker().name(checkerName2).repository(project).create();
CheckKey checkKey1 = CheckKey.create(project, patchSetId, checkerUuid1);
checkOperations.newCheck(checkKey1).state(CheckState.RUNNING).upsert();
@@ -93,6 +96,7 @@
CheckInfo expectedCheckInfo2 = checkOperations.check(checkKey2).asInfo();
expectedCheckInfo2.repository = project.get();
+ expectedCheckInfo2.checkerName = checkerName2;
expectedCheckInfo2.blocking = ImmutableSet.of();
expectedCheckInfo2.checkerStatus = CheckerStatus.ENABLED;
@@ -105,7 +109,10 @@
String checkerName1 = "Checker One";
CheckerUuid checkerUuid1 =
checkerOperations.newChecker().name(checkerName1).repository(project).create();
- CheckerUuid checkerUuid2 = checkerOperations.newChecker().repository(project).create();
+
+ String checkerName2 = "Checker Two";
+ CheckerUuid checkerUuid2 =
+ checkerOperations.newChecker().name(checkerName2).repository(project).create();
CheckKey checkKey1 = CheckKey.create(project, patchSetId, checkerUuid1);
checkOperations.newCheck(checkKey1).state(CheckState.RUNNING).upsert();
@@ -121,6 +128,7 @@
CheckInfo expectedCheckInfo2 = checkOperations.check(checkKey2).asInfo();
expectedCheckInfo2.repository = project.get();
+ expectedCheckInfo2.checkerName = checkerName2;
expectedCheckInfo2.blocking = ImmutableSet.of();
expectedCheckInfo2.checkerStatus = CheckerStatus.ENABLED;
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImplTest.java b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImplTest.java
index f78fee2..8a2d2d4 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImplTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImplTest.java
@@ -70,7 +70,7 @@
CheckerInfo foundChecker = getCheckerFromServer(checkerUuid);
assertThat(foundChecker.uuid).isEqualTo(checkerUuid.get());
- assertThat(foundChecker.name).isNull();
+ assertThat(foundChecker.name).isNotNull();
assertThat(foundChecker.repository).isEqualTo(allProjects.get());
assertThat(foundChecker.status).isEqualTo(CheckerStatus.ENABLED);
assertThat(foundChecker.query).isEqualTo("status:open");
@@ -257,9 +257,9 @@
CheckerUuid checkerUuid =
checkerOperations.newChecker().name("ABC-789-this-name-must-be-unique").create();
- Optional<String> checkerName = checkerOperations.checker(checkerUuid).get().getName();
+ String checkerName = checkerOperations.checker(checkerUuid).get().getName();
- assertThat(checkerName).hasValue("ABC-789-this-name-must-be-unique");
+ assertThat(checkerName).isEqualTo("ABC-789-this-name-must-be-unique");
}
@Test
@@ -416,8 +416,8 @@
checkerOperations.checker(checkerUuid).forUpdate().name("updated name").update();
- Optional<String> currentName = checkerOperations.checker(checkerUuid).get().getName();
- assertThat(currentName).hasValue("updated name");
+ String currentName = checkerOperations.checker(checkerUuid).get().getName();
+ assertThat(currentName).isEqualTo("updated name");
}
@Test
@@ -591,6 +591,17 @@
}
@Test
+ public void nameCanBeUnset() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().create();
+
+ checkerOperations.checker(checkerUuid).forInvalidation().unsetName().invalidate();
+
+ ConfigInvalidException thrown =
+ assertThrows(ConfigInvalidException.class, () -> checkers.getChecker(checkerUuid));
+ assertThat(thrown).hasMessageThat().contains("checker.name is not set in config file");
+ }
+
+ @Test
public void repositoryCanBeUnset() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().create();
@@ -670,7 +681,7 @@
Checker checker = checkerOperations.checker(checkerUuid).get();
CheckerInfo checkerInfo = checkerOperations.checker(checkerUuid).asInfo();
assertThat(checkerInfo.uuid).isEqualTo(checker.getUuid().get());
- assertThat(checkerInfo.name).isEqualTo(checker.getName().get());
+ assertThat(checkerInfo.name).isEqualTo(checker.getName());
assertThat(checkerInfo.description).isEqualTo(checker.getDescription().get());
assertThat(checkerInfo.url).isEqualTo(checker.getUrl().get());
assertThat(checkerInfo.created).isEqualTo(checker.getCreated());
@@ -734,6 +745,7 @@
private CheckerInput createArbitraryCheckerInput() {
CheckerInput checkerInput = new CheckerInput();
checkerInput.uuid = "test:my-checker";
+ checkerInput.name = "My Checker";
checkerInput.repository = allProjects.get();
return checkerInput;
}
diff --git a/javatests/com/google/gerrit/plugins/checks/db/CheckerConfigTest.java b/javatests/com/google/gerrit/plugins/checks/db/CheckerConfigTest.java
index 15b85fe..35c741f 100644
--- a/javatests/com/google/gerrit/plugins/checks/db/CheckerConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/db/CheckerConfigTest.java
@@ -52,6 +52,7 @@
private TestRepository<?> testRepository;
private final CheckerUuid checkerUuid = CheckerUuid.parse("test:my-checker");
+ private final String checkerName = "My Checker";
private final Project.NameKey checkerRepository = Project.nameKey("my-repo");
private final TimeZone timeZone = TimeZone.getTimeZone("America/Los_Angeles");
@@ -82,23 +83,51 @@
}
@Test
- public void setNameDuringCreation() throws Exception {
- String anotherName = "another-name";
+ public void specifiedCheckerNameIsRespectedForNewChecker() throws Exception {
+ CheckerCreation checkerCreation =
+ getPrefilledCheckerCreationBuilder().setName(checkerName).build();
+ createChecker(checkerCreation);
- CheckerCreation checkerCreation = getPrefilledCheckerCreationBuilder().build();
+ CheckerConfig checkerConfig = loadChecker(checkerUuid);
+ assertThat(checkerConfig).hasName(checkerName);
+ assertThat(checkerConfig).configStringList("name").containsExactly("My Checker");
+ }
+
+ @Test
+ public void nameOfCheckerUpdateOverridesCheckerCreation() throws Exception {
+ String anotherName = "Another Name";
+
+ CheckerCreation checkerCreation =
+ getPrefilledCheckerCreationBuilder().setName(checkerName).build();
CheckerUpdate checkerUpdate = CheckerUpdate.builder().setName(anotherName).build();
createChecker(checkerCreation, checkerUpdate);
CheckerConfig checkerConfig = loadChecker(checkerCreation.getCheckerUuid());
- assertThat(checkerConfig).hasNameThat().value().isEqualTo(anotherName);
+ assertThat(checkerConfig).hasName(anotherName);
assertThat(checkerConfig).configStringList("name").containsExactly(anotherName);
}
@Test
+ public void nameOfNewCheckerMustNotBeEmpty() throws Exception {
+ CheckerCreation checkerCreation = getPrefilledCheckerCreationBuilder().setName("").build();
+ CheckerConfig checkerConfig =
+ CheckerConfig.createForNewChecker(projectName, repository, checkerCreation);
+
+ try (MetaDataUpdate metaDataUpdate = createMetaDataUpdate()) {
+ Throwable thrown = assertThrows(Throwable.class, () -> checkerConfig.commit(metaDataUpdate));
+ assertThat(thrown).hasCauseThat().isInstanceOf(ConfigInvalidException.class);
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains(String.format("Name of the checker %s must be defined", checkerUuid));
+ }
+ }
+
+ @Test
public void descriptionDefaultsToOptionalEmpty() throws Exception {
CheckerCreation checkerCreation =
CheckerCreation.builder()
.setCheckerUuid(checkerUuid)
+ .setName(checkerName)
.setRepository(checkerRepository)
.build();
createChecker(checkerCreation);
@@ -137,6 +166,7 @@
CheckerCreation checkerCreation =
CheckerCreation.builder()
.setCheckerUuid(checkerUuid)
+ .setName(checkerName)
.setRepository(checkerRepository)
.build();
createChecker(checkerCreation);
@@ -237,6 +267,23 @@
}
@Test
+ public void nameInConfigMayNotBeUndefined() throws Exception {
+ populateCheckerConfig(
+ checkerUuid,
+ "[checker]\n uuid = "
+ + checkerUuid
+ + "\n repository = "
+ + checkerRepository
+ + "\n status = ENABLED");
+
+ ConfigInvalidException thrown =
+ assertThrows(ConfigInvalidException.class, () -> loadChecker(checkerUuid));
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains("checker.name is not set in config file for checker " + checkerUuid);
+ }
+
+ @Test
public void correctCommitMessageForCheckerUpdate() throws Exception {
createArbitraryChecker(checkerUuid);
assertThatCommitMessage(checkerUuid).isEqualTo("Create checker");
@@ -247,31 +294,38 @@
}
@Test
- public void nameCanBeUpdatedAndRemoved() throws Exception {
+ public void nameCanBeUpdated() throws Exception {
CheckerCreation checkerCreation =
CheckerCreation.builder()
.setCheckerUuid(checkerUuid)
+ .setName(checkerName)
.setRepository(checkerRepository)
.build();
createChecker(checkerCreation);
CheckerConfig checkerConfig = loadChecker(checkerUuid);
- assertThat(checkerConfig).hasNameThat().isAbsent();
+ assertThat(checkerConfig).hasName(checkerName);
- String newName = "new-name";
+ String newName = "New Name";
CheckerUpdate checkerUpdate = CheckerUpdate.builder().setName(newName).build();
updateChecker(checkerUuid, checkerUpdate);
checkerConfig = loadChecker(checkerUuid);
- assertThat(checkerConfig).hasNameThat().value().isEqualTo(newName);
+ assertThat(checkerConfig).hasName(newName);
assertThat(checkerConfig).configStringList("name").containsExactly(newName);
+ }
- checkerUpdate = CheckerUpdate.builder().setName("").build();
- updateChecker(checkerUuid, checkerUpdate);
+ @Test
+ public void nameCannotBeRemoved() throws Exception {
+ createArbitraryChecker(checkerUuid);
- checkerConfig = loadChecker(checkerUuid);
- assertThat(checkerConfig).hasNameThat().isAbsent();
- assertThat(checkerConfig).configStringList("name").isEmpty();
+ CheckerUpdate checkerUpdate = CheckerUpdate.builder().setName("").build();
+
+ IOException thrown =
+ assertThrows(IOException.class, () -> updateChecker(checkerUuid, checkerUpdate));
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains(String.format("Name of the checker %s must be defined", checkerUuid));
}
@Test
@@ -325,6 +379,7 @@
CheckerCreation checkerCreation =
CheckerCreation.builder()
.setCheckerUuid(checkerUuid)
+ .setName(checkerName)
.setRepository(checkerRepository)
.build();
createChecker(checkerCreation);
@@ -514,7 +569,10 @@
}
private CheckerCreation.Builder getPrefilledCheckerCreationBuilder() {
- return CheckerCreation.builder().setCheckerUuid(checkerUuid).setRepository(checkerRepository);
+ return CheckerCreation.builder()
+ .setCheckerUuid(checkerUuid)
+ .setName(checkerName)
+ .setRepository(checkerRepository);
}
private CheckerConfig createChecker(CheckerCreation checkerCreation) throws Exception {
