AbstractGetCodeOwnersForPath: Replace suggest flag with overridable method
Instead of having a boolean suggest flag that is true for
GetCodeOwnersForPathInChange and false for GetCodeOwnersForPathInBranch
add a filterCodeOwners that can be overridden by the subclasses to
filter out additional results. Use this method in
GetCodeOwnersForPathInChange to filter out the service users that should
not show up in the code owner suggestion. Since the methods to detect
and filter out service users are only needed in
GetCodeOwnersForPathInChange they are moved into this class.
The new filterCodeOwners method has a parameter to provide the resource
on which the request is being performed. For this parameter we want to
use the resource type that is used by the subclass. To make this
possible we add a generic type for the resource to
AbstractGetCodeOwnersForPath.
Since we are already touching the filterOutServiceUsers method, add some
comments to explain the return values to avoid misreading them.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ia7bbf579e808af4f3d615aa6edce1b24006c4afe
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index e3c7a1f..a5f7b5d 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -31,6 +31,7 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
+import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerInfo;
import com.google.gerrit.plugins.codeowners.backend.CodeOwner;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy;
@@ -43,7 +44,6 @@
import com.google.gerrit.server.account.AccountDirectory.FillOptions;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.account.Accounts;
-import com.google.gerrit.server.account.ServiceUserClassifier;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -56,7 +56,6 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
-import java.util.function.Predicate;
import java.util.stream.Stream;
import org.kohsuke.args4j.Option;
@@ -64,7 +63,8 @@
* Abstract base class for REST endpoints that get the code owners for an arbitrary path in a branch
* or a revision of a change.
*/
-public abstract class AbstractGetCodeOwnersForPath {
+public abstract class AbstractGetCodeOwnersForPath<R extends AbstractPathResource>
+ implements RestReadView<R> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@VisibleForTesting public static final int DEFAULT_LIMIT = 10;
@@ -76,9 +76,7 @@
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerConfigHierarchy codeOwnerConfigHierarchy;
private final Provider<CodeOwnerResolver> codeOwnerResolver;
- private final ServiceUserClassifier serviceUserClassifier;
private final CodeOwnerJson.Factory codeOwnerJsonFactory;
- private final boolean suggest;
private final EnumSet<ListAccountsOption> options;
private final Set<String> hexOptions;
@@ -116,9 +114,7 @@
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
Provider<CodeOwnerResolver> codeOwnerResolver,
- ServiceUserClassifier serviceUserClassifier,
- CodeOwnerJson.Factory codeOwnerJsonFactory,
- boolean suggest) {
+ CodeOwnerJson.Factory codeOwnerJsonFactory) {
this.accountVisibility = accountVisibility;
this.accounts = accounts;
this.accountControlFactory = accountControlFactory;
@@ -126,14 +122,12 @@
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.codeOwnerConfigHierarchy = codeOwnerConfigHierarchy;
this.codeOwnerResolver = codeOwnerResolver;
- this.serviceUserClassifier = serviceUserClassifier;
this.codeOwnerJsonFactory = codeOwnerJsonFactory;
- this.suggest = suggest;
this.options = EnumSet.noneOf(ListAccountsOption.class);
this.hexOptions = new HashSet<>();
}
- protected Response<List<CodeOwnerInfo>> applyImpl(AbstractPathResource rsrc)
+ protected Response<List<CodeOwnerInfo>> applyImpl(R rsrc)
throws AuthException, BadRequestException, PermissionBackendException {
parseHexOptions();
validateLimit();
@@ -231,42 +225,37 @@
* normally doesn't make sense since they will not react to review requests.
* </ul>
*/
- private ImmutableSet<CodeOwner> filterCodeOwners(
- AbstractPathResource rsrc, ImmutableSet<CodeOwner> codeOwners) {
- Stream<CodeOwner> filteredCodeOwners =
- codeOwners.stream().filter(filterOutNonVisibleCodeOwners(rsrc));
-
- if (suggest) {
- filteredCodeOwners = filteredCodeOwners.filter(filterOutServiceUsers());
- }
-
- return filteredCodeOwners.collect(toImmutableSet());
+ private ImmutableSet<CodeOwner> filterCodeOwners(R rsrc, ImmutableSet<CodeOwner> codeOwners) {
+ return filterCodeOwners(rsrc, getVisibleCodeOwners(rsrc, codeOwners)).collect(toImmutableSet());
}
- private Predicate<CodeOwner> filterOutNonVisibleCodeOwners(AbstractPathResource rsrc) {
- return codeOwner -> {
- if (isVisibleTo(rsrc, codeOwner)) {
- return true;
- }
- logger.atFine().log(
- "Filtering out %s because this code owner cannot see the branch %s",
- codeOwner, rsrc.getBranch().branch());
- return false;
- };
+ /**
+ * To be overridden by subclasses to filter out additional code owners.
+ *
+ * @param rsrc resource on which the request is being performed
+ * @param codeOwners stream of code owners that should be filtered
+ * @return the filtered stream of code owners
+ */
+ protected Stream<CodeOwner> filterCodeOwners(R rsrc, Stream<CodeOwner> codeOwners) {
+ return codeOwners;
}
- private Predicate<CodeOwner> filterOutServiceUsers() {
- return codeOwner -> {
- if (!isServiceUser(codeOwner)) {
- return true;
- }
- logger.atFine().log("Filtering out %s because this code owner is a service user", codeOwner);
- return false;
- };
+ private Stream<CodeOwner> getVisibleCodeOwners(R rsrc, ImmutableSet<CodeOwner> allCodeOwners) {
+ return allCodeOwners.stream()
+ .filter(
+ codeOwner -> {
+ if (isVisibleTo(rsrc, codeOwner)) {
+ return true;
+ }
+ logger.atFine().log(
+ "Filtering out %s because this code owner cannot see the branch %s",
+ codeOwner, rsrc.getBranch().branch());
+ return false;
+ });
}
/** Whether the given resource is visible to the given code owner. */
- private boolean isVisibleTo(AbstractPathResource rsrc, CodeOwner codeOwner) {
+ private boolean isVisibleTo(R rsrc, CodeOwner codeOwner) {
// We always check for the visibility of the branch.
// This is also correct for the GetCodeOwnersForPathInChange subclass where branch is the
// destination branch of the change. For changes the intention of the visibility check is to
@@ -284,11 +273,6 @@
.testOrFalse(RefPermission.READ);
}
- /** Whether the given code owner is a service user. */
- private boolean isServiceUser(CodeOwner codeOwner) {
- return serviceUserClassifier.isServiceUser(codeOwner.accountId());
- }
-
private void parseHexOptions() throws BadRequestException {
for (String hexOption : hexOptions) {
try {
@@ -362,8 +346,7 @@
* <p>Must be only used to complete the suggestion list when it is found that the path is owned by
* all user.
*/
- private void fillUpWithRandomUsers(
- AbstractPathResource rsrc, Set<CodeOwner> codeOwners, int limit) {
+ private void fillUpWithRandomUsers(R rsrc, Set<CodeOwner> codeOwners, int limit) {
if (codeOwners.size() >= limit) {
// limit is already reach, we don't need to add further suggestions
return;
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
index a94d848..3fab537 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
@@ -22,14 +22,12 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerInfo;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.Accounts;
-import com.google.gerrit.server.account.ServiceUserClassifier;
import com.google.gerrit.server.change.IncludedInResolver;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -56,8 +54,8 @@
*
* <p>The path may or may not exist in the branch.
*/
-public class GetCodeOwnersForPathInBranch extends AbstractGetCodeOwnersForPath
- implements RestReadView<CodeOwnersInBranchCollection.PathResource> {
+public class GetCodeOwnersForPathInBranch
+ extends AbstractGetCodeOwnersForPath<CodeOwnersInBranchCollection.PathResource> {
private final GitRepositoryManager repoManager;
private String revision;
@@ -80,7 +78,6 @@
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
Provider<CodeOwnerResolver> codeOwnerResolver,
- ServiceUserClassifier serviceUserClassifier,
CodeOwnerJson.Factory codeOwnerJsonFactory,
GitRepositoryManager repoManager) {
super(
@@ -91,10 +88,7 @@
codeOwnersPluginConfiguration,
codeOwnerConfigHierarchy,
codeOwnerResolver,
- serviceUserClassifier,
- codeOwnerJsonFactory,
- /** suggest = */
- false);
+ codeOwnerJsonFactory);
this.repoManager = repoManager;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index 216d264..0595177 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -14,11 +14,12 @@
package com.google.gerrit.plugins.codeowners.restapi;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.extensions.common.AccountVisibility;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerInfo;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwner;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
@@ -30,6 +31,8 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.List;
+import java.util.function.Predicate;
+import java.util.stream.Stream;
/**
* REST endpoint that gets the code owners for an arbitrary path in a revision of a change.
@@ -39,8 +42,12 @@
*
* <p>The path may or may not exist in the revision of the change.
*/
-public class GetCodeOwnersForPathInChange extends AbstractGetCodeOwnersForPath
- implements RestReadView<CodeOwnersInChangeCollection.PathResource> {
+public class GetCodeOwnersForPathInChange
+ extends AbstractGetCodeOwnersForPath<CodeOwnersInChangeCollection.PathResource> {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final ServiceUserClassifier serviceUserClassifier;
+
@Inject
GetCodeOwnersForPathInChange(
AccountVisibility accountVisibility,
@@ -60,10 +67,8 @@
codeOwnersPluginConfiguration,
codeOwnerConfigHierarchy,
codeOwnerResolver,
- serviceUserClassifier,
- codeOwnerJsonFactory,
- /** suggest = */
- true);
+ codeOwnerJsonFactory);
+ this.serviceUserClassifier = serviceUserClassifier;
}
@Override
@@ -71,4 +76,27 @@
throws RestApiException, PermissionBackendException {
return super.applyImpl(rsrc);
}
+
+ @Override
+ protected Stream<CodeOwner> filterCodeOwners(
+ CodeOwnersInChangeCollection.PathResource rsrc, Stream<CodeOwner> codeOwners) {
+ return codeOwners.filter(filterOutServiceUsers());
+ }
+
+ private Predicate<CodeOwner> filterOutServiceUsers() {
+ return codeOwner -> {
+ if (!isServiceUser(codeOwner)) {
+ // Returning true from the Predicate here means that the code owner should be kept.
+ return true;
+ }
+ logger.atFine().log("Filtering out %s because this code owner is a service user", codeOwner);
+ // Returning false from the Predicate here means that the code owner should be filtered out.
+ return false;
+ };
+ }
+
+ /** Whether the given code owner is a service user. */
+ private boolean isServiceUser(CodeOwner codeOwner) {
+ return serviceUserClassifier.isServiceUser(codeOwner.accountId());
+ }
}
