Merge "GetCheckerIT: Add tests for newly added checker fields"
diff --git a/java/com/google/gerrit/plugins/checks/Checker.java b/java/com/google/gerrit/plugins/checks/Checker.java
index 8bf6709..67a29f8 100644
--- a/java/com/google/gerrit/plugins/checks/Checker.java
+++ b/java/com/google/gerrit/plugins/checks/Checker.java
@@ -15,17 +15,36 @@
package com.google.gerrit.plugins.checks;
import com.google.auto.value.AutoValue;
+import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.index.query.IndexPredicate;
+import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.plugins.checks.api.BlockingCondition;
import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.index.change.ChangeField;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
+import com.google.gerrit.server.query.change.ChangeQueryProcessor;
+import com.google.gerrit.server.query.change.ChangeStatusPredicate;
+import com.google.gerrit.server.query.change.ProjectPredicate;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.RetryHelper.ActionType;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Provider;
import java.sql.Timestamp;
+import java.util.List;
import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
/** Definition of a checker. */
@AutoValue
public abstract class Checker {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
/**
* Returns the UUID of the checker.
@@ -126,6 +145,109 @@
return builder().setUuid(uuid);
}
+ public boolean isCheckerRelevant(ChangeData cd, ChangeQueryBuilder changeQueryBuilder)
+ throws OrmException {
+ if (!getQuery().isPresent()) {
+ return cd.change().isNew();
+ }
+
+ Predicate<ChangeData> predicate;
+ try {
+ predicate = createQueryPredicate(changeQueryBuilder);
+ } catch (ConfigInvalidException e) {
+ logger.atWarning().withCause(e).log("skipping invalid query for checker %s", getUuid());
+ return false;
+ }
+
+ return predicate.asMatchable().match(cd);
+ }
+
+ public List<ChangeData> queryMatchingChanges(
+ RetryHelper retryHelper,
+ ChangeQueryBuilder changeQueryBuilder,
+ Provider<ChangeQueryProcessor> changeQueryProcessorProvider)
+ throws ConfigInvalidException, OrmException {
+ return executeIndexQueryWithRetry(
+ retryHelper, changeQueryProcessorProvider, createQueryPredicate(changeQueryBuilder));
+ }
+
+ private Predicate<ChangeData> createQueryPredicate(ChangeQueryBuilder changeQueryBuilder)
+ throws ConfigInvalidException {
+ Predicate<ChangeData> predicate = new ProjectPredicate(getRepository().get());
+
+ if (getQuery().isPresent()) {
+ String query = getQuery().get();
+ Predicate<ChangeData> predicateForQuery;
+ try {
+ predicateForQuery = changeQueryBuilder.parse(query);
+ } catch (QueryParseException e) {
+ throw new ConfigInvalidException(
+ String.format("change query of checker %s is invalid: %s", getUuid(), query), e);
+ }
+
+ if (!predicateForQuery.isMatchable()) {
+ // Assuming nobody modified the query behind Gerrit's back, this is programmer error:
+ // CheckerQuery should not be able to produce non-matchable queries.
+ throw new ConfigInvalidException(
+ String.format("change query of checker %s is non-matchable: %s", getUuid(), query));
+ }
+
+ predicate = Predicate.and(predicate, predicateForQuery);
+ }
+
+ if (!hasStatusPredicate(predicate)) {
+ predicate = Predicate.and(ChangeStatusPredicate.open(), predicate);
+ }
+
+ return predicate;
+ }
+
+ private static boolean hasStatusPredicate(Predicate<ChangeData> predicate) {
+ if (predicate instanceof IndexPredicate) {
+ return ((IndexPredicate<ChangeData>) predicate)
+ .getField()
+ .getName()
+ .equals(ChangeField.STATUS.getName());
+ }
+ return predicate.getChildren().stream().anyMatch(Checker::hasStatusPredicate);
+ }
+
+ // TODO(ekempin): Retrying the query should be done by ChangeQueryProcessor.
+ private List<ChangeData> executeIndexQueryWithRetry(
+ RetryHelper retryHelper,
+ Provider<ChangeQueryProcessor> changeQueryProcessorProvider,
+ Predicate<ChangeData> predicate)
+ throws OrmException {
+ try {
+ return retryHelper.execute(
+ ActionType.INDEX_QUERY,
+ () -> changeQueryProcessorProvider.get().query(predicate).entities(),
+ OrmException.class::isInstance);
+ } catch (Exception e) {
+ Throwables.throwIfUnchecked(e);
+ Throwables.throwIfInstanceOf(e, OrmException.class);
+ throw new OrmException(e);
+ }
+ }
+
+ /**
+ * Checks whether a {@link Checker} is required for submission or not.
+ *
+ * @return true if the {@link Checker} required for submission.
+ */
+ public boolean isRequired() {
+ ImmutableSet<BlockingCondition> blockingConditions = getBlockingConditions();
+ if (blockingConditions.isEmpty()) {
+ return false;
+ } else if (blockingConditions.size() > 1
+ || !blockingConditions.contains(BlockingCondition.STATE_NOT_PASSING)) {
+ // When a new blocking condition is introduced, this needs to be adjusted to respect that.
+ String errorMessage = String.format("illegal blocking conditions %s", blockingConditions);
+ throw new IllegalStateException(errorMessage);
+ }
+ return true;
+ }
+
/** A builder for an {@link Checker}. */
@AutoValue.Builder
public abstract static class Builder {
diff --git a/java/com/google/gerrit/plugins/checks/Checks.java b/java/com/google/gerrit/plugins/checks/Checks.java
index 747d81b..7d6cd03 100644
--- a/java/com/google/gerrit/plugins/checks/Checks.java
+++ b/java/com/google/gerrit/plugins/checks/Checks.java
@@ -14,10 +14,13 @@
package com.google.gerrit.plugins.checks;
+import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
+import com.google.gerrit.plugins.checks.api.CombinedCheckState;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gwtorm.server.OrmException;
+import java.io.IOException;
import java.util.List;
import java.util.Optional;
@@ -37,15 +40,51 @@
*
* @param projectName the name of the project
* @param patchSetId the ID of the patch set
+ * @param options options for getting checks.
* @return the checks, {@link Optional#empty()} if no checks with the given UUID exists
* @throws OrmException if the checks couldn't be retrieved from the storage
*/
- ImmutableList<Check> getChecks(Project.NameKey projectName, PatchSet.Id patchSetId)
- throws OrmException;
+ ImmutableList<Check> getChecks(
+ Project.NameKey projectName, PatchSet.Id patchSetId, GetCheckOptions options)
+ throws OrmException, IOException;
/**
* Returns a {@link Optional} holding a single check. {@code Optional.empty()} if the check does
* not exist.
+ *
+ * @param checkKey the key of the target check.
+ * @param options options for getting a check.
+ * @return the target check if it exists. A backfilled check will be returned if {@link
+ * GetCheckOptions#backfillChecks()} is true.
+ * @throws OrmException if the check couldn't be retrieved from the storage
+ * @throws IOException if the check couldn't be retrieved from the storage
*/
- Optional<Check> getCheck(CheckKey checkKey) throws OrmException;
+ Optional<Check> getCheck(CheckKey checkKey, GetCheckOptions options)
+ throws OrmException, IOException;
+
+ /**
+ * Returns the combined check state of a given patch set.
+ *
+ * @param projectName the name of the project.
+ * @param patchSetId the ID of the patch set
+ * @return the {@link CombinedCheckState} of the current patch set.
+ * @throws IOException if failed to get the {@link CombinedCheckState}.
+ * @throws OrmException if failed to get the {@link CombinedCheckState}.
+ */
+ CombinedCheckState getCombinedCheckState(Project.NameKey projectName, PatchSet.Id patchSetId)
+ throws IOException, OrmException;
+
+ @AutoValue
+ abstract class GetCheckOptions {
+ public static GetCheckOptions defaults() {
+ return new AutoValue_Checks_GetCheckOptions(false);
+ }
+
+ public static GetCheckOptions withBackfilling() {
+ return new AutoValue_Checks_GetCheckOptions(true);
+ }
+
+ /** Backfills checks for relevant checkers with default when they don't exist yet. */
+ public abstract boolean backfillChecks();
+ }
}
diff --git a/java/com/google/gerrit/plugins/checks/Module.java b/java/com/google/gerrit/plugins/checks/Module.java
index ba535e2..987a227 100644
--- a/java/com/google/gerrit/plugins/checks/Module.java
+++ b/java/com/google/gerrit/plugins/checks/Module.java
@@ -21,10 +21,15 @@
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.plugins.checks.api.ApiModule;
+import com.google.gerrit.plugins.checks.api.ChangeCheckAttributeFactory;
+import com.google.gerrit.plugins.checks.api.ChangeCheckAttributeFactory.GetChangeOptions;
import com.google.gerrit.plugins.checks.db.NoteDbCheckersModule;
+import com.google.gerrit.server.DynamicOptions;
+import com.google.gerrit.server.change.ChangeAttributeFactory;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.MergeValidationListener;
import com.google.gerrit.server.git.validators.RefOperationValidationListener;
+import com.google.gerrit.server.restapi.change.GetChange;
public class Module extends FactoryModule {
@Override
@@ -46,6 +51,11 @@
.to(CheckerRefOperationValidator.class)
.in(SINGLETON);
+ DynamicSet.bind(binder(), ChangeAttributeFactory.class).to(ChangeCheckAttributeFactory.class);
+ bind(DynamicOptions.DynamicBean.class)
+ .annotatedWith(Exports.named(GetChange.class))
+ .to(GetChangeOptions.class);
+
install(new ApiModule());
}
}
diff --git a/java/com/google/gerrit/plugins/checks/PostCheck.java b/java/com/google/gerrit/plugins/checks/PostCheck.java
index 9327842..ed4ce33 100644
--- a/java/com/google/gerrit/plugins/checks/PostCheck.java
+++ b/java/com/google/gerrit/plugins/checks/PostCheck.java
@@ -18,6 +18,7 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestCollectionModifyView;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.plugins.checks.Checks.GetCheckOptions;
import com.google.gerrit.plugins.checks.api.CheckInfo;
import com.google.gerrit.plugins.checks.api.CheckInput;
import com.google.gerrit.plugins.checks.api.CheckResource;
@@ -78,7 +79,7 @@
CheckerUuid checkerUuid = CheckerUuid.parse(input.checkerUuid);
CheckKey key = CheckKey.create(rsrc.getProject(), rsrc.getPatchSet().getId(), checkerUuid);
- Optional<Check> check = checks.getCheck(key);
+ Optional<Check> check = checks.getCheck(key, GetCheckOptions.defaults());
Check updatedCheck;
if (!check.isPresent()) {
checkers
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/TestModule.java b/java/com/google/gerrit/plugins/checks/acceptance/TestModule.java
index 3348659..c04a4d3 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/TestModule.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/TestModule.java
@@ -14,11 +14,14 @@
package com.google.gerrit.plugins.checks.acceptance;
+import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.plugins.checks.Module;
import com.google.gerrit.plugins.checks.acceptance.testsuite.CheckOperations;
import com.google.gerrit.plugins.checks.acceptance.testsuite.CheckOperationsImpl;
import com.google.gerrit.plugins.checks.acceptance.testsuite.CheckerOperations;
import com.google.gerrit.plugins.checks.acceptance.testsuite.CheckerOperationsImpl;
+import com.google.gerrit.plugins.checks.rules.ChecksSubmitRule;
+import com.google.gerrit.server.rules.SubmitRule;
import com.google.inject.AbstractModule;
public class TestModule extends AbstractModule {
@@ -30,5 +33,8 @@
// setup in tests as realistic as possible by delegating to the original module.
bind(CheckerOperations.class).to(CheckerOperationsImpl.class);
bind(CheckOperations.class).to(CheckOperationsImpl.class);
+ bind(SubmitRule.class)
+ .annotatedWith(Exports.named("ChecksSubmitRule"))
+ .to(ChecksSubmitRule.class);
}
}
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
index 9cd2aa5..5d78aae 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
@@ -24,6 +24,7 @@
import com.google.gerrit.plugins.checks.CheckUpdate;
import com.google.gerrit.plugins.checks.CheckerRef;
import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.Checks.GetCheckOptions;
import com.google.gerrit.plugins.checks.ChecksUpdate;
import com.google.gerrit.plugins.checks.ListChecksOption;
import com.google.gerrit.plugins.checks.api.CheckInfo;
@@ -76,12 +77,12 @@
@Override
public boolean exists() throws Exception {
- return checks.getCheck(key).isPresent();
+ return checks.getCheck(key, GetCheckOptions.defaults()).isPresent();
}
@Override
public Check get() throws Exception {
- return checks.getCheck(key).get();
+ return checks.getCheck(key, GetCheckOptions.defaults()).get();
}
@Override
diff --git a/java/com/google/gerrit/plugins/checks/api/BlockingCondition.java b/java/com/google/gerrit/plugins/checks/api/BlockingCondition.java
index 74b43bf..f0d032d 100644
--- a/java/com/google/gerrit/plugins/checks/api/BlockingCondition.java
+++ b/java/com/google/gerrit/plugins/checks/api/BlockingCondition.java
@@ -14,6 +14,8 @@
package com.google.gerrit.plugins.checks.api;
+import java.util.Set;
+
/**
* Conditions evaluated on a check in the context of a change that determine whether the check
* blocks submission of a change.
@@ -24,4 +26,8 @@
* CombinedCheckState#isPassing() is passing}.
*/
STATE_NOT_PASSING;
+
+ public static Boolean isRequired(Set<BlockingCondition> blocking) {
+ return blocking.contains(STATE_NOT_PASSING);
+ }
}
diff --git a/java/com/google/gerrit/plugins/checks/api/ChangeCheckAttributeFactory.java b/java/com/google/gerrit/plugins/checks/api/ChangeCheckAttributeFactory.java
new file mode 100644
index 0000000..aacbb1e
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/api/ChangeCheckAttributeFactory.java
@@ -0,0 +1,73 @@
+// 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.api;
+
+import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.server.DynamicOptions.BeanProvider;
+import com.google.gerrit.server.DynamicOptions.DynamicBean;
+import com.google.gerrit.server.change.ChangeAttributeFactory;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import org.kohsuke.args4j.Option;
+
+/**
+ * Factory that populates a single {@link ChangeCheckInfo} instance in the {@code plugins} field of
+ * a {@code ChangeInfo}.
+ */
+@Singleton
+public class ChangeCheckAttributeFactory implements ChangeAttributeFactory {
+ public static class GetChangeOptions implements DynamicBean {
+ @Option(name = "--combined", usage = "include combined check state")
+ boolean combined;
+ }
+
+ private final Checks checks;
+
+ @Inject
+ ChangeCheckAttributeFactory(Checks checks) {
+ this.checks = checks;
+ }
+
+ @Override
+ public ChangeCheckInfo create(ChangeData cd, BeanProvider beanProvider, String plugin) {
+ DynamicBean opts = beanProvider.getDynamicBean(plugin);
+ if (opts == null) {
+ return null;
+ }
+ try {
+ if (opts instanceof GetChangeOptions) {
+ return forGetChange(cd, (GetChangeOptions) opts);
+ }
+ // TODO(dborowitz): Compute from cache in query path.
+ } catch (OrmException | IOException e) {
+ throw new RuntimeException(e);
+ }
+ throw new IllegalStateException("unexpected options type: " + opts);
+ }
+
+ private ChangeCheckInfo forGetChange(ChangeData cd, GetChangeOptions opts)
+ throws OrmException, IOException {
+ if (opts == null || !opts.combined) {
+ return null;
+ }
+ ChangeCheckInfo info = new ChangeCheckInfo();
+ info.combinedState =
+ checks.getCombinedCheckState(cd.project(), cd.change().currentPatchSetId());
+ return info;
+ }
+}
diff --git a/java/com/google/gerrit/plugins/checks/api/ChangeCheckInfo.java b/java/com/google/gerrit/plugins/checks/api/ChangeCheckInfo.java
new file mode 100644
index 0000000..257b8fc
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/api/ChangeCheckInfo.java
@@ -0,0 +1,56 @@
+// 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.api;
+
+import static java.util.Objects.requireNonNull;
+
+import com.google.common.base.MoreObjects;
+import com.google.gerrit.extensions.common.PluginDefinedInfo;
+import java.util.Objects;
+
+/** Information about checks on a change, stored in a plugin-defined field in {@code ChangeInfo}. */
+public class ChangeCheckInfo extends PluginDefinedInfo {
+ /** Combined check state of the change. */
+ public CombinedCheckState combinedState;
+
+ public ChangeCheckInfo() {}
+
+ public ChangeCheckInfo(String pluginName, CombinedCheckState combinedState) {
+ this.name = requireNonNull(pluginName);
+ this.combinedState = requireNonNull(combinedState);
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (!(o instanceof ChangeCheckInfo)) {
+ return false;
+ }
+ ChangeCheckInfo i = (ChangeCheckInfo) o;
+ return Objects.equals(name, i.name) && Objects.equals(combinedState, i.combinedState);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(name, combinedState);
+ }
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this)
+ .add("name", name)
+ .add("combinedState", combinedState)
+ .toString();
+ }
+}
diff --git a/java/com/google/gerrit/plugins/checks/api/ChecksCollection.java b/java/com/google/gerrit/plugins/checks/api/ChecksCollection.java
index 1c34dc4..db02463 100644
--- a/java/com/google/gerrit/plugins/checks/api/ChecksCollection.java
+++ b/java/com/google/gerrit/plugins/checks/api/ChecksCollection.java
@@ -26,6 +26,7 @@
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.Checks.GetCheckOptions;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gwtorm.server.OrmException;
@@ -36,18 +37,13 @@
@Singleton
public class ChecksCollection implements ChildCollection<RevisionResource, CheckResource> {
- private final CheckBackfiller checkBackfiller;
private final Checks checks;
private final DynamicMap<RestView<CheckResource>> views;
private final ListChecks listChecks;
@Inject
ChecksCollection(
- CheckBackfiller checkBackfiller,
- Checks checks,
- DynamicMap<RestView<CheckResource>> views,
- ListChecks listChecks) {
- this.checkBackfiller = checkBackfiller;
+ Checks checks, DynamicMap<RestView<CheckResource>> views, ListChecks listChecks) {
this.checks = checks;
this.views = views;
this.listChecks = listChecks;
@@ -67,12 +63,7 @@
() -> new BadRequestException(String.format("invalid checker UUID: %s", id.get())));
CheckKey checkKey =
CheckKey.create(parent.getProject(), parent.getPatchSet().getId(), checkerUuid);
- Optional<Check> check = checks.getCheck(checkKey);
- if (!check.isPresent()) {
- check =
- checkBackfiller.getBackfilledCheckForRelevantChecker(
- checkerUuid, parent.getNotes(), checkKey.patchSet());
- }
+ Optional<Check> check = checks.getCheck(checkKey, GetCheckOptions.withBackfilling());
return new CheckResource(
parent,
check.orElseThrow(
diff --git a/java/com/google/gerrit/plugins/checks/api/ListChecks.java b/java/com/google/gerrit/plugins/checks/api/ListChecks.java
index a787e69..ed76af0 100644
--- a/java/com/google/gerrit/plugins/checks/api/ListChecks.java
+++ b/java/com/google/gerrit/plugins/checks/api/ListChecks.java
@@ -14,8 +14,6 @@
package com.google.gerrit.plugins.checks.api;
-import static java.util.stream.Collectors.toMap;
-
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.client.ListOption;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -24,23 +22,18 @@
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckJson;
-import com.google.gerrit.plugins.checks.Checker;
-import com.google.gerrit.plugins.checks.CheckerUuid;
-import com.google.gerrit.plugins.checks.Checkers;
import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.Checks.GetCheckOptions;
import com.google.gerrit.plugins.checks.ListChecksOption;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.io.IOException;
import java.util.EnumSet;
-import java.util.Map;
import org.kohsuke.args4j.Option;
public class ListChecks implements RestReadView<RevisionResource> {
- private final CheckBackfiller checkBackfiller;
private final CheckJson.Factory checkJsonFactory;
- private final Checkers checkers;
private final Checks checks;
private final EnumSet<ListChecksOption> options = EnumSet.noneOf(ListChecksOption.class);
@@ -56,14 +49,8 @@
}
@Inject
- ListChecks(
- CheckBackfiller checkBackfiller,
- CheckJson.Factory checkJsonFactory,
- Checkers checkers,
- Checks checks) {
- this.checkBackfiller = checkBackfiller;
+ ListChecks(CheckJson.Factory checkJsonFactory, Checks checks) {
this.checkJsonFactory = checkJsonFactory;
- this.checkers = checkers;
this.checks = checks;
}
@@ -71,24 +58,16 @@
public ImmutableList<CheckInfo> apply(RevisionResource resource)
throws AuthException, BadRequestException, ResourceConflictException, OrmException,
IOException {
+ ImmutableList.Builder<CheckInfo> result = ImmutableList.builder();
+
+ GetCheckOptions getCheckOptions = GetCheckOptions.withBackfilling();
+ ImmutableList<Check> allChecks =
+ checks.getChecks(resource.getProject(), resource.getPatchSet().getId(), getCheckOptions);
+
CheckJson checkJson = checkJsonFactory.create(options);
- Map<CheckerUuid, Checker> checkersByUuid =
- checkers.checkersOf(resource.getProject()).stream()
- .collect(toMap(Checker::getUuid, c -> c));
-
- ImmutableList.Builder<CheckInfo> result =
- ImmutableList.builderWithExpectedSize(checkersByUuid.size());
- for (Check check : checks.getChecks(resource.getProject(), resource.getPatchSet().getId())) {
- checkersByUuid.remove(check.key().checkerUuid());
+ for (Check check : allChecks) {
result.add(checkJson.format(check));
}
-
- for (Check check :
- checkBackfiller.getBackfilledChecksForRelevantCheckers(
- checkersByUuid.values(), resource.getNotes(), resource.getPatchSet().getId())) {
- result.add(checkJson.format(check));
- }
-
return result.build();
}
}
diff --git a/java/com/google/gerrit/plugins/checks/api/ListPendingChecks.java b/java/com/google/gerrit/plugins/checks/api/ListPendingChecks.java
index a4535bc..4ad556c 100644
--- a/java/com/google/gerrit/plugins/checks/api/ListPendingChecks.java
+++ b/java/com/google/gerrit/plugins/checks/api/ListPendingChecks.java
@@ -14,26 +14,44 @@
package com.google.gerrit.plugins.checks.api;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.extensions.restapi.TopLevelResource;
-import com.google.gerrit.plugins.checks.AdministrateCheckersPermission;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.plugins.checks.Check;
+import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.Checker;
import com.google.gerrit.plugins.checks.CheckerUuid;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.plugins.checks.Checkers;
+import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.Checks.GetCheckOptions;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
+import com.google.gerrit.server.query.change.ChangeQueryProcessor;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.kohsuke.args4j.Option;
public class ListPendingChecks implements RestReadView<TopLevelResource> {
- private final PermissionBackend permissionBackend;
- private final AdministrateCheckersPermission permission;
+ private final Checkers checkers;
+ private final Checks checks;
+ private final RetryHelper retryHelper;
+ private final Provider<ChangeQueryBuilder> queryBuilderProvider;
+ private final Provider<ChangeQueryProcessor> changeQueryProcessorProvider;
private CheckerUuid checkerUuid;
- private String scheme;
private List<CheckState> states = new ArrayList<>(CheckState.values().length);
@Option(
@@ -45,11 +63,6 @@
this.checkerUuid = checkerUuid;
}
- @Option(name = "--scheme", metaVar = "SCHEME", usage = "checker scheme")
- public void setScheme(String scheme) {
- this.scheme = scheme;
- }
-
@Option(name = "--state", metaVar = "STATE", usage = "check state")
public void addState(CheckState state) {
this.states.add(state);
@@ -57,30 +70,92 @@
@Inject
public ListPendingChecks(
- PermissionBackend permissionBackend, AdministrateCheckersPermission permission) {
- this.permissionBackend = permissionBackend;
- this.permission = permission;
+ Checkers checkers,
+ Checks checks,
+ RetryHelper retryHelper,
+ Provider<ChangeQueryBuilder> queryBuilderProvider,
+ Provider<ChangeQueryProcessor> changeQueryProcessorProvider) {
+ this.checkers = checkers;
+ this.checks = checks;
+ this.retryHelper = retryHelper;
+ this.queryBuilderProvider = queryBuilderProvider;
+ this.changeQueryProcessorProvider = changeQueryProcessorProvider;
}
@Override
public List<PendingChecksInfo> apply(TopLevelResource resource)
- throws RestApiException, PermissionBackendException {
- permissionBackend.currentUser().check(permission);
-
+ throws RestApiException, IOException, ConfigInvalidException, OrmException {
if (states.isEmpty()) {
// If no state was specified, assume NOT_STARTED by default.
states.add(CheckState.NOT_STARTED);
}
- if (checkerUuid == null && scheme == null) {
- throw new BadRequestException("checker or scheme is required");
+ if (checkerUuid == null) {
+ throw new BadRequestException("checker UUID is required");
}
- if (checkerUuid != null && scheme != null) {
- throw new BadRequestException("checker and scheme are mutually exclusive");
+ Checker checker =
+ checkers
+ .getChecker(checkerUuid)
+ .orElseThrow(
+ () ->
+ new UnprocessableEntityException(
+ String.format("checker %s not found", checkerUuid)));
+
+ if (checker.getStatus() == CheckerStatus.DISABLED) {
+ return ImmutableList.of();
}
- // TODO(ekempin): Implement this REST endpoint
- throw new MethodNotAllowedException("not implemented");
+ // The query system can only match against the current patch set; ignore non-current patch sets
+ // for now.
+ List<ChangeData> changes =
+ checker.queryMatchingChanges(
+ retryHelper, queryBuilderProvider.get(), changeQueryProcessorProvider);
+ List<PendingChecksInfo> pendingChecks = new ArrayList<>(changes.size());
+ for (ChangeData cd : changes) {
+ getPostFilteredPendingChecks(cd.project(), cd.currentPatchSet().getId())
+ .ifPresent(pendingChecks::add);
+ }
+ return pendingChecks;
+ }
+
+ private Optional<PendingChecksInfo> getPostFilteredPendingChecks(
+ Project.NameKey project, PatchSet.Id patchSetId) throws OrmException, IOException {
+ CheckState checkState = getCheckState(project, patchSetId);
+ if (!states.contains(checkState)) {
+ return Optional.empty();
+ }
+ return Optional.of(createPendingChecksInfo(project, patchSetId, checkerUuid, checkState));
+ }
+
+ private CheckState getCheckState(Project.NameKey project, PatchSet.Id patchSetId)
+ throws OrmException, IOException {
+ Optional<Check> check =
+ checks.getCheck(
+ CheckKey.create(project, patchSetId, checkerUuid), GetCheckOptions.defaults());
+
+ // Backfill if check is not present.
+ // Backfilling is only done for relevant checkers (checkers where the repository and the query
+ // matches the change). Since the change was found by executing the query of the checker we know
+ // that the checker is relevant for this patch set and hence backfilling should be done.
+ return check.map(Check::state).orElse(CheckState.NOT_STARTED);
+ }
+
+ private static PendingChecksInfo createPendingChecksInfo(
+ Project.NameKey project,
+ PatchSet.Id patchSetId,
+ CheckerUuid checkerUuid,
+ CheckState checkState) {
+ PendingChecksInfo pendingChecksInfo = new PendingChecksInfo();
+
+ pendingChecksInfo.patchSet = new CheckablePatchSetInfo();
+ pendingChecksInfo.patchSet.project = project.get();
+ pendingChecksInfo.patchSet.changeNumber = patchSetId.getParentKey().get();
+ pendingChecksInfo.patchSet.patchSetId = patchSetId.get();
+
+ pendingChecksInfo.pendingChecks =
+ ImmutableMap.of(checkerUuid.toString(), new PendingCheckInfo(checkState));
+
+ return pendingChecksInfo;
}
}
diff --git a/java/com/google/gerrit/plugins/checks/api/PendingCheckInfo.java b/java/com/google/gerrit/plugins/checks/api/PendingCheckInfo.java
index 5e46bde..6ba6275 100644
--- a/java/com/google/gerrit/plugins/checks/api/PendingCheckInfo.java
+++ b/java/com/google/gerrit/plugins/checks/api/PendingCheckInfo.java
@@ -14,6 +14,9 @@
package com.google.gerrit.plugins.checks.api;
+import com.google.common.base.MoreObjects;
+import java.util.Objects;
+
/**
* REST API representation of a pending check.
*
@@ -24,4 +27,27 @@
public class PendingCheckInfo {
/** State of the check. */
public CheckState state;
+
+ public PendingCheckInfo(CheckState state) {
+ this.state = state;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(state);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (!(obj instanceof PendingCheckInfo)) {
+ return false;
+ }
+ PendingCheckInfo o = (PendingCheckInfo) obj;
+ return Objects.equals(state, o.state);
+ }
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this).add("state", state).toString();
+ }
}
diff --git a/java/com/google/gerrit/plugins/checks/api/PendingChecks.java b/java/com/google/gerrit/plugins/checks/api/PendingChecks.java
index 8b0811b..361472b 100644
--- a/java/com/google/gerrit/plugins/checks/api/PendingChecks.java
+++ b/java/com/google/gerrit/plugins/checks/api/PendingChecks.java
@@ -52,17 +52,6 @@
}
/**
- * Lists the pending checks for all checkers of the specified checker scheme.
- *
- * @param scheme the checker scheme for which pending checks should be listed
- * @param checkStates the states that should be considered as pending, if not specified {@link
- * CheckState#NOT_STARTED} is assumed.
- * @return the pending checks
- */
- List<PendingChecksInfo> listForScheme(String scheme, CheckState... checkStates)
- throws RestApiException;
-
- /**
* A default implementation which allows source compatibility when adding new methods to the
* interface.
*/
@@ -71,10 +60,5 @@
public List<PendingChecksInfo> list(CheckerUuid checkerUuid, CheckState... checkStates) {
throw new NotImplementedException();
}
-
- @Override
- public List<PendingChecksInfo> listForScheme(String scheme, CheckState... checkStates) {
- throw new NotImplementedException();
- }
}
}
diff --git a/java/com/google/gerrit/plugins/checks/api/PendingChecksImpl.java b/java/com/google/gerrit/plugins/checks/api/PendingChecksImpl.java
index 0b9c139..ca4cf18 100644
--- a/java/com/google/gerrit/plugins/checks/api/PendingChecksImpl.java
+++ b/java/com/google/gerrit/plugins/checks/api/PendingChecksImpl.java
@@ -46,17 +46,4 @@
throw asRestApiException("Cannot list pending checks", e);
}
}
-
- @Override
- public List<PendingChecksInfo> listForScheme(String scheme, CheckState... checkStates)
- throws RestApiException {
- try {
- ListPendingChecks listPendingChecks = listPendingChecksProvider.get();
- listPendingChecks.setScheme(scheme);
- Stream.of(checkStates).forEach(listPendingChecks::addState);
- return listPendingChecks.apply(TopLevelResource.INSTANCE);
- } catch (Exception e) {
- throw asRestApiException("Cannot list pending checks for scheme", e);
- }
- }
}
diff --git a/java/com/google/gerrit/plugins/checks/api/PendingChecksInfo.java b/java/com/google/gerrit/plugins/checks/api/PendingChecksInfo.java
index 01944b6..48311de 100644
--- a/java/com/google/gerrit/plugins/checks/api/PendingChecksInfo.java
+++ b/java/com/google/gerrit/plugins/checks/api/PendingChecksInfo.java
@@ -14,7 +14,6 @@
package com.google.gerrit.plugins.checks.api;
-import com.google.gerrit.plugins.checks.CheckerUuid;
import java.util.Map;
/** REST API representation of pending checks on patch set. */
@@ -23,5 +22,5 @@
public CheckablePatchSetInfo patchSet;
/** Pending checks on the patch set by checker UUID. */
- public Map<CheckerUuid, PendingCheckInfo> pendingChecks;
+ public Map<String, PendingCheckInfo> pendingChecks;
}
diff --git a/java/com/google/gerrit/plugins/checks/api/CheckBackfiller.java b/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
similarity index 61%
rename from java/com/google/gerrit/plugins/checks/api/CheckBackfiller.java
rename to java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
index d25e8af..dfd3892 100644
--- a/java/com/google/gerrit/plugins/checks/api/CheckBackfiller.java
+++ b/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
@@ -12,26 +12,20 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-package com.google.gerrit.plugins.checks.api;
+package com.google.gerrit.plugins.checks.db;
import com.google.common.collect.ImmutableList;
-import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.index.query.IndexPredicate;
-import com.google.gerrit.index.query.Predicate;
-import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.Checker;
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.Checkers;
+import com.google.gerrit.plugins.checks.api.CheckState;
+import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.AnonymousUser;
-import com.google.gerrit.server.index.change.ChangeField;
-import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.query.change.ChangeData.Factory;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
-import com.google.gerrit.server.query.change.ChangeStatusPredicate;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -43,31 +37,25 @@
@Singleton
class CheckBackfiller {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- private final ChangeData.Factory changeDataFactory;
private final Checkers checkers;
private final Provider<AnonymousUser> anonymousUserProvider;
private final Provider<ChangeQueryBuilder> queryBuilderProvider;
@Inject
CheckBackfiller(
- Factory changeDataFactory,
Checkers checkers,
Provider<AnonymousUser> anonymousUserProvider,
Provider<ChangeQueryBuilder> queryBuilderProvider) {
- this.changeDataFactory = changeDataFactory;
this.checkers = checkers;
this.anonymousUserProvider = anonymousUserProvider;
this.queryBuilderProvider = queryBuilderProvider;
}
ImmutableList<Check> getBackfilledChecksForRelevantCheckers(
- Collection<Checker> candidates, ChangeNotes notes, PatchSet.Id psId) throws OrmException {
+ Collection<Checker> candidates, ChangeData cd, PatchSet.Id psId) throws OrmException {
if (candidates.isEmpty()) {
return ImmutableList.of();
}
- ChangeData cd = changeDataFactory.create(notes);
if (!psId.equals(cd.change().currentPatchSetId())) {
// The query system can only match against the current patch set; it doesn't make sense to
// backfill checkers for old patch sets.
@@ -79,7 +67,7 @@
PatchSet ps = cd.patchSet(psId);
ChangeQueryBuilder queryBuilder = newQueryBuilder();
for (Checker checker : candidates) {
- if (matches(checker, cd, queryBuilder)) {
+ if (checker.isCheckerRelevant(cd, queryBuilder)) {
// Add synthetic check at the creation time of the patch set.
result.add(newBackfilledCheck(cd, ps, checker));
}
@@ -88,8 +76,7 @@
}
Optional<Check> getBackfilledCheckForRelevantChecker(
- CheckerUuid candidate, ChangeNotes notes, PatchSet.Id psId) throws OrmException, IOException {
- ChangeData cd = changeDataFactory.create(notes);
+ CheckerUuid candidate, ChangeData cd, PatchSet.Id psId) throws OrmException, IOException {
if (!psId.equals(cd.change().currentPatchSetId())) {
// The query system can only match against the current patch set; it doesn't make sense to
// backfill checkers for old patch sets.
@@ -105,7 +92,7 @@
}
if (!checker.isPresent()
|| checker.get().getStatus() != CheckerStatus.ENABLED
- || !matches(checker.get(), cd, newQueryBuilder())) {
+ || !checker.get().isCheckerRelevant(cd, newQueryBuilder())) {
return Optional.empty();
}
return Optional.of(newBackfilledCheck(cd, cd.patchSet(psId), checker.get()));
@@ -122,41 +109,4 @@
private ChangeQueryBuilder newQueryBuilder() {
return queryBuilderProvider.get().asUser(anonymousUserProvider.get());
}
-
- private static boolean matches(Checker checker, ChangeData cd, ChangeQueryBuilder queryBuilder)
- throws OrmException {
- if (!checker.getQuery().isPresent()) {
- return cd.change().isNew();
- }
- String query = checker.getQuery().get();
- Predicate<ChangeData> predicate;
- try {
- predicate = queryBuilder.parse(query);
- } catch (QueryParseException e) {
- logger.atWarning().withCause(e).log(
- "skipping invalid query for checker %s: %s", checker.getUuid(), query);
- return false;
- }
- if (!predicate.isMatchable()) {
- // Assuming nobody modified the query behind Gerrit's back, this is programmer error:
- // CheckerQuery should not be able to produce non-matchable queries.
- logger.atWarning().log(
- "skipping non-matchable query for checker %s: %s", checker.getUuid(), query);
- return false;
- }
- if (!hasStatusPredicate(predicate)) {
- predicate = Predicate.and(ChangeStatusPredicate.open(), predicate);
- }
- return predicate.asMatchable().match(cd);
- }
-
- private static boolean hasStatusPredicate(Predicate<ChangeData> predicate) {
- if (predicate instanceof IndexPredicate) {
- return ((IndexPredicate<ChangeData>) predicate)
- .getField()
- .getName()
- .equals(ChangeField.STATUS.getName());
- }
- return predicate.getChildren().stream().anyMatch(CheckBackfiller::hasStatusPredicate);
- }
}
diff --git a/java/com/google/gerrit/plugins/checks/db/CheckersByRepositoryNotes.java b/java/com/google/gerrit/plugins/checks/db/CheckersByRepositoryNotes.java
index 66432ba..09fdcb2 100644
--- a/java/com/google/gerrit/plugins/checks/db/CheckersByRepositoryNotes.java
+++ b/java/com/google/gerrit/plugins/checks/db/CheckersByRepositoryNotes.java
@@ -330,9 +330,7 @@
* <p>Doesn't validate the entries are valid checker UUIDs.
*/
private static ImmutableSortedSet<String> parseNote(byte[] raw) {
- return Splitter.on('\n')
- .splitToList(new String(raw, UTF_8))
- .stream()
+ return Splitter.on('\n').splitToList(new String(raw, UTF_8)).stream()
.collect(toImmutableSortedSet(naturalOrder()));
}
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbCheckers.java b/java/com/google/gerrit/plugins/checks/db/NoteDbCheckers.java
index f6c0c52..be8bd8e 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbCheckers.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbCheckers.java
@@ -63,10 +63,7 @@
@Override
public ImmutableList<Checker> listCheckers() throws IOException {
try (Repository allProjectsRepo = repoManager.openRepository(allProjectsName)) {
- return allProjectsRepo
- .getRefDatabase()
- .getRefsByPrefix(CheckerRef.REFS_CHECKERS)
- .stream()
+ return allProjectsRepo.getRefDatabase().getRefsByPrefix(CheckerRef.REFS_CHECKERS).stream()
.flatMap(ref -> Streams.stream(tryLoadChecker(allProjectsRepo, ref)))
.sorted(comparing(Checker::getUuid))
.collect(toImmutableList());
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbCheckersUpdate.java b/java/com/google/gerrit/plugins/checks/db/NoteDbCheckersUpdate.java
index c856c54..6da5a2a 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbCheckersUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbCheckersUpdate.java
@@ -162,8 +162,13 @@
CheckersByRepositoryNotes checkersByRepositoryNotes =
CheckersByRepositoryNotes.load(allProjectsName, allProjectsRepo);
- checkersByRepositoryNotes.insert(
- checkerCreation.getCheckerUuid(), checkerCreation.getRepository());
+ if (!checkerUpdate.getStatus().isPresent()
+ || checkerUpdate.getStatus().get() == CheckerStatus.ENABLED) {
+ // Only inserts to the notes if the status is not set or set as "ENABLED". Does not insert
+ // if the checker is DISABLED.
+ checkersByRepositoryNotes.insert(
+ checkerCreation.getCheckerUuid(), checkerCreation.getRepository());
+ }
commit(allProjectsRepo, checkerConfig, checkersByRepositoryNotes);
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
index 81779ce..c01b359 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
@@ -15,67 +15,166 @@
package com.google.gerrit.plugins.checks.db;
import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.Checker;
import com.google.gerrit.plugins.checks.CheckerUuid;
+import com.google.gerrit.plugins.checks.Checkers;
import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.api.CheckState;
+import com.google.gerrit.plugins.checks.api.CheckerStatus;
+import com.google.gerrit.plugins.checks.api.CombinedCheckState;
import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.PatchSet.Id;
import com.google.gerrit.reviewdb.client.Project;
-import com.google.gerrit.server.PatchSetUtil;
-import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.reviewdb.client.Project.NameKey;
+import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
import java.util.Optional;
import java.util.stream.Stream;
/** Class to read checks from NoteDb. */
@Singleton
class NoteDbChecks implements Checks {
- private final ChangeNotes.Factory changeNotesFactory;
- private final PatchSetUtil psUtil;
+ private final ChangeData.Factory changeDataFactory;
private final CheckNotes.Factory checkNotesFactory;
+ private final Checkers checkers;
+ private final CheckBackfiller checkBackfiller;
+ private final Provider<AnonymousUser> anonymousUserProvider;
+ private final Provider<ChangeQueryBuilder> queryBuilderProvider;
@Inject
NoteDbChecks(
- ChangeNotes.Factory changeNotesFactory,
- PatchSetUtil psUtil,
- CheckNotes.Factory checkNotesFactory) {
- this.changeNotesFactory = changeNotesFactory;
- this.psUtil = psUtil;
+ ChangeData.Factory changeDataFactory,
+ CheckNotes.Factory checkNotesFactory,
+ Checkers checkers,
+ CheckBackfiller checkBackfiller,
+ Provider<AnonymousUser> anonymousUserProvider,
+ Provider<ChangeQueryBuilder> queryBuilderProvider) {
+ this.changeDataFactory = changeDataFactory;
this.checkNotesFactory = checkNotesFactory;
+ this.checkers = checkers;
+ this.checkBackfiller = checkBackfiller;
+ this.anonymousUserProvider = anonymousUserProvider;
+ this.queryBuilderProvider = queryBuilderProvider;
}
@Override
- public ImmutableList<Check> getChecks(Project.NameKey projectName, PatchSet.Id psId)
- throws OrmException {
- return getChecksAsStream(projectName, psId).collect(toImmutableList());
+ public ImmutableList<Check> getChecks(
+ Project.NameKey projectName, PatchSet.Id psId, GetCheckOptions options)
+ throws IOException, OrmException {
+ return getChecksFromNoteDb(projectName, psId, options);
}
@Override
- public Optional<Check> getCheck(CheckKey checkKey) throws OrmException {
+ public Optional<Check> getCheck(CheckKey checkKey, GetCheckOptions options)
+ throws OrmException, IOException {
// TODO(gerrit-team): Instead of reading the complete notes map, read just one note.
- return getChecksAsStream(checkKey.project(), checkKey.patchSet())
- .filter(c -> c.key().checkerUuid().equals(checkKey.checkerUuid()))
- .findAny();
+ Optional<Check> result =
+ getChecksFromNoteDb(checkKey.project(), checkKey.patchSet(), GetCheckOptions.defaults())
+ .stream()
+ .filter(c -> c.key().checkerUuid().equals(checkKey.checkerUuid()))
+ .findAny();
+
+ if (!result.isPresent() && options.backfillChecks()) {
+ ChangeData changeData =
+ changeDataFactory.create(checkKey.project(), checkKey.patchSet().getParentKey());
+ return checkBackfiller.getBackfilledCheckForRelevantChecker(
+ checkKey.checkerUuid(), changeData, checkKey.patchSet());
+ }
+
+ return result;
}
- private Stream<Check> getChecksAsStream(Project.NameKey projectName, PatchSet.Id psId)
- throws OrmException {
+ private ImmutableList<Check> getChecksFromNoteDb(
+ Project.NameKey projectName, PatchSet.Id psId, GetCheckOptions options)
+ throws OrmException, IOException {
// TODO(gerrit-team): Instead of reading the complete notes map, read just one note.
- ChangeNotes notes = changeNotesFactory.create(projectName, psId.getParentKey());
- PatchSet patchSet = psUtil.get(notes, psId);
- CheckNotes checkNotes = checkNotesFactory.create(notes.getChange());
-
+ ChangeData changeData = changeDataFactory.create(projectName, psId.getParentKey());
+ PatchSet patchSet = changeData.patchSet(psId);
+ CheckNotes checkNotes = checkNotesFactory.create(changeData.change());
checkNotes.load();
- return checkNotes
- .getChecks()
- .getOrDefault(patchSet.getRevision(), NoteDbCheckMap.empty())
- .checks
- .entrySet()
- .stream()
- .map(e -> e.getValue().toCheck(projectName, psId, CheckerUuid.parse(e.getKey())));
+
+ ImmutableList<Check> existingChecks =
+ checkNotes.getChecks().getOrDefault(patchSet.getRevision(), NoteDbCheckMap.empty()).checks
+ .entrySet().stream()
+ .map(e -> e.getValue().toCheck(projectName, psId, CheckerUuid.parse(e.getKey())))
+ .collect(toImmutableList());
+
+ if (!options.backfillChecks()) {
+ return existingChecks;
+ }
+
+ ImmutableList<Checker> checkersForBackfiller =
+ getCheckersForBackfiller(projectName, existingChecks);
+ ImmutableList<Check> backfilledChecks =
+ checkBackfiller.getBackfilledChecksForRelevantCheckers(
+ checkersForBackfiller, changeData, psId);
+
+ return Stream.concat(existingChecks.stream(), backfilledChecks.stream())
+ .collect(toImmutableList());
+ }
+
+ @Override
+ public CombinedCheckState getCombinedCheckState(NameKey projectName, Id patchSetId)
+ throws IOException, OrmException {
+ ChangeData changeData = changeDataFactory.create(projectName, patchSetId.changeId);
+ ImmutableMap<String, Checker> allCheckersOfProject =
+ checkers.checkersOf(projectName).stream()
+ .collect(ImmutableMap.toImmutableMap(c -> c.getUuid().toString(), c -> c));
+
+ // Always backfilling checks to have a meaningful "CombinedCheckState" even when there are some
+ // or all checks missing.
+ ImmutableMap<String, Check> checks =
+ getChecks(projectName, patchSetId, GetCheckOptions.withBackfilling()).stream()
+ .collect(ImmutableMap.toImmutableMap(c -> c.key().checkerUuid().toString(), c -> c));
+
+ ChangeQueryBuilder queryBuilder =
+ queryBuilderProvider.get().asUser(anonymousUserProvider.get());
+ ImmutableListMultimap.Builder<CheckState, Boolean> statesAndRequired =
+ ImmutableListMultimap.builder();
+
+ for (Map.Entry<String, Check> entry : checks.entrySet()) {
+ String checkerUuid = entry.getKey();
+ Check check = entry.getValue();
+
+ Checker checker = allCheckersOfProject.get(checkerUuid);
+ if (checker == null) {
+ // A not-relevant check.
+ statesAndRequired.put(check.state(), false);
+ continue;
+ }
+
+ boolean isRequired =
+ checker.getStatus() == CheckerStatus.ENABLED
+ && checker.isRequired()
+ && checker.isCheckerRelevant(changeData, queryBuilder);
+ statesAndRequired.put(check.state(), isRequired);
+ }
+
+ return CombinedCheckState.combine(statesAndRequired.build());
+ }
+
+ private ImmutableList<Checker> getCheckersForBackfiller(
+ Project.NameKey projectName, List<Check> existingChecks) throws IOException {
+ ImmutableSet<CheckerUuid> checkersWithExistingChecks =
+ existingChecks.stream().map(c -> c.key().checkerUuid()).collect(toImmutableSet());
+ return checkers.checkersOf(projectName).stream()
+ .filter(c -> !checkersWithExistingChecks.contains(c.getUuid()))
+ .collect(toImmutableList());
}
}
diff --git a/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java b/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java
new file mode 100644
index 0000000..30b5b5c
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java
@@ -0,0 +1,107 @@
+// 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.rules;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.common.data.SubmitRecord.Status;
+import com.google.gerrit.common.data.SubmitRequirement;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.api.CombinedCheckState;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.project.SubmitRuleOptions;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.rules.SubmitRule;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.util.Collection;
+
+@Singleton
+public class ChecksSubmitRule implements SubmitRule {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private static final SubmitRequirement DEFAULT_SUBMIT_REQUIREMENT_FOR_CHECKS =
+ SubmitRequirement.builder()
+ .setFallbackText("All required checks must pass")
+ .setType("checks_pass")
+ .build();
+
+ public static class Module extends FactoryModule {
+ @Override
+ public void configure() {
+ bind(SubmitRule.class)
+ .annotatedWith(Exports.named("ChecksSubmitRule"))
+ .to(ChecksSubmitRule.class);
+ }
+ }
+
+ private final Checks checks;
+
+ @Inject
+ public ChecksSubmitRule(Checks checks) {
+ this.checks = checks;
+ }
+
+ @Override
+ public Collection<SubmitRecord> evaluate(ChangeData changeData, SubmitRuleOptions options) {
+ Project.NameKey project = changeData.project();
+ Change.Id changeId = changeData.getId();
+
+ PatchSet.Id currentPathSetId;
+ try {
+ currentPathSetId = changeData.currentPatchSet().getId();
+ } catch (OrmException e) {
+ String errorMessage =
+ String.format("failed to load the current patch set of change %s", changeId);
+ logger.atSevere().withCause(e).log(errorMessage);
+ return singletonRecordForRuleError(errorMessage);
+ }
+
+ CombinedCheckState combinedCheckState;
+ try {
+ combinedCheckState = checks.getCombinedCheckState(project, currentPathSetId);
+ } catch (IOException | OrmException e) {
+ String errorMessage =
+ String.format("failed to evaluate check states for change %s", changeId);
+ logger.atSevere().withCause(e).log(errorMessage);
+ return singletonRecordForRuleError(errorMessage);
+ }
+
+ SubmitRecord submitRecord = new SubmitRecord();
+ if (combinedCheckState.isPassing()) {
+ submitRecord.status = Status.OK;
+ return ImmutableList.of(submitRecord);
+ }
+
+ submitRecord.status = Status.NOT_READY;
+ submitRecord.requirements = ImmutableList.of(DEFAULT_SUBMIT_REQUIREMENT_FOR_CHECKS);
+ return ImmutableSet.of(submitRecord);
+ }
+
+ private static Collection<SubmitRecord> singletonRecordForRuleError(String reason) {
+ SubmitRecord submitRecord = new SubmitRecord();
+ submitRecord.errorMessage = reason;
+ submitRecord.status = SubmitRecord.Status.RULE_ERROR;
+ return ImmutableList.of(submitRecord);
+ }
+}
diff --git a/java/com/google/gerrit/plugins/checks/testing/PendingChecksInfoSubject.java b/java/com/google/gerrit/plugins/checks/testing/PendingChecksInfoSubject.java
new file mode 100644
index 0000000..72c6269
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/testing/PendingChecksInfoSubject.java
@@ -0,0 +1,67 @@
+// 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.testing;
+
+import static com.google.common.truth.Truth.assertAbout;
+
+import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.MapSubject;
+import com.google.common.truth.Subject;
+import com.google.gerrit.plugins.checks.api.CheckablePatchSetInfo;
+import com.google.gerrit.plugins.checks.api.PendingCheckInfo;
+import com.google.gerrit.plugins.checks.api.PendingChecksInfo;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.Project;
+import java.util.Map;
+
+public class PendingChecksInfoSubject extends Subject<PendingChecksInfoSubject, PendingChecksInfo> {
+ public static PendingChecksInfoSubject assertThat(PendingChecksInfo pendingChecksInfo) {
+ return assertAbout(PendingChecksInfoSubject::new).that(pendingChecksInfo);
+ }
+
+ private PendingChecksInfoSubject(FailureMetadata metadata, PendingChecksInfo actual) {
+ super(metadata, actual);
+ }
+
+ public void hasProject(Project.NameKey expectedProject) {
+ check("patchSet().project()").that(patchSet().project).isEqualTo(expectedProject.get());
+ }
+
+ public void hasPatchSet(PatchSet.Id expectedPatchSetId) {
+ CheckablePatchSetInfo patchSet = patchSet();
+ check("patchSet().changeNumber()")
+ .that(patchSet.changeNumber)
+ .isEqualTo(expectedPatchSetId.getParentKey().get());
+ check("patchSet().id()").that(patchSet.patchSetId).isEqualTo(expectedPatchSetId.get());
+ }
+
+ public MapSubject hasPendingChecksMapThat() {
+ return check("pendingChecks()").that(pendingChecks());
+ }
+
+ private CheckablePatchSetInfo patchSet() {
+ isNotNull();
+ CheckablePatchSetInfo patchSet = actual().patchSet;
+ check("patchSet()").that(patchSet).isNotNull();
+ return patchSet;
+ }
+
+ private Map<String, PendingCheckInfo> pendingChecks() {
+ isNotNull();
+ Map<String, PendingCheckInfo> pendingChecks = actual().pendingChecks;
+ check("pendingChecks()").that(pendingChecks).isNotNull();
+ return pendingChecks;
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/checks/BUILD b/javatests/com/google/gerrit/plugins/checks/BUILD
index d87309f..814e5b8 100644
--- a/javatests/com/google/gerrit/plugins/checks/BUILD
+++ b/javatests/com/google/gerrit/plugins/checks/BUILD
@@ -11,6 +11,7 @@
"//java/com/google/gerrit/index:query_exception",
"//java/com/google/gerrit/reviewdb:server",
"//java/com/google/gerrit/server",
+ "//java/com/google/gerrit/server/util/time",
"//java/com/google/gerrit/testing:gerrit-test-util",
"//lib:guava",
"//lib/guice",
diff --git a/javatests/com/google/gerrit/plugins/checks/CheckerDefinitionTest.java b/javatests/com/google/gerrit/plugins/checks/CheckerDefinitionTest.java
new file mode 100644
index 0000000..7b9481e
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/CheckerDefinitionTest.java
@@ -0,0 +1,60 @@
+// 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.plugins.checks.api.BlockingCondition.STATE_NOT_PASSING;
+
+import com.google.common.collect.ImmutableSortedSet;
+import com.google.gerrit.plugins.checks.api.BlockingCondition;
+import com.google.gerrit.plugins.checks.api.CheckerStatus;
+import com.google.gerrit.reviewdb.client.Project.NameKey;
+import com.google.gerrit.server.util.time.TimeUtil;
+import java.util.EnumSet;
+import org.eclipse.jgit.lib.ObjectId;
+import org.junit.Test;
+
+public class CheckerDefinitionTest {
+
+ @Test
+ public void notRequiredIfNoBlockingCondition() {
+ Checker checker = newChecker().setBlockingConditions(ImmutableSortedSet.of()).build();
+
+ assertThat(checker.isRequired()).isFalse();
+ }
+
+ @Test
+ public void requiredIfHasBlockingConditionStateNotPassing() {
+ Checker checker =
+ newChecker().setBlockingConditions(ImmutableSortedSet.of(STATE_NOT_PASSING)).build();
+
+ assertThat(checker.isRequired()).isTrue();
+ }
+
+ @Test
+ public void allBlockingConditionsConsidered() {
+ assertThat(EnumSet.allOf(BlockingCondition.class)).containsExactly(STATE_NOT_PASSING);
+ }
+
+ private Checker.Builder newChecker() {
+ return Checker.builder()
+ .setRepository(new NameKey("test-repo"))
+ .setStatus(CheckerStatus.ENABLED)
+ .setUuid(CheckerUuid.parse("schema:any-id"))
+ .setCreatedOn(TimeUtil.nowTs())
+ .setUpdatedOn(TimeUtil.nowTs())
+ .setRefState(ObjectId.zeroId());
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/CheckerRefsIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/CheckerRefsIT.java
index 478eed1..af4ad01 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/CheckerRefsIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/CheckerRefsIT.java
@@ -26,6 +26,7 @@
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.checks.CheckerRef;
import com.google.gerrit.plugins.checks.CheckerUuid;
+import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.notedb.Sequences;
@@ -101,9 +102,9 @@
@Test
public void submitToCheckerRefsIsDisabled() throws Exception {
- CheckerUuid checkerUuid = checkerOperations.newChecker().create();
+ CheckerUuid checkerUuid =
+ checkerOperations.newChecker().status(CheckerStatus.DISABLED).create();
String checkerRef = checkerUuid.toRefName();
-
String changeId = createChangeWithoutCommitValidation(checkerRef);
grantLabel(
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/CheckersIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/CheckersIT.java
index 9e47902..d351813 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/CheckersIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/CheckersIT.java
@@ -19,6 +19,7 @@
import com.google.gerrit.plugins.checks.Checker;
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.Checkers;
+import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.reviewdb.client.Project;
import java.util.stream.Stream;
import org.junit.Test;
@@ -45,19 +46,19 @@
@Test
public void checkersOfOmitsDisabledCheckers() throws Exception {
+ // Creates a disabled checker.
+ checkerOperations.newChecker().repository(project).status(CheckerStatus.DISABLED).create();
+ // Creates an enabled checker and then disabled it by an update.
CheckerUuid checkerUuid1 = checkerOperations.newChecker().repository(project).create();
- CheckerUuid checkerUuid2 = checkerOperations.newChecker().repository(project).create();
checkerOperations.checker(checkerUuid1).forUpdate().disable().update();
+ // Creates an enabled checker.
+ CheckerUuid checkerUuid2 = checkerOperations.newChecker().repository(project).create();
assertThat(getCheckerUuidsOf(project)).containsExactly(checkerUuid2);
}
private Stream<CheckerUuid> getCheckerUuidsOf(Project.NameKey projectName) throws Exception {
- return plugin
- .getSysInjector()
- .getInstance(Checkers.class)
- .checkersOf(projectName)
- .stream()
+ return plugin.getSysInjector().getInstance(Checkers.class).checkersOf(projectName).stream()
.map(Checker::getUuid);
}
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/BUILD b/javatests/com/google/gerrit/plugins/checks/acceptance/api/BUILD
index 7db608a..b341d12 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/BUILD
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/BUILD
@@ -13,5 +13,6 @@
"//plugins/checks:checks__plugin",
"//plugins/checks/java/com/google/gerrit/plugins/checks/acceptance",
"//plugins/checks/java/com/google/gerrit/plugins/checks/acceptance/testsuite",
+ "//plugins/checks/java/com/google/gerrit/plugins/checks/testing",
],
)
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java
new file mode 100644
index 0000000..771a581
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java
@@ -0,0 +1,117 @@
+// 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.acceptance.api;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
+import static com.google.gerrit.plugins.checks.api.BlockingCondition.STATE_NOT_PASSING;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.PluginDefinedInfo;
+import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.CheckerUuid;
+import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
+import com.google.gerrit.plugins.checks.api.ChangeCheckInfo;
+import com.google.gerrit.plugins.checks.api.CheckState;
+import com.google.gerrit.plugins.checks.api.CombinedCheckState;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import java.util.Optional;
+import org.junit.Before;
+import org.junit.Test;
+
+public class ChangeCheckInfoIT extends AbstractCheckersTest {
+ private Change.Id changeId;
+ private PatchSet.Id psId;
+
+ @Before
+ public void setUp() throws Exception {
+ psId = createChange().getPatchSetId();
+ changeId = psId.getParentKey();
+ }
+
+ @Test
+ public void infoRequiresOption() throws Exception {
+ assertThat(
+ getChangeCheckInfo(gApi.changes().id(changeId.get()).get(ImmutableListMultimap.of())))
+ .isEmpty();
+ assertThat(getChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+ }
+
+ @Test
+ public void noCheckers() throws Exception {
+ assertThat(checkerOperations.checkersOf(project)).isEmpty();
+ assertThat(getChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+ }
+
+ @Test
+ public void backfillRelevantChecker() throws Exception {
+ checkerOperations.newChecker().repository(project).query("topic:foo").create();
+ assertThat(getChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+ gApi.changes().id(changeId.get()).topic("foo");
+ assertThat(getChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.IN_PROGRESS));
+ }
+
+ @Test
+ public void combinedCheckState() throws Exception {
+ CheckerUuid optionalCheckerUuid = checkerOperations.newChecker().repository(project).create();
+ CheckerUuid requiredCheckerUuid =
+ checkerOperations
+ .newChecker()
+ .repository(project)
+ .blockingConditions(STATE_NOT_PASSING)
+ .create();
+ assertThat(getChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.IN_PROGRESS));
+ checkOperations
+ .newCheck(CheckKey.create(project, psId, optionalCheckerUuid))
+ .setState(CheckState.SUCCESSFUL)
+ .upsert();
+ assertThat(getChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.IN_PROGRESS));
+ checkOperations
+ .newCheck(CheckKey.create(project, psId, requiredCheckerUuid))
+ .setState(CheckState.FAILED)
+ .upsert();
+ assertThat(getChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.FAILED));
+ }
+
+ private Optional<ChangeCheckInfo> getChangeCheckInfo(Change.Id id) throws Exception {
+ return getChangeCheckInfo(
+ gApi.changes().id(id.get()).get(ImmutableListMultimap.of("checks--combined", "true")));
+ }
+
+ private static Optional<ChangeCheckInfo> getChangeCheckInfo(ChangeInfo changeInfo) {
+ if (changeInfo.plugins == null) {
+ return Optional.empty();
+ }
+ ImmutableList<PluginDefinedInfo> infos =
+ changeInfo.plugins.stream().filter(i -> i.name.equals("checks")).collect(toImmutableList());
+ if (infos.isEmpty()) {
+ return Optional.empty();
+ }
+ assertThat(infos).hasSize(1);
+ assertThat(infos.get(0)).isInstanceOf(ChangeCheckInfo.class);
+ return Optional.of((ChangeCheckInfo) infos.get(0));
+ }
+}
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 9f4b267..52c7334 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
@@ -48,8 +48,7 @@
.url("http://example.com/my-checker")
.create();
List<CheckerInfo> expectedCheckerInfos =
- ImmutableList.of(checkerUuid1, checkerUuid2, checkerUuid3)
- .stream()
+ ImmutableList.of(checkerUuid1, checkerUuid2, checkerUuid3).stream()
.sorted()
.map(uuid -> checkerOperations.checker(uuid).asInfo())
.collect(toList());
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 eded791..ec925bc 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
@@ -85,15 +85,14 @@
Collection<CheckInfo> info =
checksApiFactory.revision(patchSetId).list(ListChecksOption.CHECKER);
- Timestamp psCreated = getPatchSetCreated(patchSetId.getParentKey());
CheckInfo expected1 = new CheckInfo();
expected1.project = checkKey1.project().get();
expected1.changeNumber = checkKey1.patchSet().getParentKey().get();
expected1.patchSetId = checkKey1.patchSet().get();
expected1.checkerUuid = checkKey1.checkerUuid().toString();
expected1.state = CheckState.RUNNING;
- expected1.created = psCreated;
- expected1.updated = psCreated;
+ expected1.created = checkOperations.check(checkKey1).get().created();
+ expected1.updated = expected1.created;
expected1.blocking = ImmutableSet.of();
expected1.checkerStatus = CheckerStatus.ENABLED;
@@ -103,8 +102,8 @@
expected2.patchSetId = checkKey2.patchSet().get();
expected2.checkerUuid = checkKey2.checkerUuid().toString();
expected2.state = CheckState.RUNNING;
- expected2.created = psCreated;
- expected2.updated = psCreated;
+ expected2.created = checkOperations.check(checkKey2).get().created();
+ expected2.updated = expected2.created;
expected2.checkerName = "Checker Two";
expected2.blocking = ImmutableSet.of();
expected2.checkerStatus = CheckerStatus.ENABLED;
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListPendingChecksIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListPendingChecksIT.java
index 7cb8d53..5c7d2d4 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListPendingChecksIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListPendingChecksIT.java
@@ -15,24 +15,61 @@
package com.google.gerrit.plugins.checks.acceptance.api;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.plugins.checks.testing.PendingChecksInfoSubject.assertThat;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static org.hamcrest.CoreMatchers.instanceOf;
+import com.google.common.collect.Iterables;
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.common.data.Permission;
import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
+import com.google.gerrit.plugins.checks.api.CheckState;
+import com.google.gerrit.plugins.checks.api.PendingCheckInfo;
+import com.google.gerrit.plugins.checks.api.PendingChecksInfo;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.project.testing.Util;
+import com.google.gerrit.testing.TestTimeUtil;
import com.google.inject.Inject;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.junit.After;
+import org.junit.Before;
import org.junit.Test;
public class ListPendingChecksIT extends AbstractCheckersTest {
@Inject private RequestScopeOperations requestScopeOperations;
+ private PatchSet.Id patchSetId;
+
+ @Before
+ public void setUp() throws Exception {
+ TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
+ TestTimeUtil.setClock(Timestamp.from(Instant.EPOCH));
+
+ patchSetId = createChange().getPatchSetId();
+ }
+
+ @After
+ public void resetTime() {
+ TestTimeUtil.useSystemTime();
+ }
+
@Test
- public void specifyingEitherCheckerUuidOrSchemeIsRequired() throws Exception {
- exception.expect(BadRequestException.class);
- exception.expectMessage("checker or scheme is required");
- pendingChecksApi.listForScheme(null);
+ public void specifyingCheckerUuidIsRequired() throws Exception {
+ // The extension API doesn't allow to not specify a checker UUID. Call the endpoint over REST to
+ // test this.
+ RestResponse response = adminRestSession.get("/plugins/checks/checks.pending/");
+ response.assertBadRequest();
+ assertThat(response.getEntityContent()).isEqualTo("checker UUID is required");
}
@Test
@@ -43,37 +80,306 @@
}
@Test
- public void cannotSpecifyCheckerUuidAndScheme() throws Exception {
- // The extension API doesn't allow to specify checker UUID and scheme at the same time. Call the
- // endpoint over REST to test this.
- RestResponse response =
- adminRestSession.get(
- String.format(
- "/plugins/checks/checks.pending/?checker=%s&scheme=%s", "foo:bar", "foo"));
- response.assertBadRequest();
- assertThat(response.getEntityContent()).isEqualTo("checker and scheme are mutually exclusive");
+ public void cannotListPendingChecksForNonExistingChecker() throws Exception {
+ exception.expect(UnprocessableEntityException.class);
+ exception.expectMessage("checker non:existing not found");
+ pendingChecksApi.list("non:existing");
}
@Test
- public void cannotListPendingChecksWithoutAdministrateCheckers() throws Exception {
+ public void listPendingChecksNotStartedStateAssumed() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
+ // Create a check with state "NOT_STARTED" that we expect to be returned.
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ // Create a check with state "FAILED" that we expect to be ignored.
+ PatchSet.Id patchSetId2 = createChange().getPatchSetId();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId2, checkerUuid))
+ .setState(CheckState.FAILED)
+ .upsert();
+
+ // Create a check with state "NOT_STARTED" for other checker that we expect to be ignored.
+ CheckerUuid checkerUuid2 = checkerOperations.newChecker().repository(project).create();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid2))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ List<PendingChecksInfo> pendingChecksList = pendingChecksApi.list(checkerUuid);
+ assertThat(pendingChecksList).hasSize(1);
+ PendingChecksInfo pendingChecks = Iterables.getOnlyElement(pendingChecksList);
+ assertThat(pendingChecks).hasProject(project);
+ assertThat(pendingChecks).hasPatchSet(patchSetId);
+ assertThat(pendingChecks)
+ .hasPendingChecksMapThat()
+ .containsExactly(checkerUuid.toString(), new PendingCheckInfo(CheckState.NOT_STARTED));
+ }
+
+ @Test
+ public void listPendingChecksForSpecifiedState() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
+ // Create a check with state "FAILED" that we expect to be returned.
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.FAILED)
+ .upsert();
+
+ // Create a check with state "NOT_STARTED" that we expect to be ignored.
+ PatchSet.Id patchSetId2 = createChange().getPatchSetId();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId2, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ // Create a check with state "FAILED" for other checker that we expect to be ignored.
+ CheckerUuid checkerUuid2 = checkerOperations.newChecker().repository(project).create();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid2))
+ .setState(CheckState.FAILED)
+ .upsert();
+
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.FAILED);
+ assertThat(pendingChecksList).hasSize(1);
+ PendingChecksInfo pendingChecks = Iterables.getOnlyElement(pendingChecksList);
+ assertThat(pendingChecks).hasProject(project);
+ assertThat(pendingChecks).hasPatchSet(patchSetId);
+ assertThat(pendingChecks)
+ .hasPendingChecksMapThat()
+ .containsExactly(checkerUuid.toString(), new PendingCheckInfo(CheckState.FAILED));
+ }
+
+ @Test
+ public void listPendingChecksForMultipleSpecifiedStates() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
+ // Create a check with state "NOT_STARTED" that we expect to be returned.
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ // Create a check with state "SCHEDULED" that we expect to be returned.
+ PatchSet.Id patchSetId2 = createChange().getPatchSetId();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId2, checkerUuid))
+ .setState(CheckState.SCHEDULED)
+ .upsert();
+
+ // Create a check with state "SUCCESSFUL" that we expect to be ignored.
+ PatchSet.Id patchSetId3 = createChange().getPatchSetId();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId3, checkerUuid))
+ .setState(CheckState.SUCCESSFUL)
+ .upsert();
+
+ // Create a check with state "NOT_STARTED" for other checker that we expect to be ignored.
+ CheckerUuid checkerUuid2 = checkerOperations.newChecker().repository(project).create();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid2))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED, CheckState.SCHEDULED);
+ assertThat(pendingChecksList).hasSize(2);
+
+ // The sorting of the pendingChecksList matches the sorting in which the matching changes are
+ // returned from the change index, which is by last updated timestamp. Use this knowledge here
+ // to do the assertions although the REST endpoint doesn't document a guaranteed sort order.
+ PendingChecksInfo pendingChecksChange2 = pendingChecksList.get(0);
+ assertThat(pendingChecksChange2).hasProject(project);
+ assertThat(pendingChecksChange2).hasPatchSet(patchSetId2);
+ assertThat(pendingChecksChange2)
+ .hasPendingChecksMapThat()
+ .containsExactly(checkerUuid.toString(), new PendingCheckInfo(CheckState.SCHEDULED));
+
+ PendingChecksInfo pendingChecksChange1 = pendingChecksList.get(1);
+ assertThat(pendingChecksChange1).hasProject(project);
+ assertThat(pendingChecksChange1).hasPatchSet(patchSetId);
+ assertThat(pendingChecksChange1)
+ .hasPendingChecksMapThat()
+ .containsExactly(checkerUuid.toString(), new PendingCheckInfo(CheckState.NOT_STARTED));
+ }
+
+ @Test
+ public void backfillForApplyingChecker() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ List<PendingChecksInfo> pendingChecksList = pendingChecksApi.list(checkerUuid);
+ assertThat(pendingChecksList).hasSize(1);
+ PendingChecksInfo pendingChecks = Iterables.getOnlyElement(pendingChecksList);
+ assertThat(pendingChecks).hasProject(project);
+ assertThat(pendingChecks).hasPatchSet(patchSetId);
+ assertThat(pendingChecks)
+ .hasPendingChecksMapThat()
+ .containsExactly(checkerUuid.toString(), new PendingCheckInfo(CheckState.NOT_STARTED));
+ }
+
+ @Test
+ public void noBackfillForCheckerThatDoesNotApplyToTheProject() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(allProjects).create();
+ assertThat(pendingChecksApi.list(checkerUuid)).isEmpty();
+ }
+
+ @Test
+ public void noBackfillForCheckerThatDoesNotApplyToTheChange() throws Exception {
+ CheckerUuid checkerUuid =
+ checkerOperations.newChecker().repository(project).query("message:not-matching").create();
+ assertThat(pendingChecksApi.list(checkerUuid)).isEmpty();
+ }
+
+ @Test
+ public void listPendingChecksForDisabledChecker() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).isNotEmpty();
+
+ checkerOperations.checker(checkerUuid).forUpdate().disable().update();
+ pendingChecksList = pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).isEmpty();
+ }
+
+ @Test
+ public void listPendingChecksFiltersOutChecksForClosedChangesIfQueryDoesntSpecifyStatus()
+ throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).query("").create();
+
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ PatchSet.Id patchSetId2 = createChange().getPatchSetId();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId2, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).hasSize(2);
+
+ gApi.changes().id(patchSetId2.getParentKey().toString()).abandon();
+
+ pendingChecksList = pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).hasSize(1);
+ }
+
+ @Test
+ public void listPendingChecksReturnsChecksForClosedChangesIfQuerySpecifiesStatus()
+ throws Exception {
+ CheckerUuid checkerUuid =
+ checkerOperations.newChecker().repository(project).query("is:open OR is:closed").create();
+
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ PatchSet.Id patchSetId2 = createChange().getPatchSetId();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId2, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).hasSize(2);
+
+ gApi.changes().id(patchSetId2.getParentKey().toString()).abandon();
+
+ pendingChecksList = pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).hasSize(2);
+ }
+
+ @Test
+ public void listPendingChecksForInvalidCheckerFails() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ checkerOperations.checker(checkerUuid).forUpdate().forceInvalidConfig().update();
+
+ exception.expect(RestApiException.class);
+ exception.expectMessage("Cannot list pending checks");
+ exception.expectCause(instanceOf(ConfigInvalidException.class));
+ pendingChecksApi.list(checkerUuid);
+ }
+
+ @Test
+ public void listPendingChecksWithoutAdministrateCheckersCapabilityWorks() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
requestScopeOperations.setApiUser(user.getId());
-
- exception.expect(AuthException.class);
- exception.expectMessage("not permitted");
- pendingChecksApi.list("foo:bar");
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).hasSize(1);
+ PendingChecksInfo pendingChecks = Iterables.getOnlyElement(pendingChecksList);
+ assertThat(pendingChecks).hasProject(project);
+ assertThat(pendingChecks).hasPatchSet(patchSetId);
+ assertThat(pendingChecks)
+ .hasPendingChecksMapThat()
+ .containsExactly(checkerUuid.toString(), new PendingCheckInfo(CheckState.NOT_STARTED));
}
@Test
- public void listPendingChecksForCheckerNotImplemented() throws Exception {
- exception.expect(MethodNotAllowedException.class);
- exception.expectMessage("not implemented");
- pendingChecksApi.list("foo:bar");
+ public void pendingChecksDontIncludeChecksForNonVisibleChanges() throws Exception {
+ // restrict project visibility so that it is only visible to administrators
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ Util.allow(u.getConfig(), Permission.READ, adminGroupUuid(), "refs/*");
+ Util.block(u.getConfig(), Permission.READ, REGISTERED_USERS, "refs/*");
+ u.save();
+ }
+
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ // Check is returned for admin user.
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).isNotEmpty();
+
+ // Check is not returned for non-admin user.
+ requestScopeOperations.setApiUser(user.getId());
+ pendingChecksList = pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).isEmpty();
}
@Test
- public void listPendingChecksForSchemeNotImplemented() throws Exception {
- exception.expect(MethodNotAllowedException.class);
- exception.expectMessage("not implemented");
- pendingChecksApi.listForScheme("foo");
+ public void pendingChecksDontIncludeChecksForPrivateChangesOfOtherUsers() throws Exception {
+ // make change private so that it is only visible to the admin user
+ gApi.changes().id(patchSetId.getParentKey().get()).setPrivate(true);
+
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ // Check is returned for admin user.
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).isNotEmpty();
+
+ // Check is not returned for non-admin user.
+ requestScopeOperations.setApiUser(user.getId());
+ pendingChecksList = pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).isEmpty();
}
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/db/BUILD b/javatests/com/google/gerrit/plugins/checks/acceptance/db/BUILD
new file mode 100644
index 0000000..03caa7d
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/db/BUILD
@@ -0,0 +1,17 @@
+package(default_visibility = ["//plugins/checks:visibility"])
+
+load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests")
+
+acceptance_tests(
+ srcs = glob(["*IT.java"]),
+ group = "get_combined_check_state",
+ labels = [
+ "get_combined_check_state",
+ ],
+ deps = [
+ "//java/com/google/gerrit/git/testing",
+ "//plugins/checks:checks__plugin",
+ "//plugins/checks/java/com/google/gerrit/plugins/checks/acceptance",
+ "//plugins/checks/java/com/google/gerrit/plugins/checks/acceptance/testsuite",
+ ],
+)
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/db/GetCombinedCheckStateIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/db/GetCombinedCheckStateIT.java
new file mode 100644
index 0000000..0eb82ae
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/db/GetCombinedCheckStateIT.java
@@ -0,0 +1,121 @@
+// 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.acceptance.db;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.CheckerUuid;
+import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
+import com.google.gerrit.plugins.checks.acceptance.testsuite.TestCheckerCreation;
+import com.google.gerrit.plugins.checks.api.BlockingCondition;
+import com.google.gerrit.plugins.checks.api.CheckState;
+import com.google.gerrit.plugins.checks.api.CheckerStatus;
+import com.google.gerrit.plugins.checks.api.CombinedCheckState;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import org.junit.Before;
+import org.junit.Test;
+
+public class GetCombinedCheckStateIT extends AbstractCheckersTest {
+ // Tests against Gerrit Java API once it exists.
+ private Checks checks;
+ private PatchSet.Id patchSetId;
+
+ @Before
+ public void setUp() throws Exception {
+ checks = plugin.getHttpInjector().getInstance(Checks.class);
+
+ patchSetId = createChange().getPatchSetId();
+ }
+
+ @Test
+ public void returnsNotRelevantWhenNoCheck() throws Exception {
+ CombinedCheckState combinedCheckState = checks.getCombinedCheckState(project, patchSetId);
+
+ assertThat(combinedCheckState).isEqualTo(CombinedCheckState.NOT_RELEVANT);
+ }
+
+ @Test
+ public void returnsSuccessfulWhenForSuccessfulCheckWhoseCheckerIsDisabled() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().status(CheckerStatus.DISABLED).create();
+ setCheckSuccessful(checkerUuid);
+
+ CombinedCheckState combinedCheckState = checks.getCombinedCheckState(project, patchSetId);
+
+ assertThat(combinedCheckState).isEqualTo(CombinedCheckState.SUCCESSFUL);
+ }
+
+ @Test
+ public void returnsWarningForFailedCheckWhoseCheckerIsNotRelevant() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().query("status:merged").create();
+ setCheckState(checkerUuid, CheckState.FAILED);
+
+ CombinedCheckState combinedCheckState = checks.getCombinedCheckState(project, patchSetId);
+
+ assertThat(combinedCheckState).isEqualTo(CombinedCheckState.WARNING);
+ }
+
+ @Test
+ public void returnsFailedWhenAnyRequiredCheckerFailed() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().create();
+ setCheckSuccessful(checkerUuid);
+ CheckerUuid otherCheckerUuid = newRequiredChecker().create();
+ setCheckState(otherCheckerUuid, CheckState.FAILED);
+
+ CombinedCheckState combinedCheckState = checks.getCombinedCheckState(project, patchSetId);
+
+ assertThat(combinedCheckState).isEqualTo(CombinedCheckState.FAILED);
+ }
+
+ @Test
+ public void returnsSuccessfulWhenAllRequiredCheckersSucceeded() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().create();
+ setCheckSuccessful(checkerUuid);
+ CheckerUuid otherCheckerUuid = newRequiredChecker().create();
+ setCheckSuccessful(otherCheckerUuid);
+
+ CombinedCheckState combinedCheckState = checks.getCombinedCheckState(project, patchSetId);
+
+ assertThat(combinedCheckState).isEqualTo(CombinedCheckState.SUCCESSFUL);
+ }
+
+ @Test
+ public void returnsInProgressWithOnlyBackFilledCheck() throws Exception {
+ newRequiredChecker().create();
+ // No check state is created.
+
+ CombinedCheckState combinedCheckState = checks.getCombinedCheckState(project, patchSetId);
+
+ assertThat(combinedCheckState).isEqualTo(CombinedCheckState.IN_PROGRESS);
+ }
+
+ private TestCheckerCreation.Builder newRequiredChecker() {
+ return checkerOperations
+ .newChecker()
+ .repository(project)
+ .status(CheckerStatus.ENABLED)
+ .blockingConditions(BlockingCondition.STATE_NOT_PASSING);
+ }
+
+ private void setCheckSuccessful(CheckerUuid checkerUuid) {
+ setCheckState(checkerUuid, CheckState.SUCCESSFUL);
+ }
+
+ private void setCheckState(CheckerUuid checkerUuid, CheckState checkState) {
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
+ checkOperations.newCheck(checkKey).setState(checkState).upsert();
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/rules/BUILD b/javatests/com/google/gerrit/plugins/checks/acceptance/rules/BUILD
new file mode 100644
index 0000000..61d2a35
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/rules/BUILD
@@ -0,0 +1,16 @@
+load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests")
+
+acceptance_tests(
+ srcs = glob(["*IT.java"]),
+ group = "checks_submit_rules",
+ labels = [
+ "checks_submit_rules",
+ ],
+ deps = [
+ "//java/com/google/gerrit/git/testing",
+ "//java/com/google/gerrit/server/util/time",
+ "//plugins/checks:checks__plugin",
+ "//plugins/checks/java/com/google/gerrit/plugins/checks/acceptance",
+ "//plugins/checks/java/com/google/gerrit/plugins/checks/acceptance/testsuite",
+ ],
+)
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/rules/ChecksSubmitRuleIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/rules/ChecksSubmitRuleIT.java
new file mode 100644
index 0000000..32b8b03
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/rules/ChecksSubmitRuleIT.java
@@ -0,0 +1,162 @@
+// 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.acceptance.rules;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.SubmitRequirementInfo;
+import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.CheckerUuid;
+import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
+import com.google.gerrit.plugins.checks.acceptance.testsuite.TestCheckerCreation;
+import com.google.gerrit.plugins.checks.api.BlockingCondition;
+import com.google.gerrit.plugins.checks.api.CheckState;
+import com.google.gerrit.plugins.checks.api.CheckerStatus;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.config.AllProjectsName;
+import com.google.inject.Inject;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * TODO(xchangcheng): add more tests after figuring out the expecting behavior of {@code
+ * CombinedCheckState}.
+ */
+public class ChecksSubmitRuleIT extends AbstractCheckersTest {
+ private static final SubmitRequirementInfo SUBMIT_REQUIREMENT_INFO =
+ new SubmitRequirementInfo(
+ "NOT_READY", "All required checks must pass", "checks_pass", ImmutableMap.of());
+
+ @Inject private AllProjectsName allProjects;
+ private String testChangeId;
+ private PatchSet.Id testPatchSetId;
+
+ @Before
+ public void setUp() throws Exception {
+ PushOneCommit.Result result = createChange();
+ testPatchSetId = result.getPatchSetId();
+ testChangeId = result.getChangeId();
+
+ // Approves "Code-Review" label so that the change only needs to meet the submit requirements
+ // about checks.
+ approve(testChangeId);
+ }
+
+ @Test
+ public void nonApplicableCheckerNotBlockingSubmit() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().create();
+ postCheckResult(checkerUuid, CheckState.FAILED);
+ // Updates the checker so that it isn't applicable to the change any more.
+ checkerOperations.checker(checkerUuid).forUpdate().repository(allProjects).update();
+
+ ChangeInfo changeInfo = gApi.changes().id(testChangeId).get();
+
+ assertThat(changeInfo.submittable).isTrue();
+ assertThat(changeInfo.requirements).isEmpty();
+ }
+
+ @Test
+ public void disabledCheckerDoesNotBlockingSubmit() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().create();
+ postCheckResult(checkerUuid, CheckState.FAILED);
+ checkerOperations.checker(checkerUuid).forUpdate().disable().update();
+
+ ChangeInfo changeInfo = gApi.changes().id(testChangeId).get();
+
+ assertThat(changeInfo.submittable).isTrue();
+ assertThat(changeInfo.requirements).isEmpty();
+ }
+
+ // @Test
+ // public void enabledCheckerNotBlockingSubmitIfNotRequired() throws Exception {
+ // CheckerUuid checkerUuid = newRequiredChecker().create();
+ // postCheckResult(checkerUuid, CheckState.FAILED);
+ // checkerOperations
+ // .checker(checkerUuid)
+ // .forUpdate()
+ // .blockingConditions(ImmutableSortedSet.of())
+ // .update();
+ //
+ // ChangeInfo changeInfo = gApi.changes().id(testChangeId).get();
+ //
+ // assertThat(changeInfo.submittable).isTrue();
+ // }
+
+ @Test
+ public void enabledCheckerNotBlockingSubmitIfNotInBlockingState() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().create();
+ postCheckResult(checkerUuid, CheckState.SUCCESSFUL);
+
+ ChangeInfo changeInfo = gApi.changes().id(testChangeId).get();
+
+ assertThat(changeInfo.submittable).isTrue();
+ assertThat(changeInfo.requirements).isEmpty();
+ }
+
+ @Test
+ public void enabledCheckerBlockingSubmitIfInBlockingState() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().create();
+ postCheckResult(checkerUuid, CheckState.FAILED);
+
+ ChangeInfo changeInfo = gApi.changes().id(testChangeId).get();
+
+ assertThat(changeInfo.submittable).isFalse();
+ assertThat(changeInfo.requirements).containsExactly(SUBMIT_REQUIREMENT_INFO);
+ }
+
+ @Test
+ public void multipleCheckerBlockingSubmit() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().create();
+ // Two enabled and required checkers. They are blocking if any of them isn't passing.
+ CheckerUuid testCheckerUuid2 = newRequiredChecker().create();
+ postCheckResult(checkerUuid, CheckState.SUCCESSFUL);
+ postCheckResult(testCheckerUuid2, CheckState.FAILED);
+
+ ChangeInfo changeInfo = gApi.changes().id(testChangeId).get();
+
+ assertThat(changeInfo.submittable).isFalse();
+ assertThat(changeInfo.requirements).containsExactly(SUBMIT_REQUIREMENT_INFO);
+ }
+
+ // @Test
+ // public void multipleCheckerNotBlockingSubmit() throws Exception {
+ // CheckerUuid checkerUuid = newRequiredChecker().create();
+ // // Two enabled checkers. The one failed doesn't block because it's not required.
+ // CheckerUuid testCheckerUuidNotRequired =
+ // newRequiredChecker().blockingConditions(ImmutableSortedSet.of()).create();
+ // postCheckResult(testCheckerUuidNotRequired, CheckState.FAILED);
+ // postCheckResult(checkerUuid, CheckState.SUCCESSFUL);
+ //
+ // ChangeInfo changeInfo = gApi.changes().id(testChangeId).get();
+ //
+ // assertThat(changeInfo.submittable).isTrue();
+ // }
+
+ private TestCheckerCreation.Builder newRequiredChecker() {
+ return checkerOperations
+ .newChecker()
+ .repository(project)
+ .status(CheckerStatus.ENABLED)
+ .blockingConditions(BlockingCondition.STATE_NOT_PASSING);
+ }
+
+ private void postCheckResult(CheckerUuid checkerUuid, CheckState checkState) {
+ CheckKey checkKey = CheckKey.create(project, testPatchSetId, checkerUuid);
+ checkOperations.newCheck(checkKey).setState(checkState).upsert();
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/checks/api/BlockingConditionTest.java b/javatests/com/google/gerrit/plugins/checks/api/BlockingConditionTest.java
new file mode 100644
index 0000000..a0e1b22
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/api/BlockingConditionTest.java
@@ -0,0 +1,30 @@
+// 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.api;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.testing.GerritBaseTests;
+import org.junit.Test;
+
+public class BlockingConditionTest extends GerritBaseTests {
+ @Test
+ public void isRequired() {
+ assertThat(BlockingCondition.isRequired(ImmutableSet.of())).isFalse();
+ assertThat(BlockingCondition.isRequired(ImmutableSet.of(BlockingCondition.STATE_NOT_PASSING)))
+ .isTrue();
+ }
+}
diff --git a/src/main/resources/Documentation/about.md b/src/main/resources/Documentation/about.md
new file mode 100644
index 0000000..b7be92e
--- /dev/null
+++ b/src/main/resources/Documentation/about.md
@@ -0,0 +1,2 @@
+The @PLUGIN@ plugin provides a REST API and UI extensions for integrating CI
+systems with Gerrit.
diff --git a/src/main/resources/Documentation/rest-api-changes.md b/src/main/resources/Documentation/rest-api-changes.md
new file mode 100644
index 0000000..904fc16
--- /dev/null
+++ b/src/main/resources/Documentation/rest-api-changes.md
@@ -0,0 +1,43 @@
+# /changes REST API
+
+This page describes additions to the Gerrit change-related REST endpoints that
+are added by the @PLUGIN@ plugin.
+
+Please also take note of the general information on the
+[changes REST API](../../../Documentation/rest-api-changes.html).
+
+### <a id="get-change"> Get Change options
+_'GET /changes/[\{change-id\}](../../../Documentation/rest-api-changes.html#change-id)?@PLUGIN@--combined'_
+
+Adding the query parameter `@PLUGIN@--combined` adds a `CheckChangeInfo` entity
+to the `plugins` list in
+[`ChangeInfo`](../../../Documentation/rest-api-changes.html#change-info). All
+fields reflect up-to-date information read from primary storage, not a secondary
+index.
+
+### <a id="check-change-info"> CheckChangeInfo
+
+The `CheckChangeInfo` describes check information on a change.
+
+| Field Name | Description |
+| ---------------------- | ----------- |
+| `combined_check_state` | The [combined state](#combined-check-state) of all checks on the change.
+
+### <a id="combined-check-state"> CombinedCheckState (enum)
+
+The `CheckState` enum can have the following values:
+
+* `FAILED`: At least one required check failed; other checks may have passed, or
+ still be running.
+
+* `WARNING`: All relevant checks terminated, and at least one optional check
+ failed, but no required checks failed.
+
+* `IN_PROGRESS`: At least one relevant check is in a non-terminated
+ [state](rest-api-checks.md#check-state) (`NOT_STARTED`, `SCHEDULED`,
+ `RUNNING`), and no required checks failed. Some optional checks may have
+ failed.
+
+* `SUCCESSFUL`: All relevant checks terminated successfully.
+
+* `NOT_RELEVANT:` No checks are relevant to this change.
diff --git a/src/main/resources/Documentation/rest-api-pending-checks.md b/src/main/resources/Documentation/rest-api-pending-checks.md
index 39e2a1a..466dff3 100644
--- a/src/main/resources/Documentation/rest-api-pending-checks.md
+++ b/src/main/resources/Documentation/rest-api-pending-checks.md
@@ -11,7 +11,7 @@
### <a id="get-checker"> List Pending Checks
_'GET /checks.pending/'_
-Lists pending checks for a checker or for all checkers of a scheme.
+Lists pending checks for a checker.
Checks are pending if they are in a non-final state and the external
checker system intends to post further updates on them.
@@ -23,19 +23,17 @@
Request parameters:
* <a id="checker-param"> `checker`: the UUID of the checker for which
- pending checks should be listed (optional, if not specified `scheme`
- must be set)
-* <a id="scheme-param"> `scheme`: the scheme of the checkers for which
- pending checks should be listed (optional, if not specified `checker`
- must be set)
+ pending checks should be listed (required)
* <a id="state-param"> `state`: state that should be considered as
pending (optional, by default the state `NOT_STARTED` is assumed,
this option may be specified multiple times to request checks
matching any of several states)
-Note that only users with the [Administrate
-Checkers](access-control.md#capability_administrateCheckers) global capability
-are permitted to list pending checks.
+This REST endpoint only returns pending checks for current patch sets.
+
+Note that all users are allowed to list pending checks but the result
+includes only checks on changes that are visible to the calling user.
+This means pending checks for non-visible changes are filtered out.
#### Request by checker
@@ -81,56 +79,6 @@
]
```
-#### Request by checker scheme
-
-```
- GET /checks.pending/?scheme=test&state=NOT_STARTED&state=SCHEDULED HTTP/1.0
-```
-
-As response a list of [PendingChecksInfo](#pending-checks-info)
-entities is returned that describes the pending checks.
-
-#### Response by checker scheme
-
-```
- HTTP/1.1 200 OK
- Content-Disposition: attachment
- Content-Type: application/json; charset=UTF-8
- )]}'
- [
- {
- "patch_set": {
- "project": "test-project",
- "change_number": 1,
- "patch_set_id": 1,
- }
- "pending_checks": {
- "test:my-checker": {
- "state": "NOT_STARTED",
- },
- "test:my-other-checker": {
- "state": "SCHEDULED",
- }
- }
- },
- {
- "patch_set": {
- "project": "test-project",
- "change_number": 5,
- "patch_set_id": 2,
- }
- "pending_checks": {
- "test:my-checker": {
- "state": "NOT_STARTED",
- },
- "test:my-other-checker": {
- "state": "NOT_STARTED",
- }
- }
- }
- ]
-```
-
## <a id="json-entities"> JSON Entities
### <a id="checkable-patch-set-info"> CheckablePatchSetInfo
