Introduce GenericMatcher for file-based regex fallback logic
Allow having fallback logic when using the RegexMatchers,
introducing a new specialised subclass called GenericMatcher
which would only be used when none of the other matchers
are selected.
This allows to define rules as the following one:
matchers:
- generic: .*
owners:
- group/developers
- regex: .*java
owners:
- group/java-developers
A change having the foo.java would then be assigned only
to group/java-developers, because the generic rule would
in theory match but is only used as fallback.
If a change contains two files foo.java and bar.c, the
foo.java will continue to be assigned to the group/java-developers
while the bar.c would be assigned to group/developers as
fallback rule.
Bug: Issue 16480
Change-Id: I2cffb42f72313a660e5f496c84b370a875f34907
diff --git a/owners-autoassign/src/main/resources/Documentation/config.md b/owners-autoassign/src/main/resources/Documentation/config.md
index fa8a30f..7de8eec 100644
--- a/owners-autoassign/src/main/resources/Documentation/config.md
+++ b/owners-autoassign/src/main/resources/Documentation/config.md
@@ -69,6 +69,11 @@
(partial or full) and exact string comparison. For exact match, path is
relative to the root of the repo.
+> **NOTE:** The `generic` matcher is a special type of regex matching that
+> is applied only when none of the other sections are matching. It is
+> used to define fallback rules. The `generic: .*` is the top-level fallback
+> and can be used with other more specific `generic` matchers.
+
The plugin analyzes the latest patch set by looking at each file directory and
building an OWNERS hierarchy. It stops once it finds an OWNERS file that has
“inherited” set to false (by default it’s true.)
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 3ca6f35..a38b7a3 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
@@ -19,6 +19,8 @@
import static com.googlesource.gerrit.owners.common.AutoassignConfigModule.PROJECT_CONFIG_AUTOASSIGN_FIELD;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.acceptance.config.GlobalPluginConfig;
import com.google.gerrit.common.RawInputUtil;
@@ -34,7 +36,9 @@
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
import java.util.Collection;
+import java.util.HashMap;
import java.util.List;
+import java.util.Map;
import java.util.stream.Collectors;
import org.junit.Ignore;
import org.junit.Test;
@@ -52,6 +56,10 @@
@Inject
private ProjectConfig.Factory projectConfigFactory;
+ private TestAccount user2;
+
+ private TestAccount admin2;
+
AbstractAutoassignIT(String section, ReviewerState assignedUserState) {
this.section = section;
this.assignedUserState = assignedUserState;
@@ -77,6 +85,9 @@
projectConfig.commit(md);
projectCache.evict(project);
}
+
+ user2 = accountCreator.user2();
+ admin2 = accountCreator.admin2();
}
@Test
@@ -187,7 +198,7 @@
public void shouldAutoassignUserMatchingPath() throws Exception {
String ownerEmail = user.email();
- addOwnersToRepo("", "suffix", ".java", ownerEmail, NOT_INHERITED);
+ addOwnersToRepo("", NOT_INHERITED, "suffix", ".java", ownerEmail);
Collection<AccountInfo> reviewers =
getAutoassignedAccounts(change(createChange("test change", "foo.java", "foo")).get());
@@ -200,7 +211,7 @@
public void shouldNotAutoassignUserNotMatchingPath() throws Exception {
String ownerEmail = user.email();
- addOwnersToRepo("", "suffix", ".java", ownerEmail, NOT_INHERITED);
+ addOwnersToRepo("", NOT_INHERITED, "suffix", ".java", ownerEmail);
ChangeApi changeApi = change(createChange("test change", "foo.bar", "foo"));
Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi.get());
@@ -209,13 +220,109 @@
}
@Test
+ public void shouldAutoassignUserWithGenericTopLevelFallback() throws Exception {
+ String ownerEmail = user.email();
+ String owner2Email = user2.email();
+ String admin2Email = admin2.email();
+
+ addOwnersToRepo(
+ "",
+ NOT_INHERITED,
+ "suffix",
+ ".java",
+ ownerEmail,
+ "generic",
+ ".*\\.c",
+ admin2Email,
+ "generic",
+ ".*",
+ owner2Email);
+
+ ChangeApi changeApi =
+ change(createChangeWithFiles("test change", "foo.bar", "foo", "foo.java", "Java code"));
+ Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi.get());
+
+ assertThat(reviewers).isNotNull();
+ assertThat(reviewersEmail(reviewers)).containsExactly(ownerEmail, owner2Email);
+ }
+
+ @Test
+ public void shouldAutoassignUserWithGenericMidLevelFallback() throws Exception {
+ String ownerEmail = user.email();
+ String owner2Email = user2.email();
+ String admin2Email = admin2.email();
+
+ addOwnersToRepo(
+ "",
+ NOT_INHERITED,
+ "suffix",
+ ".java",
+ ownerEmail,
+ "generic",
+ ".*\\.c",
+ admin2Email,
+ "generic",
+ ".*",
+ owner2Email);
+
+ ChangeApi changeApi =
+ change(createChangeWithFiles("test change", "foo.c", "foo", "foo.java", "Java code"));
+ Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi.get());
+
+ assertThat(reviewers).isNotNull();
+ assertThat(reviewersEmail(reviewers)).containsExactly(ownerEmail, admin2Email);
+ }
+
+ @Test
+ public void shouldNotAutoassignUserWithNonMatchingGenericFallback() throws Exception {
+ String ownerEmail = user.email();
+ String owner2Email = user2.email();
+
+ addOwnersToRepo(
+ "", NOT_INHERITED, "suffix", ".java", ownerEmail, "generic", "\\.c", owner2Email);
+
+ ChangeApi changeApi =
+ change(createChangeWithFiles("test change", "foo.bar", "foo", "foo.groovy", "Groovy code"));
+ Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi.get());
+
+ assertThat(reviewers).isNull();
+ }
+
+ @Test
+ public void shouldAutoassignUserWithMultipleGenericFallback() throws Exception {
+ String admin2Email = admin2.email();
+ String ownerEmail = user.email();
+ String owner2Email = user2.email();
+
+ addOwnersToRepo(
+ "",
+ NOT_INHERITED,
+ "suffix",
+ ".java",
+ admin2Email,
+ "generic",
+ ".*\\.c",
+ ownerEmail,
+ "generic",
+ ".*",
+ owner2Email);
+
+ ChangeApi changeApi =
+ change(createChangeWithFiles("test change", "foo.bar", "foo", "foo.c", "C code"));
+ Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi.get());
+
+ assertThat(reviewers).isNotNull();
+ assertThat(reviewersEmail(reviewers)).containsExactly(ownerEmail, owner2Email);
+ }
+
+ @Test
public void shouldAutoassignUserMatchingPathWithInheritance() throws Exception {
String childOwnersEmail = accountCreator.user2().email();
String parentOwnersEmail = user.email();
String childpath = "childpath/";
- addOwnersToRepo("", "suffix", ".java", parentOwnersEmail, NOT_INHERITED);
- addOwnersToRepo(childpath, "suffix", ".java", childOwnersEmail, INHERITED);
+ addOwnersToRepo("", NOT_INHERITED, "suffix", ".java", parentOwnersEmail);
+ addOwnersToRepo(childpath, INHERITED, "suffix", ".java", childOwnersEmail);
ChangeApi changeApi = change(createChange("test change", childpath + "foo.java", "foo"));
Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi.get());
@@ -231,7 +338,7 @@
String childpath = "childpath/";
addOwnersToRepo("", parentOwnersEmail, NOT_INHERITED);
- addOwnersToRepo(childpath, "suffix", ".java", childOwnersEmail, NOT_INHERITED);
+ addOwnersToRepo(childpath, NOT_INHERITED, "suffix", ".java", childOwnersEmail);
ChangeApi changeApi = change(createChange("test change", childpath + "foo.java", "foo"));
Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi.get());
@@ -240,6 +347,18 @@
assertThat(reviewersEmail(reviewers)).containsExactly(childOwnersEmail);
}
+ protected PushOneCommit.Result createChangeWithFiles(String subject, String... filesWithContent)
+ throws Exception {
+ Map<String, String> files = new HashMap<>();
+ for (int i = 0; i < filesWithContent.length; ) {
+ String fileName = filesWithContent[i++];
+ String fileContent = filesWithContent[i++];
+ files.put(fileName, fileContent);
+ }
+ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo, subject, files);
+ return push.to("refs/for/master");
+ }
+
private Collection<AccountInfo> getAutoassignedAccounts(ChangeInfo changeInfo)
throws RestApiException {
Collection<AccountInfo> reviewers =
@@ -265,33 +384,35 @@
.assertOkStatus();
}
- private void addOwnersToRepo(
- String parentPath,
- String matchingType,
- String patternMatch,
- String ownerEmail,
- boolean inherited)
+ private void addOwnersToRepo(String parentPath, boolean inherited, String... matchingRules)
throws Exception {
+ StringBuilder ownersStringBuilder =
+ new StringBuilder("inherited: " + inherited + "\n" + "matchers:\n");
+ for (int i = 0; i < matchingRules.length; ) {
+ String matchingType = matchingRules[i++];
+ String patternMatch = matchingRules[i++];
+ String ownerEmail = matchingRules[i++];
+
+ ownersStringBuilder
+ .append("- ")
+ .append(matchingType)
+ .append(": ")
+ .append(patternMatch)
+ .append("\n")
+ .append(" ")
+ .append(section)
+ .append(":\n")
+ .append(" - ")
+ .append(ownerEmail)
+ .append("\n");
+ }
pushFactory
.create(
admin.newIdent(),
testRepo,
"Set OWNERS",
parentPath + "OWNERS",
- "inherited: "
- + inherited
- + "\n"
- + "matchers:\n"
- + "- "
- + matchingType
- + ": "
- + patternMatch
- + "\n"
- + " "
- + section
- + ":\n"
- + " - "
- + ownerEmail)
+ ownersStringBuilder.toString())
.to("refs/heads/master")
.assertOkStatus();
}
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 02c3fdf..84d8f6c 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
@@ -119,6 +119,8 @@
.map(el -> new PartialRegExMatcher(el, owners, reviewers, groupOwners));
Optional<Matcher> exactMatcher =
getText(node, "exact").map(el -> new ExactMatcher(el, owners, reviewers, groupOwners));
+ Optional<Matcher> genericMatcher =
+ getText(node, "generic").map(el -> new GenericMatcher(el, owners, reviewers, groupOwners));
return Optional.ofNullable(
suffixMatcher.orElseGet(
@@ -128,10 +130,13 @@
partialRegexMatcher.orElseGet(
() ->
exactMatcher.orElseGet(
- () -> {
- log.warn("Ignoring invalid element " + node.toString());
- return null;
- })))));
+ () ->
+ genericMatcher.orElseGet(
+ () -> {
+ log.warn(
+ "Ignoring invalid element " + node.toString());
+ return null;
+ }))))));
}
private static Optional<String> getText(JsonNode node, String field) {
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/GenericMatcher.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/GenericMatcher.java
new file mode 100644
index 0000000..e37065b
--- /dev/null
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/GenericMatcher.java
@@ -0,0 +1,27 @@
+// 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.common;
+
+import com.google.gerrit.entities.Account;
+import java.util.Set;
+
+public class GenericMatcher extends RegExMatcher {
+
+ public GenericMatcher(
+ String path, Set<Account.Id> owners, Set<Account.Id> reviewers, Set<String> groupOwners) {
+ super(path, owners, reviewers, groupOwners);
+ }
+}
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 681b2ea..6159e08 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
@@ -25,6 +25,7 @@
import com.google.common.collect.Multimaps;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Account.Id;
import com.google.gerrit.entities.Patch;
@@ -37,11 +38,11 @@
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
-import java.util.Iterator;
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;
@@ -52,6 +53,18 @@
private static final Logger log = LoggerFactory.getLogger(PathOwners.class);
+ private enum MatcherLevel {
+ Regular,
+ Fallback,
+ CatchAll;
+
+ static MatcherLevel forMatcher(Matcher matcher) {
+ return matcher instanceof GenericMatcher
+ ? (matcher.path.equals(".*") ? CatchAll : Fallback)
+ : Regular;
+ }
+ }
+
private final SetMultimap<String, Account.Id> owners;
private final SetMultimap<String, Account.Id> reviewers;
@@ -221,16 +234,43 @@
HashMap<String, Matcher> newMatchers,
String path,
OwnersMap ownersMap) {
- Iterator<Matcher> it = fullMatchers.values().iterator();
- while (it.hasNext()) {
- Matcher matcher = it.next();
+
+ Map<MatcherLevel, List<Matcher>> matchersByLevel =
+ fullMatchers.values().stream().collect(Collectors.groupingBy(MatcherLevel::forMatcher));
+ if (findAndAddMatchers(
+ newMatchers, path, ownersMap, matchersByLevel.get(MatcherLevel.Regular))) {
+ return;
+ }
+
+ if (findAndAddMatchers(
+ newMatchers, path, ownersMap, matchersByLevel.get(MatcherLevel.Fallback))) {
+ return;
+ }
+
+ findAndAddMatchers(newMatchers, path, ownersMap, matchersByLevel.get(MatcherLevel.CatchAll));
+ }
+
+ private boolean findAndAddMatchers(
+ HashMap<String, Matcher> newMatchers,
+ String path,
+ OwnersMap ownersMap,
+ @Nullable List<Matcher> matchers) {
+ if (matchers == null) {
+ return false;
+ }
+
+ boolean matchingFound = false;
+
+ for (Matcher matcher : matchers) {
if (matcher.matches(path)) {
newMatchers.put(matcher.getPath(), matcher);
ownersMap.addFileOwners(path, matcher.getOwners());
ownersMap.addFileGroupOwners(path, matcher.getGroupOwners());
ownersMap.addFileReviewers(path, matcher.getReviewers());
+ matchingFound = true;
}
}
+ return matchingFound;
}
private PathOwnersEntry resolvePathEntry(
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/MatcherConfig.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/MatcherConfig.java
index a2b20d8..51e0b74 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/MatcherConfig.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/MatcherConfig.java
@@ -22,6 +22,7 @@
public static final String MATCH_REGEX = "regex";
public static final String MATCH_SUFFIX = "suffix";
public static final String MATCH_PARTIAL_REGEX = "partial_regex";
+ public static final String MATCH_GENERIC = "generic";
private final String matchType;
private final String matchExpr;
@@ -43,6 +44,10 @@
return new MatcherConfig(MATCH_PARTIAL_REGEX, expr, owners);
}
+ public static MatcherConfig genericMatcher(String expr, String... owners) {
+ return new MatcherConfig(MATCH_GENERIC, expr, owners);
+ }
+
public MatcherConfig(String matchType, String matchExpr, String[] owners) {
super();
this.matchType = matchType;
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 2bcf2a7..8fea7ed 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
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static com.googlesource.gerrit.owners.common.MatcherConfig.exactMatcher;
+import static com.googlesource.gerrit.owners.common.MatcherConfig.genericMatcher;
import static com.googlesource.gerrit.owners.common.MatcherConfig.partialRegexMatcher;
import static com.googlesource.gerrit.owners.common.MatcherConfig.regexMatcher;
import static com.googlesource.gerrit.owners.common.MatcherConfig.suffixMatcher;
@@ -232,7 +233,12 @@
replayAll();
Optional<OwnersConfig> ownersConfigOpt =
- getOwnersConfig(createConfig(false, new String[0], suffixMatcher(".txt", ACCOUNT_B)));
+ getOwnersConfig(
+ createConfig(
+ false,
+ new String[0],
+ suffixMatcher(".txt", ACCOUNT_B),
+ genericMatcher(".*", ACCOUNT_A)));
assertThat(ownersConfigOpt).isPresent();
OwnersConfig ownersConfig = ownersConfigOpt.get();
diff --git a/owners/src/main/resources/Documentation/config.md b/owners/src/main/resources/Documentation/config.md
index 680b550..92b7d5e 100644
--- a/owners/src/main/resources/Documentation/config.md
+++ b/owners/src/main/resources/Documentation/config.md
@@ -69,6 +69,11 @@
(partial or full) and exact string comparison. For exact match, path is
relative to the root of the repo.
+> **NOTE:** The `generic` matcher is a special type of regex matching that
+> is applied only when none of the other sections are matching. It is
+> used to define fallback rules. The `generic: .*` is the top-level fallback
+> and can be used with other more specific `generic` matchers.
+
The plugin analyzes the latest patch set by looking at each file directory and
building an OWNERS hierarchy. It stops once it finds an OWNERS file that has
“inherited” set to false (by default it’s true.)
