AbstractGetCodeOwnersForPath: Include filtered out code owners into debug logs
Filtered out code owners are relevant for debugging (e.g. to understand
why a certain user was not suggested). Hence the debug logs should tell
if a code owner was filtered out.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I315e0ec2a1f67662eb77249052a2bb0cfbd381e1
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index 0fc90f9..56aa627 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -212,7 +212,7 @@
Set<CodeOwner> codeOwners = new HashSet<>();
ListMultimap<CodeOwner, CodeOwnerAnnotation> annotations = LinkedListMultimap.create();
AtomicBoolean ownedByAllUsers = new AtomicBoolean(false);
- List<String> debugLogs = new ArrayList<>();
+ ImmutableList.Builder<String> debugLogsBuilder = ImmutableList.builder();
codeOwnerConfigHierarchy.visit(
rsrc.getBranch(),
rsrc.getRevision(),
@@ -221,7 +221,7 @@
CodeOwnerResolverResult pathCodeOwners =
codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, rsrc.getPath());
- debugLogs.addAll(pathCodeOwners.messages());
+ debugLogsBuilder.addAll(pathCodeOwners.messages());
codeOwners.addAll(pathCodeOwners.codeOwners());
annotations.putAll(pathCodeOwners.annotations());
@@ -268,8 +268,8 @@
if (!ownedByAllUsers.get()) {
CodeOwnerResolverResult globalCodeOwners = getGlobalCodeOwners(rsrc.getBranch().project());
- debugLogs.add("resolve global code owners");
- debugLogs.addAll(globalCodeOwners.messages());
+ debugLogsBuilder.add("resolve global code owners");
+ debugLogsBuilder.addAll(globalCodeOwners.messages());
globalCodeOwners
.codeOwners()
@@ -295,7 +295,10 @@
ImmutableSet<CodeOwner> filteredCodeOwners =
filterCodeOwners(
- rsrc, ImmutableMultimap.copyOf(annotations), ImmutableSet.copyOf(codeOwners));
+ rsrc,
+ ImmutableMultimap.copyOf(annotations),
+ ImmutableSet.copyOf(codeOwners),
+ debugLogsBuilder);
CodeOwnerScorings codeOwnerScorings =
createScorings(
rsrc,
@@ -322,8 +325,9 @@
codeOwnersInfo.codeOwners =
codeOwnerJsonFactory.create(getFillOptions()).format(sortedAndLimitedCodeOwners);
codeOwnersInfo.ownedByAllUsers = ownedByAllUsers.get() ? true : null;
- codeOwnersInfo.debugLogs = debug ? debugLogs : null;
+ ImmutableList<String> debugLogs = debugLogsBuilder.build();
+ codeOwnersInfo.debugLogs = debug ? debugLogs : null;
logger.atFine().log("debug logs: %s", debugLogs);
return Response.ok(codeOwnersInfo);
@@ -375,8 +379,9 @@
private ImmutableSet<CodeOwner> filterCodeOwners(
R rsrc,
ImmutableMultimap<CodeOwner, CodeOwnerAnnotation> annotations,
- ImmutableSet<CodeOwner> codeOwners) {
- return filterCodeOwners(rsrc, annotations, getVisibleCodeOwners(rsrc, codeOwners))
+ ImmutableSet<CodeOwner> codeOwners,
+ ImmutableList.Builder<String> debugLogs) {
+ return filterCodeOwners(rsrc, annotations, getVisibleCodeOwners(rsrc, codeOwners), debugLogs)
.collect(toImmutableSet());
}
@@ -386,12 +391,14 @@
* @param rsrc resource on which the request is being performed
* @param annotations annotations that were set on the code owners
* @param codeOwners stream of code owners that should be filtered
+ * @param debugLogs builder to collect debug logs that may be returned to the caller
* @return the filtered stream of code owners
*/
protected Stream<CodeOwner> filterCodeOwners(
R rsrc,
ImmutableMultimap<CodeOwner, CodeOwnerAnnotation> annotations,
- Stream<CodeOwner> codeOwners) {
+ Stream<CodeOwner> codeOwners,
+ ImmutableList.Builder<String> debugLogs) {
return codeOwners;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index 5e7235b..8b7281c 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -18,9 +18,9 @@
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.NO_REVIEWER_SCORING_VALUE;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.extensions.common.AccountVisibility;
import com.google.gerrit.extensions.restapi.Response;
@@ -56,7 +56,6 @@
*/
public class GetCodeOwnersForPathInChange
extends AbstractGetCodeOwnersForPath<CodeOwnersInChangeCollection.PathResource> {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@VisibleForTesting
public static final CodeOwnerAnnotation NEVER_SUGGEST_ANNOTATION =
@@ -132,50 +131,54 @@
protected Stream<CodeOwner> filterCodeOwners(
CodeOwnersInChangeCollection.PathResource rsrc,
ImmutableMultimap<CodeOwner, CodeOwnerAnnotation> annotations,
- Stream<CodeOwner> codeOwners) {
+ Stream<CodeOwner> codeOwners,
+ ImmutableList.Builder<String> debugLogs) {
return codeOwners
- .filter(filterOutChangeOwner(rsrc))
- .filter(filterOutCodeOwnersThatAreAnnotatedWithNeverSuggest(annotations))
- .filter(filterOutServiceUsers());
+ .filter(filterOutChangeOwner(rsrc, debugLogs))
+ .filter(filterOutCodeOwnersThatAreAnnotatedWithNeverSuggest(annotations, debugLogs))
+ .filter(filterOutServiceUsers(debugLogs));
}
private Predicate<CodeOwner> filterOutChangeOwner(
- CodeOwnersInChangeCollection.PathResource rsrc) {
+ CodeOwnersInChangeCollection.PathResource rsrc, ImmutableList.Builder<String> debugLogs) {
return codeOwner -> {
if (!codeOwner.accountId().equals(rsrc.getRevisionResource().getChange().getOwner())) {
// 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 the change owner", codeOwner);
+ debugLogs.add(
+ String.format("filtering out %s because this code owner is the change owner", codeOwner));
// Returning false from the Predicate here means that the code owner should be filtered out.
return false;
};
}
private Predicate<CodeOwner> filterOutCodeOwnersThatAreAnnotatedWithNeverSuggest(
- ImmutableMultimap<CodeOwner, CodeOwnerAnnotation> annotations) {
+ ImmutableMultimap<CodeOwner, CodeOwnerAnnotation> annotations,
+ ImmutableList.Builder<String> debugLogs) {
return codeOwner -> {
boolean neverSuggest = annotations.containsEntry(codeOwner, NEVER_SUGGEST_ANNOTATION);
if (!neverSuggest) {
// 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 annotated with %s",
- codeOwner, NEVER_SUGGEST_ANNOTATION);
+ debugLogs.add(
+ String.format(
+ "filtering out %s because this code owner is annotated with %s",
+ codeOwner, NEVER_SUGGEST_ANNOTATION.key()));
// Returning false from the Predicate here means that the code owner should be filtered out.
return false;
};
}
- private Predicate<CodeOwner> filterOutServiceUsers() {
+ private Predicate<CodeOwner> filterOutServiceUsers(ImmutableList.Builder<String> debugLogs) {
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);
+ debugLogs.add(
+ String.format("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;
};
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
index 2294d61..9869a54 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
@@ -36,6 +36,7 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.codeowners.api.CodeOwners;
import com.google.gerrit.plugins.codeowners.api.CodeOwnersInfo;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwner;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet;
import com.google.gerrit.plugins.codeowners.restapi.GetCodeOwnersForPathInChange;
@@ -636,4 +637,53 @@
.inOrder();
assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue();
}
+
+ @Test
+ public void filteredOutCodeOwnersAreMentionedInDebugLogs() throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ // Create a service user.
+ TestAccount serviceUser =
+ accountCreator.create("serviceUser", "service.user@example.com", "Service User", null);
+ groupOperations
+ .group(groupCache.get(AccountGroup.nameKey("Service Users")).get().getGroupUUID())
+ .forUpdate()
+ .addMember(serviceUser.id())
+ .update();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(changeOwner.email())
+ .addCodeOwnerEmail(serviceUser.email())
+ .addCodeOwnerEmail(admin.email())
+ .addAnnotation(admin.email(), GetCodeOwnersForPathInChange.NEVER_SUGGEST_ANNOTATION)
+ .addCodeOwnerEmail(user.email())
+ .build())
+ .create();
+
+ String path = "/foo/bar/baz.md";
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().withDebug(/* debug= */ true), path);
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user.id());
+ assertThat(codeOwnersInfo)
+ .hasDebugLogsThatContainAllOf(
+ String.format(
+ "filtering out %s because this code owner is the change owner",
+ CodeOwner.create(changeOwner.id())),
+ String.format(
+ "filtering out %s because this code owner is a service user",
+ CodeOwner.create(serviceUser.id())),
+ String.format(
+ "filtering out %s because this code owner is annotated with %s",
+ CodeOwner.create(admin.id()),
+ GetCodeOwnersForPathInChange.NEVER_SUGGEST_ANNOTATION.key()));
+ }
}
