fix: gitlab codeowners exclusion handling (#12021)

fix: gitlab codeowners exclusion handling

Co-authored-by: daniel.mohedano <daniel.mohedano@datadoghq.com>

Author: daniel-mohedano

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/codeowners/CodeownersImpl.java

@@ -20,12 +20,9 @@
 
 public class CodeownersImpl implements Codeowners {
 
-  // Entries grouped by section in order of appearance (the unnamed section that precedes the first
-  // header comes first). Within a section, the highest-priority (last in the file) matching entry
-  // wins and owners from every matching section are then combined.
-  private final Collection<Deque<Entry>> sections;
+  private final Collection<Section> sections;
 
-  private CodeownersImpl(Collection<Deque<Entry>> sections) {
+  private CodeownersImpl(Collection<Section> sections) {
     this.sections = sections;
   }
 
@@ -37,15 +34,20 @@ private CodeownersImpl(Collection<Deque<Entry>> sections) {
   public @Nullable Collection<String> getOwners(@Nonnull String path) {
     char[] pathCharacters = path.toCharArray();
     Set<String> owners = null;
-    for (Deque<Entry> section : sections) {
-      for (Entry entry : section) {
-        if (entry.getMatcher().consume(pathCharacters, 0) >= 0) {
-          if (owners == null) {
-            owners = new LinkedHashSet<>();
-          }
-          owners.addAll(entry.getOwners());
-          break; // highest-priority match within a section wins
+    for (Section section : sections) {
+      if (section.isExcluded(pathCharacters)) {
+        if (owners == null) {
+          owners = new LinkedHashSet<>();
         }
+        continue;
+      }
+
+      Entry entry = section.findMatchingEntry(pathCharacters);
+      if (entry != null) {
+        if (owners == null) {
+          owners = new LinkedHashSet<>();
+        }
+        owners.addAll(entry.getOwners());
       }
     }
     return owners != null ? new ArrayList<>(owners) : null;
@@ -57,9 +59,9 @@ public boolean exist() {
   }
 
   public static Codeowners parse(Reader r) throws IOException {
-    Deque<Entry> defaultSection = new ArrayDeque<>();
-    Map<String, Deque<Entry>> namedSections = new LinkedHashMap<>();
-    Deque<Entry> currentSection = defaultSection;
+    Section defaultSection = new Section();
+    Map<String, Section> namedSections = new LinkedHashMap<>();
+    Section currentSection = defaultSection;
 
     CharacterMatcher.Factory characterMatcherFactory = new CharacterMatcher.Factory();
     BufferedReader br = new BufferedReader(r);
@@ -73,21 +75,51 @@ public static Codeowners parse(Reader r) throws IOException {
       if (header != null) {
         sectionDefaultOwners = header.getDefaultOwners();
         String key = header.getName().trim().toLowerCase(Locale.ROOT);
-        currentSection = namedSections.computeIfAbsent(key, k -> new ArrayDeque<>());
+        currentSection = namedSections.computeIfAbsent(key, k -> new Section());
         continue;
       }
 
       Entry entry = entryBuilder.parse(sectionDefaultOwners);
       if (entry != null) {
-        // within a section, the last entry in the file has the highest priority
-        currentSection.offerFirst(entry);
+        currentSection.add(entry);
       }
     }
 
-    // Unnamed section is evaluated first, then named sections in order of first appearance
-    List<Deque<Entry>> sections = new ArrayList<>(namedSections.size() + 1);
+    List<Section> sections = new ArrayList<>(namedSections.size() + 1);
     sections.add(defaultSection);
     sections.addAll(namedSections.values());
     return new CodeownersImpl(sections);
   }
+
+  private static final class Section {
+
+    private final Deque<Entry> entries = new ArrayDeque<>();
+    private final Collection<Entry> exclusions = new ArrayList<>();
+
+    private void add(Entry entry) {
+      if (entry.isExclusion()) {
+        exclusions.add(entry);
+      } else {
+        entries.offerFirst(entry);
+      }
+    }
+
+    private boolean isExcluded(char[] path) {
+      for (Entry exclusion : exclusions) {
+        if (exclusion.getMatcher().consume(path, 0) >= 0) {
+          return true;
+        }
+      }
+      return false;
+    }
+
+    private @Nullable Entry findMatchingEntry(char[] path) {
+      for (Entry entry : entries) {
+        if (entry.getMatcher().consume(path, 0) >= 0) {
+          return entry;
+        }
+      }
+      return null;
+    }
+  }
 }

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/codeowners/Entry.java

@@ -7,10 +7,12 @@ public class Entry {
 
   private final Matcher matcher;
   private final Collection<String> owners;
+  private final boolean exclusion;
 
-  public Entry(Matcher matcher, Collection<String> owners) {
+  public Entry(Matcher matcher, Collection<String> owners, boolean exclusion) {
     this.matcher = matcher;
     this.owners = owners;
+    this.exclusion = exclusion;
   }
 
   public Matcher getMatcher() {
@@ -20,4 +22,8 @@ public Matcher getMatcher() {
   public Collection<String> getOwners() {
     return owners;
   }
+
+  public boolean isExclusion() {
+    return exclusion;
+  }
 }

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/codeowners/EntryBuilder.java

@@ -7,7 +7,6 @@
 import datadog.trace.civisibility.codeowners.matcher.EndOfLineMatcher;
 import datadog.trace.civisibility.codeowners.matcher.EndOfSegmentMatcher;
 import datadog.trace.civisibility.codeowners.matcher.Matcher;
-import datadog.trace.civisibility.codeowners.matcher.NegatingMatcher;
 import datadog.trace.civisibility.codeowners.matcher.QuestionMarkMatcher;
 import datadog.trace.civisibility.codeowners.matcher.RangeMatcher;
 import java.util.ArrayDeque;
@@ -69,12 +68,17 @@ public EntryBuilder(CharacterMatcher.Factory characterMatcherFactory, String s)
         return null;
       }
 
+      boolean exclusion = c[offset] == '!';
+      if (exclusion) {
+        offset++;
+      }
+
       Matcher matcher = parseMatcher();
-      Collection<String> owners = parseOwners();
-      if (owners.isEmpty()) {
+      Collection<String> owners = exclusion ? Collections.emptyList() : parseOwners();
+      if (!exclusion && owners.isEmpty()) {
         owners = sectionDefaultOwners;
       }
-      return new Entry(matcher, owners);
+      return new Entry(matcher, owners, exclusion);
 
     } catch (Exception e) {
       log.warn("Skipping malformed CODEOWNERS entry: {}", new String(c), e);
@@ -159,11 +163,6 @@ private boolean isSectionHeader() {
   }
 
   private Matcher parseMatcher() {
-    if (c[offset] == '!') {
-      offset++;
-      return new NegatingMatcher(parseMatcher());
-    }
-
     boolean patternContainsSlashes = false;
     if (c[offset] == '/') {
       // opening slash gets special treatment

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/codeowners/matcher/NegatingMatcher.java

@@ -58,6 +58,8 @@ static Stream<Arguments> testCodeownersMatchingArguments() {
         // --- GitLab sections: a path's owners combine the winning match from every section,
         // including the unnamed section (the leading "* @global-owner") ---
         arguments(GITLAB_SAMPLE, "random/file.txt", singletonList("@global-owner")),
+        // exclusions also apply to the unnamed section
+        arguments(GITLAB_SAMPLE, "unowned.txt", emptyList()),
         // section default owners are inherited and combined with the global owner
         arguments(GITLAB_SAMPLE, "docs/guide.md", asList("@global-owner", "@docs-team")),
         arguments(GITLAB_SAMPLE, "README.md", asList("@global-owner", "@docs-team")),
@@ -84,7 +86,20 @@ static Stream<Arguments> testCodeownersMatchingArguments() {
         // entries in both blocks resolves to the last matching entry's owners, not both
         arguments(GITLAB_SAMPLE, "service/util.go", asList("@global-owner", "@backend-team-a")),
         arguments(
-            GITLAB_SAMPLE, "service/api/handler.go", asList("@global-owner", "@backend-team-b")));
+            GITLAB_SAMPLE, "service/api/handler.go", asList("@global-owner", "@backend-team-b")),
+        // exclusions suppress only their section and cannot be overridden by a later entry
+        arguments(
+            GITLAB_SAMPLE,
+            "generated-assets/output.txt",
+            asList("@global-owner", "@generated-files-team")),
+        arguments(
+            GITLAB_SAMPLE,
+            "generated-assets/excluded/output.txt",
+            asList("@global-owner", "@other-generated-files-team")),
+        arguments(
+            GITLAB_SAMPLE,
+            "generated-assets/excluded/special.txt",
+            asList("@global-owner", "@other-generated-files-team")));
   }
 
   private static Codeowners parse(String resource) throws IOException {

dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/codeowners/CodeownersTest.java

@@ -4,7 +4,9 @@
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.params.provider.Arguments.arguments;
 
 import datadog.trace.civisibility.codeowners.matcher.CharacterMatcher;
@@ -23,15 +25,18 @@ class EntryBuilderTest {
   void testEntryMatch(String pattern, List<String> owners, String path, boolean expectedResult) {
     CharacterMatcher.Factory matcherFactory = new CharacterMatcher.Factory();
     Entry entry = new EntryBuilder(matcherFactory, pattern + " " + ownersToString(owners)).parse();
-    Entry negatedEntry =
+    Entry exclusionEntry =
         new EntryBuilder(matcherFactory, "!" + pattern + " " + ownersToString(owners)).parse();
 
     boolean result = entry.getMatcher().consume(path.toCharArray(), 0) >= 0;
-    boolean negatedResult = negatedEntry.getMatcher().consume(path.toCharArray(), 0) >= 0;
+    boolean exclusionResult = exclusionEntry.getMatcher().consume(path.toCharArray(), 0) >= 0;
 
     assertEquals(owners, entry.getOwners());
+    assertFalse(entry.isExclusion());
+    assertEquals(emptyList(), exclusionEntry.getOwners());
+    assertTrue(exclusionEntry.isExclusion());
     assertEquals(expectedResult, result);
-    assertEquals(!expectedResult, negatedResult);
+    assertEquals(expectedResult, exclusionResult);
   }
 
   static Stream<Arguments> testEntryMatchArguments() {
@@ -274,4 +279,15 @@ void testEntryInheritsSectionDefaultOwners() {
     Entry noOwners = new EntryBuilder(matcherFactory, "generated/").parse(emptyList());
     assertEquals(emptyList(), noOwners.getOwners());
   }
+
+  @Test
+  void testExclusionDoesNotInheritSectionDefaultOwners() {
+    CharacterMatcher.Factory matcherFactory = new CharacterMatcher.Factory();
+
+    Entry exclusion =
+        new EntryBuilder(matcherFactory, "!generated/").parse(singletonList("@docs-team"));
+
+    assertTrue(exclusion.isExclusion());
+    assertEquals(emptyList(), exclusion.getOwners());
+  }
 }

dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/codeowners/EntryBuilderTest.java

@@ -4,6 +4,8 @@
 # Entries before any section behave like a plain (GitHub-style) CODEOWNERS file. They form an
 # unnamed section whose owners are combined with those of every other matching section.
 *  @global-owner
+# Exclusions can also apply to the unnamed section.
+!unowned.txt
 
 # A section with default owners: every entry inherits them unless it declares its own.
 [Documentation] @docs-team
@@ -44,3 +46,13 @@ service/
 
 [BACKEND] @backend-team-b
 service/api/
+
+# An exclusion suppresses its section even if a later entry matches the same path.
+[Generated Files] @generated-files-team
+generated-assets/
+!generated-assets/excluded/
+generated-assets/excluded/special.txt @special-owner
+
+# Exclusions do not suppress ownership from other sections.
+[Other Generated Files] @other-generated-files-team
+generated-assets/excluded/