Add glob-based support for wildcards in CODEOWNERS file parser (disabled, behind a feature flag)

tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/Azure.Sdk.Tools.CodeOwnersParser.Tests.csproj

@@ -0,0 +1,23 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFramework>net6.0</TargetFramework>
+    <LangVersion>10.0</LangVersion>
+    <Nullable>enable</Nullable>
+    <WarningsAsErrors>Nullable</WarningsAsErrors>
+    <IsPackable>false</IsPackable>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.1.0" />
+    <PackageReference Include="NUnit" Version="3.13.3" />
+    <PackageReference Include="NUnit3TestAdapter" Version="4.2.1" />
+    <PackageReference Include="NUnit.Analyzers" Version="3.3.0" />
+    <PackageReference Include="coverlet.collector" Version="3.1.2" />
+  </ItemGroup>
+
+  <ItemGroup>
+    <ProjectReference Include="..\CodeOwnersParser\Azure.Sdk.Tools.CodeOwnersParser.csproj" />
+  </ItemGroup>
+
+</Project>

tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeOwnersFileTests.cs

@@ -0,0 +1,168 @@
+using System;
+using System.Collections.Generic;
+using Microsoft.Extensions.FileSystemGlobbing;
+using NUnit.Framework;
+
+namespace Azure.Sdk.Tools.CodeOwnersParser.Tests;
+
+[TestFixture]
+public class CodeOwnersFileTests
+{
+    /// <summary>
+    /// A battery of test cases specifying behavior of new logic matching target
+    /// path to CODEOWNERS entries , and comparing it to existing, legacy logic.
+    ///
+    /// The logic that has changed lives in CodeOwnersFile.FindOwnersForClosestMatch.
+    ///
+    /// The new logic supports matching against wildcards, while the old one doesn't.
+    ///
+    /// In the test case table below, any discrepancy between legacy and new
+    /// parser expected matches that doesn't pertain to wildcard matching denotes
+    /// a potential backward compatibility and/or existing defect in the legacy parser.
+    /// 
+    /// For further details, please see:
+    /// https://github.com/Azure/azure-sdk-tools/issues/2770
+    /// </summary>
+    private static readonly TestCase[] testCases =
+    {
+        // @formatter:off
+        //       TestCase:        Path:                 Expected match:
+        //           Name,        Target , Codeown.   , Legacy , New
+        new(         "1" ,           "a" , "a"        ,   true , true  ),
+        new(         "2" ,           "a" , "a/"       ,   true , false ), // New parser doesn't match as codeowners path expects directory, but it is unclear if target is directory, or not.
+        new(         "3" ,         "a/b" , "a/b"      ,   true , true  ),
+        new(         "4" ,         "a/b" , "/a/b"     ,   true , true  ),
+        new(         "5" ,         "a/b" , "a/b/"     ,   true , false ), // New parser doesn't match as codeowners path expects directory, but it is unclear if target is directory, or not.
+        new(         "6" ,        "/a/b" , "a/b"      ,   true , true  ),
+        new(         "7" ,        "/a/b" , "/a/b"     ,   true , true  ),
+        new(         "8" ,        "/a/b" , "a/b/"     ,   true , false ), // New parser doesn't match as codeowners path expects directory, but it is unclear if target is directory, or not.
+        new(         "9" ,        "a/b/" , "a/b"      ,   true , true  ),
+        new(        "10" ,        "a/b/" , "/a/b"     ,   true , true  ),
+        new(        "11" ,        "a/b/" , "a/b/"     ,   true , true  ),
+        new(        "12" ,       "/a/b/" , "a/b"      ,   true , true  ),
+        new(        "13" ,       "/a/b/" , "/a/b"     ,   true , true  ),
+        new(        "14" ,       "/a/b/" , "a/b/"     ,   true , true  ),
+        new(        "15" ,       "/a/b/" , "/a/b/"    ,   true , true  ),
+        new(        "16" ,      "/a/b/c" , "a/b"      ,   true , true  ),
+        new(        "17" ,      "/a/b/c" , "/a/b"     ,   true , true  ),
+        new(        "18" ,      "/a/b/c" , "a/b/"     ,   true , true  ),
+        new(        "19" ,    "/a/b/c/d" , "/a/b/"    ,   true , true  ),
+        new(    "casing" ,         "ABC" , "abc"      ,   true , false ), // New parser doesn't match as it is case-sensitive, per codeowners spec
+        new(  "chained1" ,       "a/b/c" , "a"        ,   true , true  ),
+        new(  "chained2" ,       "a/b/c" , "b"        ,  false , true  ), // New parser matches per codeowners and .gitignore spec
+        new(  "chained3" ,       "a/b/c" , "b/"       ,  false , true  ), // New parser matches per codeowners and .gitignore spec
+        new(  "chained4" ,       "a/b/c" , "c"        ,  false , true  ), // New parser matches per codeowners and .gitignore spec
+        new(  "chained5" ,       "a/b/c" , "c/"       ,  false , false ),
+        new(  "chained6" ,     "a/b/c/d" , "c/"       ,  false , true  ), // New parser matches per codeowners and .gitignore spec
+        new(  "chained7" ,   "a/b/c/d/e" , "c/"       ,  false , true  ), // New parser matches per codeowners and .gitignore spec
+        new(  "chained8" ,       "a/b/c" , "b/c"      ,  false , false ), // TODO need to verify if CODEOWNERS actually follows this rule of "middle slashes prevent path relativity" from .gitignore, or not.
+        new(  "chained9" ,           "a" , "a/b/c"    ,  false , false ),
+        new( "chained10" ,           "c" , "a/b/c"    ,  false , false ),
+        // Cases not supported by the new parser.
+        new(   "unsupp1" ,          "!a" , "!a"       ,   true , false ),
+        new(   "unsupp2" ,           "b" , "!a"       ,  false , false ),
+        new(   "unsupp3" ,        "a[b"  , "a[b"      ,   true , false ),
+        new(   "unsupp4" ,        "a]b"  , "a]b"      ,   true , false ),
+        new(   "unsupp5" ,        "a?b"  , "a?b"      ,   true , false ),
+        new(   "unsupp6" ,        "axb"  , "a?b"      ,  false , false ),
+        // The cases below test for wildcard support by the new parser. Legacy parser skips over wildcards.
+        new(       "**1" ,           "a" , "**/a"     ,  false , true  ), 
+        new(       "**2" ,           "a" , "**/b/a"   ,  false , false ), 
+        new(       "**3" ,           "a" , "**/a/b"   ,  false , false ), 
+        new(       "**4" ,           "a" , "/**/a"    ,  false , true  ),
+        new(       "**5" ,         "a/b" , "a/**/b"   ,  false , true  ),
+        new(       "**6" ,       "a/x/b" , "a/**/b"   ,  false , true  ),
+        new(       "**7" ,       "a/y/b" , "a/**/b"   ,  false , true  ),
+        new(       "**8" ,     "a/x/y/b" , "a/**/b"   ,  false , true  ),
+        new(       "**9" ,   "c/a/x/y/b" , "a/**/b"   ,  false , false ),
+        new(       "*10" ,   "a/b/cxy/d" , "/**/*x*/" ,  false , true  ),
+        new(        "1*" ,           "a" , "*"        ,  false , true  ),
+        new(        "2*" ,         "a/b" , "a/*"      ,  false , true  ),
+        // There is discrepancy between GitHub CODEOWNERS behavior [1] and .gitignore behavior here
+        // CODEOWNERS will not match this path, while .gitignore will
+        // [1] The "docs/*" example in https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#example-of-a-codeowners-file
+        // [2] Confirmed empirically. The .gitignore will match "a/*" to "a/b" and thus ignore everything.
+        new(        "3*" ,       "a/b/c" , "a/*"      ,  false , true  ),
+        new(        "4*" ,       "x/a/b" , "a/*"      ,  false , false ),
+        new(       "1*/" ,         "a/b" , "a/*/"     ,  false , false ),
+        new(       "2*/" ,        "a/b/" , "a/*/"     ,  false , true  ),
+        new(       "3*/" ,       "a/b/c" , "a/*/"     ,  false , true  ),
+        new(      "1*/*" ,         "a/b" , "a/*/*"    ,  false , false ),
+        new(      "2*/*" ,     "a/b/c/d" , "a/*/*/d"  ,  false , true  ),
+        new(      "3*/*" ,   "a/b/x/c/d" , "a/*/*/d"  ,  false , false ),
+        new(     "1**/*" ,   "a/b/x/c/d" , "a/**/*/d" ,  false , true  ),
+        new(        "*1" ,         "a/b" , "*/b"      ,  false , true  ),
+        new(      "*/*1" ,         "a/b" , "*/*/b"    ,  false , false ),
+        new(       "1**" ,           "a" , "a/**"     ,  false , false ),
+        new(       "2**" ,          "a/" , "a/**"     ,  false , true  ),
+        new(       "3**" ,         "a/b" , "a/**"     ,  false , true  ),
+        new(       "4**" ,        "a/b/" , "a/**"     ,  false , true  ),
+        new(    "*.ext1" ,      "a/x.md" , "*.md"     ,  false , true  ),
+        new(    "*.ext2" ,    "a/b/x.md" , "*.md"     ,  false , true  ),
+        new(    "*.ext3" , "a/b.md/x.md" , "*.md"     ,  false , true  ),
+        new(    "*.ext4" ,        "a/md" , "*.md"     ,  false , false ),
+        new(    "*.ext5" ,         "a.b" , "a.*"      ,  false , true  ),
+        new(    "*.ext6" ,        "a.b/" , "a.*"      ,  false , true  ),
+        new(    "*.ext5" ,         "a.b" , "a.*/"     ,  false , false ),
+        new(    "*.ext7" ,        "a.b/" , "a.*/"     ,  false , true  ),
+        new(    "*.ext8" ,        "a.b"  , "/a.*"     ,  false , true  ),
+        new(    "*.ext9" ,        "a.b/" , "/a.*"     ,  false , true  ),
+        new(   "*.ext10" ,      "x/a.b/" , "/a.*"     ,  false , false ),
+        // New parser should return false, but returns true due to https://github.com/dotnet/runtime/issues/80076
+        // TODO globbug1 actually covers-up problem with the parser, where it converts "*" to "**/*".
+        new(  "globbug1" ,         "a/b" , "*"        ,  false , true  ),
+        new(  "globbug2" ,      "a/b/c/" , "a/*/"     ,  false , true  )
+        // @formatter:on
+    };
+
+    /// <summary>
+    /// A repro for https://github.com/dotnet/runtime/issues/80076
+    /// </summary>
+    [Test]
+    public void TestGlobBugRepro()
+    {
+        var globMatcher = new Matcher(StringComparison.Ordinal);
+        globMatcher.AddInclude("/*/");
+
+        var dir = new InMemoryDirectoryInfo(
+            rootDir: "/", 
+            files: new List<string> { "/a/b" });
+
+        var patternMatchingResult = globMatcher.Execute(dir);
+        // The expected behavior is "Is.False", but actual behavior is "Is.True".
+        Assert.That(patternMatchingResult.HasMatches, Is.True);
+    }
+
+    /// <summary>
+    /// Exercises Azure.Sdk.Tools.CodeOwnersParser.Tests.CodeOwnersFileTests.testCases.
+    /// See comment on that member for details.
+    /// </summary>
+    [TestCaseSource(nameof(testCases))]
+    public void TestParseAndFindOwnersForClosestMatch(TestCase testCase)
+    {
+        List<CodeOwnerEntry>? codeownersEntries =
+            CodeOwnersFile.ParseContent(testCase.CodeownersPath + "@owner");
+
+        VerifyFindOwnersForClosestMatch(testCase, codeownersEntries, useNewImpl: false, testCase.ExpectedLegacyMatch);
+        VerifyFindOwnersForClosestMatch(testCase, codeownersEntries, useNewImpl: true, testCase.ExpectedNewMatch);
+    }
+
+    private static void VerifyFindOwnersForClosestMatch(TestCase testCase,
+        List<CodeOwnerEntry> codeownersEntries,
+        bool useNewImpl,
+        bool expectedMatch)
+    {
+        CodeOwnerEntry? entryLegacy =
+            // Act
+            CodeOwnersFile.FindOwnersForClosestMatch(
+                codeownersEntries,
+                testCase.TargetPath,
+                useNewFindOwnersForClosestMatchImpl: useNewImpl);
+
+        Assert.That(entryLegacy.Owners.Count, Is.EqualTo(expectedMatch ? 1 : 0));
+    }
+
+    // ReSharper disable once NotAccessedPositionalProperty.Global
+    //   Reason: Name is present to make it easier to refer to and distinguish test cases in VS test runner.
+    public record TestCase(string Name, string TargetPath, string CodeownersPath, bool ExpectedLegacyMatch, bool ExpectedNewMatch);
+}

tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/Azure.Sdk.Tools.RetrieveCodeOwners.Tests.csproj

@@ -2,13 +2,17 @@
 
   <PropertyGroup>
     <TargetFramework>net6.0</TargetFramework>
+    <Nullable>enable</Nullable>
+    <WarningsAsErrors>Nullable</WarningsAsErrors>
     <IsPackable>false</IsPackable>
   </PropertyGroup>
 
   <ItemGroup>
-    <PackageReference Include="NUnit" Version="3.13.2" />
-    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.11.0" />
-    <PackageReference Include="NUnit3TestAdapter" Version="4.1.0" />
+    <PackageReference Include="NUnit" Version="3.13.3" />
+    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.1.0" />
+    <PackageReference Include="NUnit3TestAdapter" Version="4.2.1" />
+    <PackageReference Include="NUnit.Analyzers" Version="3.3.0" />
+    <PackageReference Include="coverlet.collector" Version="3.1.2" />
   </ItemGroup>
 
   <ItemGroup>
@@ -17,7 +21,7 @@
 
   <ItemGroup>
     <None Update="CODEOWNERS">
-      <CopyToOutputDirectory>Always</CopyToOutputDirectory>
+      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
     </None>
   </ItemGroup>
 </Project>

tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/MainTests.cs

@@ -41,17 +41,17 @@ public void TestOnNormalOutput(string targetDirectory, bool includeUserAliasesOn
         [TestCase("https://testLink")]
         public void TestOnError(string codeOwnerPath)
         {
-            Assert.AreEqual(1, Program.Main(codeOwnerPath, "sdk"));
+            Assert.That(Program.Main(codeOwnerPath, "sdk"), Is.EqualTo(1));
         }
 
         private static void TestExpectResult(List<string> expectReturn, string output)
         {
-            CodeOwnerEntry codeOwnerEntry = JsonSerializer.Deserialize<CodeOwnerEntry>(output);
+            CodeOwnerEntry? codeOwnerEntry = JsonSerializer.Deserialize<CodeOwnerEntry>(output);
             List<string> actualReturn = codeOwnerEntry!.Owners;
-            Assert.AreEqual(expectReturn.Count, actualReturn.Count);
+            Assert.That(actualReturn.Count, Is.EqualTo(expectReturn.Count));
             for (int i = 0; i < actualReturn.Count; i++)
             {
-                Assert.AreEqual(expectReturn[i], actualReturn[i]);
+                Assert.That(actualReturn[i], Is.EqualTo(expectReturn[i]));
             }
         }
     }

