Validate reviewer filters
REST:
* Validate reviewer filters before adding them to reviewers.config.
git:
* Validate reviewer filters before update of reviewers.config.
Introduce a query validation API.
Feature: Issue 12650
Change-Id: I1404598869ccf15714063a088e1410cc8ba5274a
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/ForProjectValidator.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/ForProjectValidator.java
new file mode 100644
index 0000000..3492e03
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/ForProjectValidator.java
@@ -0,0 +1,112 @@
+// Copyright (C) 2020 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.googlesource.gerrit.plugins.reviewers;
+
+import autovaluegson.factory.shaded.com.google.common.collect.Lists;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.PatchSet.Id;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.events.CommitReceivedEvent;
+import com.google.gerrit.server.git.CodeReviewCommit;
+import com.google.gerrit.server.git.ValidationError;
+import com.google.gerrit.server.git.validators.CommitValidationException;
+import com.google.gerrit.server.git.validators.CommitValidationListener;
+import com.google.gerrit.server.git.validators.CommitValidationMessage;
+import com.google.gerrit.server.git.validators.MergeValidationException;
+import com.google.gerrit.server.git.validators.MergeValidationListener;
+import com.google.gerrit.server.project.ProjectState;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import com.googlesource.gerrit.plugins.reviewers.config.ForProject;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Repository;
+
+/** Validates changes to reviewers.config through push or merge. */
+@Singleton
+public class ForProjectValidator implements MergeValidationListener, CommitValidationListener {
+ @VisibleForTesting public static String MALFORMED_CONFIG = "Malformed reviewers.config";
+
+ private final ForProject.Factory forProjectFactory;
+
+ @Inject
+ public ForProjectValidator(ForProject.Factory forProjectFactory) {
+ this.forProjectFactory = forProjectFactory;
+ }
+
+ @Override
+ public void onPreMerge(
+ Repository repo,
+ CodeReviewCommit.CodeReviewRevWalk crrw,
+ CodeReviewCommit commit,
+ ProjectState destProject,
+ BranchNameKey destBranch,
+ Id patchSetId,
+ IdentifiedUser caller)
+ throws MergeValidationException {
+ if (!RefNames.REFS_CONFIG.equals(destBranch.branch())) {
+ return;
+ }
+
+ ForProject cfg = forProjectFactory.create();
+ try {
+ cfg.load(destProject.getNameKey(), repo, commit);
+ } catch (IOException ioe) {
+ throw new MergeValidationException("Unable to read config.", ioe);
+ } catch (ConfigInvalidException cie) {
+ throw new MergeValidationException(MALFORMED_CONFIG, cie);
+ }
+
+ if (!cfg.getValidationErrors().isEmpty()) {
+ throw new MergeValidationException(formatValidationErrors(cfg.getValidationErrors()));
+ }
+ }
+
+ @Override
+ public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
+ throws CommitValidationException {
+ if (RefNames.REFS_CONFIG.equals(receiveEvent.getBranchNameKey().branch())) {
+ ForProject cfg = forProjectFactory.create();
+ try {
+ cfg.load(receiveEvent.getProjectNameKey(), receiveEvent.revWalk, receiveEvent.commit);
+ } catch (IOException ioe) {
+ throw new CommitValidationException("Unable to read config.", ioe);
+ } catch (ConfigInvalidException cie) {
+ throw new CommitValidationException(MALFORMED_CONFIG);
+ }
+ if (!cfg.getValidationErrors().isEmpty()) {
+ ArrayList<CommitValidationMessage> messages = Lists.newArrayList();
+ messages.add(new CommitValidationMessage(MALFORMED_CONFIG, true));
+ cfg.getValidationErrors()
+ .forEach(ve -> messages.add(new CommitValidationMessage(" " + ve.getMessage(), true)));
+ throw new CommitValidationException(MALFORMED_CONFIG, messages);
+ }
+ }
+ return Collections.emptyList();
+ }
+
+ private static String formatValidationErrors(List<ValidationError> errors) {
+ StringBuilder errorMessage = new StringBuilder();
+ errorMessage.append("[\"");
+ errors.forEach(ve -> errorMessage.append(ve.getMessage() + "\", \""));
+ errorMessage.append("]");
+ return "Malformed reviewers.config filters: " + errorMessage.toString();
+ }
+}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/Module.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/Module.java
index c069a0c..20b09be 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/Module.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/Module.java
@@ -28,8 +28,11 @@
import com.google.gerrit.extensions.webui.JavaScriptPlugin;
import com.google.gerrit.extensions.webui.WebUiPlugin;
import com.google.gerrit.server.change.ReviewerSuggestion;
+import com.google.gerrit.server.git.validators.CommitValidationListener;
+import com.google.gerrit.server.git.validators.MergeValidationListener;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
+import com.googlesource.gerrit.plugins.reviewers.config.ConfigModule;
import com.googlesource.gerrit.plugins.reviewers.config.GlobalConfig;
public class Module extends FactoryModule {
@@ -52,7 +55,8 @@
bind(CapabilityDefinition.class)
.annotatedWith(Exports.named(MODIFY_REVIEWERS_CONFIG))
.to(ModifyReviewersConfigCapability.class);
-
+ DynamicSet.bind(binder(), MergeValidationListener.class).to(ForProjectValidator.class);
+ DynamicSet.bind(binder(), CommitValidationListener.class).to(ForProjectValidator.class);
if (suggestOnly) {
install(
new AbstractModule() {
@@ -89,6 +93,7 @@
.toInstance(new JavaScriptPlugin("rv-reviewers.js"));
}
});
+ install(new ConfigModule());
}
protected void bindWorkQueue() {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/PutReviewers.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/PutReviewers.java
index e9945e4..58ae003 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/PutReviewers.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/PutReviewers.java
@@ -16,12 +16,15 @@
import static com.googlesource.gerrit.plugins.reviewers.ModifyReviewersConfigCapability.MODIFY_REVIEWERS_CONFIG;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.extensions.api.access.PluginPermission;
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.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
@@ -30,6 +33,7 @@
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.AccountResolver.UnresolvableAccountException;
+import com.google.gerrit.server.git.ValidationError;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.group.GroupResolver;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -45,6 +49,7 @@
import com.googlesource.gerrit.plugins.reviewers.config.ForProject;
import java.io.IOException;
import java.util.List;
+import java.util.Set;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
@@ -67,6 +72,7 @@
private final String pluginName;
private final FiltersFactory filters;
+ private final ForProject.Factory forProjectFactory;
private final Provider<MetaDataUpdate.User> metaDataUpdateFactory;
private final ProjectCache projectCache;
private final AccountResolver accountResolver;
@@ -77,6 +83,7 @@
PutReviewers(
@PluginName String pluginName,
FiltersFactory filters,
+ ForProject.Factory forProjectFactory,
Provider<MetaDataUpdate.User> metaDataUpdateFactory,
ProjectCache projectCache,
AccountResolver accountResolver,
@@ -84,6 +91,7 @@
PermissionBackend permissionBackend) {
this.pluginName = pluginName;
this.filters = filters;
+ this.forProjectFactory = forProjectFactory;
this.metaDataUpdateFactory = metaDataUpdateFactory;
this.projectCache = projectCache;
this.accountResolver = accountResolver;
@@ -95,8 +103,7 @@
public Response<List<ReviewerFilter>> apply(ProjectResource rsrc, Input input)
throws RestApiException, PermissionBackendException {
Project.NameKey projectName = rsrc.getNameKey();
- ForProject forProject = new ForProject();
-
+ ForProject forProject = forProjectFactory.create();
PermissionBackend.WithUser userPermission = permissionBackend.user(rsrc.getUser());
if (!userPermission.project(rsrc.getNameKey()).testOrFalse(ProjectPermission.WRITE_CONFIG)
&& !userPermission.testOrFalse(new PluginPermission(pluginName, MODIFY_REVIEWERS_CONFIG))) {
@@ -113,6 +120,7 @@
try {
StringBuilder message = new StringBuilder(pluginName).append(" plugin: ");
forProject.load(md);
+ Set<ValidationError> previousErrors = ImmutableSet.copyOf(forProject.getValidationErrors());
if (input.action == Action.ADD) {
message
.append("Add ")
@@ -131,6 +139,12 @@
.append(input.filter);
forProject.removeReviewer(input.filter, input.reviewer, input.type);
}
+ Set<ValidationError> errors =
+ Sets.difference(ImmutableSet.copyOf(forProject.getValidationErrors()), previousErrors);
+ if (!errors.isEmpty()) {
+ throw new BadRequestException(
+ String.format("Unsupported query: %s", errors.iterator().next().getMessage()));
+ }
message.append("\n");
md.setMessage(message.toString());
try {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewerFilter.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewerFilter.java
index b991760..810c2bd 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewerFilter.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewerFilter.java
@@ -14,6 +14,7 @@
package com.googlesource.gerrit.plugins.reviewers;
+import com.google.common.annotations.VisibleForTesting;
import java.util.Objects;
import java.util.Set;
@@ -30,6 +31,7 @@
protected String filter;
protected Set<String> reviewers;
protected Set<String> ccs;
+ protected String filterError;
String getFilter() {
return filter;
@@ -43,6 +45,11 @@
return ccs;
}
+ @VisibleForTesting
+ public String getFilterError() {
+ return filterError;
+ }
+
@Override
public boolean equals(Object o) {
if (o instanceof ReviewerFilter) {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ConfigModule.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ConfigModule.java
new file mode 100644
index 0000000..fbf7d64
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ConfigModule.java
@@ -0,0 +1,25 @@
+// Copyright (C) 2022 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.googlesource.gerrit.plugins.reviewers.config;
+
+import com.google.gerrit.extensions.config.FactoryModule;
+
+public class ConfigModule extends FactoryModule {
+
+ @Override
+ protected void configure() {
+ factory(ForProject.Factory.class);
+ factory(ReviewerFilterCollection.Factory.class);
+ }
+}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/FiltersFactory.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/FiltersFactory.java
index 04c3d62..9bdf676 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/FiltersFactory.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/FiltersFactory.java
@@ -30,11 +30,16 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final PluginConfigFactory configFactory;
+ private final ReviewerFilterCollection.Factory filterCollectionFactory;
private final String pluginName;
@Inject
- public FiltersFactory(PluginConfigFactory configFactory, @PluginName String pluginName) {
+ public FiltersFactory(
+ PluginConfigFactory configFactory,
+ ReviewerFilterCollection.Factory filterCollectionFactory,
+ @PluginName String pluginName) {
this.configFactory = configFactory;
+ this.filterCollectionFactory = filterCollectionFactory;
this.pluginName = pluginName;
}
@@ -46,6 +51,6 @@
logger.atSevere().log("Unable to get config for project %s", projectName.get());
cfg = new Config();
}
- return new ReviewerFilterCollection(cfg).getAll();
+ return filterCollectionFactory.create(cfg).getAll();
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ForProject.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ForProject.java
index c53fdba..125fc9d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ForProject.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ForProject.java
@@ -17,21 +17,37 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.server.git.ValidationError;
import com.google.gerrit.server.git.meta.VersionedMetaData;
+import com.google.inject.Inject;
import com.googlesource.gerrit.plugins.reviewers.ReviewerType;
import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Config;
-public class ForProject extends VersionedMetaData {
+public class ForProject extends VersionedMetaData implements ValidationError.Sink {
@VisibleForTesting public static final String FILENAME = "reviewers.config";
@VisibleForTesting public static final String SECTION_FILTER = "filter";
@VisibleForTesting public static final String KEY_CC = "cc";
@VisibleForTesting public static final String KEY_REVIEWER = "reviewer";
+ public interface Factory {
+ public ForProject create();
+ }
+
+ private final ReviewerFilterCollection.Factory filterCollectionFactory;
private Config cfg;
private ReviewerFilterCollection filters;
+ private List<ValidationError> validationErrors;
+
+ @Inject
+ public ForProject(ReviewerFilterCollection.Factory filterCollectionFactory) {
+ this.filterCollectionFactory = filterCollectionFactory;
+ }
public void addReviewer(String filter, String reviewer, ReviewerType type) {
switch (type) {
@@ -53,6 +69,26 @@
}
}
+ /**
+ * Get the validation errors, if any were discovered.
+ *
+ * @return list of errors; empty list if there are no errors.
+ */
+ public List<ValidationError> getValidationErrors() {
+ if (validationErrors != null) {
+ return Collections.unmodifiableList(validationErrors);
+ }
+ return Collections.emptyList();
+ }
+
+ @Override
+ public void error(ValidationError error) {
+ if (validationErrors == null) {
+ validationErrors = new ArrayList<>(4);
+ }
+ validationErrors.add(error);
+ }
+
@Override
protected String getRefName() {
return RefNames.REFS_CONFIG;
@@ -61,7 +97,7 @@
@Override
protected void onLoad() throws IOException, ConfigInvalidException {
this.cfg = readConfig(FILENAME);
- this.filters = new ReviewerFilterCollection(cfg);
+ this.filters = filterCollectionFactory.create(cfg, this);
}
@Override
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewerFilterCollection.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewerFilterCollection.java
index bd44ccd..91b49e8 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewerFilterCollection.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewerFilterCollection.java
@@ -21,29 +21,80 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
+import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.server.git.ValidationError;
+import com.google.inject.assistedinject.Assisted;
+import com.google.inject.assistedinject.AssistedInject;
import com.googlesource.gerrit.plugins.reviewers.ReviewerFilter;
import java.util.List;
+import java.util.Optional;
import org.eclipse.jgit.lib.Config;
/** Representation of the collection of {@link ReviewerFilter}s in a {@link Config}. */
class ReviewerFilterCollection {
+ private final ReviewersQueryValidator queryValidator;
private final Config cfg;
+ private final Optional<ValidationError.Sink> validationErrorSink;
- ReviewerFilterCollection(Config cfg) {
+ interface Factory {
+ ReviewerFilterCollection create(Config cfg);
+
+ ReviewerFilterCollection create(Config cfg, ValidationError.Sink validationErrorSink);
+ }
+
+ @AssistedInject
+ ReviewerFilterCollection(ReviewersQueryValidator queryValidator, @Assisted Config cfg) {
+ this(queryValidator, cfg, null);
+ }
+
+ @AssistedInject
+ ReviewerFilterCollection(
+ ReviewersQueryValidator queryValidator,
+ @Assisted Config cfg,
+ @Assisted ValidationError.Sink validationErrorSink) {
+ this.queryValidator = queryValidator;
this.cfg = cfg;
+ this.validationErrorSink = Optional.ofNullable(validationErrorSink);
+ check();
}
List<ReviewerFilter> getAll() {
ImmutableList.Builder<ReviewerFilter> b = ImmutableList.builder();
for (String f : cfg.getSubsections(SECTION_FILTER)) {
- b.add(new ReviewerFilterSection(f));
+ b.add(newReviewerFilter(f));
}
return b.build();
}
+ /* Validates all the filter in this collection and adds the ValidationErrors
+ * to the ValidationError.Sink. */
+ private void check() {
+ for (String f : cfg.getSubsections(SECTION_FILTER)) {
+ checkForErrors(f);
+ }
+ }
+
ReviewerFilterSection get(String filter) {
- return new ReviewerFilterSection(filter);
+ return newReviewerFilter(filter);
+ }
+
+ private ReviewerFilterSection newReviewerFilter(String filter) {
+ ReviewerFilterSection section = new ReviewerFilterSection(filter);
+ checkForErrors(filter).ifPresent(err -> section.filterError(err));
+ return section;
+ }
+
+ /* Checks if filterQuery is a valid query. If not it adds the corresponding
+ * ValidationError to the ValidationError.Sink and returns the error. */
+ private Optional<String> checkForErrors(String filterQuery) {
+ try {
+ queryValidator.validateQuery(filterQuery);
+ } catch (QueryParseException qpe) {
+ validationErrorSink.ifPresent(ves -> ves.error(ValidationError.create(qpe.getMessage())));
+ return Optional.of(qpe.getMessage());
+ }
+ return Optional.empty();
}
class ReviewerFilterSection extends ReviewerFilter {
@@ -74,6 +125,10 @@
save();
}
+ public void filterError(String error) {
+ this.filterError = error;
+ }
+
private void save() {
if (this.reviewers.isEmpty() && this.ccs.isEmpty()) {
cfg.unsetSection(SECTION_FILTER, filter);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewersQueryValidator.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewersQueryValidator.java
new file mode 100644
index 0000000..4793a57
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewersQueryValidator.java
@@ -0,0 +1,36 @@
+// Copyright (C) 2020 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.googlesource.gerrit.plugins.reviewers.config;
+
+import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+
+/** Validates that a reviewer filter query is formatted correctly. */
+@Singleton
+public class ReviewersQueryValidator {
+ private final Provider<ChangeQueryBuilder> queryBuilder;
+
+ @Inject
+ public ReviewersQueryValidator(Provider<ChangeQueryBuilder> queryBuilder) {
+ this.queryBuilder = queryBuilder;
+ }
+
+ void validateQuery(String query) throws QueryParseException {
+ queryBuilder.get().parse(query);
+ }
+}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/reviewers/AbstractReviewersPluginTest.java b/src/test/java/com/googlesource/gerrit/plugins/reviewers/AbstractReviewersPluginTest.java
index cd13812..a9ab6e4 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/reviewers/AbstractReviewersPluginTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/reviewers/AbstractReviewersPluginTest.java
@@ -23,11 +23,13 @@
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.entities.RefNames;
import com.google.inject.Inject;
import java.util.Arrays;
+import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
@@ -39,10 +41,21 @@
@Inject protected ProjectOperations projectOperations;
protected void createFilters(TestFilter... filters) throws Exception {
- createFiltersFor(testRepo, filters);
+ createFiltersFor(testRepo, Optional.empty(), filters);
}
protected void createFiltersFor(TestRepository<?> repo, TestFilter... filters) throws Exception {
+ createFiltersFor(repo, Optional.empty(), filters);
+ }
+
+ protected void createFiltersWithError(String errorMessage, TestFilter... filters)
+ throws Exception {
+ createFiltersFor(testRepo, Optional.of(errorMessage), filters);
+ }
+
+ protected void createFiltersFor(
+ TestRepository<?> repo, Optional<String> errorMessage, TestFilter... filters)
+ throws Exception {
String previousHead = repo.getRepository().getBranch();
checkoutRefsMetaConfig(repo);
Config cfg = new Config();
@@ -53,10 +66,15 @@
SECTION_FILTER, f.filter, KEY_REVIEWER, Lists.newArrayList(f.reviewers));
cfg.setStringList(SECTION_FILTER, f.filter, KEY_CC, Lists.newArrayList(f.ccs));
});
- pushFactory
- .create(admin.newIdent(), repo, "Add reviewers", FILENAME, cfg.toText())
- .to(RefNames.REFS_CONFIG)
- .assertOkStatus();
+ PushOneCommit.Result result =
+ pushFactory
+ .create(admin.newIdent(), repo, "Add reviewers", FILENAME, cfg.toText())
+ .to(RefNames.REFS_CONFIG);
+ if (errorMessage.isPresent()) {
+ result.assertErrorStatus(errorMessage.get());
+ } else {
+ result.assertOkStatus();
+ }
repo.reset(previousHead);
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewerFilterCollectionIT.java b/src/test/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewerFilterCollectionIT.java
new file mode 100644
index 0000000..7d4fd4f
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewerFilterCollectionIT.java
@@ -0,0 +1,55 @@
+// Copyright (C) 2021 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.googlesource.gerrit.plugins.reviewers.config;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.googlesource.gerrit.plugins.reviewers.config.ForProject.KEY_REVIEWER;
+import static com.googlesource.gerrit.plugins.reviewers.config.ForProject.SECTION_FILTER;
+
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.extensions.config.FactoryModule;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Test;
+
+@NoHttpd
+@TestPlugin(
+ name = "reviewers",
+ sysModule =
+ "com.googlesource.gerrit.plugins.reviewers.config.ReviewerFilterCollectionIT$TestModule")
+public class ReviewerFilterCollectionIT extends LightweightPluginDaemonTest {
+
+ @Test
+ public void malformedFilterErrorPropagated() throws Exception {
+ String malformedQuery = "malformed:query";
+ Config malformed = new Config();
+ malformed.setString(SECTION_FILTER, malformedQuery, KEY_REVIEWER, "User");
+ assertThat(filters().create(malformed).get(malformedQuery).getFilterError())
+ .isEqualTo(String.format("Unsupported operator %s", malformedQuery));
+ }
+
+ private ReviewerFilterCollection.Factory filters() {
+ return plugin.getSysInjector().getInstance(ReviewerFilterCollection.Factory.class);
+ }
+
+ public static class TestModule extends FactoryModule {
+
+ @Override
+ public void configure() {
+ factory(ReviewerFilterCollection.Factory.class);
+ }
+ }
+}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewersConfigIT.java b/src/test/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewersConfigIT.java
index 5f71d91..be761fb 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewersConfigIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/reviewers/config/ReviewersConfigIT.java
@@ -21,6 +21,7 @@
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.entities.Project;
import com.googlesource.gerrit.plugins.reviewers.AbstractReviewersPluginTest;
+import com.googlesource.gerrit.plugins.reviewers.ForProjectValidator;
import org.eclipse.jgit.junit.TestRepository;
import org.junit.Before;
import org.junit.Test;
@@ -29,6 +30,7 @@
@TestPlugin(name = "reviewers", sysModule = "com.googlesource.gerrit.plugins.reviewers.Module")
public class ReviewersConfigIT extends AbstractReviewersPluginTest {
private static final String BRANCH_MAIN = "branch:main";
+ private static final String MALFORMED_FILTER = "branches:master,stable2";
private static final String NO_FILTER = "*";
private static final String JANE_DOE = "jane.doe@example.com";
private static final String JOHN_DOE = "john.doe@example.com";
@@ -77,6 +79,12 @@
filter(BRANCH_MAIN).reviewer(JOHN_DOE).cc(JANE_DOE));
}
+ @Test
+ public void malformedFilterQuery() throws Exception {
+ createFiltersWithError(
+ ForProjectValidator.MALFORMED_CONFIG, filter(MALFORMED_FILTER).reviewer(JOHN_DOE));
+ }
+
private void assertProjectHasFilters(Project.NameKey project, TestFilter... filters) {
assertThat(filters().withInheritance(project))
.containsExactlyElementsIn(ImmutableList.copyOf(filters));
