Follow the whole chain of parent projects for OWNERS
The OWNERS file can be stored in refs/meta/config for being
used as global fallback. However, when following the project
hierarchy it was only considering the 1st-level parent and not
all the other parents up the chain.
Get the full chain of parents and use it for the processing
of the equivalent PathOwners to be used in the Prolog rules
evaluation.
Bug: Issue 16893
Change-Id: I9bf46bcdf977e5645d6b44c4f08a792fdf3a4230
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 7181ab6..88a719c 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
@@ -26,10 +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 java.util.stream.Collectors;
import org.eclipse.jgit.lib.Repository;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -54,17 +53,17 @@
ProjectState projectState = StoredValues.PROJECT_STATE.get(engine);
GitRepositoryManager gitRepositoryManager = StoredValues.REPO_MANAGER.get(engine);
- List<Project.NameKey> maybeParentProjectNameKey =
- Optional.ofNullable(projectState.getProject().getParent())
- .map(Arrays::asList)
- .orElse(Collections.emptyList());
+ 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,
- maybeParentProjectNameKey,
+ parentProjectsNameKeys,
settings.isBranchDisabled(branch) ? Optional.empty() : Optional.of(branch),
patchList,
settings.expandGroups());
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 47e542f..b2d7458 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
@@ -15,7 +15,6 @@
package com.googlesource.gerrit.owners.restapi;
-import com.google.common.base.Predicates;
import com.google.common.collect.Maps;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
@@ -36,6 +35,7 @@
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.inject.Inject;
import com.google.inject.Singleton;
import com.googlesource.gerrit.owners.common.Accounts;
@@ -44,7 +44,12 @@
import com.googlesource.gerrit.owners.entities.FilesOwnersResponse;
import com.googlesource.gerrit.owners.entities.GroupOwner;
import com.googlesource.gerrit.owners.entities.Owner;
-import java.util.*;
+import java.util.EnumSet;
+import java.util.HashMap;
+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;
@@ -87,12 +92,11 @@
Change change = revision.getChange();
int id = revision.getChangeResource().getChange().getChangeId();
- List<Project.NameKey> maybeParentProjectNameKey =
- projectCache
- .get(change.getProject())
- .map(p -> Arrays.asList(p.getProject().getParent()))
- .filter(Predicates.notNull())
- .orElse(Collections.emptyList());
+ List<Project.NameKey> projectParents =
+ projectCache.get(change.getProject()).stream()
+ .flatMap(s -> s.parents().stream())
+ .map(ProjectState::getNameKey)
+ .collect(Collectors.toList());
try (Repository repository = repositoryManager.openRepository(change.getProject())) {
PatchList patchList = patchListCache.get(change, ps);
@@ -103,7 +107,7 @@
accounts,
repositoryManager,
repository,
- maybeParentProjectNameKey,
+ projectParents,
pluginSettings.isBranchDisabled(branch) ? Optional.empty() : Optional.of(branch),
patchList,
pluginSettings.expandGroups());
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 cd23d0d..e79f156 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
@@ -20,28 +20,45 @@
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit.Result;
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.acceptance.config.GlobalPluginConfig;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.client.SubmitType;
+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.Response;
import com.googlesource.gerrit.owners.entities.FilesOwnersResponse;
import com.googlesource.gerrit.owners.entities.GroupOwner;
import com.googlesource.gerrit.owners.entities.Owner;
+import java.util.Arrays;
import javax.servlet.http.HttpServletResponse;
import org.apache.commons.compress.utils.Sets;
+import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.transport.FetchResult;
+import org.eclipse.jgit.util.FS;
import org.junit.Test;
@TestPlugin(name = "owners", httpModule = "com.googlesource.gerrit.owners.OwnersRestApiModule")
@UseLocalDisk
public class GetFilesOwnersIT extends LightweightPluginDaemonTest {
+ private static final String REFS_META_CONFIG = RefNames.REFS_META + "config";
private GetFilesOwners ownersApi;
private Owner rootOwner;
private Owner projectOwner;
+ private NameKey parentProjectName;
+ private NameKey childProjectName;
+ private TestRepository<InMemoryRepository> childRepo;
+ private TestRepository<InMemoryRepository> parentRepo;
+ private TestRepository<InMemoryRepository> allProjectsRepo;
@Override
public void setUpTestPlugin() throws Exception {
@@ -50,17 +67,34 @@
rootOwner = new Owner(admin.fullName(), admin.id().get());
projectOwner = new Owner(user.fullName(), user.id().get());
ownersApi = plugin.getSysInjector().getInstance(GetFilesOwners.class);
+
+ parentProjectName =
+ createProjectOverAPI("parent", allProjects, true, SubmitType.FAST_FORWARD_ONLY);
+ parentRepo = cloneProjectWithMetaRefs(parentProjectName);
+
+ childProjectName =
+ createProjectOverAPI("child", parentProjectName, true, SubmitType.FAST_FORWARD_ONLY);
+ childRepo = cloneProject(childProjectName);
+
+ allProjectsRepo = cloneProjectWithMetaRefs(allProjects);
}
@Test
public void shouldReturnExactFileOwners() throws Exception {
addOwnerFileToRoot(true);
- String changeId = createChange().getChangeId();
+ assertChangeHasOwners(createChange().getChangeId());
+ }
- Response<FilesOwnersResponse> resp =
- assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
+ @Test
+ public void shouldReturnExactFileOwnersWhenOwnersIsSetToAllProjects() throws Exception {
+ addOwnerFileWithMatchers(allProjectsRepo, REFS_META_CONFIG, true);
+ assertChangeHasOwners(createChange(childRepo).getChangeId());
+ }
- assertThat(resp.value().files).containsExactly("a.txt", Sets.newHashSet(rootOwner));
+ @Test
+ public void shouldReturnExactFileOwnersWhenOwnersIsSetToParentProject() throws Exception {
+ addOwnerFileWithMatchers(parentRepo, REFS_META_CONFIG, true);
+ assertChangeHasOwners(createChange(childRepo).getChangeId());
}
@Test
@@ -137,6 +171,11 @@
addOwnerFileToProjectConfig(projectNameKey, true);
String changeId = createChange().getChangeId();
+ assertChangeHasOwners(changeId);
+ }
+
+ private void assertChangeHasOwners(String changeId)
+ throws AuthException, BadRequestException, ResourceConflictException, Exception {
Response<FilesOwnersResponse> resp =
assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
@@ -189,6 +228,11 @@
}
private void addOwnerFileWithMatchersToRoot(boolean inherit) throws Exception {
+ addOwnerFileWithMatchers(testRepo, "master", inherit);
+ }
+
+ private void addOwnerFileWithMatchers(TestRepository<?> repo, String targetRef, boolean inherit)
+ throws Exception {
// Add OWNERS file to root:
//
// inherited: true
@@ -196,15 +240,47 @@
// - suffix: .txt
// owners:
// - admin@mail.com
- merge(
+ Result changeCreated =
createChange(
- testRepo,
- "master",
+ repo,
+ targetRef,
"Add OWNER file",
"OWNERS",
String.format(
"inherited: %s\nmatchers:\n" + "- suffix: .txt\n owners:\n - %s\n",
inherit, admin.email()),
- ""));
+ "");
+ changeCreated.assertOkStatus();
+ merge(changeCreated);
+ }
+
+ public TestRepository<InMemoryRepository> cloneProjectWithMetaRefs(Project.NameKey project)
+ throws Exception {
+ String uri = registerRepoConnection(project, admin);
+ String initialRef = "refs/remotes/origin/config";
+ DfsRepositoryDescription desc = new DfsRepositoryDescription("clone of " + project.get());
+
+ InMemoryRepository.Builder b = new InMemoryRepository.Builder().setRepositoryDescription(desc);
+ if (uri.startsWith("ssh://")) {
+ // SshTransport depends on a real FS to read ~/.ssh/config, but InMemoryRepository by default
+ // uses a null FS.
+ // Avoid leaking user state into our tests.
+ b.setFS(FS.detect().setUserHome(null));
+ }
+ InMemoryRepository dest = b.build();
+ Config cfg = dest.getConfig();
+ cfg.setString("remote", "origin", "url", uri);
+ cfg.setStringList(
+ "remote",
+ "origin",
+ "fetch",
+ Arrays.asList(
+ "+refs/heads/*:refs/remotes/origin/*", "+refs/meta/config:refs/remotes/origin/config"));
+ TestRepository<InMemoryRepository> testRepo = GitUtil.newTestRepository(dest);
+ FetchResult result = testRepo.git().fetch().setRemote("origin").call();
+ if (result.getTrackingRefUpdate(initialRef) != null) {
+ testRepo.reset(initialRef);
+ }
+ return testRepo;
}
}