tools/code-owners-parser/CodeOwnersParser.sln

@@ -15,6 +15,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution
 		..\..\eng\common\scripts\get-codeowners.ps1 = ..\..\eng\common\scripts\get-codeowners.ps1
 	EndProjectSection
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Azure.Sdk.Tools.CodeOwnersParser.Tests", "Azure.Sdk.Tools.CodeOwnersParser.Tests\Azure.Sdk.Tools.CodeOwnersParser.Tests.csproj", "{66C9FF6A-32DD-4C3C-ABE1-F1F58C1C8129}"
+EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Any CPU = Debug|Any CPU
@@ -33,6 +35,10 @@ Global
 		{798B8CAC-68FC-49FD-A0F6-51C0DC4A4D1D}.Debug|Any CPU.Build.0 = Debug|Any CPU
 		{798B8CAC-68FC-49FD-A0F6-51C0DC4A4D1D}.Release|Any CPU.ActiveCfg = Release|Any CPU
 		{798B8CAC-68FC-49FD-A0F6-51C0DC4A4D1D}.Release|Any CPU.Build.0 = Release|Any CPU
+		{66C9FF6A-32DD-4C3C-ABE1-F1F58C1C8129}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{66C9FF6A-32DD-4C3C-ABE1-F1F58C1C8129}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{66C9FF6A-32DD-4C3C-ABE1-F1F58C1C8129}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{66C9FF6A-32DD-4C3C-ABE1-F1F58C1C8129}.Release|Any CPU.Build.0 = Release|Any CPU
 	EndGlobalSection
 	GlobalSection(SolutionProperties) = preSolution
 		HideSolutionNode = FALSE

