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())); + } }