Merge branch 'origin/stable-3.4' into stable-3.5
* origin/stable-3.4:
Filter out the files owners based on the owners' code-review value
Change-Id: I0f590874bdbfac256daa2642543e5a723952e202
diff --git a/README.md b/README.md
index 4893600..9c1decb 100644
--- a/README.md
+++ b/README.md
@@ -118,7 +118,8 @@
This project can be imported into the Eclipse IDE:
-Add the plugin name to the `CUSTOM_PLUGINS` in Gerrit core in
+Add the plugin name to the `CUSTOM_PLUGINS` (and in case when you want to run
+tests from the IDE to `CUSTOM_PLUGINS_TEST_DEPS`) in Gerrit core in
`tools/bzl/plugins.bzl` file and run:
```
diff --git a/external_plugin_deps.bzl b/external_plugin_deps.bzl
index bca48e4..5ad1930 100644
--- a/external_plugin_deps.bzl
+++ b/external_plugin_deps.bzl
@@ -2,13 +2,12 @@
JACKSON_VER = "2.9.7"
-def external_plugin_deps(omit_jackson_core = True):
- if not omit_jackson_core:
- maven_jar(
- name = "jackson-core",
- artifact = "com.fasterxml.jackson.core:jackson-core:" + JACKSON_VER,
- sha1 = "4b7f0e0dc527fab032e9800ed231080fdc3ac015",
- )
+def external_plugin_deps():
+ maven_jar(
+ name = "jackson-core",
+ artifact = "com.fasterxml.jackson.core:jackson-core:" + JACKSON_VER,
+ sha1 = "4b7f0e0dc527fab032e9800ed231080fdc3ac015",
+ )
maven_jar(
name = "jackson-databind",
diff --git a/owners-autoassign/BUILD b/owners-autoassign/BUILD
index 024be42..4aa4cec 100644
--- a/owners-autoassign/BUILD
+++ b/owners-autoassign/BUILD
@@ -15,8 +15,8 @@
],
resources = glob(["src/main/**/*"]),
deps = [
+ ":owners-api-neverlink",
"//owners-common",
- "//plugins/owners-api",
],
)
@@ -28,10 +28,16 @@
visibility = ["//visibility:public"],
deps = PLUGIN_DEPS_NEVERLINK + [
"//owners-common",
- "//plugins/owners-api",
+ ":owners-api-neverlink",
],
)
+java_library(
+ name = "owners-api-neverlink",
+ neverlink = 1,
+ exports = ["//plugins/owners-api"],
+)
+
junit_tests(
name = "owners_autoassign_tests",
testonly = 1,
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoassignModule.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoassignModule.java
index ae3127b..5e6ad99 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoassignModule.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoassignModule.java
@@ -18,28 +18,44 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
+import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
+import com.google.inject.Scopes;
+import com.googlesource.gerrit.owners.api.OwnersAttentionSet;
public class AutoassignModule extends AbstractModule {
- private final AutoAssignConfig config;
- @Inject
- AutoassignModule(AutoAssignConfig config) {
- this.config = config;
- }
+ private final Class<? extends OwnersAttentionSet> ownersAttentionSetImpl;
+ private final AutoAssignConfig config;
@VisibleForTesting
AutoassignModule() {
- this.config = new AutoAssignConfig();
+ this(DefaultAddAllOwnersToAttentionSet.class, new AutoAssignConfig());
+ }
+
+ @Inject
+ AutoassignModule(AutoAssignConfig config) {
+ this(DefaultAddAllOwnersToAttentionSet.class, config);
+ }
+
+ @VisibleForTesting
+ public AutoassignModule(
+ Class<? extends OwnersAttentionSet> ownersAttentionSetImpl, AutoAssignConfig config) {
+ this.ownersAttentionSetImpl = ownersAttentionSetImpl;
+ this.config = config;
}
@Override
protected void configure() {
+ install(PathOwnersEntriesCache.module());
bind(ReviewerManager.class)
.to(config.isAsyncReviewers() ? AsyncReviewerManager.class : SyncReviewerManager.class);
DynamicSet.bind(binder(), GitReferenceUpdatedListener.class).to(GitRefListener.class);
+ DynamicItem.bind(binder(), OwnersAttentionSet.class)
+ .to(ownersAttentionSetImpl)
+ .in(Scopes.SINGLETON);
install(new AutoassignConfigModule());
}
}
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/DefaultAddAllOwnersToAttentionSet.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/DefaultAddAllOwnersToAttentionSet.java
new file mode 100644
index 0000000..d53a239
--- /dev/null
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/DefaultAddAllOwnersToAttentionSet.java
@@ -0,0 +1,29 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+// implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners.common;
+
+import com.google.gerrit.entities.Account.Id;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.googlesource.gerrit.owners.api.OwnersAttentionSet;
+import java.util.Collection;
+
+class DefaultAddAllOwnersToAttentionSet implements OwnersAttentionSet {
+
+ @Override
+ public Collection<Id> addToAttentionSet(ChangeInfo changeInfo, Collection<Id> owners) {
+ return owners;
+ }
+}
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
index 0810cee..d6fd574 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
@@ -40,7 +40,8 @@
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeNotesCommit;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
-import com.google.gerrit.server.patch.PatchList;
+import com.google.gerrit.server.patch.DiffSummary;
+import com.google.gerrit.server.patch.DiffSummaryKey;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListKey;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
@@ -88,6 +89,8 @@
private final AutoAssignConfig cfg;
+ private final PathOwnersEntriesCache cache;
+
@Inject
public GitRefListener(
GerritApi api,
@@ -99,7 +102,8 @@
OneOffRequestContext oneOffReqCtx,
Provider<CurrentUser> currentUserProvider,
ChangeNotes.Factory notesFactory,
- AutoAssignConfig cfg) {
+ AutoAssignConfig cfg,
+ PathOwnersEntriesCache cache) {
this.api = api;
this.patchListCache = patchListCache;
this.projectCache = projectCache;
@@ -110,6 +114,7 @@
this.currentUserProvider = currentUserProvider;
this.notesFactory = notesFactory;
this.cfg = cfg;
+ this.cache = cache;
}
@Override
@@ -224,7 +229,7 @@
.collect(Collectors.toList()))
.orElse(Collections.emptyList());
- PatchList patchList = getPatchList(repository, event, change);
+ DiffSummary patchList = getDiffSummary(repository, event, change);
if (patchList != null) {
PathOwners owners =
new PathOwners(
@@ -234,7 +239,9 @@
parentProjectsNameKeys,
cfg.isBranchDisabled(change.branch) ? Optional.empty() : Optional.of(change.branch),
patchList,
- cfg.expandGroups());
+ cfg.expandGroups(),
+ projectNameKey.get(),
+ cache);
Set<Account.Id> allReviewers = Sets.newHashSet();
allReviewers.addAll(owners.get().values());
allReviewers.addAll(owners.getReviewers().values());
@@ -252,7 +259,7 @@
}
}
- private PatchList getPatchList(Repository repository, Event event, ChangeInfo change) {
+ private DiffSummary getDiffSummary(Repository repository, Event event, ChangeInfo change) {
ObjectId newId = null;
PatchListKey plKey;
try {
@@ -266,7 +273,8 @@
}
plKey = PatchListKey.againstCommit(null, newId, IGNORE_NONE);
}
- return patchListCache.get(plKey, Project.nameKey(change.project));
+ return patchListCache.getDiffSummary(
+ DiffSummaryKey.fromPatchListKey(plKey), Project.nameKey(change.project));
} catch (PatchListNotAvailableException | IOException e) {
logger.warn("Could not load patch list for change {}", change.id, e);
}
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
index 64ba615..bf56e26 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
@@ -19,12 +19,11 @@
import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.server.project.NoSuchProjectException;
-
import java.util.Collection;
public interface ReviewerManager {
- public void addReviewers(
- NameKey projectNameKey, ChangeApi cApi, Collection<Account.Id> accountsIds)
- throws ReviewerManagerException, NoSuchProjectException;
+ public void addReviewers(
+ NameKey projectNameKey, ChangeApi cApi, Collection<Account.Id> accountsIds)
+ throws ReviewerManagerException, NoSuchProjectException;
}
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/SyncReviewerManager.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/SyncReviewerManager.java
index 65e0bd1..83ba531 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/SyncReviewerManager.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/SyncReviewerManager.java
@@ -17,15 +17,14 @@
package com.googlesource.gerrit.owners.common;
import com.google.gerrit.entities.Account;
-import com.google.gerrit.entities.Account.Id;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.extensions.api.GerritApi;
-import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.AttentionSetInput;
import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewerInput;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.registration.DynamicItem;
@@ -88,7 +87,6 @@
this.cfg = cfg;
}
- @Override
public void addReviewers(
NameKey projectNameKey, ChangeApi cApi, Collection<Account.Id> accountsIds)
throws ReviewerManagerException, NoSuchProjectException {
@@ -109,7 +107,7 @@
Collection<Account.Id> validOwnersForAttentionSet = new ArrayList<>(accountsIds.size());
for (Account.Id account : accountsIds) {
if (!currentReviewers.contains(account.get()) && isVisibleTo(changeInfo, account)) {
- AddReviewerInput addReviewerInput = new AddReviewerInput();
+ ReviewerInput addReviewerInput = new ReviewerInput();
addReviewerInput.reviewer = account.toString();
addReviewerInput.state = reviewerState;
in.reviewers.add(addReviewerInput);
@@ -119,7 +117,8 @@
}
} else {
log.warn(
- "Not adding account {} as reviewer to change {} because the associated ref is not visible",
+ "Not adding account {} as reviewer to change {} because the associated ref is not"
+ + " visible",
account,
changeInfo._number);
}
@@ -139,7 +138,7 @@
in.ignoreAutomaticAttentionSetRules = true;
in.addToAttentionSet =
- reviewersAccounts.stream()
+ ownersForAttentionSet.get().addToAttentionSet(changeInfo, reviewersAccounts).stream()
.map(
(reviewer) ->
new AttentionSetInput(
@@ -154,7 +153,7 @@
}
}
- private boolean isVisibleTo(ChangeInfo changeInfo, Id account) {
+ private boolean isVisibleTo(ChangeInfo changeInfo, Account.Id account) {
ChangeData changeData =
changeDataFactory.create(
Project.nameKey(changeInfo.project), Change.id(changeInfo._number));
diff --git a/owners-autoassign/src/main/resources/Documentation/attention-set.md b/owners-autoassign/src/main/resources/Documentation/attention-set.md
index 7ad9a55..0ae7fa5 100644
--- a/owners-autoassign/src/main/resources/Documentation/attention-set.md
+++ b/owners-autoassign/src/main/resources/Documentation/attention-set.md
@@ -11,7 +11,7 @@
a generic interface that can be used to customize Gerrit's default
attention-set behaviour.
-## owner-api setup
+## owners-api setup
Copy the `owners-api.jar` libModule into the $GERRIT_SITE/lib directory
and add the following entry to `gerrit.config`:
diff --git a/owners-autoassign/src/main/resources/Documentation/config.md b/owners-autoassign/src/main/resources/Documentation/config.md
index 7de8eec..a99e3b1 100644
--- a/owners-autoassign/src/main/resources/Documentation/config.md
+++ b/owners-autoassign/src/main/resources/Documentation/config.md
@@ -1,3 +1,13 @@
+## Setup
+
+The owners-autoassign plugin depends on the shared library `owners-api.jar`
+which needs to be installed into the `$GERRIT_SITE/lib` and requires a
+restart of the Gerrit service.
+
+Once the `owners-api.jar` is loaded at Gerrit startup, the `owners-autoassign.jar`
+file can be installed like a regular Gerrit plugin, by being dropped to the
+`GRRIT_SITE/plugins` directory or installed through the plugin manager.
+
## Global configuration
The global plugin configuration is read from the `$GERRIT_SITE/etc/owners-autoassign.config`
diff --git a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/AbstractAutoassignIT.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/AbstractAutoassignIT.java
index a38b7a3..52fc441 100644
--- a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/AbstractAutoassignIT.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/AbstractAutoassignIT.java
@@ -24,9 +24,9 @@
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.acceptance.config.GlobalPluginConfig;
import com.google.gerrit.common.RawInputUtil;
-import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.ChangeEditApi;
+import com.google.gerrit.extensions.api.changes.ReviewerInput;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -35,6 +35,8 @@
import com.google.gerrit.server.project.ProjectConfig;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
+import com.google.inject.Module;
+import com.googlesource.gerrit.owners.api.OwnersApiModule;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
@@ -73,6 +75,11 @@
}
@Override
+ public Module createModule() {
+ return new OwnersApiModule();
+ }
+
+ @Override
public void setUpTestPlugin() throws Exception {
super.setUpTestPlugin();
@@ -145,7 +152,7 @@
assertThat(reviewers).hasSize(1);
// Switch user from CC to Reviewer or the other way around
- AddReviewerInput switchReviewerInput = new AddReviewerInput();
+ ReviewerInput switchReviewerInput = new ReviewerInput();
switchReviewerInput.reviewer = ownerEmail;
switchReviewerInput.state =
assignedUserState == ReviewerState.REVIEWER ? ReviewerState.CC : ReviewerState.REVIEWER;
diff --git a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerIT.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerIT.java
index 43aa41a..cb34173 100644
--- a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerIT.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerIT.java
@@ -35,12 +35,11 @@
import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
-import com.googlesource.gerrit.owners.common.AutoassignConfigModule;
+import com.google.inject.Module;
+import com.googlesource.gerrit.owners.api.OwnersApiModule;
import java.util.HashMap;
import java.util.Map;
import java.util.stream.StreamSupport;
-import com.googlesource.gerrit.owners.common.ReviewerManager;
-
import org.eclipse.jgit.transport.ReceiveCommand.Type;
import org.junit.Test;
@@ -56,9 +55,15 @@
String anOldObjectId = "anOldRef";
String aNewObjectId = "aNewRef";
+ @Override
+ public Module createModule() {
+ return new OwnersApiModule();
+ }
+
public static class TestModule extends AbstractModule {
@Override
protected void configure() {
+ install(PathOwnersEntriesCache.module());
DynamicSet.bind(binder(), GitReferenceUpdatedListener.class).to(GitRefListenerTest.class);
install(new AutoassignConfigModule());
}
diff --git a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerTest.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerTest.java
index 9cc6e96..b651572 100644
--- a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerTest.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/GitRefListenerTest.java
@@ -27,10 +27,6 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
-import com.googlesource.gerrit.owners.common.Accounts;
-import com.googlesource.gerrit.owners.common.AutoAssignConfig;
-import com.googlesource.gerrit.owners.common.GitRefListener;
-import com.googlesource.gerrit.owners.common.ReviewerManager;
import org.eclipse.jgit.lib.Repository;
import org.junit.Ignore;
@@ -50,7 +46,8 @@
OneOffRequestContext oneOffReqCtx,
Provider<CurrentUser> currentUserProvider,
ChangeNotes.Factory notesFactory,
- AutoAssignConfig cfg) {
+ AutoAssignConfig cfg,
+ PathOwnersEntriesCache cache) {
super(
api,
patchListCache,
@@ -61,7 +58,8 @@
oneOffReqCtx,
currentUserProvider,
notesFactory,
- cfg);
+ cfg,
+ cache);
}
@Override
diff --git a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignWithAttentionSetIT.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignWithAttentionSetIT.java
index 36795a6..1cfcf04 100644
--- a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignWithAttentionSetIT.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignWithAttentionSetIT.java
@@ -30,12 +30,10 @@
import com.google.gerrit.extensions.api.groups.GroupInput;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.ChangeInfo;
-import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
import com.google.inject.Module;
-import com.google.inject.Scopes;
import com.googlesource.gerrit.owners.api.OwnersApiModule;
import com.googlesource.gerrit.owners.api.OwnersAttentionSet;
import java.util.Collection;
@@ -58,11 +56,7 @@
public static class TestModule extends AbstractModule {
@Override
protected void configure() {
- install(new AutoassignModule());
-
- DynamicItem.bind(binder(), OwnersAttentionSet.class)
- .to(SelectFirstOwnerForAttentionSet.class)
- .in(Scopes.SINGLETON);
+ install(new AutoassignModule(SelectFirstOwnerForAttentionSet.class, new AutoAssignConfig()));
}
}
@@ -88,7 +82,7 @@
@Test
public void shouldAddToAttentionSetOneUserIfAnotherUserHasNoPermission() throws Exception {
- TestAccount userWithAccessToProject = accountCreator.user();
+ TestAccount userWithAccessToProject = accountCreator.user1();
TestAccount userWithNoAccessToProject = accountCreator.user2();
AccountGroup.UUID groupWithNoAccessToProject =
diff --git a/owners-common/common.bzl b/owners-common/common.bzl
index c571145..d2ce9f5 100644
--- a/owners-common/common.bzl
+++ b/owners-common/common.bzl
@@ -1,5 +1,5 @@
EXTERNAL_DEPS = [
- "@jackson-core//jar:neverlink",
+ "@jackson-core//jar",
"@jackson-databind//jar",
"@jackson-annotations//jar",
"@jackson-dataformat-yaml//jar",
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java
index 84d8f6c..2c8c619 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java
@@ -44,6 +44,10 @@
Boolean inherited =
Optional.ofNullable(jsonNode.get("inherited")).map(JsonNode::asBoolean).orElse(true);
ret.setInherited(inherited);
+ ret.setLabel(
+ Optional.ofNullable(jsonNode.get("label"))
+ .map(JsonNode::asText)
+ .flatMap(LabelDefinition::parse));
addClassicMatcher(jsonNode, ret);
addMatchers(jsonNode, ret);
return Optional.of(ret);
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/LabelDefinition.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/LabelDefinition.java
new file mode 100644
index 0000000..08664fe
--- /dev/null
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/LabelDefinition.java
@@ -0,0 +1,97 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners.common;
+
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.LabelId;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Describes the label together with score (the latter is optional) that is configured in the OWNERS
+ * file. File owners have to give the score for change to be submittable.
+ */
+public class LabelDefinition {
+ public static final LabelDefinition CODE_REVIEW = new LabelDefinition(LabelId.CODE_REVIEW, null);
+
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private static final Pattern LABEL_PATTERN =
+ Pattern.compile("^([a-zA-Z0-9-]+)(?:(?:\\s*,\\s*)(\\d))?$");
+
+ private final String name;
+ private final Optional<Short> score;
+
+ LabelDefinition(String name, Short score) {
+ this.name = name;
+ this.score = Optional.ofNullable(score);
+ }
+
+ public String getName() {
+ return name;
+ }
+
+ public Optional<Short> getScore() {
+ return score;
+ }
+
+ @Override
+ public String toString() {
+ StringBuilder builder = new StringBuilder();
+ builder.append("LabelDefinition [name=");
+ builder.append(name);
+ builder.append(", score=");
+ builder.append(score);
+ builder.append("]");
+ return builder.toString();
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(name, score);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+
+ if ((obj == null) || getClass() != obj.getClass()) {
+ return false;
+ }
+
+ LabelDefinition other = (LabelDefinition) obj;
+ return Objects.equals(name, other.name) && Objects.equals(score, other.score);
+ }
+
+ public static Optional<LabelDefinition> parse(String definition) {
+ return Optional.ofNullable(definition)
+ .filter(def -> !def.isBlank())
+ .map(
+ def -> {
+ Matcher labelDef = LABEL_PATTERN.matcher(def.trim());
+ if (!labelDef.matches()) {
+ logger.atSevere().log("Parsing label definition [%s] has failed.", def);
+ return null;
+ }
+
+ return new LabelDefinition(
+ labelDef.group(1),
+ Optional.ofNullable(labelDef.group(2)).map(Short::valueOf).orElse(null));
+ });
+ }
+}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java
index d6e40eb..7e7e705 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java
@@ -19,6 +19,7 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
/**
@@ -36,6 +37,9 @@
/** Map name of matcher and Matcher (value + Set Owners) */
private Map<String, Matcher> matchers = Maps.newHashMap();
+ /** Label that is required for submit. CodeReview if nothing is specified. */
+ private Optional<LabelDefinition> label = Optional.empty();
+
@Override
public String toString() {
return "OwnersConfig [inherited="
@@ -44,6 +48,8 @@
+ owners
+ ", matchers="
+ matchers
+ + ", label="
+ + label
+ "]";
}
@@ -78,4 +84,12 @@
public Matcher addMatcher(Matcher matcher) {
return this.matchers.put(matcher.path, matcher);
}
+
+ public void setLabel(Optional<LabelDefinition> label) {
+ this.label = label;
+ }
+
+ public Optional<LabelDefinition> getLabel() {
+ return label;
+ }
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java
index c4897bb..760b8a6 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java
@@ -21,6 +21,7 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Account.Id;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
public class OwnersMap {
@@ -30,6 +31,7 @@
private Map<String, Set<Account.Id>> fileOwners = Maps.newHashMap();
private Map<String, Set<Account.Id>> fileReviewers = Maps.newHashMap();
private Map<String, Set<String>> fileGroupOwners = Maps.newHashMap();
+ private Optional<LabelDefinition> label = Optional.empty();
@Override
public String toString() {
@@ -119,4 +121,12 @@
fileGroupOwners.computeIfAbsent(file, (f) -> Sets.newHashSet()).addAll(groupOwners);
}
+
+ public Optional<LabelDefinition> getLabel() {
+ return label;
+ }
+
+ public void setLabel(Optional<LabelDefinition> label) {
+ this.label = label;
+ }
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
index 6159e08..b904715 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
@@ -21,6 +21,7 @@
import static com.googlesource.gerrit.owners.common.JgitWrapper.getBlobAsBytes;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimaps;
import com.google.common.collect.SetMultimap;
@@ -32,8 +33,9 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.patch.PatchList;
-import com.google.gerrit.server.patch.PatchListEntry;
+import com.google.gerrit.server.patch.DiffSummary;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.project.ProjectState;
import java.io.IOException;
import java.util.Collection;
import java.util.Collections;
@@ -42,6 +44,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
+import java.util.concurrent.ExecutionException;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.Repository;
import org.slf4j.Logger;
@@ -73,10 +76,10 @@
private final List<Project.NameKey> parentProjectsNames;
- private final PatchList patchList;
-
private final ConfigurationParser parser;
+ private final Set<String> modifiedPaths;
+
private final Accounts accounts;
private final GitRepositoryManager repositoryManager;
@@ -89,30 +92,81 @@
private final boolean expandGroups;
+ private final Optional<LabelDefinition> label;
+
public PathOwners(
Accounts accounts,
GitRepositoryManager repositoryManager,
Repository repository,
List<Project.NameKey> parentProjectsNames,
Optional<String> branchWhenEnabled,
- PatchList patchList,
- boolean expandGroups) {
+ Map<String, FileDiffOutput> fileDiffMap,
+ boolean expandGroups,
+ String project,
+ PathOwnersEntriesCache cache) {
+ this(
+ accounts,
+ repositoryManager,
+ repository,
+ parentProjectsNames,
+ branchWhenEnabled,
+ getModifiedPaths(fileDiffMap),
+ expandGroups,
+ project,
+ cache);
+ }
+
+ public PathOwners(
+ Accounts accounts,
+ GitRepositoryManager repositoryManager,
+ Repository repository,
+ List<Project.NameKey> parentProjectsNames,
+ Optional<String> branchWhenEnabled,
+ DiffSummary diffSummary,
+ boolean expandGroups,
+ String project,
+ PathOwnersEntriesCache cache) {
+ this(
+ accounts,
+ repositoryManager,
+ repository,
+ parentProjectsNames,
+ branchWhenEnabled,
+ ImmutableSet.copyOf(diffSummary.getPaths()),
+ expandGroups,
+ project,
+ cache);
+ }
+
+ public PathOwners(
+ Accounts accounts,
+ GitRepositoryManager repositoryManager,
+ Repository repository,
+ List<Project.NameKey> parentProjectsNames,
+ Optional<String> branchWhenEnabled,
+ Set<String> modifiedPaths,
+ boolean expandGroups,
+ String project,
+ PathOwnersEntriesCache cache) {
this.repositoryManager = repositoryManager;
this.repository = repository;
this.parentProjectsNames = parentProjectsNames;
- this.patchList = patchList;
+ this.modifiedPaths = modifiedPaths;
this.parser = new ConfigurationParser(accounts);
this.accounts = accounts;
this.expandGroups = expandGroups;
- OwnersMap map = branchWhenEnabled.map(branch -> fetchOwners(branch)).orElse(new OwnersMap());
+ OwnersMap map =
+ branchWhenEnabled
+ .map(branch -> fetchOwners(project, branch, cache))
+ .orElse(new OwnersMap());
owners = Multimaps.unmodifiableSetMultimap(map.getPathOwners());
reviewers = Multimaps.unmodifiableSetMultimap(map.getPathReviewers());
matchers = map.getMatchers();
fileOwners = map.getFileOwners();
fileGroupOwners = map.getFileGroupOwners();
+ label = map.getLabel();
}
-
/**
* Returns a read only view of the paths to owners mapping.
*
@@ -147,27 +201,44 @@
return expandGroups;
}
+ public Optional<LabelDefinition> getLabel() {
+ return label;
+ }
+
+ public static List<Project.NameKey> getParents(ProjectState projectState) {
+ return projectState.parents().stream()
+ .map(ProjectState::getNameKey)
+ .collect(Collectors.toList());
+ }
+
/**
* Fetched the owners for the associated patch list.
*
* @return A structure containing matchers paths to owners
*/
- private OwnersMap fetchOwners(String branch) {
+ private OwnersMap fetchOwners(String project, String branch, PathOwnersEntriesCache cache) {
OwnersMap ownersMap = new OwnersMap();
try {
// Using a `map` would have needed a try/catch inside the lamba, resulting in more code
- List<PathOwnersEntry> parentsPathOwnersEntries =
- getPathOwnersEntries(parentProjectsNames, RefNames.REFS_CONFIG);
- PathOwnersEntry projectEntry = getPathOwnersEntry(repository, RefNames.REFS_CONFIG);
- PathOwnersEntry rootEntry = getPathOwnersEntry(repository, branch);
+ List<ReadOnlyPathOwnersEntry> parentsPathOwnersEntries =
+ getPathOwnersEntries(parentProjectsNames, RefNames.REFS_CONFIG, cache);
+ ReadOnlyPathOwnersEntry projectEntry =
+ getPathOwnersEntryOrEmpty(project, repository, RefNames.REFS_CONFIG, cache);
+ PathOwnersEntry rootEntry = getPathOwnersEntryOrNew(project, repository, branch, cache);
- Set<String> modifiedPaths = getModifiedPaths();
Map<String, PathOwnersEntry> entries = new HashMap<>();
PathOwnersEntry currentEntry = null;
for (String path : modifiedPaths) {
currentEntry =
resolvePathEntry(
- path, branch, projectEntry, parentsPathOwnersEntries, rootEntry, entries);
+ project,
+ path,
+ branch,
+ projectEntry,
+ parentsPathOwnersEntries,
+ rootEntry,
+ entries,
+ cache);
// add owners and reviewers to file for matcher predicates
ownersMap.addFileOwners(path, currentEntry.getOwners());
@@ -195,38 +266,59 @@
ownersMap.setMatchers(newMatchers);
}
}
+ ownersMap.setLabel(Optional.ofNullable(currentEntry).flatMap(PathOwnersEntry::getLabel));
return ownersMap;
- } catch (IOException e) {
+ } catch (IOException | ExecutionException e) {
log.warn("Invalid OWNERS file", e);
return ownersMap;
}
}
- private List<PathOwnersEntry> getPathOwnersEntries(
- List<Project.NameKey> projectNames, String branch) throws IOException {
- ImmutableList.Builder<PathOwnersEntry> pathOwnersEntries = ImmutableList.builder();
+ private List<ReadOnlyPathOwnersEntry> getPathOwnersEntries(
+ List<Project.NameKey> projectNames, String branch, PathOwnersEntriesCache cache)
+ throws IOException, ExecutionException {
+ ImmutableList.Builder<ReadOnlyPathOwnersEntry> pathOwnersEntries = ImmutableList.builder();
for (Project.NameKey projectName : projectNames) {
try (Repository repo = repositoryManager.openRepository(projectName)) {
- pathOwnersEntries = pathOwnersEntries.add(getPathOwnersEntry(repo, branch));
+ pathOwnersEntries =
+ pathOwnersEntries.add(
+ getPathOwnersEntryOrEmpty(projectName.get(), repo, branch, cache));
}
}
return pathOwnersEntries.build();
}
- private PathOwnersEntry getPathOwnersEntry(Repository repo, String branch) throws IOException {
+ private ReadOnlyPathOwnersEntry getPathOwnersEntryOrEmpty(
+ String project, Repository repo, String branch, PathOwnersEntriesCache cache)
+ throws ExecutionException {
+ return getPathOwnersEntry(project, repo, branch, cache)
+ .map(v -> (ReadOnlyPathOwnersEntry) v)
+ .orElse(PathOwnersEntry.EMPTY);
+ }
+
+ private PathOwnersEntry getPathOwnersEntryOrNew(
+ String project, Repository repo, String branch, PathOwnersEntriesCache cache)
+ throws ExecutionException {
+ return getPathOwnersEntry(project, repo, branch, cache).orElseGet(PathOwnersEntry::new);
+ }
+
+ private Optional<PathOwnersEntry> getPathOwnersEntry(
+ String project, Repository repo, String branch, PathOwnersEntriesCache cache)
+ throws ExecutionException {
String rootPath = "OWNERS";
- return getOwnersConfig(repo, rootPath, branch)
+ return cache
+ .get(project, branch, rootPath, () -> getOwnersConfig(repo, rootPath, branch))
.map(
conf ->
new PathOwnersEntry(
rootPath,
conf,
accounts,
+ Optional.empty(),
Collections.emptySet(),
Collections.emptySet(),
Collections.emptySet(),
- Collections.emptySet()))
- .orElse(new PathOwnersEntry());
+ Collections.emptySet()));
}
private void processMatcherPerPath(
@@ -274,13 +366,15 @@
}
private PathOwnersEntry resolvePathEntry(
+ String project,
String path,
String branch,
- PathOwnersEntry projectEntry,
- List<PathOwnersEntry> parentsPathOwnersEntries,
+ ReadOnlyPathOwnersEntry projectEntry,
+ List<ReadOnlyPathOwnersEntry> parentsPathOwnersEntries,
PathOwnersEntry rootEntry,
- Map<String, PathOwnersEntry> entries)
- throws IOException {
+ Map<String, PathOwnersEntry> entries,
+ PathOwnersEntriesCache cache)
+ throws ExecutionException {
String[] parts = path.split("/");
PathOwnersEntry currentEntry = rootEntry;
StringBuilder builder = new StringBuilder();
@@ -289,7 +383,7 @@
calculateCurrentEntry(rootEntry, projectEntry, currentEntry);
// Inherit from Parent Project if OWNER in Project enables inheritance
- for (PathOwnersEntry parentPathOwnersEntry : parentsPathOwnersEntries) {
+ for (ReadOnlyPathOwnersEntry parentPathOwnersEntry : parentsPathOwnersEntries) {
calculateCurrentEntry(projectEntry, parentPathOwnersEntry, currentEntry);
}
@@ -305,23 +399,33 @@
currentEntry = entries.get(partial);
} else {
String ownersPath = partial + "OWNERS";
- Optional<OwnersConfig> conf = getOwnersConfig(repository, ownersPath, branch);
- final Set<Id> owners = currentEntry.getOwners();
- final Set<Id> reviewers = currentEntry.getReviewers();
- Collection<Matcher> inheritedMatchers = currentEntry.getMatchers().values();
- Set<String> groupOwners = currentEntry.getGroupOwners();
+ PathOwnersEntry pathFallbackEntry = currentEntry;
currentEntry =
- conf.map(
- c ->
- new PathOwnersEntry(
- ownersPath,
- c,
- accounts,
- owners,
- reviewers,
- inheritedMatchers,
- groupOwners))
- .orElse(currentEntry);
+ cache
+ .get(
+ project,
+ branch,
+ ownersPath,
+ () -> getOwnersConfig(repository, ownersPath, branch))
+ .map(
+ c -> {
+ Optional<LabelDefinition> label = pathFallbackEntry.getLabel();
+ final Set<Id> owners = pathFallbackEntry.getOwners();
+ final Set<Id> reviewers = pathFallbackEntry.getReviewers();
+ Collection<Matcher> inheritedMatchers =
+ pathFallbackEntry.getMatchers().values();
+ Set<String> groupOwners = pathFallbackEntry.getGroupOwners();
+ return new PathOwnersEntry(
+ ownersPath,
+ c,
+ accounts,
+ label,
+ owners,
+ reviewers,
+ inheritedMatchers,
+ groupOwners);
+ })
+ .orElse(pathFallbackEntry);
entries.put(partial, currentEntry);
}
}
@@ -329,7 +433,9 @@
}
private void calculateCurrentEntry(
- PathOwnersEntry rootEntry, PathOwnersEntry projectEntry, PathOwnersEntry currentEntry) {
+ ReadOnlyPathOwnersEntry rootEntry,
+ ReadOnlyPathOwnersEntry projectEntry,
+ PathOwnersEntry currentEntry) {
if (rootEntry.isInherited()) {
for (Matcher matcher : projectEntry.getMatchers().values()) {
if (!currentEntry.hasMatcher(matcher.getPath())) {
@@ -342,26 +448,29 @@
if (currentEntry.getOwnersPath() == null) {
currentEntry.setOwnersPath(projectEntry.getOwnersPath());
}
+ if (currentEntry.getLabel().isEmpty()) {
+ currentEntry.setLabel(projectEntry.getLabel());
+ }
}
}
/**
- * Parses the patch list for any paths that were modified.
+ * Parses the diff list for any paths that were modified.
*
* @return set of modified paths.
*/
- private Set<String> getModifiedPaths() {
+ private static Set<String> getModifiedPaths(Map<String, FileDiffOutput> patchList) {
Set<String> paths = Sets.newHashSet();
- for (PatchListEntry patch : patchList.getPatches()) {
+ for (Map.Entry<String, FileDiffOutput> patch : patchList.entrySet()) {
// Ignore commit message and Merge List
- String newName = patch.getNewName();
+ String newName = patch.getKey();
if (!COMMIT_MSG.equals(newName) && !MERGE_LIST.equals(newName)) {
paths.add(newName);
// If a file was moved then we need approvals for old and new
// path
- if (patch.getChangeType() == Patch.ChangeType.RENAMED) {
- paths.add(patch.getOldName());
+ if (patch.getValue().changeType() == Patch.ChangeType.RENAMED) {
+ paths.add(patch.getValue().oldPath().get());
}
}
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java
new file mode 100644
index 0000000..04afebd
--- /dev/null
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java
@@ -0,0 +1,156 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners.common;
+
+import com.google.common.cache.RemovalNotification;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.cache.CacheRemovalListener;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import com.google.inject.Singleton;
+import com.google.inject.TypeLiteral;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+
+public interface PathOwnersEntriesCache {
+ String CACHE_NAME = "path_owners_entries";
+
+ static Module module() {
+ return new CacheModule() {
+ @Override
+ protected void configure() {
+ cache(CACHE_NAME, Key.class, new TypeLiteral<Optional<OwnersConfig>>() {});
+ bind(PathOwnersEntriesCache.class).to(PathOwnersEntriesCacheImpl.class);
+ DynamicSet.bind(binder(), GitReferenceUpdatedListener.class)
+ .to(OwnersRefUpdateListener.class);
+ DynamicSet.bind(binder(), CacheRemovalListener.class).to(OwnersCacheRemovalListener.class);
+ }
+ };
+ }
+
+ Optional<OwnersConfig> get(
+ String project, String branch, String path, Callable<Optional<OwnersConfig>> loader)
+ throws ExecutionException;
+
+ void invalidate(String project, String branch);
+
+ void invalidateIndexKey(Key key);
+
+ class Key {
+ final String project;
+ final String branch;
+ final String path;
+
+ Key(String project, String branch, String path) {
+ this.project = project;
+ this.branch = branch;
+ this.path = path;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(branch, path, project);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+
+ if (obj == null) {
+ return false;
+ }
+
+ if (getClass() != obj.getClass()) {
+ return false;
+ }
+
+ Key other = (Key) obj;
+ return Objects.equals(branch, other.branch)
+ && Objects.equals(path, other.path)
+ && Objects.equals(project, other.project);
+ }
+
+ @Override
+ public String toString() {
+ StringBuilder builder = new StringBuilder();
+ builder.append("Key [project=");
+ builder.append(project);
+ builder.append(", branch=");
+ builder.append(branch);
+ builder.append(", path=");
+ builder.append(path);
+ builder.append("]");
+ return builder.toString();
+ }
+ }
+
+ @Singleton
+ class OwnersRefUpdateListener implements GitReferenceUpdatedListener {
+ private final PathOwnersEntriesCache cache;
+ private final String allUsersName;
+
+ @Inject
+ OwnersRefUpdateListener(PathOwnersEntriesCache cache, AllUsersName allUsersName) {
+ this.cache = cache;
+ this.allUsersName = allUsersName.get();
+ }
+
+ @Override
+ public void onGitReferenceUpdated(Event event) {
+ if (supportedEvent(allUsersName, event)) {
+ cache.invalidate(event.getProjectName(), event.getRefName());
+ }
+ }
+
+ static boolean supportedEvent(String allUsersName, Event event) {
+ String refName = event.getRefName();
+ return !allUsersName.equals(event.getProjectName())
+ && (refName.equals(RefNames.REFS_CONFIG) || !RefNames.isGerritRef(refName));
+ }
+ }
+
+ @Singleton
+ class OwnersCacheRemovalListener implements CacheRemovalListener<Key, PathOwnersEntry> {
+ private final PathOwnersEntriesCache cache;
+ private final String cacheName;
+
+ @Inject
+ OwnersCacheRemovalListener(@PluginName String pluginName, PathOwnersEntriesCache cache) {
+ this.cache = cache;
+ this.cacheName = String.format("%s.%s", pluginName, CACHE_NAME);
+ }
+
+ @Override
+ public void onRemoval(
+ String pluginName,
+ String cacheName,
+ RemovalNotification<Key, PathOwnersEntry> notification) {
+ if (!this.cacheName.equals(cacheName)) {
+ return;
+ }
+
+ cache.invalidateIndexKey(notification.getKey());
+ }
+ }
+}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheImpl.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheImpl.java
new file mode 100644
index 0000000..3af7a58
--- /dev/null
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheImpl.java
@@ -0,0 +1,93 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners.common;
+
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.common.collect.HashMultimap;
+import com.google.common.collect.Multimap;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+import java.time.Duration;
+import java.util.Collection;
+import java.util.Optional;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+
+@Singleton
+class PathOwnersEntriesCacheImpl implements PathOwnersEntriesCache {
+
+ private final Cache<Key, Optional<OwnersConfig>> cache;
+ private final Multimap<String, Key> keysIndex;
+ private final LoadingCache<String, Object> keyLocks;
+
+ @Inject
+ PathOwnersEntriesCacheImpl(@Named(CACHE_NAME) Cache<Key, Optional<OwnersConfig>> cache) {
+ this.cache = cache;
+ this.keysIndex = HashMultimap.create();
+ this.keyLocks =
+ CacheBuilder.newBuilder()
+ .expireAfterAccess(Duration.ofMinutes(10L))
+ .build(CacheLoader.from(Object::new));
+ }
+
+ @Override
+ public Optional<OwnersConfig> get(
+ String project, String branch, String path, Callable<Optional<OwnersConfig>> loader)
+ throws ExecutionException {
+ Key key = new Key(project, branch, path);
+ return cache.get(
+ key,
+ () -> {
+ Optional<OwnersConfig> entry = loader.call();
+ String indexKey = indexKey(project, branch);
+ synchronized (keyLocks.getUnchecked(indexKey)) {
+ keysIndex.put(indexKey, key);
+ }
+ return entry;
+ });
+ }
+
+ @Override
+ public void invalidate(String project, String branch) {
+ String indexKey = indexKey(project, branch);
+ Collection<Key> keysToInvalidate;
+
+ synchronized (keyLocks.getUnchecked(indexKey)) {
+ keysToInvalidate = keysIndex.removeAll(indexKey);
+ }
+
+ keysToInvalidate.forEach(cache::invalidate);
+ }
+
+ @Override
+ public void invalidateIndexKey(Key key) {
+ String indexKey = indexKey(key.project, key.branch);
+
+ synchronized (keyLocks.getUnchecked(indexKey)) {
+ Collection<Key> values = keysIndex.asMap().get(indexKey);
+ if (values != null) {
+ values.remove(key);
+ }
+ }
+ }
+
+ private String indexKey(String project, String branch) {
+ return new StringBuilder(project).append('@').append(branch).toString();
+ }
+}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
index 168d8bd..8e4c53c 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
@@ -22,6 +22,7 @@
import com.google.gerrit.entities.Account;
import java.util.Collection;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
@@ -30,26 +31,23 @@
*
* <p>Used internally by PathOwners to represent and compute the owners at a specific path.
*/
-class PathOwnersEntry {
- private final boolean inherited;
- private Set<Account.Id> owners = Sets.newHashSet();
- private Set<Account.Id> reviewers = Sets.newHashSet();
- private String ownersPath;
- private Map<String, Matcher> matchers = Maps.newHashMap();
- private Set<String> groupOwners = Sets.newHashSet();
+class PathOwnersEntry extends ReadOnlyPathOwnersEntry {
+ static final ReadOnlyPathOwnersEntry EMPTY = new ReadOnlyPathOwnersEntry(true) {};
public PathOwnersEntry() {
- inherited = true;
+ super(true);
}
public PathOwnersEntry(
String path,
OwnersConfig config,
Accounts accounts,
+ Optional<LabelDefinition> inheritedLabel,
Set<Account.Id> inheritedOwners,
Set<Account.Id> inheritedReviewers,
Collection<Matcher> inheritedMatchers,
Set<String> inheritedGroupOwners) {
+ super(config.isInherited());
this.ownersPath = path;
this.owners =
config.getOwners().stream()
@@ -72,19 +70,10 @@
for (Matcher matcher : inheritedMatchers) {
addMatcher(matcher);
}
+ this.label = config.getLabel().or(() -> inheritedLabel);
+ } else {
+ this.label = config.getLabel();
}
- this.inherited = config.isInherited();
- }
-
- @Override
- public String toString() {
- return "PathOwnersEntry [ownersPath="
- + ownersPath
- + ", owners="
- + owners
- + ", matchers="
- + matchers
- + "]";
}
public void addMatcher(Matcher matcher) {
@@ -92,6 +81,52 @@
this.matchers.put(matcher.getPath(), matcher.merge(currMatchers));
}
+ public void setOwners(Set<Account.Id> owners) {
+ this.owners = owners;
+ }
+
+ public void setReviewers(Set<Account.Id> reviewers) {
+ this.reviewers = reviewers;
+ }
+
+ public void setOwnersPath(String ownersPath) {
+ this.ownersPath = ownersPath;
+ }
+
+ public void setMatchers(Map<String, Matcher> matchers) {
+ this.matchers = matchers;
+ }
+
+ public void setLabel(Optional<LabelDefinition> label) {
+ this.label = label;
+ }
+
+ public void addMatchers(Collection<Matcher> values) {
+ for (Matcher matcher : values) {
+ addMatcher(matcher);
+ }
+ }
+
+ @Override
+ protected String className() {
+ return getClass().getSimpleName();
+ }
+}
+
+abstract class ReadOnlyPathOwnersEntry {
+ protected final boolean inherited;
+ protected Optional<LabelDefinition> label;
+ protected Set<Account.Id> owners = Sets.newHashSet();
+ protected Set<Account.Id> reviewers = Sets.newHashSet();
+ protected String ownersPath;
+ protected Map<String, Matcher> matchers = Maps.newHashMap();
+ protected Set<String> groupOwners = Sets.newHashSet();
+
+ protected ReadOnlyPathOwnersEntry(boolean inherited) {
+ this.inherited = inherited;
+ label = Optional.empty();
+ }
+
public Map<String, Matcher> getMatchers() {
return matchers;
}
@@ -104,38 +139,20 @@
return groupOwners;
}
- public void setOwners(Set<Account.Id> owners) {
- this.owners = owners;
- }
-
public Set<Account.Id> getReviewers() {
return reviewers;
}
- public void setReviewers(Set<Account.Id> reviewers) {
- this.reviewers = reviewers;
- }
-
public String getOwnersPath() {
return ownersPath;
}
- public void setOwnersPath(String ownersPath) {
- this.ownersPath = ownersPath;
- }
-
- public void setMatchers(Map<String, Matcher> matchers) {
- this.matchers = matchers;
- }
-
public boolean isInherited() {
return inherited;
}
- public void addMatchers(Collection<Matcher> values) {
- for (Matcher matcher : values) {
- addMatcher(matcher);
- }
+ public Optional<LabelDefinition> getLabel() {
+ return label;
}
public boolean hasMatcher(String path) {
@@ -145,4 +162,22 @@
public static String stripOwnerDomain(String owner) {
return Splitter.on('@').split(owner).iterator().next();
}
+
+ @Override
+ public String toString() {
+ return className()
+ + " [ownersPath="
+ + ownersPath
+ + ", owners="
+ + owners
+ + ", matchers="
+ + matchers
+ + ", label="
+ + label
+ + "]";
+ }
+
+ protected String className() {
+ return "ReadOnlyPathOwnersEntry";
+ }
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PluginSettings.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PluginSettings.java
index ef60d58..26b0f62 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PluginSettings.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PluginSettings.java
@@ -34,6 +34,7 @@
private final String ownersPluginName;
private final Config globalPluginConfig;
private final boolean expandGroups;
+ private final boolean enableSubmitRequirement;
@Inject
public PluginSettings(PluginConfigFactory configFactory, @PluginName String ownersPluginName) {
@@ -45,6 +46,8 @@
ImmutableSet.copyOf(globalPluginConfig.getStringList("owners", "disable", "branch"));
this.expandGroups = globalPluginConfig.getBoolean("owners", "expandGroups", true);
+ this.enableSubmitRequirement =
+ globalPluginConfig.getBoolean("owners", "enableSubmitRequirement", false);
}
/**
@@ -75,6 +78,11 @@
return expandGroups;
}
+ /** @return <code>true</code> when OWNERS file should be evaluated through the submit rule */
+ public boolean enableSubmitRequirement() {
+ return enableSubmitRequirement;
+ }
+
/**
* Project-specific config of the owners plugin.
*
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java
index f298164..ae3fad2 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java
@@ -21,13 +21,9 @@
import com.google.common.base.Charsets;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListEntry;
import java.io.IOException;
-import java.util.Arrays;
-import java.util.List;
import java.util.Optional;
-import java.util.stream.Collectors;
import org.eclipse.jgit.lib.Repository;
import org.junit.Ignore;
import org.powermock.api.easymock.PowerMock;
@@ -38,7 +34,6 @@
protected Repository repository;
protected Repository parentRepository1;
protected Repository parentRepository2;
- protected PatchList patchList;
protected ConfigurationParser parser;
protected TestAccounts accounts = new TestAccounts();
protected Optional<String> branch = Optional.of("master");
@@ -81,17 +76,6 @@
.anyTimes();
}
- void creatingPatch(String... fileNames) {
- creatingPatchList(Arrays.asList(fileNames));
- }
-
- void creatingPatchList(List<String> names) {
- patchList = PowerMock.createMock(PatchList.class);
- List<PatchListEntry> entries =
- names.stream().map(name -> expectEntry(name)).collect(Collectors.toList());
- expect(patchList.getPatches()).andReturn(entries);
- }
-
PatchListEntry expectEntry(String name) {
PatchListEntry entry = PowerMock.createMock(PatchListEntry.class);
expect(entry.getNewName()).andReturn(name).anyTimes();
@@ -105,9 +89,15 @@
return parser.getOwnersConfig(string.getBytes(Charsets.UTF_8));
}
- public String createConfig(boolean inherited, String[] owners, MatcherConfig... matchers) {
+ String createConfig(boolean inherited, String[] owners, MatcherConfig... matchers) {
+ return createConfig(inherited, Optional.empty(), owners, matchers);
+ }
+
+ String createConfig(
+ boolean inherited, Optional<String> label, String[] owners, MatcherConfig... matchers) {
StringBuilder sb = new StringBuilder();
sb.append("inherited: " + inherited + "\n");
+ label.ifPresent(l -> sb.append("label: " + l + "\n"));
if (owners.length > 0) {
sb.append("owners: \n");
for (String owner : owners) {
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/LabelDefinitionTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/LabelDefinitionTest.java
new file mode 100644
index 0000000..126e239
--- /dev/null
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/LabelDefinitionTest.java
@@ -0,0 +1,59 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners.common;
+
+import static com.google.common.truth.Truth8.assertThat;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Optional;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class LabelDefinitionTest {
+ @Parameterized.Parameters
+ public static Collection<Object[]> labels() {
+ return Arrays.asList(
+ new Object[][] {
+ {null, Optional.empty()},
+ {"", Optional.empty()},
+ {"foo,", Optional.empty()},
+ {"foo", Optional.of(new LabelDefinition("foo", null))},
+ {"foo,1", Optional.of(new LabelDefinition("foo", (short) 1))},
+ {"foo, 1", Optional.of(new LabelDefinition("foo", (short) 1))},
+ {"foo , 1", Optional.of(new LabelDefinition("foo", (short) 1))},
+ {"foo ,1 ", Optional.of(new LabelDefinition("foo", (short) 1))}
+ });
+ }
+
+ private final String input;
+ private final Optional<LabelDefinition> expected;
+
+ public LabelDefinitionTest(String input, Optional<LabelDefinition> expected) {
+ this.input = input;
+ this.expected = expected;
+ }
+
+ @Test
+ public void shouldParseLabelDefinition() {
+ // when
+ Optional<LabelDefinition> result = LabelDefinition.parse(input);
+
+ // then
+ assertThat(result).isEqualTo(expected);
+ }
+}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/OwnersRefUpdateListenerTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/OwnersRefUpdateListenerTest.java
new file mode 100644
index 0000000..fa20c79
--- /dev/null
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/OwnersRefUpdateListenerTest.java
@@ -0,0 +1,72 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners.common;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
+import com.google.gerrit.server.config.AllProjectsNameProvider;
+import com.google.gerrit.server.config.AllUsersNameProvider;
+import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache.OwnersRefUpdateListener;
+import java.util.Arrays;
+import java.util.Collection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class OwnersRefUpdateListenerTest {
+ @Parameterized.Parameters
+ public static Collection<Object[]> events() {
+ return Arrays.asList(
+ new Object[][] {
+ {mockEvent(ALL_USERS_NAME, null), false},
+ {mockEvent(AllProjectsNameProvider.DEFAULT, RefNames.REFS_CHANGES), false},
+ {mockEvent(AllProjectsNameProvider.DEFAULT, RefNames.REFS_SEQUENCES), false},
+ {mockEvent(AllProjectsNameProvider.DEFAULT, RefNames.REFS_CONFIG), true},
+ {mockEvent("foo", RefNames.fullName("bar")), true},
+ {mockEvent("foo", RefNames.REFS_CONFIG), true}
+ });
+ }
+
+ private static String ALL_USERS_NAME = AllUsersNameProvider.DEFAULT;
+
+ private final GitReferenceUpdatedListener.Event input;
+ private final boolean expected;
+
+ public OwnersRefUpdateListenerTest(GitReferenceUpdatedListener.Event input, boolean expected) {
+ this.input = input;
+ this.expected = expected;
+ }
+
+ @Test
+ public void shouldParseLabelDefinition() {
+ // when
+ boolean result = OwnersRefUpdateListener.supportedEvent(ALL_USERS_NAME, input);
+
+ // then
+ assertThat(result).isEqualTo(expected);
+ }
+
+ private static GitReferenceUpdatedListener.Event mockEvent(String project, String ref) {
+ GitReferenceUpdatedListener.Event eventMock = mock(GitReferenceUpdatedListener.Event.class);
+ when(eventMock.getProjectName()).thenReturn(project);
+ when(eventMock.getRefName()).thenReturn(ref);
+ return eventMock;
+ }
+}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheMock.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheMock.java
new file mode 100644
index 0000000..752bf7f
--- /dev/null
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCacheMock.java
@@ -0,0 +1,44 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners.common;
+
+import java.util.Optional;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import org.junit.Ignore;
+
+/** This is a test implementation that doesn't cache anything but calls loader instead. */
+@Ignore
+public class PathOwnersEntriesCacheMock implements PathOwnersEntriesCache {
+ int hit = 0;
+
+ @Override
+ public void invalidate(String project, String branch) {}
+
+ @Override
+ public void invalidateIndexKey(Key key) {}
+
+ @Override
+ public Optional<OwnersConfig> get(
+ String project, String branch, String path, Callable<Optional<OwnersConfig>> loader)
+ throws ExecutionException {
+ try {
+ hit++;
+ return loader.call();
+ } catch (Exception e) {
+ throw new ExecutionException(e);
+ }
+ }
+}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
index e8b4167..2e23f33 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
@@ -14,12 +14,16 @@
package com.googlesource.gerrit.owners.common;
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
import static com.googlesource.gerrit.owners.common.MatcherConfig.suffixMatcher;
-import static java.util.Collections.EMPTY_LIST;
+import static java.util.Collections.emptyList;
import static org.easymock.EasyMock.eq;
import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.expectLastCall;
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
import static org.powermock.api.easymock.PowerMock.replayAll;
import com.google.gerrit.entities.Account;
@@ -27,7 +31,6 @@
import com.google.gerrit.entities.RefNames;
import java.io.IOException;
import java.util.Arrays;
-import java.util.Collections;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
@@ -47,6 +50,10 @@
private static final String CLASSIC_OWNERS = "classic/OWNERS";
private static final boolean EXPAND_GROUPS = true;
private static final boolean DO_NOT_EXPAND_GROUPS = false;
+ private static final String EXPECTED_LABEL = "expected-label";
+ private static final String A_LABEL = "a-label";
+ private static PathOwnersEntriesCache CACHE_MOCK = new PathOwnersEntriesCacheMock();
+
public static final String CLASSIC_FILE_TXT = "classic/file.txt";
public static final Project.NameKey parentRepository1NameKey =
Project.NameKey.parse("parentRepository1");
@@ -68,10 +75,12 @@
accounts,
repositoryManager,
repository,
- Collections.EMPTY_LIST,
+ emptyList(),
branch,
- patchList,
- EXPAND_GROUPS);
+ Set.of(CLASSIC_FILE_TXT),
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Set<Account.Id> ownersSet = owners.get().get(CLASSIC_OWNERS);
assertEquals(2, ownersSet.size());
assertTrue(ownersSet.contains(USER_A_ID));
@@ -88,10 +97,12 @@
accounts,
repositoryManager,
repository,
- EMPTY_LIST,
+ emptyList(),
branch,
- patchList,
- DO_NOT_EXPAND_GROUPS);
+ Set.of(CLASSIC_FILE_TXT),
+ DO_NOT_EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Set<String> ownersSet = owners.getFileGroupOwners().get(CLASSIC_FILE_TXT);
assertEquals(2, ownersSet.size());
assertTrue(ownersSet.contains(USER_A));
@@ -108,25 +119,37 @@
accounts,
repositoryManager,
repository,
- EMPTY_LIST,
+ emptyList(),
Optional.empty(),
- patchList,
- EXPAND_GROUPS);
+ Set.of(CLASSIC_FILE_TXT),
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Set<Account.Id> ownersSet = owners.get().get(CLASSIC_OWNERS);
assertEquals(0, ownersSet.size());
}
@Test
public void testClassicWithInheritance() throws Exception {
- expectConfig("OWNERS", createConfig(true, owners(USER_C_EMAIL_COM)));
- expectConfig(CLASSIC_OWNERS, createConfig(true, owners(USER_A_EMAIL_COM, USER_B_EMAIL_COM)));
+ expectConfig("OWNERS", createConfig(true, Optional.of(A_LABEL), owners(USER_C_EMAIL_COM)));
+ expectConfig(
+ CLASSIC_OWNERS,
+ createConfig(
+ true, Optional.of(EXPECTED_LABEL), owners(USER_A_EMAIL_COM, USER_B_EMAIL_COM)));
- creatingPatchList(Arrays.asList("classic/file.txt"));
replayAll();
PathOwners owners2 =
new PathOwners(
- accounts, repositoryManager, repository, EMPTY_LIST, branch, patchList, EXPAND_GROUPS);
+ accounts,
+ repositoryManager,
+ repository,
+ emptyList(),
+ branch,
+ Set.of("classic/file.txt"),
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Set<Account.Id> ownersSet2 = owners2.get().get(CLASSIC_OWNERS);
// in this case we are inheriting the acct3 from /OWNERS
@@ -134,6 +157,10 @@
assertTrue(ownersSet2.contains(USER_A_ID));
assertTrue(ownersSet2.contains(USER_B_ID));
assertTrue(ownersSet2.contains(USER_C_ID));
+
+ // expect that classic configuration takes precedence over `OWNERS` file for the label
+ // definition
+ assertThat(owners2.getLabel().map(LabelDefinition::getName)).hasValue(EXPECTED_LABEL);
}
@Test
@@ -142,15 +169,26 @@
expectConfig(
"OWNERS",
RefNames.REFS_CONFIG,
- createConfig(true, owners(), suffixMatcher(".sql", USER_A_EMAIL_COM, USER_B_EMAIL_COM)));
+ createConfig(
+ true,
+ Optional.of(EXPECTED_LABEL),
+ owners(),
+ suffixMatcher(".sql", USER_A_EMAIL_COM, USER_B_EMAIL_COM)));
String fileName = "file.sql";
- creatingPatchList(Collections.singletonList(fileName));
replayAll();
PathOwners owners =
new PathOwners(
- accounts, repositoryManager, repository, EMPTY_LIST, branch, patchList, EXPAND_GROUPS);
+ accounts,
+ repositoryManager,
+ repository,
+ emptyList(),
+ branch,
+ Set.of(fileName),
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners();
assertEquals(1, fileOwners.size());
@@ -159,20 +197,28 @@
assertEquals(2, ownersSet.size());
assertTrue(ownersSet.contains(USER_A_ID));
assertTrue(ownersSet.contains(USER_B_ID));
+ assertThat(owners.getLabel().map(LabelDefinition::getName)).hasValue(EXPECTED_LABEL);
}
@Test
public void testProjectInheritFromParentProject() throws Exception {
- expectConfig("OWNERS", "master", createConfig(true, owners()));
- expectConfig("OWNERS", RefNames.REFS_CONFIG, repository, createConfig(true, owners()));
+ expectConfig("OWNERS", "master", createConfig(true, Optional.of(EXPECTED_LABEL), owners()));
+ expectConfig(
+ "OWNERS",
+ RefNames.REFS_CONFIG,
+ repository,
+ createConfig(true, Optional.of("foo"), owners()));
expectConfig(
"OWNERS",
RefNames.REFS_CONFIG,
parentRepository1,
- createConfig(true, owners(), suffixMatcher(".sql", USER_A_EMAIL_COM, USER_B_EMAIL_COM)));
+ createConfig(
+ true,
+ Optional.of(A_LABEL),
+ owners(),
+ suffixMatcher(".sql", USER_A_EMAIL_COM, USER_B_EMAIL_COM)));
String fileName = "file.sql";
- creatingPatchList(Collections.singletonList(fileName));
mockParentRepository(parentRepository1NameKey, parentRepository1);
replayAll();
@@ -184,8 +230,10 @@
repository,
Arrays.asList(parentRepository1NameKey),
branch,
- patchList,
- EXPAND_GROUPS);
+ Set.of(fileName),
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners();
assertEquals(fileOwners.size(), 1);
@@ -194,6 +242,10 @@
assertEquals(2, ownersSet.size());
assertTrue(ownersSet.contains(USER_A_ID));
assertTrue(ownersSet.contains(USER_B_ID));
+
+ // expect that `master` configuration overwrites the label definition of both `refs/meta/config`
+ // and parent repo
+ assertThat(owners.getLabel().map(LabelDefinition::getName)).hasValue(EXPECTED_LABEL);
}
@Test
@@ -204,16 +256,17 @@
"OWNERS",
RefNames.REFS_CONFIG,
parentRepository1,
- createConfig(true, owners(), suffixMatcher(".sql", USER_A_EMAIL_COM)));
+ createConfig(
+ true, Optional.of(EXPECTED_LABEL), owners(), suffixMatcher(".sql", USER_A_EMAIL_COM)));
expectConfig(
"OWNERS",
RefNames.REFS_CONFIG,
parentRepository2,
- createConfig(true, owners(), suffixMatcher(".java", USER_B_EMAIL_COM)));
+ createConfig(
+ true, Optional.of(A_LABEL), owners(), suffixMatcher(".java", USER_B_EMAIL_COM)));
String sqlFileName = "file.sql";
String javaFileName = "file.java";
- creatingPatchList(Arrays.asList(sqlFileName, javaFileName));
mockParentRepository(parentRepository1NameKey, parentRepository1);
mockParentRepository(parentRepository2NameKey, parentRepository2);
@@ -226,8 +279,10 @@
repository,
Arrays.asList(parentRepository1NameKey, parentRepository2NameKey),
branch,
- patchList,
- EXPAND_GROUPS);
+ Set.of(sqlFileName, javaFileName),
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners();
assertEquals(fileOwners.size(), 2);
@@ -239,6 +294,9 @@
Set<Account.Id> ownersSet2 = fileOwners.get(javaFileName);
assertEquals(1, ownersSet2.size());
assertTrue(ownersSet2.contains(USER_B_ID));
+
+ // expect that closer parent (parentRepository1) overwrites the label definition
+ assertThat(owners.getLabel().map(LabelDefinition::getName)).hasValue(EXPECTED_LABEL);
}
private void mockParentRepository(Project.NameKey repositoryName, Repository repository)
@@ -251,38 +309,123 @@
@Test
public void testClassicWithInheritanceAndDeepNesting() throws Exception {
expectConfig("OWNERS", createConfig(true, owners(USER_C_EMAIL_COM)));
- expectConfig("dir/OWNERS", createConfig(true, owners(USER_B_EMAIL_COM)));
- expectConfig("dir/subdir/OWNERS", createConfig(true, owners(USER_A_EMAIL_COM)));
+ expectConfig("dir/OWNERS", createConfig(true, Optional.of(A_LABEL), owners(USER_B_EMAIL_COM)));
+ expectConfig(
+ "dir/subdir/OWNERS",
+ createConfig(true, Optional.of(EXPECTED_LABEL), owners(USER_A_EMAIL_COM)));
- creatingPatchList(Arrays.asList("dir/subdir/file.txt"));
replayAll();
PathOwners owners =
new PathOwners(
- accounts, repositoryManager, repository, EMPTY_LIST, branch, patchList, EXPAND_GROUPS);
+ accounts,
+ repositoryManager,
+ repository,
+ emptyList(),
+ branch,
+ Set.of("dir/subdir/file.txt"),
+ EXPAND_GROUPS,
+ "foo",
+ CACHE_MOCK);
Set<Account.Id> ownersSet = owners.get().get("dir/subdir/OWNERS");
assertEquals(3, ownersSet.size());
assertTrue(ownersSet.contains(USER_A_ID));
assertTrue(ownersSet.contains(USER_B_ID));
assertTrue(ownersSet.contains(USER_C_ID));
+
+ // expect that more specific configuration overwrites the label definition
+ assertThat(owners.getLabel().map(LabelDefinition::getName)).hasValue(EXPECTED_LABEL);
}
@Test
- public void testParsingYaml() {
- String yamlString = ("inherited: true\nowners:\n- " + USER_C_EMAIL_COM);
+ public void testParsingYamlWithLabelWithScore() {
+ String yamlString =
+ "inherited: true\nlabel: " + EXPECTED_LABEL + ",1\nowners:\n- " + USER_C_EMAIL_COM;
Optional<OwnersConfig> config = getOwnersConfig(yamlString);
+
assertTrue(config.isPresent());
- assertTrue(config.get().isInherited());
- assertEquals(1, config.get().getOwners().size());
- assertTrue(config.get().getOwners().contains(USER_C_EMAIL_COM));
+
+ OwnersConfig ownersConfig = config.get();
+ assertTrue(ownersConfig.isInherited());
+ assertThat(ownersConfig.getLabel()).isPresent();
+
+ LabelDefinition label = ownersConfig.getLabel().get();
+ assertThat(label.getName()).isEqualTo(EXPECTED_LABEL);
+ assertThat(label.getScore()).hasValue(1);
+
+ Set<String> owners = ownersConfig.getOwners();
+ assertEquals(1, owners.size());
+ assertTrue(owners.contains(USER_C_EMAIL_COM));
+ }
+
+ @Test
+ public void testParsingYamlWithLabelWithoutScore() {
+ String yamlString =
+ "inherited: true\nlabel: " + EXPECTED_LABEL + "\nowners:\n- " + USER_C_EMAIL_COM;
+ Optional<OwnersConfig> config = getOwnersConfig(yamlString);
+
+ assertTrue(config.isPresent());
+
+ OwnersConfig ownersConfig = config.get();
+ assertTrue(ownersConfig.isInherited());
+ assertThat(ownersConfig.getLabel()).isPresent();
+
+ LabelDefinition label = ownersConfig.getLabel().get();
+ assertThat(label.getName()).isEqualTo(EXPECTED_LABEL);
+ assertThat(label.getScore()).isEmpty();
+
+ Set<String> owners = ownersConfig.getOwners();
+ assertEquals(1, owners.size());
+ assertTrue(owners.contains(USER_C_EMAIL_COM));
+ }
+
+ @Test
+ public void testPathOwnersEntriesCacheIsCalled() throws Exception {
+ expectConfig("OWNERS", "master", createConfig(true, Optional.of(EXPECTED_LABEL), owners()));
+ expectConfig(
+ "OWNERS",
+ RefNames.REFS_CONFIG,
+ repository,
+ createConfig(true, Optional.of("foo"), owners()));
+ expectConfig("dir/OWNERS", createConfig(true, Optional.of(A_LABEL), owners(USER_B_EMAIL_COM)));
+ expectConfig(
+ "dir/subdir/OWNERS",
+ createConfig(true, Optional.of(EXPECTED_LABEL), owners(USER_A_EMAIL_COM)));
+ expectConfig(
+ "OWNERS",
+ RefNames.REFS_CONFIG,
+ parentRepository1,
+ createConfig(true, Optional.of("bar"), owners()));
+
+ mockParentRepository(parentRepository1NameKey, parentRepository1);
+ replayAll();
+
+ PathOwnersEntriesCacheMock cacheMock = new PathOwnersEntriesCacheMock();
+ PathOwners owners =
+ new PathOwners(
+ accounts,
+ repositoryManager,
+ repository,
+ Arrays.asList(parentRepository1NameKey),
+ branch,
+ Set.of("dir/subdir/file.txt"),
+ EXPAND_GROUPS,
+ "foo",
+ cacheMock);
+
+ assertThat(owners.getFileOwners()).isNotEmpty();
+ int expectedCacheCalls =
+ 1 /* for refs/meta/config/OWNERS */
+ + 3 /* for each parent directory of 'file.txt' */
+ + 1 /* for parent's refs/meta/config/OWNERS */;
+ assertThat(cacheMock.hit).isEqualTo(expectedCacheCalls);
}
private void mockOwners(String... owners) throws IOException {
expectNoConfig("OWNERS");
expectConfig(CLASSIC_OWNERS, createConfig(false, owners(owners)));
- creatingPatchList(Arrays.asList(CLASSIC_FILE_TXT));
replayAll();
}
}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
index 8fea7ed..ac011b0 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
@@ -22,13 +22,12 @@
import static com.googlesource.gerrit.owners.common.MatcherConfig.regexMatcher;
import static com.googlesource.gerrit.owners.common.MatcherConfig.suffixMatcher;
import static com.googlesource.gerrit.owners.common.StreamUtils.iteratorStream;
-import static java.util.Collections.EMPTY_LIST;
+import static java.util.Collections.emptyList;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.powermock.api.easymock.PowerMock.replayAll;
import com.google.gerrit.entities.Account;
-import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
@@ -141,21 +140,27 @@
expectConfig("OWNERS", parentConfig);
expectConfig("project/OWNERS", childConfig);
- creatingPatchList(
- Arrays.asList(
- "project/file.txt", // matches exact in
- // project owners d,e
- "file1.txt", // no matches so nothing for this
- "project/afile2.sql", // matches two matchers so we have b,c,d
- "project/bfile.txt", // no matching
- "projectalfa", // matches PartialRegex
- "project/file.sql")); // only .sql matching b,c
replayAll();
// function under test
PathOwners owners =
new PathOwners(
- accounts, repositoryManager, repository, EMPTY_LIST, branch, patchList, EXPAND_GROUPS);
+ accounts,
+ repositoryManager,
+ repository,
+ emptyList(),
+ branch,
+ Set.of(
+ "project/file.txt", // matches exact in
+ // project owners d,e
+ "file1.txt", // no matches so nothing for this
+ "project/afile2.sql", // matches two matchers so we have b,c,d
+ "project/bfile.txt", // no matching
+ "projectalfa", // matches PartialRegex
+ "project/file.sql"), // only .sql matching b,c
+ EXPAND_GROUPS,
+ "foo",
+ new PathOwnersEntriesCacheMock());
// assertions on classic owners
Set<Account.Id> ownersSet = owners.get().get("project/OWNERS");
@@ -253,12 +258,19 @@
expectConfig("OWNERS", configString);
expectNoConfig("project/OWNERS");
- creatingPatch("project/file.sql", "another.txt");
replayAll();
PathOwners owners =
new PathOwners(
- accounts, repositoryManager, repository, EMPTY_LIST, branch, patchList, EXPAND_GROUPS);
+ accounts,
+ repositoryManager,
+ repository,
+ emptyList(),
+ branch,
+ Set.of("project/file.sql", "another.txt"),
+ EXPAND_GROUPS,
+ "foo",
+ new PathOwnersEntriesCacheMock());
Set<String> ownedFiles = owners.getFileOwners().keySet();
assertThat(ownedFiles).containsExactly("project/file.sql");
diff --git a/owners/BUILD b/owners/BUILD
index d04d83d..f08ec84 100644
--- a/owners/BUILD
+++ b/owners/BUILD
@@ -4,6 +4,7 @@
load("//tools/bzl:junit.bzl", "junit_tests")
PROLOG_PREDICATES = glob(["src/main/java/gerrit_owners/**/*.java"]) + [
+ "src/main/java/com/googlesource/gerrit/owners/OwnersMetrics.java",
"src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java",
]
@@ -11,8 +12,8 @@
name = "gerrit-owners-predicates",
srcs = PROLOG_PREDICATES,
deps = [
+ "//owners-common",
"@prolog-runtime//jar:neverlink",
- "//owners-common:owners-common",
] + PLUGIN_DEPS_NEVERLINK,
)
@@ -49,15 +50,37 @@
java_library(
name = "owners__plugin_test_deps",
testonly = 1,
+ srcs = glob(
+ ["src/test/java/**/*.java"],
+ exclude = [
+ "src/test/java/**/*Test.java",
+ "src/test/java/**/*IT.java",
+ ],
+ ),
visibility = ["//visibility:public"],
exports = PLUGIN_DEPS + PLUGIN_TEST_DEPS + [
":owners",
],
+ deps = PLUGIN_DEPS + PLUGIN_TEST_DEPS + [
+ ":owners",
+ ],
)
junit_tests(
name = "owners_tests",
- srcs = glob(["src/test/java/**/*.java"]),
+ srcs = glob(["src/test/java/**/*Test.java"]),
tags = ["owners"],
- deps = ["owners__plugin_test_deps"],
+ deps = [
+ ":owners__plugin_test_deps",
+ ],
)
+
+[junit_tests(
+ name = f[:f.index(".")].replace("/", "_"),
+ srcs = [f],
+ tags = ["owners"],
+ visibility = ["//visibility:public"],
+ deps = [
+ ":owners__plugin_test_deps",
+ ],
+) for f in glob(["src/test/java/**/*IT.java"])]
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnerPredicateProvider.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnerPredicateProvider.java
index 7b1399a..d15da8e 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnerPredicateProvider.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnerPredicateProvider.java
@@ -21,14 +21,19 @@
import com.google.gerrit.server.rules.PredicateProvider;
import com.google.inject.Inject;
import com.googlesource.gerrit.owners.common.Accounts;
+import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache;
import com.googlesource.gerrit.owners.common.PluginSettings;
/** Gerrit OWNERS Prolog Predicate Provider. */
@Listen
public class OwnerPredicateProvider implements PredicateProvider {
@Inject
- public OwnerPredicateProvider(Accounts accounts, PluginSettings config) {
- OwnersStoredValues.initialize(accounts, config);
+ public OwnerPredicateProvider(
+ Accounts accounts,
+ PluginSettings config,
+ PathOwnersEntriesCache cache,
+ OwnersMetrics metrics) {
+ OwnersStoredValues.initialize(accounts, config, cache, metrics);
}
@Override
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersApprovalHasOperand.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersApprovalHasOperand.java
new file mode 100644
index 0000000..ff00cfd
--- /dev/null
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersApprovalHasOperand.java
@@ -0,0 +1,52 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners;
+
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder.ChangeHasOperandFactory;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+
+/** Class contributing an "approval_owners" operand to the "has" predicate. */
+@Singleton
+class OwnersApprovalHasOperand implements ChangeHasOperandFactory {
+ static final String OPERAND = "approval";
+
+ static class OwnerApprovalHasOperandModule extends AbstractModule {
+ @Override
+ protected void configure() {
+ bind(ChangeHasOperandFactory.class)
+ .annotatedWith(Exports.named(OPERAND))
+ .to(OwnersApprovalHasOperand.class);
+ }
+ }
+
+ private final OwnersApprovalHasPredicate ownersApprovalHasPredicate;
+
+ @Inject
+ OwnersApprovalHasOperand(OwnersApprovalHasPredicate ownersApprovalHasPredicate) {
+ this.ownersApprovalHasPredicate = ownersApprovalHasPredicate;
+ }
+
+ @Override
+ public Predicate<ChangeData> create(ChangeQueryBuilder builder) throws QueryParseException {
+ return ownersApprovalHasPredicate;
+ }
+}
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersApprovalHasPredicate.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersApprovalHasPredicate.java
new file mode 100644
index 0000000..bb2d474
--- /dev/null
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersApprovalHasPredicate.java
@@ -0,0 +1,57 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners;
+
+import com.google.gerrit.entities.SubmitRecord;
+import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.SubmitRequirementPredicate;
+import com.google.gerrit.server.rules.SubmitRule;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.Optional;
+
+/**
+ * A predicate that checks if a given change has all necessary owner approvals. Matches with changes
+ * that have an owner approval. This predicate wraps the existing {@link OwnersSubmitRequirement}
+ * (that implements the {@link SubmitRule}) to perform the logic.
+ */
+@Singleton
+class OwnersApprovalHasPredicate extends SubmitRequirementPredicate {
+
+ private final OwnersSubmitRequirement ownersSubmitRequirement;
+
+ @Inject
+ OwnersApprovalHasPredicate(
+ @PluginName String pluginName, OwnersSubmitRequirement ownersSubmitRequirement) {
+ super("has", OwnersApprovalHasOperand.OPERAND + "_" + pluginName);
+ this.ownersSubmitRequirement = ownersSubmitRequirement;
+ }
+
+ @Override
+ public boolean match(ChangeData cd) {
+ Optional<SubmitRecord> submitRecord = ownersSubmitRequirement.evaluate(cd);
+ return submitRecord.map(sr -> sr.status == SubmitRecord.Status.OK).orElse(true);
+ }
+
+ /**
+ * Assuming that it is similarly expensive to calculate this as the 'code-owners' plugin hence
+ * giving the same value.
+ */
+ @Override
+ public int getCost() {
+ return 10;
+ }
+}
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersMetrics.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersMetrics.java
new file mode 100644
index 0000000..5cc8e19
--- /dev/null
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersMetrics.java
@@ -0,0 +1,60 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners;
+
+import com.google.gerrit.metrics.Counter0;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.Description.Units;
+import com.google.gerrit.metrics.MetricMaker;
+import com.google.gerrit.metrics.Timer0;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+
+@Singleton
+class OwnersMetrics {
+ final Counter0 countConfigLoads;
+ final Timer0 loadConfig;
+
+ final Counter0 countSubmitRuleRuns;
+ final Timer0 runSubmitRule;
+
+ @Inject
+ OwnersMetrics(MetricMaker metricMaker) {
+ this.countConfigLoads =
+ createCounter(
+ metricMaker, "count_configuration_loads", "Total number of owners configuration loads");
+ this.loadConfig =
+ createTimer(
+ metricMaker,
+ "load_configuration_latency",
+ "Latency for loading owners configuration for a change");
+
+ this.countSubmitRuleRuns =
+ createCounter(
+ metricMaker, "count_submit_rule_runs", "Total number of owners submit rule runs");
+ this.runSubmitRule =
+ createTimer(
+ metricMaker, "run_submit_rule_latency", "Latency for running the owners submit rule");
+ }
+
+ private static Counter0 createCounter(MetricMaker metricMaker, String name, String description) {
+ return metricMaker.newCounter(name, new Description(description).setRate());
+ }
+
+ private static Timer0 createTimer(MetricMaker metricMaker, String name, String description) {
+ return metricMaker.newTimer(
+ name, new Description(description).setCumulative().setUnit(Units.MILLISECONDS));
+ }
+}
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersModule.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersModule.java
index e3a915b..7d2c4d5 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersModule.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersModule.java
@@ -15,16 +15,38 @@
package com.googlesource.gerrit.owners;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.server.rules.PredicateProvider;
import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache;
+import com.googlesource.gerrit.owners.common.PluginSettings;
public class OwnersModule extends AbstractModule {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final PluginSettings pluginSettings;
+
+ @Inject
+ OwnersModule(PluginSettings pluginSettings) {
+ this.pluginSettings = pluginSettings;
+ }
+
@Override
protected void configure() {
+ install(PathOwnersEntriesCache.module());
DynamicSet.bind(binder(), PredicateProvider.class)
.to(OwnerPredicateProvider.class)
.asEagerSingleton();
install(new OwnersRestApiModule());
+
+ if (pluginSettings.enableSubmitRequirement()) {
+ install(new OwnersSubmitRequirement.OwnersSubmitRequirementModule());
+ install(new OwnersApprovalHasOperand.OwnerApprovalHasOperandModule());
+ } else {
+ logger.atInfo().log(
+ "OwnersSubmitRequirement is disabled therefore it will not be evaluated.");
+ }
}
}
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
index 88a719c..c4d27e4 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
@@ -17,18 +17,20 @@
package com.googlesource.gerrit.owners;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.patch.PatchList;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.rules.StoredValue;
import com.google.gerrit.server.rules.StoredValues;
import com.googlecode.prolog_cafe.lang.Prolog;
import com.googlesource.gerrit.owners.common.Accounts;
import com.googlesource.gerrit.owners.common.PathOwners;
+import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache;
import com.googlesource.gerrit.owners.common.PluginSettings;
import java.util.List;
+import java.util.Map;
import java.util.Optional;
-import java.util.stream.Collectors;
import org.eclipse.jgit.lib.Repository;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -39,34 +41,39 @@
public static StoredValue<PathOwners> PATH_OWNERS;
- public static synchronized void initialize(Accounts accounts, PluginSettings settings) {
+ public static synchronized void initialize(
+ Accounts accounts,
+ PluginSettings settings,
+ PathOwnersEntriesCache cache,
+ OwnersMetrics metrics) {
if (PATH_OWNERS != null) {
return;
}
log.info("Initializing OwnerStoredValues");
PATH_OWNERS =
- new StoredValue<PathOwners>() {
+ new StoredValue<>() {
@Override
protected PathOwners createValue(Prolog engine) {
- PatchList patchList = StoredValues.PATCH_LIST.get(engine);
+ Map<String, FileDiffOutput> patchList = StoredValues.DIFF_LIST.get(engine);
Repository repository = StoredValues.REPOSITORY.get(engine);
ProjectState projectState = StoredValues.PROJECT_STATE.get(engine);
GitRepositoryManager gitRepositoryManager = StoredValues.REPO_MANAGER.get(engine);
- List<Project.NameKey> parentProjectsNameKeys =
- projectState.parents().stream()
- .map(ProjectState::getNameKey)
- .collect(Collectors.toList());
-
- String branch = StoredValues.getChange(engine).getDest().branch();
- return new PathOwners(
- accounts,
- gitRepositoryManager,
- repository,
- parentProjectsNameKeys,
- settings.isBranchDisabled(branch) ? Optional.empty() : Optional.of(branch),
- patchList,
- settings.expandGroups());
+ metrics.countConfigLoads.increment();
+ try (Timer0.Context ctx = metrics.loadConfig.start()) {
+ List<Project.NameKey> parentProjectsNameKeys = PathOwners.getParents(projectState);
+ String branch = StoredValues.getChange(engine).getDest().branch();
+ return new PathOwners(
+ accounts,
+ gitRepositoryManager,
+ repository,
+ parentProjectsNameKeys,
+ settings.isBranchDisabled(branch) ? Optional.empty() : Optional.of(branch),
+ patchList,
+ settings.expandGroups(),
+ projectState.getName(),
+ cache);
+ }
}
};
}
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
new file mode 100644
index 0000000..0a2c5dd
--- /dev/null
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
@@ -0,0 +1,366 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners;
+
+import static com.google.gerrit.server.project.ProjectCache.illegalState;
+import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toSet;
+
+import com.google.common.base.Joiner;
+import com.google.common.collect.Streams;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.LabelFunction;
+import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.LabelTypes;
+import com.google.gerrit.entities.LegacySubmitRequirement;
+import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.SubmitRecord;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.metrics.Timer0;
+import com.google.gerrit.server.approval.ApprovalsUtil;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
+import com.google.gerrit.server.patch.DiffOperations;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.rules.SubmitRule;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import com.googlesource.gerrit.owners.common.Accounts;
+import com.googlesource.gerrit.owners.common.LabelDefinition;
+import com.googlesource.gerrit.owners.common.PathOwners;
+import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache;
+import com.googlesource.gerrit.owners.common.PluginSettings;
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+
+@Singleton
+public class OwnersSubmitRequirement implements SubmitRule {
+ public static class OwnersSubmitRequirementModule extends AbstractModule {
+ @Override
+ public void configure() {
+ bind(SubmitRule.class)
+ .annotatedWith(Exports.named("OwnersSubmitRequirement"))
+ .to(OwnersSubmitRequirement.class);
+ }
+ }
+
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private static final LegacySubmitRequirement SUBMIT_REQUIREMENT =
+ LegacySubmitRequirement.builder().setFallbackText("Owners").setType("owners").build();
+
+ private final OwnersMetrics metrics;
+ private final PluginSettings pluginSettings;
+ private final ProjectCache projectCache;
+ private final Accounts accounts;
+ private final GitRepositoryManager repoManager;
+ private final DiffOperations diffOperations;
+ private final ApprovalsUtil approvalsUtil;
+ private final PathOwnersEntriesCache cache;
+
+ @Inject
+ OwnersSubmitRequirement(
+ OwnersMetrics metrics,
+ PluginSettings pluginSettings,
+ ProjectCache projectCache,
+ Accounts accounts,
+ GitRepositoryManager repoManager,
+ DiffOperations diffOperations,
+ ApprovalsUtil approvalsUtil,
+ PathOwnersEntriesCache cache) {
+ this.metrics = metrics;
+ this.pluginSettings = pluginSettings;
+ this.projectCache = projectCache;
+ this.accounts = accounts;
+ this.repoManager = repoManager;
+ this.diffOperations = diffOperations;
+ this.approvalsUtil = approvalsUtil;
+ this.cache = cache;
+ }
+
+ @Override
+ public Optional<SubmitRecord> evaluate(ChangeData cd) {
+ requireNonNull(cd, "changeData");
+
+ Change change = cd.change();
+ Project.NameKey project = cd.project();
+ int changeId = cd.getId().get();
+ if (change.isClosed()) {
+ logger.atFine().log(
+ "Project '%s': change #%d is closed therefore OWNERS submit requirements are skipped.",
+ project, changeId);
+ return Optional.empty();
+ }
+
+ metrics.countSubmitRuleRuns.increment();
+ try (Timer0.Context ctx = metrics.runSubmitRule.start()) {
+ ProjectState projectState = getProjectState(project);
+ PathOwners pathOwners = getPathOwners(cd, projectState);
+ Map<String, Set<Account.Id>> fileOwners = pathOwners.getFileOwners();
+ if (fileOwners.isEmpty()) {
+ logger.atFinest().log(
+ "Project '%s': change #%d has no OWNERS submit requirements defined. "
+ + "Skipping submit requirements.",
+ project, changeId);
+ return Optional.empty();
+ }
+
+ ChangeNotes notes = cd.notes();
+ requireNonNull(notes, "notes");
+ LabelTypes labelTypes = projectState.getLabelTypes(notes);
+ LabelDefinition label = resolveLabel(labelTypes, pathOwners.getLabel());
+ Optional<LabelAndScore> ownersLabel = ownersLabel(labelTypes, label, project);
+
+ Map<Account.Id, List<PatchSetApproval>> approvalsByAccount =
+ Streams.stream(approvalsUtil.byPatchSet(notes, cd.currentPatchSet().id()))
+ .collect(Collectors.groupingBy(PatchSetApproval::accountId));
+
+ Account.Id uploader = notes.getCurrentPatchSet().uploader();
+
+ Set<String> missingApprovals =
+ fileOwners.entrySet().stream()
+ .filter(
+ requiredApproval ->
+ ownersLabel
+ .map(
+ ol ->
+ isApprovalMissing(
+ requiredApproval, uploader, approvalsByAccount, ol))
+ .orElse(true))
+ .map(Map.Entry::getKey)
+ .collect(toSet());
+
+ return Optional.of(
+ missingApprovals.isEmpty()
+ ? ok()
+ : notReady(
+ label.getName(),
+ String.format(
+ "Missing approvals for path(s): [%s]",
+ Joiner.on(", ").join(missingApprovals))));
+ } catch (IOException e) {
+ String msg =
+ String.format(
+ "Project '%s': repository cannot be opened to evaluate OWNERS submit requirements.",
+ project);
+ logger.atSevere().withCause(e).log("%s", msg);
+ throw new IllegalStateException(msg, e);
+ } catch (DiffNotAvailableException e) {
+ String msg =
+ String.format(
+ "Project '%s' change #%d: unable to get diff to evaluate OWNERS submit requirements.",
+ project, changeId);
+ logger.atSevere().withCause(e).log("%s", msg);
+ throw new IllegalStateException(msg, e);
+ }
+ }
+
+ private ProjectState getProjectState(Project.NameKey project) {
+ ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
+ if (projectState.hasPrologRules()) {
+ logger.atInfo().atMostEvery(1, TimeUnit.DAYS).log(
+ "Project '%s' has prolog rules enabled. "
+ + "It may interfere with the OWNERS submit requirements evaluation.",
+ project);
+ }
+ return projectState;
+ }
+
+ private PathOwners getPathOwners(ChangeData cd, ProjectState projectState)
+ throws IOException, DiffNotAvailableException {
+ metrics.countConfigLoads.increment();
+ try (Timer0.Context ctx = metrics.loadConfig.start()) {
+ String branch = cd.change().getDest().branch();
+
+ List<Project.NameKey> parents = PathOwners.getParents(projectState);
+ Project.NameKey nameKey = projectState.getNameKey();
+ try (Repository repo = repoManager.openRepository(nameKey)) {
+ PathOwners pathOwners =
+ new PathOwners(
+ accounts,
+ repoManager,
+ repo,
+ parents,
+ pluginSettings.isBranchDisabled(branch) ? Optional.empty() : Optional.of(branch),
+ getDiff(nameKey, cd.currentPatchSet().commitId()),
+ pluginSettings.expandGroups(),
+ nameKey.get(),
+ cache);
+
+ return pathOwners;
+ }
+ }
+ }
+
+ /**
+ * The idea is to select the label type that is configured for owner to cast the vote. If nothing
+ * is configured in the OWNERS file then `Code-Review` will be selected.
+ *
+ * @param labelTypes labels configured for project
+ * @param label and score definition that is configured in the OWNERS file
+ */
+ static LabelDefinition resolveLabel(LabelTypes labelTypes, Optional<LabelDefinition> label) {
+ return label.orElse(LabelDefinition.CODE_REVIEW);
+ }
+
+ /**
+ * Create {@link LabelAndScore} definition with a label LabelType if label can be found or empty
+ * otherwise. Note that score definition is copied from the OWNERS.
+ *
+ * @param labelTypes labels configured for project
+ * @param label and score definition (optional) that is resolved from the OWNERS file
+ * @param project that change is evaluated for
+ */
+ static Optional<LabelAndScore> ownersLabel(
+ LabelTypes labelTypes, LabelDefinition label, Project.NameKey project) {
+ return labelTypes
+ .byLabel(label.getName())
+ .map(type -> new LabelAndScore(type, label.getScore()))
+ .or(
+ () -> {
+ logger.atSevere().log(
+ "OWNERS label '%s' is not configured for '%s' project. Change is not submittable.",
+ label, project);
+ return Optional.empty();
+ });
+ }
+
+ static boolean isApprovalMissing(
+ Map.Entry<String, Set<Account.Id>> requiredApproval,
+ Account.Id uploader,
+ Map<Account.Id, List<PatchSetApproval>> approvalsByAccount,
+ LabelAndScore ownersLabel) {
+ return requiredApproval.getValue().stream()
+ .noneMatch(
+ fileOwner -> isApprovedByOwner(fileOwner, uploader, approvalsByAccount, ownersLabel));
+ }
+
+ static boolean isApprovedByOwner(
+ Account.Id fileOwner,
+ Account.Id uploader,
+ Map<Account.Id, List<PatchSetApproval>> approvalsByAccount,
+ LabelAndScore ownersLabel) {
+ return Optional.ofNullable(approvalsByAccount.get(fileOwner))
+ .map(
+ approvals ->
+ approvals.stream()
+ .anyMatch(
+ approval ->
+ hasSufficientApproval(approval, ownersLabel, fileOwner, uploader)))
+ .orElse(false);
+ }
+
+ static boolean hasSufficientApproval(
+ PatchSetApproval approval,
+ LabelAndScore ownersLabel,
+ Account.Id fileOwner,
+ Account.Id uploader) {
+ return ownersLabel.getLabelType().getLabelId().equals(approval.labelId())
+ && isLabelApproved(
+ ownersLabel.getLabelType(), ownersLabel.getScore(), fileOwner, uploader, approval);
+ }
+
+ static boolean isLabelApproved(
+ LabelType label,
+ Optional<Short> score,
+ Account.Id fileOwner,
+ Account.Id uploader,
+ PatchSetApproval approval) {
+ if (label.isIgnoreSelfApproval() && fileOwner.equals(uploader)) {
+ return false;
+ }
+
+ return score
+ .map(value -> approval.value() >= value)
+ .orElseGet(
+ () -> {
+ LabelFunction function = label.getFunction();
+ if (function.isMaxValueRequired()) {
+ return label.isMaxPositive(approval);
+ }
+
+ if (function.isBlock() && label.isMaxNegative(approval)) {
+ return false;
+ }
+
+ return approval.value() > label.getDefaultValue();
+ });
+ }
+
+ static class LabelAndScore {
+ private final LabelType labelType;
+ private final Optional<Short> score;
+
+ LabelAndScore(LabelType labelType, Optional<Short> score) {
+ this.labelType = labelType;
+ this.score = score;
+ }
+
+ LabelType getLabelType() {
+ return labelType;
+ }
+
+ Optional<Short> getScore() {
+ return score;
+ }
+ }
+
+ private Map<String, FileDiffOutput> getDiff(Project.NameKey project, ObjectId revision)
+ throws DiffNotAvailableException {
+ requireNonNull(project, "project");
+ requireNonNull(revision, "revision");
+
+ // Use parentNum=0 to do the comparison against the default base.
+ // For non-merge commits the default base is the only parent (aka parent 1, initial commits
+ // are not supported).
+ // For merge commits the default base is the auto-merge commit which should be used as base IOW
+ // only the changes from it should be reviewed as changes against the parent 1 were already
+ // reviewed
+ return diffOperations.listModifiedFilesAgainstParent(project, revision, 0);
+ }
+
+ private static SubmitRecord notReady(String ownersLabel, String missingApprovals) {
+ SubmitRecord submitRecord = new SubmitRecord();
+ submitRecord.status = SubmitRecord.Status.NOT_READY;
+ submitRecord.errorMessage = missingApprovals;
+ submitRecord.requirements = List.of(SUBMIT_REQUIREMENT);
+ SubmitRecord.Label label = new SubmitRecord.Label();
+ label.label = String.format("%s from owners", ownersLabel);
+ label.status = SubmitRecord.Label.Status.NEED;
+ submitRecord.labels = List.of(label);
+ return submitRecord;
+ }
+
+ private static SubmitRecord ok() {
+ SubmitRecord submitRecord = new SubmitRecord();
+ submitRecord.status = SubmitRecord.Status.OK;
+ submitRecord.requirements = List.of(SUBMIT_REQUIREMENT);
+ return submitRecord;
+ }
+}
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
index 9030bfc..02581de 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
@@ -16,10 +16,11 @@
package com.googlesource.gerrit.owners.restapi;
import com.google.common.collect.Maps;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelId;
-import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.client.ListChangesOption;
@@ -27,87 +28,98 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.patch.PatchList;
-import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.project.ProjectState;
+import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import com.googlesource.gerrit.owners.common.Accounts;
import com.googlesource.gerrit.owners.common.PathOwners;
+import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache;
import com.googlesource.gerrit.owners.common.PluginSettings;
import com.googlesource.gerrit.owners.entities.FilesOwnersResponse;
import com.googlesource.gerrit.owners.entities.GroupOwner;
import com.googlesource.gerrit.owners.entities.Owner;
+import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.Repository;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
@Singleton
public class GetFilesOwners implements RestReadView<RevisionResource> {
- private static final Logger log = LoggerFactory.getLogger(GetFilesOwners.class);
-
- private final PatchListCache patchListCache;
private final Accounts accounts;
private final AccountCache accountCache;
private final ProjectCache projectCache;
private final GitRepositoryManager repositoryManager;
private final PluginSettings pluginSettings;
private final GerritApi gerritApi;
+ private final ChangeData.Factory changeDataFactory;
+ private final PathOwnersEntriesCache cache;
+
+ static final String MISSING_CODE_REVIEW_LABEL =
+ String.format(
+ "Cannot calculate file onwers state when %s label is not configured",
+ LabelId.CODE_REVIEW);
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@Inject
GetFilesOwners(
- PatchListCache patchListCache,
Accounts accounts,
AccountCache accountCache,
ProjectCache projectCache,
GitRepositoryManager repositoryManager,
PluginSettings pluginSettings,
- GerritApi gerritApi) {
- this.patchListCache = patchListCache;
+ GerritApi gerritApi,
+ ChangeData.Factory changeDataFactory,
+ PathOwnersEntriesCache cache) {
this.accounts = accounts;
this.accountCache = accountCache;
this.projectCache = projectCache;
this.repositoryManager = repositoryManager;
this.pluginSettings = pluginSettings;
this.gerritApi = gerritApi;
+ this.changeDataFactory = changeDataFactory;
+ this.cache = cache;
}
@Override
public Response<FilesOwnersResponse> apply(RevisionResource revision)
throws AuthException, BadRequestException, ResourceConflictException, Exception {
- PatchSet ps = revision.getPatchSet();
Change change = revision.getChange();
- short codeReviewMaxValue =
+ Short codeReviewMaxValue =
revision
.getChangeResource()
.getChangeData()
.getLabelTypes()
.byLabel(LabelId.CODE_REVIEW)
- .getMaxPositive();
+ .map(LabelType::getMaxPositive)
+ .orElseThrow(
+ () -> {
+ logger.atInfo().log(
+ "Project %s has no Code-Review label configured hence getting file owners is not possible.",
+ revision.getProject());
+ return new ResourceNotFoundException(MISSING_CODE_REVIEW_LABEL);
+ });
int id = revision.getChangeResource().getChange().getChangeId();
+ Project.NameKey project = change.getProject();
List<Project.NameKey> projectParents =
- projectCache.get(change.getProject()).stream()
- .flatMap(s -> s.parents().stream())
- .map(ProjectState::getNameKey)
- .collect(Collectors.toList());
+ projectCache.get(project).map(PathOwners::getParents).orElse(Collections.emptyList());
- try (Repository repository = repositoryManager.openRepository(change.getProject())) {
- PatchList patchList = patchListCache.get(change, ps);
+ try (Repository repository = repositoryManager.openRepository(project)) {
+ Set<String> changePaths = new HashSet<>(changeDataFactory.create(change).currentFilePaths());
String branch = change.getDest().branch();
PathOwners owners =
@@ -117,8 +129,10 @@
repository,
projectParents,
pluginSettings.isBranchDisabled(branch) ? Optional.empty() : Optional.of(branch),
- patchList,
- pluginSettings.expandGroups());
+ changePaths,
+ pluginSettings.expandGroups(),
+ project.get(),
+ cache);
Map<String, Set<GroupOwner>> fileExpandedOwners =
Maps.transformValues(
@@ -196,7 +210,7 @@
changeInfo.labels.forEach(
(label, labelInfo) -> {
Optional.ofNullable(labelInfo.all)
- .map(
+ .ifPresent(
approvalInfos -> {
approvalInfos.forEach(
approvalInfo -> {
@@ -206,7 +220,6 @@
currentOwnerLabels.put(label, approvalInfo.value);
ownerToLabels.put(currentOwnerId, currentOwnerLabels);
});
- return ownerToLabels;
});
});
diff --git a/owners/src/main/java/gerrit_owners/PRED_code_review_user_1.java b/owners/src/main/java/gerrit_owners/PRED_code_review_user_1.java
index 1ac2da8..a2dd06b 100644
--- a/owners/src/main/java/gerrit_owners/PRED_code_review_user_1.java
+++ b/owners/src/main/java/gerrit_owners/PRED_code_review_user_1.java
@@ -54,7 +54,7 @@
ChangeData cd = StoredValues.CHANGE_DATA.get(engine);
Optional<LabelValue> codeReviewMaxValue =
- Optional.ofNullable(cd.getLabelTypes().byLabel(LabelId.CODE_REVIEW)).map(LabelType::getMax);
+ cd.getLabelTypes().byLabel(LabelId.CODE_REVIEW).map(LabelType::getMax);
Iterator<Account.Id> approvalsAccounts =
cd.currentApprovals().stream()
diff --git a/owners/src/main/resources/Documentation/config.md b/owners/src/main/resources/Documentation/config.md
index 92b7d5e..e8ae0eb 100644
--- a/owners/src/main/resources/Documentation/config.md
+++ b/owners/src/main/resources/Documentation/config.md
@@ -25,6 +25,37 @@
expandGroups = false
```
+owners.enableSubmitRequirement
+: If set to `true` the approvals are evaluated through the owners submit rule without a need of
+ prolog predicate being added to a project. Defaults to `false`.
+
+Example:
+
+```
+[owners]
+ enableSubmitRequirement = true
+```
+
+cache."owners.path_owners_entries".memoryLimit
+: The cache is used to hold the parsed version of `OWNERS` files in the
+ repository so that when submit rules are calculated (either through prolog
+ or through submit requirements) it is not read over and over again. The
+ cache entry gets invalidated when `OWNERS` file branch is updated.
+ By default it follows default Gerrit's cache memory limit but it makes
+ sense to adjust it as a function of number of project that use the `owners`
+ plugin multiplied by average number of active branches (plus 1 for the
+ refs/meta/config) and average number of directories (as directory hierarchy
+ back to root is checked for the `OWNERS` file existence).
+ _Note that in opposite to the previous settings the modification needs to be
+ performed in the `$GERRIT_SITE/etc/gerrit.config` file._
+
+Example
+
+```
+[cache "owners.path_owners_entries"]
+ memoryLimit = 2048
+```
+
## Configuration
Owner approval is determined based on OWNERS files located in the same
@@ -34,6 +65,7 @@
```yaml
inherited: true
+label: Code-Review, 1
owners:
- some.email@example.com
- User Name
@@ -78,6 +110,14 @@
building an OWNERS hierarchy. It stops once it finds an OWNERS file that has
“inherited” set to false (by default it’s true.)
+> **NOTE:** The `label` value (default is `Code-Review`) is taken into
+> consideration only when `owners.enableSubmitRequirement = true`.
+> Owners scores are matched against the label specified in the property in
+> question.
+> The required label's score can be provided (by default label's scores
+> configuration is used) so that owners don't have to be granted with the
+> maximum label's score. Note that only single digit (0..9) is allowed.
+
For example, imagine the following tree:
```
@@ -130,6 +170,20 @@
- Doug Smith
```
+### When `owners.enableSubmitRequirement = true`
+
+Then Gerrit would:
+
+Evaluate default submit requirement which gives `OK` if no `Code-Review -2` is
+given and at least one `Code-Review +2` is being provided.
+
+Evaluate owners submit requirement to check if `Code-Review +2` is given by
+either 'John Doe' or 'Doug Smith'. If none of them has approved then
+`Code-Review from owners` requirement is added making the change not
+submittable.
+
+### When `owners.enableSubmitRequirement = false` (default)
+
And sample rules.pl that uses this predicate to enable the submit rule if
one of the owners has given a Code Review +2
@@ -164,6 +218,45 @@
- Doug Smith
```
+### When `owners.enableSubmitRequirement = true`
+
+This case is supported with the `Code-Review` label and `OWNERS` file
+modifications.
+
+The `OWNERS` file requires the label configuration to be added (here is the
+updated version):
+
+```yaml
+inherited: true
+label: Code-Review, 1
+owners:
+- John Doe
+- Doug Smith
+```
+
+But additionally one needs to modify the label on the particular project level
+to the following version:
+
+```
+[label "Code-Review"]
+ function = NoOp
+ defaultValue = 0
+ copyMinScore = true
+ copyAllScoresOnTrivialRebase = true
+ value = -2 This shall not be merged
+ value = -1 I would prefer this is not merged as is
+ value = 0 No score
+ value = +1 Looks good to me, but someone else must approve
+ value = +2 Looks good to me, approved
+```
+
+Note that `function` is set to `NoOp`.
+
+As a result Gerrit would make the change Submittable only if 'John Doe' or
+'Doug Smith' have provided at least a `Code-Review +1`.
+
+### When `owners.enableSubmitRequirement = false` (default)
+
And a rule which makes submittable a change if at least one of the owners has
given a +1 without taking into consideration any other label:
@@ -188,11 +281,10 @@
In this case, we need to grant specific people with the _Owner-Approved_ label
without necessarily having to give Code-Review +2 rights on the entire project.
-Amend the project.config as shown in (1) and add a new label; then give
-permissions to any registered user. Finally, define a small variant of the
-Prolog rules as shown in (2).
+Amend the project.config as shown in below and add a new label; then give
+permissions to any registered user.
-(1) Example fo the project config changes with the new label with values
+Example fo the project config changes with the new label with values
(label name and values are arbitrary)
```
@@ -208,7 +300,25 @@
label-Owner-Approved = -1..+1 group Registered Users
```
-(2) Define the project's rules.pl with an amended version of Example 1:
+### When `owners.enableSubmitRequirement = true`
+
+Given now an OWNERS configuration of:
+
+```yaml
+inherited: true
+label: Owner-Approved
+owners:
+- John Doe
+- Doug Smith
+```
+
+A change cannot be submitted until 'John Doe' or 'Doug Smith' add a label
+`Owner-Approved`, independently from being able to provide any Code-Review.
+
+### When `owners.enableSubmitRequirement = false` (default)
+
+Finally, define prolog rules as shown in below (an amended version of
+Example 1):
```prolog
submit_rule(S) :-
@@ -231,7 +341,30 @@
A change cannot be submitted until John Doe or Doug Smith add a label
"Owner-Approved", independently from being able to provide any Code-Review.
-## Example 4 - Owners based on matchers
+## Example 4 - OWNERS file without matchers, default Gerrit submit rules and owners Code-Review +1 required
+
+This is a variant of `example 3` when no additional label is created but owners
+shouldn't be granted with `Code-Review +2` for all project files as it might be
+outside of their comptenence/comfort zone.
+
+### When `owners.enableSubmitRequirement = true`
+
+Given an OWNERS configuration of:
+
+```yaml
+inherited: true
+label: Code-Review, 1
+owners:
+- John Doe
+- Doug Smith
+```
+
+A change cannot be submitted until 'John Doe' or 'Doug Smith' add
+`Code-Review+1` score. Note that regular developers still need to cast the
+`Code-Review+2` for a change to be submittable as default submit rule is
+still evaluated.
+
+## Example 5 - Owners based on matchers
Often the ownership comes from the developer's skills and competencies and
cannot be purely defined by the project's directory structure.
@@ -252,6 +385,18 @@
- Matt Designer
```
+### When `owners.enableSubmitRequirement = true`
+
+Then for any change that contains files with .sql or .css extensions, besides
+to the default Gerrit submit rules, the extra constraints on the additional
+owners of the modified files will be added. The final submit is enabled if both
+Gerrit default rules are satisfied and all the owners of the .sql files
+('Mister Dba') and the .css files (either 'John Creative' or 'Matt Designer')
+have provided their `Code-Review +2` feedback (as `Code-Review` is default
+label for owners submit requirement).
+
+### When `owners.enableSubmitRequirement = false` (default)
+
And a rules.pl of:
```prolog
@@ -269,7 +414,13 @@
(Mister Dba) and the .css files (either John Creative or Matt Designer) have
provided their Code-Review +2 feedback.
-## Example 5 - Owners details on a per-file basis
+## Example 6 - Owners details on a per-file basis
+
+### When `owners.enableSubmitRequirement = true`
+
+This case is not yet covered by the owners submit requirement implementation.
+
+### When `owners.enableSubmitRequirement = false` (default)
When using the owners with a series of matchers associated to different set of
owners, it may not be trivial to understand exactly *why* change is not approved
diff --git a/owners/src/main/resources/Documentation/metrcis.md b/owners/src/main/resources/Documentation/metrcis.md
new file mode 100644
index 0000000..8f3c9f6
--- /dev/null
+++ b/owners/src/main/resources/Documentation/metrcis.md
@@ -0,0 +1,19 @@
+Metrics
+=============
+
+The following metrics are always emitted:
+
+* plugins/owners/count_configuration_loads
+ : the total number of owners configuration loads.
+
+* plugins/owners/load_configuration_latency
+ : the latency for loading owners configuration for a change.
+
+When submit requirements are enabled (`owners.enableSubmitRequirement = true`)
+these are additionally emitted:
+
+* plugins/owners/count_submit_rule_runs
+ : the total number of owners submit rule runs.
+
+* plugins/owners/run_submit_rule_latency
+ : the latency for running the owners submit rule.
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersApprovalHasOperandIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersApprovalHasOperandIT.java
new file mode 100644
index 0000000..9fdb4b9
--- /dev/null
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersApprovalHasOperandIT.java
@@ -0,0 +1,109 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+// implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.gerrit.extensions.common.SubmitRequirementResultInfo.Status.SATISFIED;
+import static com.google.gerrit.extensions.common.SubmitRequirementResultInfo.Status.UNSATISFIED;
+
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.acceptance.UseLocalDisk;
+import com.google.gerrit.acceptance.config.GlobalPluginConfig;
+import com.google.gerrit.entities.SubmitRequirement;
+import com.google.gerrit.entities.SubmitRequirementExpression;
+import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.ListChangesOption;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.SubmitRequirementResultInfo;
+import com.google.gerrit.extensions.common.SubmitRequirementResultInfo.Status;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
+import com.google.gerrit.testing.ConfigSuite;
+import com.googlesource.gerrit.owners.common.LabelDefinition;
+import java.util.Collection;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Before;
+import org.junit.Test;
+
+@TestPlugin(name = "owners", sysModule = "com.googlesource.gerrit.owners.OwnersModule")
+@UseLocalDisk
+public class OwnersApprovalHasOperandIT extends OwnersSubmitRequirementITAbstract {
+ private static final String REQUIREMENT_NAME = "Owner-Approval";
+
+ // This configuration is needed on 3.5 only and should be removed during/after the merge to
+ // stable-3.6 as it is enabled there by default.
+ @ConfigSuite.Default
+ public static Config defaultConfig() {
+ Config cfg = new Config();
+ cfg.setString(
+ "experiments",
+ null,
+ "enabled",
+ ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS);
+ return cfg;
+ }
+
+ @Before
+ public void setup() throws Exception {
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName(REQUIREMENT_NAME)
+ .setSubmittabilityExpression(SubmitRequirementExpression.create("has:approval_owners"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldOwnersRequirementBeSatisfied() throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRoot(true, LabelDefinition.parse("Code-Review,1").get(), admin2);
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ ChangeInfo changeNotReady = changeApi.get(ListChangesOption.SUBMIT_REQUIREMENTS);
+ verifySubmitRequirements(changeNotReady.submitRequirements, REQUIREMENT_NAME, UNSATISFIED);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(ReviewInput.recommend());
+ ChangeInfo ownersVoteSufficient = forChange(r).get(ListChangesOption.SUBMIT_REQUIREMENTS);
+ verifySubmitRequirements(ownersVoteSufficient.submitRequirements, REQUIREMENT_NAME, SATISFIED);
+ }
+
+ private void verifySubmitRequirements(
+ Collection<SubmitRequirementResultInfo> requirements, String name, Status status) {
+ for (SubmitRequirementResultInfo requirement : requirements) {
+ if (requirement.name.equals(name) && requirement.status == status) {
+ return;
+ }
+ }
+
+ throw new AssertionError(
+ String.format(
+ "Could not find submit requirement %s with status %s (results = %s)",
+ name,
+ status,
+ requirements.stream()
+ .map(r -> String.format("%s=%s", r.name, r.status))
+ .collect(toImmutableList())));
+ }
+}
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersMetricsIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersMetricsIT.java
new file mode 100644
index 0000000..47e2510
--- /dev/null
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersMetricsIT.java
@@ -0,0 +1,70 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners;
+
+import static com.google.common.truth.Truth.assertWithMessage;
+
+import com.codahale.metrics.MetricRegistry;
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.acceptance.UseLocalDisk;
+import com.google.gerrit.acceptance.config.GlobalPluginConfig;
+import com.google.gerrit.entities.RefNames;
+import com.google.inject.Inject;
+import org.junit.Test;
+
+@TestPlugin(name = "owners", sysModule = "com.googlesource.gerrit.owners.OwnersModule")
+@UseLocalDisk
+public class OwnersMetricsIT extends LightweightPluginDaemonTest {
+ @Inject MetricRegistry metricRegistry;
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldOwnersMetricsBeAvailable() throws Exception {
+ // one needs to at least create the OWNERS file to have metrics emitted
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRoot(true, admin2);
+
+ assertMetricExists("plugins/owners/count_configuration_loads");
+ assertMetricExists("plugins/owners/load_configuration_latency");
+ assertMetricExists("plugins/owners/count_submit_rule_runs");
+ assertMetricExists("plugins/owners/run_submit_rule_latency");
+ }
+
+ private void assertMetricExists(String name) {
+ assertWithMessage(name).that(metricRegistry.getMetrics().get(name)).isNotNull();
+ }
+
+ private void addOwnerFileToRoot(boolean inherit, TestAccount u) throws Exception {
+ // Add OWNERS file to root:
+ //
+ // inherited: true
+ // owners:
+ // - u.email()
+ pushFactory
+ .create(
+ admin.newIdent(),
+ testRepo,
+ "Add OWNER file",
+ "OWNERS",
+ String.format("inherited: %s\nowners:\n- %s\n", inherit, u.email()))
+ .to(RefNames.fullName("master"))
+ .assertOkStatus();
+ }
+}
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java
new file mode 100644
index 0000000..0d5c2c5
--- /dev/null
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java
@@ -0,0 +1,23 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+// implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners;
+
+import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.acceptance.UseLocalDisk;
+
+@TestPlugin(name = "owners", sysModule = "com.googlesource.gerrit.owners.OwnersModule")
+@UseLocalDisk
+public class OwnersSubmitRequirementIT extends OwnersSubmitRequirementITAbstract {}
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementITAbstract.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementITAbstract.java
new file mode 100644
index 0000000..d7cea60
--- /dev/null
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementITAbstract.java
@@ -0,0 +1,515 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+// implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static com.google.gerrit.server.project.testing.TestLabels.labelBuilder;
+import static com.google.gerrit.server.project.testing.TestLabels.value;
+import static java.util.stream.Collectors.joining;
+
+import com.google.gerrit.acceptance.GitUtil;
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GlobalPluginConfig;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.LabelFunction;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.LegacySubmitRequirementInfo;
+import com.google.gerrit.extensions.common.SubmitRecordInfo;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.project.testing.TestLabels;
+import com.google.inject.Inject;
+import com.googlesource.gerrit.owners.common.LabelDefinition;
+import java.util.Collection;
+import java.util.stream.Stream;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
+import org.junit.Test;
+
+abstract class OwnersSubmitRequirementITAbstract extends LightweightPluginDaemonTest {
+ private static final LegacySubmitRequirementInfo NOT_READY =
+ new LegacySubmitRequirementInfo("NOT_READY", "Owners", "owners");
+ private static final LegacySubmitRequirementInfo READY =
+ new LegacySubmitRequirementInfo("OK", "Owners", "owners");
+
+ @Inject protected RequestScopeOperations requestScopeOperations;
+ @Inject private ProjectOperations projectOperations;
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldRequireAtLeastOneApprovalForMatchingPathFromOwner() throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ TestAccount user1 = accountCreator.user1();
+ addOwnerFileWithMatchersToRoot(true, ".md", admin2, user1);
+
+ PushOneCommit.Result r = createChange("Add a file", "README.md", "foo");
+ ChangeApi changeApi = forChange(r);
+ ChangeInfo changeNotReady = changeApi.get();
+ assertThat(changeNotReady.submittable).isFalse();
+ assertThat(changeNotReady.requirements).containsExactly(NOT_READY);
+
+ changeApi.current().review(ReviewInput.approve());
+ ChangeInfo changeNotReadyAfterSelfApproval = changeApi.get();
+ assertThat(changeNotReadyAfterSelfApproval.submittable).isFalse();
+ assertThat(changeNotReadyAfterSelfApproval.requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(ReviewInput.approve());
+ ChangeInfo changeReady = forChange(r).get();
+ assertThat(changeReady.submittable).isTrue();
+ assertThat(changeReady.requirements).containsExactly(READY);
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldNotRequireApprovalForNotMatchingPath() throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileWithMatchersToRoot(true, ".md", admin2);
+
+ PushOneCommit.Result r = createChange("Add a file", "README.txt", "foo");
+ ChangeApi changeApi = forChange(r);
+ ChangeInfo changeNotReady = changeApi.get();
+ assertThat(changeNotReady.submittable).isFalse();
+ assertThat(changeNotReady.requirements).isEmpty();
+
+ changeApi.current().review(ReviewInput.approve());
+ ChangeInfo changeReady = changeApi.get();
+ assertThat(changeReady.submittable).isTrue();
+ assertThat(changeReady.requirements).isEmpty();
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldRequireApprovalFromRootOwner() throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRoot(true, admin2);
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ ChangeInfo changeNotReady = changeApi.get();
+ assertThat(changeNotReady.submittable).isFalse();
+ assertThat(changeNotReady.requirements).containsExactly(NOT_READY);
+
+ changeApi.current().review(ReviewInput.approve());
+ ChangeInfo changeNotReadyAfterSelfApproval = changeApi.get();
+ assertThat(changeNotReadyAfterSelfApproval.submittable).isFalse();
+ assertThat(changeNotReadyAfterSelfApproval.requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(ReviewInput.approve());
+ ChangeInfo changeReady = forChange(r).get();
+ assertThat(changeReady.submittable).isTrue();
+ assertThat(changeReady.requirements).containsExactly(READY);
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldBlockOwnersApprovalForMaxNegativeVote() throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRoot(true, admin2);
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ ChangeInfo changeNotReady = changeApi.get();
+ assertThat(changeNotReady.submittable).isFalse();
+ assertThat(changeNotReady.requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(ReviewInput.approve());
+ ChangeInfo changeReady = forChange(r).get();
+ assertThat(changeReady.submittable).isTrue();
+ assertThat(changeReady.requirements).containsExactly(READY);
+
+ changeApi.current().review(ReviewInput.reject());
+ assertThat(forChange(r).get().submittable).isFalse();
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldRequireVerifiedApprovalEvenIfCodeOwnerApproved() throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRoot(true, admin2);
+
+ installVerifiedLabel();
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ assertThat(changeApi.get().submittable).isFalse();
+ assertThat(changeApi.get().requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(ReviewInput.approve());
+ assertThat(forChange(r).get().submittable).isFalse();
+ assertThat(forChange(r).get().requirements).containsExactly(READY);
+ verifyHasSubmitRecord(
+ forChange(r).get().submitRecords, LabelId.VERIFIED, SubmitRecordInfo.Label.Status.NEED);
+
+ changeApi.current().review(new ReviewInput().label(LabelId.VERIFIED, 1));
+ assertThat(changeApi.get().submittable).isTrue();
+ verifyHasSubmitRecord(
+ changeApi.get().submitRecords, LabelId.VERIFIED, SubmitRecordInfo.Label.Status.OK);
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldRequireCodeOwnerApprovalEvenIfVerifiedWasApproved() throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRoot(true, admin2);
+
+ installVerifiedLabel();
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ assertThat(changeApi.get().submittable).isFalse();
+ assertThat(changeApi.get().requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(new ReviewInput().label(LabelId.VERIFIED, 1));
+ ChangeInfo changeNotReady = forChange(r).get();
+ assertThat(changeNotReady.submittable).isFalse();
+ assertThat(changeNotReady.requirements).containsExactly(NOT_READY);
+ verifyHasSubmitRecord(
+ changeNotReady.submitRecords, LabelId.VERIFIED, SubmitRecordInfo.Label.Status.OK);
+
+ forChange(r).current().review(ReviewInput.approve());
+ ChangeInfo changeReady = forChange(r).get();
+ assertThat(changeReady.submittable).isTrue();
+ assertThat(changeReady.requirements).containsExactly(READY);
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldRequireConfiguredLabelByCodeOwner() throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ String labelId = "Foo";
+ addOwnerFileToRoot(true, LabelDefinition.parse(labelId).get(), admin2);
+
+ installLabel(labelId);
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ assertThat(changeApi.get().submittable).isFalse();
+ assertThat(changeApi.get().requirements).containsExactly(NOT_READY);
+
+ changeApi.current().review(ReviewInput.approve());
+ ChangeInfo changeStillNotReady = changeApi.get();
+ assertThat(changeStillNotReady.submittable).isFalse();
+ assertThat(changeStillNotReady.requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(new ReviewInput().label(labelId, 1));
+ ChangeInfo changeReady = forChange(r).get();
+ assertThat(changeReady.submittable).isTrue();
+ assertThat(changeReady.requirements).containsExactly(READY);
+ verifyHasSubmitRecord(changeReady.submitRecords, labelId, SubmitRecordInfo.Label.Status.OK);
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldRequireConfiguredLabelByCodeOwnerEvenIfItIsNotConfiguredForProject()
+ throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ String notExistinglabelId = "Foo";
+ addOwnerFileToRoot(true, LabelDefinition.parse(notExistinglabelId).get(), admin2);
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ assertThat(changeApi.get().submittable).isFalse();
+ assertThat(changeApi.get().requirements).containsExactly(NOT_READY);
+
+ changeApi.current().review(ReviewInput.approve());
+ ChangeInfo changeStillNotReady = changeApi.get();
+ assertThat(changeStillNotReady.submittable).isFalse();
+ assertThat(changeStillNotReady.requirements).containsExactly(NOT_READY);
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldRequireConfiguredLabelScoreByCodeOwner() throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRoot(true, LabelDefinition.parse("Code-Review,1").get(), admin2);
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ ChangeInfo changeNotReady = changeApi.get();
+ assertThat(changeNotReady.submittable).isFalse();
+ assertThat(changeNotReady.requirements).containsExactly(NOT_READY);
+
+ changeApi.current().review(ReviewInput.approve());
+ ChangeInfo changeNotReadyAfterSelfApproval = changeApi.get();
+ assertThat(changeNotReadyAfterSelfApproval.submittable).isFalse();
+ assertThat(changeNotReadyAfterSelfApproval.requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(ReviewInput.recommend());
+ ChangeInfo changeReadyWithOwnerScore = forChange(r).get();
+ assertThat(changeReadyWithOwnerScore.submittable).isTrue();
+ assertThat(changeReadyWithOwnerScore.requirements).containsExactly(READY);
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldConfiguredLabelScoreByCodeOwnerBeNotSufficientIfLabelRequiresMaxValue()
+ throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRoot(true, LabelDefinition.parse("Code-Review,1").get(), admin2);
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ ChangeInfo changeNotReady = changeApi.get();
+ assertThat(changeNotReady.submittable).isFalse();
+ assertThat(changeNotReady.requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(ReviewInput.recommend());
+ ChangeInfo ownersVoteNotSufficient = changeApi.get();
+ assertThat(ownersVoteNotSufficient.submittable).isFalse();
+ assertThat(ownersVoteNotSufficient.requirements).containsExactly(READY);
+ verifyHasSubmitRecord(
+ ownersVoteNotSufficient.submitRecords,
+ LabelId.CODE_REVIEW,
+ SubmitRecordInfo.Label.Status.NEED);
+
+ requestScopeOperations.setApiUser(admin.id());
+ forChange(r).current().review(ReviewInput.approve());
+ ChangeInfo changeReadyWithMaxScore = forChange(r).get();
+ assertThat(changeReadyWithMaxScore.submittable).isTrue();
+ assertThat(changeReadyWithMaxScore.requirements).containsExactly(READY);
+ verifyHasSubmitRecord(
+ changeReadyWithMaxScore.submitRecords,
+ LabelId.CODE_REVIEW,
+ SubmitRecordInfo.Label.Status.OK);
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldConfiguredLabelScoreByCodeOwnersOverwriteSubmitRequirement() throws Exception {
+ installLabel(TestLabels.codeReview().toBuilder().setFunction(LabelFunction.NO_OP).build());
+
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRoot(true, LabelDefinition.parse("Code-Review,1").get(), admin2);
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ ChangeInfo changeNotReady = changeApi.get();
+ assertThat(changeNotReady.submittable).isFalse();
+ assertThat(changeNotReady.requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(ReviewInput.recommend());
+ ChangeInfo ownersVoteSufficient = forChange(r).get();
+ assertThat(ownersVoteSufficient.submittable).isTrue();
+ assertThat(ownersVoteSufficient.requirements).containsExactly(READY);
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldRequireApprovalFromGrandParentProjectOwner() throws Exception {
+ Project.NameKey parentProjectName =
+ createProjectOverAPI("parent", allProjects, true, SubmitType.FAST_FORWARD_ONLY);
+ Project.NameKey childProjectName =
+ createProjectOverAPI("child", parentProjectName, true, SubmitType.FAST_FORWARD_ONLY);
+ TestRepository<InMemoryRepository> childRepo = cloneProject(childProjectName);
+
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRefsMetaConfig(true, admin2, allProjects);
+
+ PushOneCommit.Result r =
+ createCommitAndPush(childRepo, "refs/for/master", "Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ ChangeInfo changeNotReady = changeApi.get();
+ assertThat(changeNotReady.submittable).isFalse();
+ assertThat(changeNotReady.requirements).containsExactly(NOT_READY);
+
+ changeApi.current().review(ReviewInput.approve());
+ ChangeInfo changeNotReadyAfterSelfApproval = changeApi.get();
+ assertThat(changeNotReadyAfterSelfApproval.submittable).isFalse();
+ assertThat(changeNotReadyAfterSelfApproval.requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(ReviewInput.approve());
+ ChangeInfo changeReady = forChange(r).get();
+ assertThat(changeReady.submittable).isTrue();
+ assertThat(changeReady.requirements).containsExactly(READY);
+ }
+
+ private void verifyHasSubmitRecord(
+ Collection<SubmitRecordInfo> records, String label, SubmitRecordInfo.Label.Status status) {
+ assertThat(
+ records.stream()
+ .flatMap(record -> record.labels.stream())
+ .filter(l -> l.label.equals(label) && l.status == status)
+ .findAny())
+ .isPresent();
+ }
+
+ private void installVerifiedLabel() throws Exception {
+ installLabel(LabelId.VERIFIED);
+ }
+
+ private void installLabel(String labelId) throws Exception {
+ LabelType verified =
+ labelBuilder(labelId, value(1, "Verified"), value(0, "No score"), value(-1, "Fails"))
+ .setFunction(LabelFunction.MAX_WITH_BLOCK)
+ .build();
+
+ installLabel(verified);
+
+ String heads = RefNames.REFS_HEADS + "*";
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel(verified.getName()).ref(heads).group(REGISTERED_USERS).range(-1, 1))
+ .update();
+ }
+
+ private void installLabel(LabelType label) throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig().upsertLabelType(label);
+ u.save();
+ }
+ }
+
+ protected ChangeApi forChange(PushOneCommit.Result r) throws RestApiException {
+ return gApi.changes().id(r.getChangeId());
+ }
+
+ private void addOwnerFileWithMatchersToRoot(
+ boolean inherit, String extension, TestAccount... users) throws Exception {
+ // Add OWNERS file to root:
+ //
+ // inherited: true
+ // matchers:
+ // - suffix: extension
+ // owners:
+ // - u1.email()
+ // - ...
+ // - uN.email()
+ pushOwnersToMaster(
+ String.format(
+ "inherited: %s\nmatchers:\n" + "- suffix: %s\n owners:\n%s",
+ inherit,
+ extension,
+ Stream.of(users)
+ .map(user -> String.format(" - %s\n", user.email()))
+ .collect(joining())));
+ }
+
+ private void addOwnerFileToRoot(boolean inherit, TestAccount u) throws Exception {
+ // Add OWNERS file to root:
+ //
+ // inherited: true
+ // owners:
+ // - u.email()
+ pushOwnersToMaster(String.format("inherited: %s\nowners:\n- %s\n", inherit, u.email()));
+ }
+
+ private void addOwnerFileToRefsMetaConfig(
+ boolean inherit, TestAccount u, Project.NameKey projectName) throws Exception {
+ // Add OWNERS file to root:
+ //
+ // inherited: true
+ // owners:
+ // - u.email()
+ pushOwnersToRefsMetaConfig(
+ String.format("inherited: %s\nowners:\n- %s\n", inherit, u.email()), projectName);
+ }
+
+ protected void addOwnerFileToRoot(boolean inherit, LabelDefinition label, TestAccount u)
+ throws Exception {
+ // Add OWNERS file to root:
+ //
+ // inherited: true
+ // label: label,score # score is optional
+ // owners:
+ // - u.email()
+ pushOwnersToMaster(
+ String.format(
+ "inherited: %s\nlabel: %s\nowners:\n- %s\n",
+ inherit,
+ String.format(
+ "%s%s",
+ label.getName(),
+ label.getScore().map(value -> String.format(",%d", value)).orElse("")),
+ u.email()));
+ }
+
+ private void pushOwnersToMaster(String owners) throws Exception {
+ pushFactory
+ .create(admin.newIdent(), testRepo, "Add OWNER file", "OWNERS", owners)
+ .to(RefNames.fullName("master"))
+ .assertOkStatus();
+ }
+
+ private void pushOwnersToRefsMetaConfig(String owners, Project.NameKey projectName)
+ throws Exception {
+ TestRepository<InMemoryRepository> project = cloneProject(projectName);
+ GitUtil.fetch(project, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG);
+ project.reset(RefNames.REFS_CONFIG);
+ pushFactory
+ .create(admin.newIdent(), project, "Add OWNER file", "OWNERS", owners)
+ .to(RefNames.REFS_CONFIG)
+ .assertOkStatus();
+ }
+}
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementTest.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementTest.java
new file mode 100644
index 0000000..97113c0
--- /dev/null
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementTest.java
@@ -0,0 +1,464 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.owners;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
+import static com.google.gerrit.server.project.testing.TestLabels.codeReview;
+import static com.google.gerrit.server.project.testing.TestLabels.labelBuilder;
+import static com.google.gerrit.server.project.testing.TestLabels.value;
+import static com.google.gerrit.server.project.testing.TestLabels.verified;
+import static com.googlesource.gerrit.owners.OwnersSubmitRequirement.hasSufficientApproval;
+import static com.googlesource.gerrit.owners.OwnersSubmitRequirement.isApprovalMissing;
+import static com.googlesource.gerrit.owners.OwnersSubmitRequirement.isApprovedByOwner;
+import static com.googlesource.gerrit.owners.OwnersSubmitRequirement.isLabelApproved;
+import static com.googlesource.gerrit.owners.OwnersSubmitRequirement.ownersLabel;
+import static com.googlesource.gerrit.owners.OwnersSubmitRequirement.resolveLabel;
+import static org.mockito.Mockito.mock;
+
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.LabelFunction;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.LabelTypes;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.entities.Project;
+import com.googlesource.gerrit.owners.OwnersSubmitRequirement.LabelAndScore;
+import com.googlesource.gerrit.owners.common.LabelDefinition;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import org.junit.Test;
+
+public class OwnersSubmitRequirementTest {
+ private static String LABEL_ID = "foo";
+ private static LabelDefinition OWNERS_LABEL = LabelDefinition.parse(LABEL_ID).get();
+ private static LabelDefinition OWNERS_LABEL_WITH_SCORE =
+ LabelDefinition.parse(String.format("%s,1", LABEL_ID)).get();
+ private static int MAX_LABEL_VALUE = 1;
+ private static Project.NameKey PROJECT = Project.nameKey("project");
+
+ @Test
+ public void shouldResolveLabelToConfiguredOne() {
+ // when
+ LabelDefinition label = resolveLabel(null, Optional.of(OWNERS_LABEL));
+
+ // then
+ assertThat(label).isEqualTo(OWNERS_LABEL);
+ }
+
+ @Test
+ public void shouldResolveLabelToCodeReview() {
+ // given
+ LabelTypes types =
+ new LabelTypes(List.of(verified(), label().setFunction(LabelFunction.NO_BLOCK).build()));
+
+ // when
+ LabelDefinition label = resolveLabel(types, Optional.empty());
+
+ // then
+ assertThat(label).isEqualTo(LabelDefinition.CODE_REVIEW);
+ }
+
+ @Test
+ public void shouldOwnersLabelContainOnlyConfiguredLabelAndItsScore() {
+ // when
+ Optional<LabelAndScore> result =
+ ownersLabel(new LabelTypes(List.of(label().build())), OWNERS_LABEL_WITH_SCORE, PROJECT);
+
+ // then
+ assertThat(result.map(label -> label.getLabelType().getName())).hasValue(LABEL_ID);
+ assertThat(result.flatMap(LabelAndScore::getScore))
+ .isEqualTo(OWNERS_LABEL_WITH_SCORE.getScore());
+ }
+
+ @Test
+ public void shouldOwnersLabelBeEmptyIfNonExistingLabelIsConfigured() {
+ // when
+ Optional<LabelAndScore> result =
+ ownersLabel(new LabelTypes(List.of(codeReview())), OWNERS_LABEL, PROJECT);
+
+ // then
+ assertThat(result).isEmpty();
+ }
+
+ @Test
+ public void shouldApprovalBeMissingWhenSomeoneElseApproved() {
+ // given
+ Account.Id fileOwner = mock(Account.Id.class);
+ Account.Id uploader = mock(Account.Id.class);
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
+ Map<Account.Id, List<PatchSetApproval>> uploaderApproval =
+ Map.of(uploader, List.of(approvedBy(uploader, LABEL_ID, MAX_LABEL_VALUE)));
+
+ // when
+ boolean isApprovalMissing =
+ isApprovalMissing(
+ Map.entry("path", Set.of(fileOwner)), uploader, uploaderApproval, ownersLabel);
+
+ // then
+ assertThat(isApprovalMissing).isTrue();
+ }
+
+ @Test
+ public void shouldApprovalBeNotMissingWhenApprovedByFileOwner() {
+ // given
+ Account.Id fileOwner = mock(Account.Id.class);
+ Account.Id uploader = mock(Account.Id.class);
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
+ Map<Account.Id, List<PatchSetApproval>> fileOwnerApproval =
+ Map.of(fileOwner, List.of(approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE)));
+
+ // when
+ boolean isApprovalMissing =
+ isApprovalMissing(
+ Map.entry("path", Set.of(fileOwner)), uploader, fileOwnerApproval, ownersLabel);
+
+ // then
+ assertThat(isApprovalMissing).isFalse();
+ }
+
+ @Test
+ public void shouldApprovalBeNotMissingWhenApprovedByAtLeastOneOwner() {
+ // given
+ Account.Id fileOwnerA = mock(Account.Id.class);
+ Account.Id fileOwnerB = mock(Account.Id.class);
+ Account.Id uploader = mock(Account.Id.class);
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
+ Map<Account.Id, List<PatchSetApproval>> fileOwnerApproval =
+ Map.of(fileOwnerA, List.of(approvedBy(fileOwnerA, LABEL_ID, MAX_LABEL_VALUE)));
+
+ // when
+ boolean isApprovalMissing =
+ isApprovalMissing(
+ Map.entry("path", Set.of(fileOwnerA, fileOwnerB)),
+ uploader,
+ fileOwnerApproval,
+ ownersLabel);
+
+ // then
+ assertThat(isApprovalMissing).isFalse();
+ }
+
+ @Test
+ public void shouldNotBeApprovedByOwnerWhenSomeoneElseApproved() {
+ // given
+ Account.Id fileOwner = mock(Account.Id.class);
+ Account.Id uploader = mock(Account.Id.class);
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
+ Map<Account.Id, List<PatchSetApproval>> uploaderApproval =
+ Map.of(uploader, List.of(approvedBy(uploader, LABEL_ID, MAX_LABEL_VALUE)));
+
+ // when
+ boolean approvedByOwner =
+ isApprovedByOwner(fileOwner, fileOwner, uploaderApproval, ownersLabel);
+
+ // then
+ assertThat(approvedByOwner).isFalse();
+ }
+
+ @Test
+ public void shouldNotBeApprovedWhenApprovalGivenForDifferentLabel() {
+ // given
+ Account.Id fileOwner = mock(Account.Id.class);
+ LabelAndScore ownersLabel =
+ new LabelAndScore(
+ label().setName("bar").setFunction(LabelFunction.MAX_NO_BLOCK).build(),
+ Optional.empty());
+ Map<Account.Id, List<PatchSetApproval>> fileOwnerForDifferentLabelApproval =
+ Map.of(fileOwner, List.of(approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE)));
+
+ // when
+ boolean approvedByOwner =
+ isApprovedByOwner(fileOwner, fileOwner, fileOwnerForDifferentLabelApproval, ownersLabel);
+
+ // then
+ assertThat(approvedByOwner).isFalse();
+ }
+
+ @Test
+ public void shouldBeApprovedByOwner() {
+ // given
+ Account.Id fileOwner = mock(Account.Id.class);
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
+ Map<Account.Id, List<PatchSetApproval>> fileOwnerApproval =
+ Map.of(fileOwner, List.of(approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE)));
+
+ // when
+ boolean approvedByOwner =
+ isApprovedByOwner(fileOwner, fileOwner, fileOwnerApproval, ownersLabel);
+
+ // then
+ assertThat(approvedByOwner).isTrue();
+ }
+
+ @Test
+ public void shouldHaveNotSufficientApprovalWhenLabelIsNotApproved() {
+ // given
+ Account.Id fileOwner = mock(Account.Id.class);
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
+
+ // when
+ boolean hasSufficientApproval =
+ hasSufficientApproval(
+ approvedBy(fileOwner, LABEL_ID, 0), ownersLabel, fileOwner, fileOwner);
+
+ // then
+ assertThat(hasSufficientApproval).isFalse();
+ }
+
+ @Test
+ public void shouldHaveNotSufficientApprovalWhenLabelDoesntMatch() {
+ // given
+ Account.Id fileOwner = mock(Account.Id.class);
+
+ // when
+ boolean hasSufficientApproval =
+ hasSufficientApproval(
+ approvedBy(fileOwner, LABEL_ID, 0),
+ new LabelAndScore(
+ LabelType.builder("foo", Collections.emptyList()).build(), Optional.empty()),
+ fileOwner,
+ fileOwner);
+
+ // then
+ assertThat(hasSufficientApproval).isFalse();
+ }
+
+ @Test
+ public void shouldHaveSufficientApprovalWhenLabelIsApproved() {
+ // given
+ Account.Id fileOwner = mock(Account.Id.class);
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
+
+ // when
+ boolean hasSufficientApproval =
+ hasSufficientApproval(
+ approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE), ownersLabel, fileOwner, fileOwner);
+
+ // then
+ assertThat(hasSufficientApproval).isTrue();
+ }
+
+ @Test
+ public void labelShouldNotBeApprovedWhenSelfApprovalIsDisabledAndOwnerApproved() {
+ // given
+ LabelType ignoreSelfApproval = label().setIgnoreSelfApproval(true).build();
+ Account.Id fileOwner = mock(Account.Id.class);
+
+ // when
+ boolean approved =
+ isLabelApproved(
+ ignoreSelfApproval,
+ Optional.empty(),
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE));
+
+ // then
+ assertThat(approved).isFalse();
+ }
+
+ @Test
+ public void labelShouldNotBeApprovedWhenMaxValueIsRequiredButNotProvided() {
+ // given
+ LabelType maxValueRequired = label().setFunction(LabelFunction.MAX_NO_BLOCK).build();
+ Account.Id fileOwner = mock(Account.Id.class);
+
+ // when
+ boolean approved =
+ isLabelApproved(
+ maxValueRequired,
+ Optional.empty(),
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LABEL_ID, 0));
+
+ // then
+ assertThat(approved).isFalse();
+ }
+
+ @Test
+ public void labelShouldBeApprovedWhenMaxValueIsRequiredAndProvided() {
+ // given
+ LabelType maxValueRequired = label().setFunction(LabelFunction.MAX_NO_BLOCK).build();
+ Account.Id fileOwner = mock(Account.Id.class);
+
+ // when
+ boolean approved =
+ isLabelApproved(
+ maxValueRequired,
+ Optional.empty(),
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE));
+
+ // then
+ assertThat(approved).isTrue();
+ }
+
+ @Test
+ public void labelShouldBeApprovedWhenMaxValueIsRequiredButLowerScoreIsConfiguredForOwner() {
+ // given
+ LabelType maxValueRequired = codeReview();
+ Short ownersScore = 1;
+ Optional<Short> requiredOwnerScore = Optional.of(ownersScore);
+ Account.Id fileOwner = mock(Account.Id.class);
+
+ // when
+ boolean approved =
+ isLabelApproved(
+ maxValueRequired,
+ requiredOwnerScore,
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LabelId.CODE_REVIEW, ownersScore));
+
+ // then
+ assertThat(approved).isTrue();
+ }
+
+ @Test
+ public void
+ labelShouldNotBeApprovedWhenMaxValueIsRequiredLowerScoreIsConfiguredForOwnerAndTooLowScoreIsCast() {
+ // given
+ LabelType anyWithBlock =
+ label()
+ .setValues(
+ Arrays.asList(
+ value(3, "Great"),
+ value(2, "Approved"),
+ value(1, "Maybe"),
+ value(0, "No score"),
+ value(-1, "Blocked")))
+ .setFunction(LabelFunction.ANY_WITH_BLOCK)
+ .build();
+ Optional<Short> requiredOwnerScore = Optional.of((short) 2);
+ Short ownersScore = 1;
+ Account.Id fileOwner = mock(Account.Id.class);
+
+ // when
+ boolean approved =
+ isLabelApproved(
+ anyWithBlock,
+ requiredOwnerScore,
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LabelId.CODE_REVIEW, ownersScore));
+
+ // then
+ assertThat(approved).isFalse();
+ }
+
+ @Test
+ public void labelShouldNotBeApprovedWhenAnyValueWithBlockIsConfiguredAndMaxNegativeIsProvided() {
+ // given
+ LabelType anyWithBlock = label().setFunction(LabelFunction.ANY_WITH_BLOCK).build();
+ Account.Id fileOwner = mock(Account.Id.class);
+
+ // when
+ boolean approved =
+ isLabelApproved(
+ anyWithBlock,
+ Optional.empty(),
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LABEL_ID, -1));
+
+ // then
+ assertThat(approved).isFalse();
+ }
+
+ @Test
+ public void labelShouldBeApprovedWhenAnyValueWithBlockIsConfiguredAndPositiveValueIsProvided() {
+ // given
+ LabelType anyWithBlock =
+ label()
+ .setValues(
+ Arrays.asList(
+ value(2, "Approved"),
+ value(1, "OK"),
+ value(0, "No score"),
+ value(-1, "Blocked")))
+ .setFunction(LabelFunction.ANY_WITH_BLOCK)
+ .build();
+ Account.Id fileOwner = mock(Account.Id.class);
+
+ // when
+ boolean approved =
+ isLabelApproved(
+ anyWithBlock,
+ Optional.empty(),
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LABEL_ID, 1));
+
+ // then
+ assertThat(approved).isTrue();
+ }
+
+ @Test
+ public void labelShouldNotBeApprovedWhenAnyValueWithBlockIsConfiguredAndDefaultValueIsProvided() {
+ // given
+ LabelType anyWithBlock =
+ label()
+ .setValues(
+ Arrays.asList(
+ value(2, "Approved"),
+ value(1, "OK"),
+ value(0, "No score"),
+ value(-1, "Blocked")))
+ .setFunction(LabelFunction.ANY_WITH_BLOCK)
+ .build();
+ Account.Id fileOwner = mock(Account.Id.class);
+
+ // when
+ boolean approved =
+ isLabelApproved(
+ anyWithBlock,
+ Optional.empty(),
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LABEL_ID, 0));
+
+ // then
+ assertThat(approved).isFalse();
+ }
+
+ private static final LabelAndScore maxNoBlockLabelFooOwnersLabel() {
+ LabelType maxValueRequired = label().setFunction(LabelFunction.MAX_NO_BLOCK).build();
+ return new LabelAndScore(maxValueRequired, Optional.empty());
+ }
+
+ private static final LabelType.Builder label() {
+ return labelBuilder(
+ LABEL_ID, value(MAX_LABEL_VALUE, "Approved"), value(0, "No score"), value(-1, "Blocked"));
+ }
+
+ private static final PatchSetApproval approvedBy(Account.Id approving, String label, int value) {
+ return PatchSetApproval.builder()
+ .key(PatchSetApproval.key(mock(PatchSet.Id.class), approving, LabelId.create(label)))
+ .granted(Timestamp.from(Instant.now()))
+ .realAccountId(approving)
+ .value(value)
+ .build();
+ }
+}
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
index a6d7276..1c551ec 100644
--- a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
@@ -16,13 +16,17 @@
package com.googlesource.gerrit.owners.restapi;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit.Result;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.acceptance.config.GlobalPluginConfig;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.entities.RefNames;
@@ -30,7 +34,9 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
+import com.google.gerrit.server.project.testing.TestLabels;
import com.googlesource.gerrit.owners.entities.FilesOwnersResponse;
import com.googlesource.gerrit.owners.entities.GroupOwner;
import com.googlesource.gerrit.owners.entities.Owner;
@@ -45,7 +51,7 @@
import org.eclipse.jgit.util.FS;
import org.junit.Test;
-@TestPlugin(name = "owners", httpModule = "com.googlesource.gerrit.owners.OwnersRestApiModule")
+@TestPlugin(name = "owners", sysModule = "com.googlesource.gerrit.owners.OwnersModule")
@UseLocalDisk
public class GetFilesOwnersIT extends LightweightPluginDaemonTest {
@@ -176,6 +182,21 @@
@Test
@UseLocalDisk
+ public void shouldReflectChangesInParentProject() throws Exception {
+ addOwnerFileToProjectConfig(allProjects, true, admin);
+
+ String changeId = createChange().getChangeId();
+ Response<FilesOwnersResponse> resp =
+ assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
+ assertThat(resp.value().files).containsExactly("a.txt", Sets.newHashSet(rootOwner));
+
+ addOwnerFileToProjectConfig(allProjects, true, user);
+ resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
+ assertThat(resp.value().files).containsExactly("a.txt", Sets.newHashSet(projectOwner));
+ }
+
+ @Test
+ @UseLocalDisk
public void shouldNotReturnInheritedOwnersFromProjectsOwners() throws Exception {
assertNotInheritFromProject(project);
}
@@ -187,6 +208,30 @@
assertNotInheritFromProject(allProjects);
}
+ @Test
+ @UseLocalDisk
+ public void shouldThrowExceptionWhenCodeReviewLabelIsNotConfigured() throws Exception {
+ addOwnerFileToProjectConfig(project, false);
+ replaceCodeReviewWithLabel(
+ TestLabels.label(
+ "Foo", TestLabels.value(1, "Foo is fine"), TestLabels.value(-1, "Foo is not fine")));
+ String changeId = createChange().getChangeId();
+
+ ResourceNotFoundException thrown =
+ assertThrows(
+ ResourceNotFoundException.class,
+ () -> ownersApi.apply(parseCurrentRevisionResource(changeId)));
+ assertThat(thrown).hasMessageThat().isEqualTo(GetFilesOwners.MISSING_CODE_REVIEW_LABEL);
+ }
+
+ private void replaceCodeReviewWithLabel(LabelType label) throws Exception {
+ try (ProjectConfigUpdate u = updateProject(allProjects)) {
+ u.getConfig().getLabelSections().remove(LabelId.CODE_REVIEW);
+ u.getConfig().upsertLabelType(label);
+ u.save();
+ }
+ }
+
private static <T> Response<T> assertResponseOk(Response<T> response) {
assertThat(response.statusCode()).isEqualTo(HttpServletResponse.SC_OK);
return response;
@@ -222,6 +267,11 @@
private void addOwnerFileToProjectConfig(Project.NameKey projectNameKey, boolean inherit)
throws Exception {
+ addOwnerFileToProjectConfig(projectNameKey, inherit, user);
+ }
+
+ private void addOwnerFileToProjectConfig(
+ Project.NameKey projectNameKey, boolean inherit, TestAccount account) throws Exception {
TestRepository<InMemoryRepository> project = cloneProject(projectNameKey);
GitUtil.fetch(project, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG);
project.reset(RefNames.REFS_CONFIG);
@@ -233,7 +283,7 @@
"OWNERS",
String.format(
"inherited: %s\nmatchers:\n" + "- suffix: .txt\n owners:\n - %s\n",
- inherit, user.email()))
+ inherit, account.email()))
.to(RefNames.REFS_CONFIG);
}