tools/code-owners-parser/CodeOwnersParser/Azure.Sdk.Tools.CodeOwnersParser.csproj

@@ -7,6 +7,7 @@
 
   <ItemGroup>
     <PackageReference Include="CommandLine.Net" Version="2.3.0" />
+    <PackageReference Include="Microsoft.Extensions.FileSystemGlobbing" Version="7.0.0" />
   </ItemGroup>
 
 </Project>

tools/code-owners-parser/CodeOwnersParser/CodeOwnersFile.cs

@@ -59,13 +59,27 @@ public static List<CodeOwnerEntry> ParseContent(string fileContent)
             return entries;
         }
 
-        public static CodeOwnerEntry ParseAndFindOwnersForClosestMatch(string codeOwnersFilePathOrUrl, string targetPath)
+        public static CodeOwnerEntry ParseAndFindOwnersForClosestMatch(
+            string codeOwnersFilePathOrUrl,
+            string targetPath,
+            bool useNewFindOwnersForClosestMatchImpl = false)
         {
             var codeOwnerEntries = ParseFile(codeOwnersFilePathOrUrl);
-            return FindOwnersForClosestMatch(codeOwnerEntries, targetPath);
+            return FindOwnersForClosestMatch(codeOwnerEntries, targetPath, useNewFindOwnersForClosestMatchImpl);
         }
 
-        public static CodeOwnerEntry FindOwnersForClosestMatch(List<CodeOwnerEntry> codeOwnerEntries, string targetPath)
+        public static CodeOwnerEntry FindOwnersForClosestMatch(
+            List<CodeOwnerEntry> codeOwnerEntries,
+            string targetPath,
+            bool useNewFindOwnersForClosestMatchImpl = false)
+        {
+            return useNewFindOwnersForClosestMatchImpl
+                ? new MatchedCodeOwnerEntry(codeOwnerEntries, targetPath).Value
+                : FindOwnersForClosestMatchLegacyImpl(codeOwnerEntries, targetPath);
+        }
+
+        private static CodeOwnerEntry FindOwnersForClosestMatchLegacyImpl(List<CodeOwnerEntry> codeOwnerEntries,
+            string targetPath)
         {
             // Normalize the start and end of the paths by trimming slash
             targetPath = targetPath.Trim('/');