Inherit OWNERS from all parent projects
As a follow-up of I50c83466, consider the full
chain of parent projects when considering the inheritance
of project-level OWNERS stored in refs/meta/config.
Change-Id: I39491bce0dda726c49f1e2ccbc6eb299b7eb6152
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 35828f4..bae340a 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
@@ -46,14 +46,17 @@
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
+import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.Set;
+import java.util.stream.Collectors;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.ObjectId;
@@ -211,10 +214,15 @@
try {
ChangeApi cApi = changes.id(cId.get());
ChangeInfo change = cApi.get();
- Optional<Project.NameKey> maybeParentProjectNameKey =
+ List<NameKey> parentProjectsNameKeys =
projectCache
- .get(Project.NameKey.parse(change.project))
- .map(p -> p.getProject().getParent());
+ .get(NameKey.parse(change.project))
+ .map(
+ p ->
+ p.parents().stream()
+ .map(ProjectState::getNameKey)
+ .collect(Collectors.toList()))
+ .orElse(Collections.emptyList());
PatchList patchList = getPatchList(repository, event, change);
if (patchList != null) {
@@ -223,7 +231,7 @@
accounts,
repositoryManager,
repository,
- maybeParentProjectNameKey,
+ parentProjectsNameKeys,
cfg.isBranchDisabled(change.branch) ? Optional.empty() : Optional.of(change.branch),
patchList,
cfg.expandGroups());
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 d37a086..7e3a311 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
@@ -20,6 +20,7 @@
import static com.google.gerrit.entities.Patch.MERGE_LIST;
import static com.googlesource.gerrit.owners.common.JgitWrapper.getBlobAsBytes;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimaps;
import com.google.common.collect.SetMultimap;
@@ -37,6 +38,7 @@
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;
@@ -56,7 +58,7 @@
private final Repository repository;
- private final Optional<Project.NameKey> maybeParentRepo;
+ private final List<Project.NameKey> parentProjectsNames;
private final PatchList patchList;
@@ -78,13 +80,13 @@
Accounts accounts,
GitRepositoryManager repositoryManager,
Repository repository,
- Optional<Project.NameKey> maybeParentRepo,
+ List<Project.NameKey> parentProjectsNames,
Optional<String> branchWhenEnabled,
PatchList patchList,
boolean expandGroups) {
this.repositoryManager = repositoryManager;
this.repository = repository;
- this.maybeParentRepo = maybeParentRepo;
+ this.parentProjectsNames = parentProjectsNames;
this.patchList = patchList;
this.parser = new ConfigurationParser(accounts);
this.accounts = accounts;
@@ -141,10 +143,8 @@
OwnersMap ownersMap = new OwnersMap();
try {
// Using a `map` would have needed a try/catch inside the lamba, resulting in more code
- Optional<PathOwnersEntry> maybeParentPathOwnersEntry =
- maybeParentRepo.isPresent()
- ? Optional.of(getPathOwnersEntry(maybeParentRepo.get(), RefNames.REFS_CONFIG))
- : Optional.empty();
+ List<PathOwnersEntry> parentsPathOwnersEntries =
+ getPathOwnersEntries(parentProjectsNames, RefNames.REFS_CONFIG);
PathOwnersEntry projectEntry = getPathOwnersEntry(repository, RefNames.REFS_CONFIG);
PathOwnersEntry rootEntry = getPathOwnersEntry(repository, branch);
@@ -154,7 +154,7 @@
for (String path : modifiedPaths) {
currentEntry =
resolvePathEntry(
- path, branch, projectEntry, maybeParentPathOwnersEntry, rootEntry, entries);
+ path, branch, projectEntry, parentsPathOwnersEntries, rootEntry, entries);
// add owners and reviewers to file for matcher predicates
ownersMap.addFileOwners(path, currentEntry.getOwners());
@@ -189,11 +189,15 @@
}
}
- private PathOwnersEntry getPathOwnersEntry(Project.NameKey projectName, String branch)
- throws IOException {
- try (Repository repo = repositoryManager.openRepository(projectName)) {
- return getPathOwnersEntry(repo, branch);
+ private List<PathOwnersEntry> getPathOwnersEntries(
+ List<Project.NameKey> projectNames, String branch) throws IOException {
+ ImmutableList.Builder<PathOwnersEntry> pathOwnersEntries = ImmutableList.builder();
+ for (Project.NameKey projectName : projectNames) {
+ try (Repository repo = repositoryManager.openRepository(projectName)) {
+ pathOwnersEntries = pathOwnersEntries.add(getPathOwnersEntry(repo, branch));
+ }
}
+ return pathOwnersEntries.build();
}
private PathOwnersEntry getPathOwnersEntry(Repository repo, String branch) throws IOException {
@@ -232,7 +236,7 @@
String path,
String branch,
PathOwnersEntry projectEntry,
- Optional<PathOwnersEntry> maybeParentPathOwnersEntry,
+ List<PathOwnersEntry> parentsPathOwnersEntries,
PathOwnersEntry rootEntry,
Map<String, PathOwnersEntry> entries)
throws IOException {
@@ -244,8 +248,8 @@
calculateCurrentEntry(rootEntry, projectEntry, currentEntry);
// Inherit from Parent Project if OWNER in Project enables inheritance
- if (maybeParentPathOwnersEntry.isPresent()) {
- calculateCurrentEntry(projectEntry, maybeParentPathOwnersEntry.get(), currentEntry);
+ for (PathOwnersEntry parentPathOwnersEntry : parentsPathOwnersEntries) {
+ calculateCurrentEntry(projectEntry, parentPathOwnersEntry, currentEntry);
}
// Iterate through the parent paths, not including the file name
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 0426488..f298164 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
@@ -36,7 +36,8 @@
public abstract class Config {
protected GitRepositoryManager repositoryManager;
protected Repository repository;
- protected Repository parentRepository;
+ protected Repository parentRepository1;
+ protected Repository parentRepository2;
protected PatchList patchList;
protected ConfigurationParser parser;
protected TestAccounts accounts = new TestAccounts();
@@ -47,7 +48,8 @@
repositoryManager = PowerMock.createMock(GitRepositoryManager.class);
repository = PowerMock.createMock(Repository.class);
- parentRepository = PowerMock.createMock(Repository.class);
+ parentRepository1 = PowerMock.createMock(Repository.class);
+ parentRepository2 = PowerMock.createMock(Repository.class);
parser = new ConfigurationParser(accounts);
}
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 07a83d1..e8b4167 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
@@ -15,6 +15,7 @@
package com.googlesource.gerrit.owners.common;
import static com.googlesource.gerrit.owners.common.MatcherConfig.suffixMatcher;
+import static java.util.Collections.EMPTY_LIST;
import static org.easymock.EasyMock.eq;
import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.expectLastCall;
@@ -30,6 +31,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
+import org.eclipse.jgit.lib.Repository;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -46,8 +48,10 @@
private static final boolean EXPAND_GROUPS = true;
private static final boolean DO_NOT_EXPAND_GROUPS = false;
public static final String CLASSIC_FILE_TXT = "classic/file.txt";
- public static final Project.NameKey parentRepositoryNameKey =
- Project.NameKey.parse("parentRepository");
+ public static final Project.NameKey parentRepository1NameKey =
+ Project.NameKey.parse("parentRepository1");
+ public static final Project.NameKey parentRepository2NameKey =
+ Project.NameKey.parse("parentRepository2");
@Override
@Before
@@ -64,7 +68,7 @@
accounts,
repositoryManager,
repository,
- Optional.empty(),
+ Collections.EMPTY_LIST,
branch,
patchList,
EXPAND_GROUPS);
@@ -84,7 +88,7 @@
accounts,
repositoryManager,
repository,
- Optional.empty(),
+ EMPTY_LIST,
branch,
patchList,
DO_NOT_EXPAND_GROUPS);
@@ -104,7 +108,7 @@
accounts,
repositoryManager,
repository,
- Optional.empty(),
+ EMPTY_LIST,
Optional.empty(),
patchList,
EXPAND_GROUPS);
@@ -122,13 +126,7 @@
PathOwners owners2 =
new PathOwners(
- accounts,
- repositoryManager,
- repository,
- Optional.empty(),
- branch,
- patchList,
- EXPAND_GROUPS);
+ accounts, repositoryManager, repository, EMPTY_LIST, branch, patchList, EXPAND_GROUPS);
Set<Account.Id> ownersSet2 = owners2.get().get(CLASSIC_OWNERS);
// in this case we are inheriting the acct3 from /OWNERS
@@ -152,13 +150,7 @@
PathOwners owners =
new PathOwners(
- accounts,
- repositoryManager,
- repository,
- Optional.empty(),
- branch,
- patchList,
- EXPAND_GROUPS);
+ accounts, repositoryManager, repository, EMPTY_LIST, branch, patchList, EXPAND_GROUPS);
Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners();
assertEquals(1, fileOwners.size());
@@ -176,13 +168,13 @@
expectConfig(
"OWNERS",
RefNames.REFS_CONFIG,
- parentRepository,
+ parentRepository1,
createConfig(true, owners(), suffixMatcher(".sql", USER_A_EMAIL_COM, USER_B_EMAIL_COM)));
String fileName = "file.sql";
creatingPatchList(Collections.singletonList(fileName));
- mockParentRepository();
+ mockParentRepository(parentRepository1NameKey, parentRepository1);
replayAll();
PathOwners owners =
@@ -190,7 +182,7 @@
accounts,
repositoryManager,
repository,
- Optional.of(parentRepositoryNameKey),
+ Arrays.asList(parentRepository1NameKey),
branch,
patchList,
EXPAND_GROUPS);
@@ -204,11 +196,55 @@
assertTrue(ownersSet.contains(USER_B_ID));
}
- private void mockParentRepository() throws IOException {
- expect(repositoryManager.openRepository(eq(parentRepositoryNameKey)))
- .andReturn(parentRepository)
- .anyTimes();
- parentRepository.close();
+ @Test
+ public void testProjectInheritFromMultipleParentProjects() throws Exception {
+ expectConfig("OWNERS", "master", createConfig(true, owners()));
+ expectConfig("OWNERS", RefNames.REFS_CONFIG, repository, createConfig(true, owners()));
+ expectConfig(
+ "OWNERS",
+ RefNames.REFS_CONFIG,
+ parentRepository1,
+ createConfig(true, owners(), suffixMatcher(".sql", USER_A_EMAIL_COM)));
+ expectConfig(
+ "OWNERS",
+ RefNames.REFS_CONFIG,
+ parentRepository2,
+ createConfig(true, 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);
+ replayAll();
+
+ PathOwners owners =
+ new PathOwners(
+ accounts,
+ repositoryManager,
+ repository,
+ Arrays.asList(parentRepository1NameKey, parentRepository2NameKey),
+ branch,
+ patchList,
+ EXPAND_GROUPS);
+
+ Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners();
+ assertEquals(fileOwners.size(), 2);
+
+ Set<Account.Id> ownersSet1 = fileOwners.get(sqlFileName);
+ assertEquals(1, ownersSet1.size());
+ assertTrue(ownersSet1.contains(USER_A_ID));
+
+ Set<Account.Id> ownersSet2 = fileOwners.get(javaFileName);
+ assertEquals(1, ownersSet2.size());
+ assertTrue(ownersSet2.contains(USER_B_ID));
+ }
+
+ private void mockParentRepository(Project.NameKey repositoryName, Repository repository)
+ throws IOException {
+ expect(repositoryManager.openRepository(eq(repositoryName))).andReturn(repository).anyTimes();
+ repository.close();
expectLastCall();
}
@@ -223,13 +259,7 @@
PathOwners owners =
new PathOwners(
- accounts,
- repositoryManager,
- repository,
- Optional.empty(),
- branch,
- patchList,
- EXPAND_GROUPS);
+ accounts, repositoryManager, repository, EMPTY_LIST, branch, patchList, EXPAND_GROUPS);
Set<Account.Id> ownersSet = owners.get().get("dir/subdir/OWNERS");
assertEquals(3, ownersSet.size());
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 aeffb90..2bcf2a7 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
@@ -21,6 +21,7 @@
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 org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.powermock.api.easymock.PowerMock.replayAll;
@@ -153,13 +154,7 @@
// function under test
PathOwners owners =
new PathOwners(
- accounts,
- repositoryManager,
- repository,
- Optional.empty(),
- branch,
- patchList,
- EXPAND_GROUPS);
+ accounts, repositoryManager, repository, EMPTY_LIST, branch, patchList, EXPAND_GROUPS);
// assertions on classic owners
Set<Account.Id> ownersSet = owners.get().get("project/OWNERS");
@@ -257,13 +252,7 @@
PathOwners owners =
new PathOwners(
- accounts,
- repositoryManager,
- repository,
- Optional.empty(),
- branch,
- patchList,
- EXPAND_GROUPS);
+ accounts, repositoryManager, repository, EMPTY_LIST, branch, patchList, EXPAND_GROUPS);
Set<String> ownedFiles = owners.getFileOwners().keySet();
assertThat(ownedFiles).containsExactly("project/file.sql");
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 72f2cf4..7181ab6 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
@@ -26,6 +26,9 @@
import com.googlesource.gerrit.owners.common.Accounts;
import com.googlesource.gerrit.owners.common.PathOwners;
import com.googlesource.gerrit.owners.common.PluginSettings;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
import java.util.Optional;
import org.eclipse.jgit.lib.Repository;
import org.slf4j.Logger;
@@ -51,8 +54,10 @@
ProjectState projectState = StoredValues.PROJECT_STATE.get(engine);
GitRepositoryManager gitRepositoryManager = StoredValues.REPO_MANAGER.get(engine);
- Optional<Project.NameKey> maybeParentProjectNameKey =
- Optional.ofNullable(projectState.getProject().getParent());
+ List<Project.NameKey> maybeParentProjectNameKey =
+ Optional.ofNullable(projectState.getProject().getParent())
+ .map(Arrays::asList)
+ .orElse(Collections.emptyList());
String branch = StoredValues.getChange(engine).getDest().branch();
return new PathOwners(
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 b05e2b2..a6c4fae 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
@@ -43,11 +43,7 @@
import com.googlesource.gerrit.owners.entities.FilesOwnersResponse;
import com.googlesource.gerrit.owners.entities.GroupOwner;
import com.googlesource.gerrit.owners.entities.Owner;
-import java.util.EnumSet;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Optional;
-import java.util.Set;
+import java.util.*;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.Repository;
import org.slf4j.Logger;
@@ -90,8 +86,11 @@
Change change = revision.getChange();
int id = revision.getChangeResource().getChange().getChangeId();
- Optional<Project.NameKey> maybeParentProjectNameKey =
- projectCache.get(change.getProject()).map(p -> p.getProject().getParent());
+ List<Project.NameKey> maybeParentProjectNameKey =
+ projectCache
+ .get(change.getProject())
+ .map(p -> Arrays.asList(p.getProject().getParent()))
+ .orElse(Collections.emptyList());
try (Repository repository = repositoryManager.openRepository(change.getProject())) {
PatchList patchList = patchListCache.get(change, ps);
diff --git a/owners/src/main/resources/Documentation/config.md b/owners/src/main/resources/Documentation/config.md
index 9b74897..680b550 100644
--- a/owners/src/main/resources/Documentation/config.md
+++ b/owners/src/main/resources/Documentation/config.md
@@ -112,7 +112,7 @@
OWNERS is used as global default.
If the global project OWNERS has the 'inherited: true', it will check for a global project OWNERS
-in the parent project.
+in all parent projects up to All-Projects.
## Example 1 - OWNERS file without matchers and default Gerrit submit rules
