Merge changes I7fdb7c87,Iea902095,Ief85d4b1,Ia4d4334e,I0859fa50
* changes:
NoteDbCheckersUpdate: fix to not include disabled checker on creation
Mark "CheckBackfiller" and its methods as package-private
Allow to provide options in Checks#getCheck
Allow to provide options in Checks#getChecks
Move "CheckBackfiller" to the "db" package
diff --git a/java/com/google/gerrit/plugins/checks/Checks.java b/java/com/google/gerrit/plugins/checks/Checks.java
index 747d81b..768908b 100644
--- a/java/com/google/gerrit/plugins/checks/Checks.java
+++ b/java/com/google/gerrit/plugins/checks/Checks.java
@@ -14,10 +14,12 @@
package com.google.gerrit.plugins.checks;
+import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
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 +39,39 @@
*
* @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;
+
+ @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/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/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/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/CheckBackfiller.java b/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
similarity index 97%
rename from java/com/google/gerrit/plugins/checks/api/CheckBackfiller.java
rename to java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
index d25e8af..f8c18e4 100644
--- a/java/com/google/gerrit/plugins/checks/api/CheckBackfiller.java
+++ b/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
@@ -12,7 +12,7 @@
// 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;
@@ -24,6 +24,8 @@
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;
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..be242f7 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
@@ -15,11 +15,15 @@
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.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.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
@@ -28,6 +32,8 @@
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.io.IOException;
+import java.util.List;
import java.util.Optional;
import java.util.stream.Stream;
@@ -37,45 +43,84 @@
private final ChangeNotes.Factory changeNotesFactory;
private final PatchSetUtil psUtil;
private final CheckNotes.Factory checkNotesFactory;
+ private final Checkers checkers;
+ private final CheckBackfiller checkBackfiller;
@Inject
NoteDbChecks(
ChangeNotes.Factory changeNotesFactory,
PatchSetUtil psUtil,
- CheckNotes.Factory checkNotesFactory) {
+ CheckNotes.Factory checkNotesFactory,
+ Checkers checkers,
+ CheckBackfiller checkBackfiller) {
this.changeNotesFactory = changeNotesFactory;
this.psUtil = psUtil;
this.checkNotesFactory = checkNotesFactory;
+ this.checkers = checkers;
+ this.checkBackfiller = checkBackfiller;
}
@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()) {
+ ChangeNotes notes =
+ changeNotesFactory.create(checkKey.project(), checkKey.patchSet().getParentKey());
+ return checkBackfiller.getBackfilledCheckForRelevantChecker(
+ checkKey.checkerUuid(), notes, 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());
-
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, notes, psId);
+
+ return Stream.concat(existingChecks.stream(), backfilledChecks.stream())
+ .collect(toImmutableList());
+ }
+
+ 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/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);
}
}
