Merge branch 'stable-3.4' into stable-3.5
* stable-3.4:
Follow the whole chain of parent projects for OWNERS
Make code_review_user/1 a native plugin predicate
Reuse the code_review_user Prolog rule across gerrit_owners.pl
Change-Id: I8fc03c71fc8d885c65ab808be864ab7050f46cbd
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 02ed2f8..baff42d 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
@@ -28,11 +28,10 @@
import com.googlesource.gerrit.owners.common.PathOwners;
import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache;
import com.googlesource.gerrit.owners.common.PluginSettings;
-import java.util.Arrays;
-import java.util.Collections;
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;
@@ -63,17 +62,17 @@
metrics.countConfigLoads.increment();
try (Timer0.Context ctx = metrics.loadConfig.start()) {
- 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 d5d403b..e93e34b 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;
@@ -35,6 +34,7 @@
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,7 +45,13 @@
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.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;
@@ -95,12 +101,11 @@
int id = revision.getChangeResource().getChange().getChangeId();
Project.NameKey project = change.getProject();
- List<Project.NameKey> maybeParentProjectNameKey =
- projectCache
- .get(project)
- .map(p -> Arrays.asList(p.getProject().getParent()))
- .filter(Predicates.notNull())
- .orElse(Collections.emptyList());
+ List<Project.NameKey> projectParents =
+ projectCache.get(project).stream()
+ .flatMap(s -> s.parents().stream())
+ .map(ProjectState::getNameKey)
+ .collect(Collectors.toList());
try (Repository repository = repositoryManager.openRepository(project)) {
Set<String> changePaths = new HashSet<>(changeDataFactory.create(change).currentFilePaths());
@@ -111,7 +116,7 @@
accounts,
repositoryManager,
repository,
- maybeParentProjectNameKey,
+ projectParents,
pluginSettings.isBranchDisabled(branch) ? Optional.empty() : Optional.of(branch),
changePaths,
pluginSettings.expandGroups(),
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
new file mode 100644
index 0000000..a2dd06b
--- /dev/null
+++ b/owners/src/main/java/gerrit_owners/PRED_code_review_user_1.java
@@ -0,0 +1,117 @@
+// 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 gerrit_owners;
+
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.LabelValue;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.rules.StoredValues;
+import com.googlecode.prolog_cafe.exceptions.PrologException;
+import com.googlecode.prolog_cafe.lang.IntegerTerm;
+import com.googlecode.prolog_cafe.lang.JavaObjectTerm;
+import com.googlecode.prolog_cafe.lang.Operation;
+import com.googlecode.prolog_cafe.lang.Predicate;
+import com.googlecode.prolog_cafe.lang.Prolog;
+import com.googlecode.prolog_cafe.lang.Term;
+import java.util.Iterator;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/** 'code_review_user'(-User) */
+public class PRED_code_review_user_1 extends Predicate.P1 {
+
+ private static final PRED_code_review_user_check CODE_REVIEW_USER_CHECK =
+ new PRED_code_review_user_check();
+ private static final PRED_code_review_user_empty CODE_REVIEW_USER_EMPTY =
+ new PRED_code_review_user_empty();
+ private static final PRED_code_review_user_next CODE_REVIEW_USER_NEXT =
+ new PRED_code_review_user_next();
+
+ public PRED_code_review_user_1(Term a1, Operation n) {
+ this.arg1 = a1;
+ this.cont = n;
+ }
+
+ @Override
+ public Operation exec(Prolog engine) throws PrologException {
+ engine.cont = cont;
+ engine.setB0();
+
+ ChangeData cd = StoredValues.CHANGE_DATA.get(engine);
+ Optional<LabelValue> codeReviewMaxValue =
+ cd.getLabelTypes().byLabel(LabelId.CODE_REVIEW).map(LabelType::getMax);
+
+ Iterator<Account.Id> approvalsAccounts =
+ cd.currentApprovals().stream()
+ .filter(a -> LabelId.CODE_REVIEW.equalsIgnoreCase(a.labelId().get()))
+ .filter(a -> codeReviewMaxValue.filter(val -> val.getValue() == a.value()).isPresent())
+ .map(a -> a.accountId())
+ .collect(Collectors.toList())
+ .iterator();
+
+ engine.r1 = arg1;
+ engine.r2 = new JavaObjectTerm(approvalsAccounts);
+
+ return engine.jtry2(CODE_REVIEW_USER_CHECK, CODE_REVIEW_USER_NEXT);
+ }
+
+ private static class PRED_code_review_user_check extends Operation {
+
+ @Override
+ public Operation exec(Prolog engine) throws PrologException {
+ Term a1 = engine.r1;
+ Term a2 = engine.r2;
+
+ @SuppressWarnings("unchecked")
+ Iterator<Account.Id> iter = (Iterator<Account.Id>) ((JavaObjectTerm) a2).object();
+ while (iter.hasNext()) {
+ Account.Id accountId = iter.next();
+ IntegerTerm accountIdTerm = new IntegerTerm(accountId.get());
+ if (!a1.unify(accountIdTerm, engine.trail)) {
+ continue;
+ }
+ return engine.cont;
+ }
+ return engine.fail();
+ }
+ }
+
+ private static class PRED_code_review_user_next extends Operation {
+
+ @Override
+ public Operation exec(Prolog engine) throws PrologException {
+ return engine.trust(CODE_REVIEW_USER_EMPTY);
+ }
+ }
+
+ private static class PRED_code_review_user_empty extends Operation {
+
+ @Override
+ public Operation exec(Prolog engine) throws PrologException {
+ Term a2 = engine.r2;
+
+ @SuppressWarnings("unchecked")
+ Iterator<Account.Id> iter = (Iterator<Account.Id>) ((JavaObjectTerm) a2).object();
+ if (!iter.hasNext()) {
+ return engine.fail();
+ }
+
+ return engine.jtry2(CODE_REVIEW_USER_CHECK, CODE_REVIEW_USER_NEXT);
+ }
+ }
+}
diff --git a/owners/src/main/prolog/gerrit_owners.pl b/owners/src/main/prolog/gerrit_owners.pl
index 1fe2a22..5fba7e7 100644
--- a/owners/src/main/prolog/gerrit_owners.pl
+++ b/owners/src/main/prolog/gerrit_owners.pl
@@ -38,8 +38,8 @@
add_owner_approval(_, In, Out) :- In = Out.
owner_approved(Path) :-
- owner(Path, User),
- gerrit:commit_label(label('Code-Review', 2), User),
+ owner(Path, user(US)),
+ code_review_user(US),
!.
owner_approved(Users, Path) :-
@@ -56,9 +56,6 @@
findall(US,code_review_user(US),Approvers),
matcher_needed(Approvers,F,FileAndUser).
-code_review_user(U) :-
- gerrit:commit_label(label('Code-Review', 2), user(U)).
-
% this loops over all the paths and if for any
% we have some labels generated then add a single
% Owner-Code-Review need to block submit button
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 5cd1599..53d13c9 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,29 +20,46 @@
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.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.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", sysModule = "com.googlesource.gerrit.owners.OwnersModule")
@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 {
@@ -51,17 +68,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
@@ -153,6 +187,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)));
@@ -210,6 +249,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
@@ -217,15 +261,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;
}
}
