Follow whole chain of parents for OWNERS in submit requirements
Extract functionality that traverses the whole chain of parents as a
static method in PathOwners and use it also to get a chain of parents
in the OwnersSubmitRequirement.
Bug: Issue 16893
Change-Id: I6db35e4b6c8800bf98c4db01394e8faf1775b8ed
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 17e4347..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
@@ -35,6 +35,7 @@
import com.google.gerrit.server.git.GitRepositoryManager;
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;
@@ -204,6 +205,12 @@
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.
*
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 baff42d..e456879 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
@@ -31,7 +31,6 @@
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;
@@ -62,11 +61,7 @@
metrics.countConfigLoads.increment();
try (Timer0.Context ctx = metrics.loadConfig.start()) {
- List<Project.NameKey> parentProjectsNameKeys =
- projectState.parents().stream()
- .map(ProjectState::getNameKey)
- .collect(Collectors.toList());
-
+ List<Project.NameKey> parentProjectsNameKeys = PathOwners.getParents(projectState);
String branch = StoredValues.getChange(engine).getDest().branch();
return new PathOwners(
accounts,
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
index 97d8098..0a2c5dd 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
@@ -51,8 +51,6 @@
import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache;
import com.googlesource.gerrit.owners.common.PluginSettings;
import java.io.IOException;
-import java.util.Arrays;
-import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
@@ -200,11 +198,7 @@
try (Timer0.Context ctx = metrics.loadConfig.start()) {
String branch = cd.change().getDest().branch();
- List<Project.NameKey> parents =
- Optional.<Project.NameKey>ofNullable(projectState.getProject().getParent())
- .map(Arrays::asList)
- .orElse(Collections.emptyList());
-
+ List<Project.NameKey> parents = PathOwners.getParents(projectState);
Project.NameKey nameKey = projectState.getNameKey();
try (Repository repo = repoManager.openRepository(nameKey)) {
PathOwners 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 e93e34b..47b104c 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
@@ -34,7 +34,6 @@
import com.google.gerrit.server.git.GitRepositoryManager;
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;
@@ -45,6 +44,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.Collections;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.HashSet;
@@ -102,10 +102,7 @@
Project.NameKey project = change.getProject();
List<Project.NameKey> projectParents =
- projectCache.get(project).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(project)) {
Set<String> changePaths = new HashSet<>(changeDataFactory.create(change).currentFilePaths());
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java
index 002dc4b..5f6be28 100644
--- a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java
@@ -23,6 +23,7 @@
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;
@@ -34,9 +35,11 @@
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;
@@ -46,6 +49,8 @@
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;
@TestPlugin(name = "owners", sysModule = "com.googlesource.gerrit.owners.OwnersModule")
@@ -359,6 +364,40 @@
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(
@@ -430,6 +469,17 @@
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:
@@ -455,4 +505,15 @@
.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();
+ }
}
