diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeownersFileTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeownersFileTests.cs index f5b5765aa19..ab96bca6284 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeownersFileTests.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeownersFileTests.cs @@ -32,7 +32,7 @@ public class CodeownersFileTests new( "/" , "a/" , true , true ), new( "/" , "/a/" , true , true ), new( "/" , "/a/b" , true , true ), - new( "/a" , "a" , true , true ), + new( "/a" , "a" , true , true ), new( "/a" , "A" , true , false ), new( "/a" , "/a" , true , true ), new( "/a" , "a/" , true , false ), @@ -49,18 +49,20 @@ public class CodeownersFileTests new( "a" , "/a/b" , true , false ), new( "a" , "/a/b/" , true , false ), new( "a" , "/x/a/b" , false , false ), - new( "/a/" , "a" , true , true ), - new( "/a/" , "/a" , true , true ), + new( "/a/" , "a" , true , false ), + new( "/a/" , "/a" , true , false ), new( "/a/" , "a/" , true , true ), new( "/a/" , "/a/" , true , true ), new( "/a/" , "/a/b" , true , true ), - new( "/a/" , "/a\\ b" , true , true ), + new( "/a/" , "/a\\ b" , true , false ), + new( "/a/" , "/a\\ b/" , true , false ), + new( "/a/" , "/a/a\\ b/" , true , true ), new( "/a/" , "/a/b/" , true , true ), new( "/a/" , "/A/b/" , true , false ), new( "/a/" , "/x/a/b" , false , false ), new( "/a/b/" , "/a" , false , false ), new( "/a/b/" , "/a/" , false , false ), - new( "/a/b/" , "/a/b" , true , true ), + new( "/a/b/" , "/a/b" , true , false ), new( "/a/b/" , "/a/b/" , true , true ), new( "/a/b/" , "/a/b/c" , true , true ), new( "/a/b/" , "/a/b/c/" , true , true ), @@ -86,59 +88,88 @@ public class CodeownersFileTests new( "/*" , "[" , false , true ), new( "/*" , "]" , false , true ), new( "/*" , "!" , false , true ), - new( "/**" , "a" , false , true ), - new( "/**" , "a/" , false , true ), - new( "/**" , "a/b" , false , true ), - new( "/**" , "[" , false , true ), - new( "/**" , "]" , false , true ), - new( "/**" , "!" , false , true ), - new( "/a/*" , "a" , false , false ), // Not sure if this should not match. + new( "/**" , "a" , false , false ), + new( "/**" , "a/" , false , false ), + new( "/**" , "a/b" , false , false ), + new( "/**" , "[" , false , false ), + new( "/**" , "]" , false , false ), + new( "/**" , "!" , false , false ), + new( "/a/**" , "a" , false , false ), + new( "/*/**" , "a" , false , false ), + new( "/*/**" , "a/" , false , false ), + new( "/*/**" , "a/b" , false , false ), + new( "/*/" , "a" , false , false ), + new( "/*/" , "a/" , false , true ), + new( "/*/b" , "a/b" , false , true ), + new( "/**/a" , "a" , false , true ), + new( "/**/a" , "a" , false , true ), + new( "/**/a" , "x/ba" , false , false ), + new( "/**/a" , "x/ba" , false , false ), + new( "/a/*" , "a" , false , false ), new( "/a/*" , "a/" , false , true ), new( "/a/*" , "a/b" , false , true ), new( "/a/*" , "a/b/" , false , false ), new( "/a/*" , "a/b/c" , false , false ), - new( "/a/**" , "a" , false , true ), // Not sure if this should match. - new( "/a/**" , "a/" , false , true ), - new( "/a/**" , "a/b" , false , true ), - new( "/a/**" , "a/b/" , false , true ), - new( "/a/**" , "a/b/c" , false , true ), - new( "/**/a/" , "a" , false , true ), + new( "/a/*/" , "a" , false , false ), + new( "/a/*/" , "a/" , false , false ), + new( "/a/*/" , "a/b" , false , false ), + new( "/a/*/" , "a/b/" , false , true ), + new( "/a/*/" , "a/b/c" , false , true ), + new( "/a/**" , "a" , false , false ), + new( "/a/**" , "a/" , false , false ), + new( "/a/**" , "a/b" , false , false ), + new( "/a/**" , "a/b/" , false , false ), + new( "/a/**" , "a/b/c" , false , false ), + new( "/a/**/" , "a" , false , false ), + new( "/a/**/" , "a/" , false , false ), + new( "/a/**/" , "a/b" , false , false ), + new( "/a/**/" , "a/b/" , false , false ), + new( "/a/**/" , "a/b/c" , false , false ), + new( "/**/a/" , "a" , false , false ), new( "/**/a/" , "a/" , false , true ), new( "/**/a/" , "a/b" , false , true ), - new( "/**/b/" , "a/b" , false , true ), - new( "/**/b/" , "a/c" , false , false ), - new( "/a/*/b/" , "a/b" , false , false ), - new( "/a/*/b/" , "a/x/b" , false , true ), + new( "/**/b/" , "a/b" , false , false ), + new( "/**/b/" , "a/b/" , false , true ), + new( "/**/b/" , "a/c/" , false , false ), + new( "/a/*/b/" , "a/b/" , false , false ), + new( "/a/*/b/" , "a/x/b/" , false , true ), new( "/a/*/b/" , "a/x/b/c" , false , true ), new( "/a/*/b/" , "a/x/c" , false , false ), new( "/a/*/b/" , "a/x/y/b" , false , false ), - new( "/a**b/" , "a/x/y/b" , false , true ), - new( "/a/**/b/" , "a/b" , false , true ), - new( "/a/**/b/" , "a/x/b" , false , true ), - new( "/a/**/b/" , "a/x/y/b" , false , true ), + new( "/a**b/" , "a/x/y/b" , false , false ), + new( "/a/**/b/" , "a/b" , false , false ), + new( "/a/**/b/" , "a/b/" , false , true ), + new( "/a/**/b/" , "a/x/b/" , false , true ), + new( "/a/**/b/" , "a/x/y/b/" , false , true ), new( "/a/**/b/" , "a/x/y/c" , false , false ), + new( "/a/**/b/" , "a-b/" , false , false ), new( "a/*/*" , "a/b" , false , false ), new( "/a/*/*/d" , "a/b/c/d" , false , true ), new( "/a/*/*/d" , "a/b/x/c/d" , false , false ), new( "/a/**/*/d" , "a/b/x/c/d" , false , true ), new( "*/*/b" , "a/b" , false , false ), - new( "/a*/" , "abc" , false , true ), - new( "/*b*/" , "abc" , false , true ), - new( "/*c/" , "abc" , false , true ), - new( "/a*c/" , "abc" , false , true ), + new( "/a*/" , "abc/" , false , true ), + new( "/a*/" , "ab/c/" , false , true ), + new( "/*b*/" , "axbyc/" , false , true ), + new( "/*c/" , "abc/" , false , true ), + new( "/*c/" , "a/abc/" , false , false ), + new( "/a*c/" , "axbyc/" , false , true ), + new( "/a*c/" , "axb/yc/" , false , false ), new( "/**/*x*/" , "a/b/cxy/d" , false , true ), new( "/a/*.md" , "a/x.md" , false , true ), new( "/*/*/*.md" , "a/b/x.md" , false , true ), - new( "**/*.md" , "a/b.md/x.md" , false , true ), + new( "/**/*.md" , "a/b.md/x.md" , false , true ), + new( "**/*.md" , "a/b.md/x.md" , false , false ), new( "/*.md" , "a/md" , false , false ), new( "/a.*" , "a.b" , false , true ), new( "/a.*" , "a.b/" , false , false ), new( "/a.*" , "x/a.b/" , false , false ), - new( "/a.*/" , "a.b" , false , true ), + new( "/a.*/" , "a.b" , false , false ), new( "/a.*/" , "a.b/" , false , true ), new( "/**/*x*/AB/*/CD" , "a/b/cxy/AB/fff/CD" , false, true ), new( "/**/*x*/AB/*/CD" , "a/b/cxy/AB/ff/ff/CD" , false, false ), new( "/**/*x*/AB/**/CD/*" , "a/b/cxy/AB/ff/ff/CD" , false, false ), + new( "/**/*x*/AB/**/CD/*" , "a/b/cxy/AB/ff/ff/CD/" , false, true ), new( "/**/*x*/AB/**/CD/*" , "a/b/cxy/AB/[]/!!/CD/h" , false, true ), // @formatter:on @@ -151,7 +182,7 @@ public class CodeownersFileTests [TestCaseSource(nameof(testCases))] public void TestGetMatchingCodeownersEntry(TestCase testCase) { - List? codeownersEntries = + List codeownersEntries = CodeownersFile.GetCodeownersEntries(testCase.CodeownersPath + "@owner"); VerifyGetMatchingCodeownersEntry(testCase, codeownersEntries, useRegexMatcher: false, testCase.ExpectedLegacyMatch); @@ -164,12 +195,12 @@ private static void VerifyGetMatchingCodeownersEntry( bool useRegexMatcher, bool expectedMatch) { - CodeownersEntry? entryLegacy = + CodeownersEntry entry = // Act CodeownersFile.GetMatchingCodeownersEntry(testCase.TargetPath, codeownersEntries, useRegexMatcher); - Assert.That(entryLegacy.Owners, Has.Count.EqualTo(expectedMatch ? 1 : 0)); + Assert.That(entry.Owners, Has.Count.EqualTo(expectedMatch ? 1 : 0)); } public record TestCase( diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersDiffTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersDiffTests.cs new file mode 100644 index 00000000000..79afb4f7a24 --- /dev/null +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersDiffTests.cs @@ -0,0 +1,272 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.IO; +using System.Linq; +using System.Text.Json; +using Azure.Sdk.Tools.CodeOwnersParser; +using NUnit.Framework; + +namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests; + +/// +/// A class containing a set of tools, implemented as unit tests, +/// allowing you to view and diff owners of files of locally cloned repositories, +/// by obtaining the owners based on specified CODEOWNERS files, +/// or using specified matching logic. +/// +/// These tools are to be run manually, locally, by a developer. +/// They do not participate in an automated regression test suite. +/// +/// To run these tools, you will have to first manually comment out the "Ignore" +/// property of the "TestFixture" annotation below. +/// Then run the desired tool as a unit test, either from your IDE, +/// or "dotnet test" command line tool. +/// +/// You can import the tool output into Excel, using .CSV import wizard and +/// selecting "|" as column separator. +/// +[TestFixture(Ignore = "Tools to be used manually")] +public class CodeownersDiffTests +{ + private const string DefaultIgnoredPathPrefixes = Program.DefaultIgnoredPrefixes; + private const string OwnersDiffOutputPathSuffix = "_owners_diff.csv"; + private const string OwnersDataOutputPathSuffix = "_owners.csv"; + + // All these paths assume that appropriate repositories are cloned into the same local + // directory as "azure-sdk-tools" repo, containing this logic. + // So the dir layout is like: + // /azure-sdk-tools/ + // /azure-sdk-for-net/ + // /azure-sdk-for-java/ + // /azure-sdk-for-.../ + // ... + private const string AzureSdkForNetTargetDirPathSuffix = "/../azure-sdk-for-net"; + private const string AzureSdkForNetCodeownersPathSuffix = AzureSdkForNetTargetDirPathSuffix + "/.github/CODEOWNERS"; + // TODO: add more repos here. + + #region Owners diff + + [Test] + public void WriteToFileCodeownersMatcherDiffForAzureSdkTools() + { + // Empty string here means to just use the root directory of the local "azure-sdk-tools" clone. + var targetDirPathSuffix = ""; + var codeownersPathSuffix = "/.github/CODEOWNERS"; + var ignoredPrefixes = ".git|artifacts"; + WriteToFileOwnersDiff(new[] + { + (targetDirPathSuffix, codeownersPathSuffix, ignoredPrefixes, useRegexMatcher: false), + (targetDirPathSuffix, codeownersPathSuffix, ignoredPrefixes, useRegexMatcher: true) + }, outputFilePrefix: "azure-sdk-tools"); + } + + [Test] + public void WriteToFileCodeownersMatcherDiffForAzureSdkForNet() + { + WriteToFileOwnersDiff( + new[] + { + (AzureSdkForNetTargetDirPathSuffix, AzureSdkForNetCodeownersPathSuffix, + DefaultIgnoredPathPrefixes, useRegexMatcher: false), + (AzureSdkForNetTargetDirPathSuffix, AzureSdkForNetCodeownersPathSuffix, + DefaultIgnoredPathPrefixes, useRegexMatcher: true) + }, + outputFilePrefix: "azure-sdk-for-net"); + } + + #endregion + + #region Owners data + + [Test] + public void WriteToFileRegexMatcherCodeownersForAzureSdkTools() + { + // Empty string here means to just use the root directory of the local "azure-sdk-tools" clone. + var targetDirPathSuffix = ""; + var codeownersPathSuffix = "/.github/CODEOWNERS"; + var ignoredPrefixes = ".git|artifacts"; + WriteToFileOwnersData( + targetDirPathSuffix, + codeownersPathSuffix, + ignoredPrefixes, + useRegexMatcher: true, + outputFilePrefix: "azure-sdk-tools"); + } + + [Test] + public void WriteToFileRegexMatcherCodeownersForAzureSdkForNet() + => WriteToFileOwnersData( + AzureSdkForNetTargetDirPathSuffix, + AzureSdkForNetCodeownersPathSuffix, + DefaultIgnoredPathPrefixes, + useRegexMatcher: true, + outputFilePrefix: "azure-sdk-for-net"); + + #endregion + + private static void WriteToFileOwnersData( + string targetDirPathSuffix, + string codeownersPathSuffix, + string ignoredPrefixes, + bool useRegexMatcher, + string outputFilePrefix) + { + var stopwatch = Stopwatch.StartNew(); + + Dictionary ownersData = RunMain( + targetDirPathSuffix, + codeownersPathSuffix, + ignoredPrefixes, + useRegexMatcher); + + List outputLines = + new List { "PATH | PATH EXPRESSION | COMMA-SEPARATED OWNERS" }; + foreach (var kvp in ownersData) + { + string path = kvp.Key; + CodeownersEntry entry = kvp.Value; + outputLines.Add( + $"{path} " + + $"| {(entry.IsValid ? entry.PathExpression : "_____")} " + + $"| {string.Join(",", entry.Owners)}"); + } + + var outputFilePath = outputFilePrefix + OwnersDataOutputPathSuffix; + File.WriteAllLines(outputFilePath, outputLines); + Console.WriteLine($"DONE writing out owners. " + + $"Output written out to {Path.GetFullPath(outputFilePath)}. " + + $"Time taken: {stopwatch.Elapsed}."); + } + + private static void WriteToFileOwnersDiff(( + string targetDirPathSuffix, + string codeownersPathSuffix, + string ignoredPrefixes, + bool useRegexMatcher)[] input, + string outputFilePrefix) + { + var stopwatch = Stopwatch.StartNew(); + + Dictionary leftOwners = RunMain( + input[0].targetDirPathSuffix, + input[0].codeownersPathSuffix, + input[0].ignoredPrefixes, + input[0].useRegexMatcher); + Dictionary rightOwners = RunMain( + input[1].targetDirPathSuffix, + input[1].codeownersPathSuffix, + input[1].ignoredPrefixes, + input[1].useRegexMatcher); + + string[] diffLines = PathOwnersDiff(leftOwners, rightOwners); + + var outputFilePath = outputFilePrefix + OwnersDiffOutputPathSuffix; + File.WriteAllLines(outputFilePath, diffLines); + Console.WriteLine($"DONE diffing. " + + $"Output written out to {Path.GetFullPath(outputFilePath)}. " + + $"Time taken: {stopwatch.Elapsed}."); + } + + private static Dictionary RunMain( + string targetDirPathSuffix, + string codeownersPathSuffixToRootDir, + string ignoredPathPrefixes, + bool useRegexMatcher) + { + // Current dir, ".", is "/artifacts/bin/Azure.Sdk.Tools.CodeOwnersParser.Tests/Debug/net6.0". + const string currentDir = "/artifacts/bin/Azure.Sdk.Tools.CodeOwnersParser.Tests/Debug/net6.0"; + string rootDir = PathNavigatingToRootDir(currentDir); + + string actualOutput, actualErr; + int returnCode; + using (var consoleOutput = new ConsoleOutput()) + { + // Act + returnCode = Program.Main( + targetPath: "/**", + codeownersFilePathOrUrl: rootDir + codeownersPathSuffixToRootDir, + excludeNonUserAliases: false, + targetDir: rootDir + targetDirPathSuffix, + ignoredPathPrefixes, + useRegexMatcher); + + actualOutput = consoleOutput.GetStdout(); + actualErr = consoleOutput.GetStderr(); + } + + var actualEntries = JsonSerializer.Deserialize>(actualOutput)!; + return actualEntries; + } + + private static string PathNavigatingToRootDir(string currentDir) + => "./" + string.Join( + "/", + currentDir + .Split("/", StringSplitOptions.RemoveEmptyEntries) + .Select(_ => "..")); + + private static string[] PathOwnersDiff( + Dictionary left, + Dictionary right) + { + Debug.Assert( + left.Keys.ToHashSet().SetEquals(right.Keys.ToHashSet()), + "The compared maps of owner data are expected to have the same paths (keys)."); + + List outputLines = new List + { + "DIFF CODE | PATH | LEFT PATH EXPRESSION | RIGHT PATH EXPRESSION | LEFT OWNERS | RIGHT OWNERS" + }; + foreach (string path in left.Keys) + { + if (left[path].IsValid && right[path].IsValid) + { + // Path matched against an entry in both "left" and "right" owners data. + // Here we determine if the owners lists match. + outputLines.AddRange(PathOwnersDiff(path, left[path], right[path])); + } + else if (left[path].IsValid && !right[path].IsValid) + { + // Path matched against an entry in the "left" owners data, but not in the right. + outputLines.Add($"PATH LEFT -_____ " + + $"| {path} | {left[path].PathExpression} | | {string.Join(",",left[path].Owners)} |"); + } + else if (!left[path].IsValid && right[path].IsValid) + { + // Path matched against an entry in the "right" owners data, but not in the right. + outputLines.Add($"PATH _____-RIGHT " + + $"| {path} | | {right[path].PathExpression} | | {string.Join(",",right[path].Owners)}"); + } + else + { + // Path did not match against any owners data, not in "left" nor "right". + outputLines.Add($"PATH _____-_____ | {path} |"); + } + } + + return outputLines.ToArray(); + } + + private static string[] PathOwnersDiff( + string path, + CodeownersEntry left, + CodeownersEntry right) + { + Debug.Assert(left.IsValid); + Debug.Assert(right.IsValid); + + List outputLines = new List(); + + if (!left.Owners.ToHashSet().SetEquals(right.Owners.ToHashSet())) + { + // Given path owners differ between "left" an "right" owners data. + outputLines.Add( + $"OWNERS DIFF | {path} " + + $"| {left.PathExpression} | {right.PathExpression} " + + $"| {string.Join(", ", left.Owners)} | {string.Join(", ", right.Owners)}"); + } + + return outputLines.ToArray(); + } +} diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramGlobPathTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramGlobPathTests.cs index 9ee3f2a00bc..3d64224fac3 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramGlobPathTests.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramGlobPathTests.cs @@ -21,7 +21,7 @@ public class ProgramGlobPathTests /// codeownersFilePathOrUrl contents of: /// /// - /// /** @2star + /// / @slash /// /* @star /// /foo/**/a.txt @foo_2star_a /// /foo/*/a.txt @foo_star_a_1 @foo_star_a_2 @@ -45,7 +45,7 @@ public class ProgramGlobPathTests /// /// /// - /// /foo/** + /// /** /// /// /// @@ -61,9 +61,9 @@ public class ProgramGlobPathTests /// /a.txt @star /// /b.txt @star /// /foo/a.txt @foo_2star_a - /// /foo/b.txt @2star + /// /foo/b.txt @slash /// /foo/bar/a.txt @foo_star_a_1 @foo_star_a_2 - /// /foo/bar/b.txt @2star + /// /foo/bar/b.txt @slash /// /// [Test] @@ -81,9 +81,9 @@ public void OutputsCodeownersForGlobPath() ["a.txt"] = new CodeownersEntry("/*", new List { "star" }), ["b.txt"] = new CodeownersEntry("/*", new List { "star" }), ["foo/a.txt"] = new CodeownersEntry("/foo/**/a.txt", new List { "foo_2star_a" }), - ["foo/b.txt"] = new CodeownersEntry("/**", new List { "2star" }), + ["foo/b.txt"] = new CodeownersEntry("/", new List { "slash" }), ["foo/bar/a.txt"] = new CodeownersEntry("/foo/*/a.txt", new List { "foo_star_a_1", "foo_star_a_2" }), - ["foo/bar/b.txt"] = new CodeownersEntry("/**", new List { "2star" }), + ["foo/bar/b.txt"] = new CodeownersEntry("/", new List { "slash" }), // @formatter:on }; @@ -97,7 +97,7 @@ public void OutputsCodeownersForGlobPath() codeownersFilePathOrUrl, excludeNonUserAliases, targetDir, - useRegexMatcher); + useRegexMatcher: useRegexMatcher); actualOutput = consoleOutput.GetStdout(); actualErr = consoleOutput.GetStderr(); @@ -145,6 +145,6 @@ private static void AssertEntries( Assert.That(expectedEntries[path], Is.EqualTo(actualEntry), $"path: {path}"); } - Assert.That(actualEntries.Count, Is.EqualTo(expectedEntries.Count)); + Assert.That(actualEntries, Has.Count.EqualTo(expectedEntries.Count)); } } diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/glob_path_CODEOWNERS b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/glob_path_CODEOWNERS index c09c06a2cc9..a74409c924d 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/glob_path_CODEOWNERS +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/glob_path_CODEOWNERS @@ -3,7 +3,7 @@ # Azure.Sdk.Tools.RetrieveCodeOwners.Tests.WildcardTests.TestWildcard # -/** @2star +/ @slash /* @star /foo/**/a.txt @foo_2star_a /foo/*/a.txt @foo_star_a_1 @foo_star_a_2 \ No newline at end of file diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs index 352f50e8976..c08bc05c840 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs @@ -13,16 +13,29 @@ namespace Azure.Sdk.Tools.RetrieveCodeOwners; /// public static class Program { + /// + /// See comment on Azure.Sdk.Tools.RetrieveCodeOwners.Program.Main, + /// for parameter "ignoredPathPrefixes". + /// + public const string DefaultIgnoredPrefixes = ".git"; + /// /// Given targetPath and CODEOWNERS file path or https url codeownersFilePathOrUrl, /// prints out to stdout owners of the targetPath as determined by the CODEOWNERS data. /// /// The path whose owners are to be determined. Can be a glob path. /// The https url or path to the CODEOWNERS file. - /// Whether owners that aren't users should be excluded from the - /// returned owners. + /// Whether owners that aren't users should be excluded from + /// the returned owners. /// - /// The directory to search for file paths in case targetPath is a glob path Unused otherwise. + /// The directory to search for file paths in case targetPath is a glob path. Unused otherwise. + /// + /// + /// A list of path prefixes, separated by |, to ignore when doing + /// glob-matching against glob targetPath. + /// Applies only if targetPath is a glob path. Unused otherwise. + /// Defaults to ".git". + /// Example usage: ".git|foo|bar" /// /// /// Whether to use the new matcher for CODEOWNERS file paths, that supports matching @@ -39,25 +52,31 @@ public static int Main( string codeownersFilePathOrUrl, bool excludeNonUserAliases = false, string? targetDir = null, + string ignoredPathPrefixes = DefaultIgnoredPrefixes, bool useRegexMatcher = false) { try { + Trace.Assert(!string.IsNullOrWhiteSpace(targetPath)); + targetPath = targetPath.Trim(); targetDir = targetDir?.Trim(); codeownersFilePathOrUrl = codeownersFilePathOrUrl.Trim(); - Trace.Assert(!string.IsNullOrWhiteSpace(targetPath)); Trace.Assert(!string.IsNullOrWhiteSpace(codeownersFilePathOrUrl)); Trace.Assert(!targetPath.IsGlobFilePath() || (targetDir != null && Directory.Exists(targetDir))); + // The "object" here is effectively an union of two types: T1 | T2, + // where T1 is the type returned by GetCodeownersForGlobPath + // and T2 is the type returned by GetCodeownersForSimplePath. object codeownersData = targetPath.IsGlobFilePath() ? GetCodeownersForGlobPath( new GlobFilePath(targetPath), targetDir!, codeownersFilePathOrUrl, excludeNonUserAliases, + SplitIgnoredPathPrefixes(), useRegexMatcher) : GetCodeownersForSimplePath( targetPath, @@ -71,6 +90,11 @@ public static int Main( Console.WriteLine(codeownersJson); return 0; + + string[] SplitIgnoredPathPrefixes() + => ignoredPathPrefixes.Split( + "|", + StringSplitOptions.TrimEntries | StringSplitOptions.RemoveEmptyEntries); } catch (Exception e) { @@ -79,18 +103,22 @@ public static int Main( } } - private static object GetCodeownersForGlobPath( + private static Dictionary GetCodeownersForGlobPath( GlobFilePath targetPath, string targetDir, string codeownersFilePathOrUrl, bool excludeNonUserAliases, + string[]? ignoredPathPrefixes = null, bool useRegexMatcher = false) { + ignoredPathPrefixes ??= Array.Empty(); + Dictionary codeownersEntries = CodeownersFile.GetMatchingCodeownersEntries( targetPath, targetDir, codeownersFilePathOrUrl, + ignoredPathPrefixes, useRegexMatcher); if (excludeNonUserAliases) @@ -99,7 +127,7 @@ private static object GetCodeownersForGlobPath( return codeownersEntries; } - private static object GetCodeownersForSimplePath( + private static CodeownersEntry GetCodeownersForSimplePath( string targetPath, string codeownersFilePathOrUrl, bool excludeNonUserAliases, diff --git a/tools/code-owners-parser/CodeOwnersParser.sln.DotSettings b/tools/code-owners-parser/CodeOwnersParser.sln.DotSettings index 1414c94c6aa..3d218284c9a 100644 --- a/tools/code-owners-parser/CodeOwnersParser.sln.DotSettings +++ b/tools/code-owners-parser/CodeOwnersParser.sln.DotSettings @@ -1,4 +1,5 @@  PR True - True \ No newline at end of file + True + True \ No newline at end of file diff --git a/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs b/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs index 30899be335f..502319580c4 100644 --- a/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs +++ b/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs @@ -49,12 +49,15 @@ public static Dictionary GetMatchingCodeownersEntries( GlobFilePath targetPath, string targetDir, string codeownersFilePathOrUrl, + string[]? ignoredPathPrefixes = null, bool useRegexMatcher = UseRegexMatcherDefault) { + ignoredPathPrefixes ??= Array.Empty(); + var codeownersEntries = GetCodeownersEntriesFromFileOrUrl(codeownersFilePathOrUrl); Dictionary codeownersEntriesByPath = targetPath - .ResolveGlob(targetDir).ToDictionary( + .ResolveGlob(targetDir, ignoredPathPrefixes).ToDictionary( path => path, path => GetMatchingCodeownersEntry( path, diff --git a/tools/code-owners-parser/CodeOwnersParser/GlobFilePath.cs b/tools/code-owners-parser/CodeOwnersParser/GlobFilePath.cs index ffbf68313a6..2529be24bde 100644 --- a/tools/code-owners-parser/CodeOwnersParser/GlobFilePath.cs +++ b/tools/code-owners-parser/CodeOwnersParser/GlobFilePath.cs @@ -26,8 +26,10 @@ public GlobFilePath(string globFilePath) public static bool IsGlobFilePath(string path) => path.Contains('*'); - public List ResolveGlob(string directoryPath) + public List ResolveGlob(string directoryPath, string[]? ignoredPathPrefixes) { + ignoredPathPrefixes ??= Array.Empty(); + var globMatcher = new Matcher(StringComparison.Ordinal); globMatcher.AddInclude(this.filePath); @@ -35,6 +37,7 @@ public List ResolveGlob(string directoryPath) matchedPaths = matchedPaths .Select(path => Path.GetRelativePath(directoryPath, path).Replace("\\", "/")) + .Where(path => ignoredPathPrefixes.All(prefix => !path.StartsWith(prefix))) .ToList(); return matchedPaths; diff --git a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs index 267975065b1..042185c6984 100644 --- a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs +++ b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Text.RegularExpressions; @@ -13,13 +14,15 @@ namespace Azure.Sdk.Tools.CodeOwnersParser /// /// CodeownersFile.GetMatchingCodeownersEntryLegacyImpl() /// - /// This new matcher supports matching against wildcards, while the old one doesn't. + /// This new matcher supports matching against wildcards (* and **), while the old one doesn't. /// This new matcher is designed to work with CODEOWNERS file validation: /// https://github.com/Azure/azure-sdk-tools/issues/4859 /// - /// To use this class, construct it. - /// - /// To obtain the value of the matched entry, reference "Value" member. + /// The validation spec is given in this comment: + /// https://github.com/Azure/azure-sdk-tools/issues/4859#issuecomment-1370360622 + /// + /// To use this class, construct it, passing as input relevant paths. + /// Then, to obtain the value of the matched entry, reference "Value" member. /// /// Reference: /// https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#codeowners-syntax @@ -58,11 +61,17 @@ public MatchedCodeownersEntry(string targetPath, List codeowner } /// - /// Returns a CodeownersEntry from codeOwnerEntries that matches targetPath - /// per algorithm described in the GitHub CODEOWNERS reference, + /// Returns a CodeownersEntry from codeownersEntries, after normalization and validation, + /// that match targetPath per algorithm described in the GitHub CODEOWNERS reference, /// as linked to in this class comment. /// - /// If there is no match, returns "new CodeownersEntry()". + /// Paths that are not valid after normalization are skipped from matching. + /// + /// If there is no match, this method returns "new CodeownersEntry()". + /// + /// For definition of "normalization", see NormalizePath(). + /// For definition of "validation", see IsCodeownersPathValid(). + /// You can also refer to the validation spec linked from this class comment. /// private CodeownersEntry GetMatchingCodeownersEntry( string targetPath, @@ -87,7 +96,7 @@ private CodeownersEntry GetMatchingCodeownersEntry( // by the virtue of not ending with "/". CodeownersEntry matchedEntry = codeownersEntries - .Where(entry => !ContainsUnsupportedCharacters(entry.PathExpression)) + .Where(entry => !ContainsUnsupportedFragments(entry.PathExpression)) // Entries listed in CODEOWNERS file below take precedence, hence we read the file from the bottom up. // By convention, entries in CODEOWNERS should be sorted top-down in the order of: // - 'RepoPath', @@ -105,10 +114,14 @@ private CodeownersEntry GetMatchingCodeownersEntry( private CodeownersEntry NoMatchCodeownersEntry { get; } = new CodeownersEntry(); + private static bool ContainsUnsupportedFragments(string codeownersPath) + => ContainsUnsupportedCharacters(codeownersPath) + || ContainsUnsupportedSequences(codeownersPath); + /// /// See the comment on unsupportedChars. /// - private bool ContainsUnsupportedCharacters(string codeownersPath) + private static bool ContainsUnsupportedCharacters(string codeownersPath) { var contains = unsupportedChars.Any(codeownersPath.Contains); if (contains) @@ -121,6 +134,40 @@ private bool ContainsUnsupportedCharacters(string codeownersPath) return contains; } + private static bool ContainsUnsupportedSequences(string codeownersPath) + { + // We do not support suffix of "/**" because it is equivalent to "/". + // We do not support suffix of "/**/" because it is equivalent to + // "/**/**" which is equivalent to "/". + string[] unsupportedSuffixSentences = { "/**", "/**/" }; + + foreach (var sequence in unsupportedSuffixSentences) + { + if (codeownersPath.EndsWith(sequence)) + { + Console.Error.WriteLine( + $"CODEOWNERS path \"{codeownersPath}\" ends with " + + $"unsupported sequence of \"{sequence}\". Replace it with \"/\". " + + "Until then this path will never match."); + return true; + } + } + + // We do not support inline "**", i.e. if it is not within slashes, i.e. "/**/". + // Any inline "**" like "/a**/" or "/**a/" or "/a**b/" + // would be equivalent to single star, hence we forbid double star, to avoid confusion. + if (codeownersPath.Replace("/**/", "").Contains("**")) + { + Console.Error.WriteLine( + $"CODEOWNERS path \"{codeownersPath}\" contains " + + "unsupported sequence of \"**\" that is not \"/**/\". Replace it with \"*\". " + + "Until then this path will never match."); + return true; + } + + return false; + } + /// /// Returns true if the regex expression representing the PathExpression /// of CODEOWNERS entry matches a prefix of targetPath. @@ -129,26 +176,33 @@ private bool Matches(string targetPath, CodeownersEntry entry) { string codeownersPath = entry.PathExpression; - Regex regex = ConvertToRegex(targetPath, codeownersPath); + Regex regex = ConvertToRegex(codeownersPath); // Is prefix match. I.e. it will return true if the regex matches // a prefix of targetPath. return regex.IsMatch(targetPath); } - private Regex ConvertToRegex(string targetPath, string codeownersPath) + private Regex ConvertToRegex(string codeownersPath) { codeownersPath = NormalizePath(codeownersPath); + Trace.Assert(IsCodeownersPathValid(codeownersPath)); string pattern = codeownersPath; - if (codeownersPath.Contains(DoubleStar) || pattern.Contains(SingleStar)) + if (codeownersPath.Contains(SingleStar) || pattern.Contains(DoubleStar)) { Console.Error.WriteLine( $"CODEOWNERS path \"{codeownersPath}\" contains reserved phrases: " + $"\"{DoubleStar}\" or \"{SingleStar}\""); } - pattern = pattern.Replace("**", DoubleStar); + // We replace "/**/", not "**", because we disallow "**" in any other context. + // Specifically: + // - because we normalize the path to start with "/", any prefix "**/" is + // effectively "/**/"; + // - any suffix "/**", for reasons explained within ContainsUnsupportedSequences(). + // - any inline "**", for reasons explained within ContainsUnsupportedSequences(). + pattern = pattern.Replace("/**/", "/" + DoubleStar + "/"); pattern = pattern.Replace("*", SingleStar); pattern = Regex.Escape(pattern); @@ -156,23 +210,19 @@ private Regex ConvertToRegex(string targetPath, string codeownersPath) // Denote that all paths are absolute by pre-pending "beginning of string" symbol. pattern = "^" + pattern; - pattern = SetPatternSuffix(targetPath, pattern); + pattern = SetPatternSuffix(pattern); - // Note that the "/**/" case is implicitly covered by "**/". - pattern = pattern.Replace($"{DoubleStar}/", "(.*)"); - // This case is necessary to cover suffix case, e.g. "/foo/bar/**". - pattern = pattern.Replace($"/{DoubleStar}", "(.*)"); - // This case is necessary to cover inline **, e.g. "/a**b/". - pattern = pattern.Replace(DoubleStar, "(.*)"); + pattern = pattern.Replace($"/{DoubleStar}/", "((/.*/)|/)"); pattern = pattern.Replace(SingleStar, "([^/]*)"); return new Regex(pattern); } - private static string SetPatternSuffix(string targetPath, string pattern) + private static string SetPatternSuffix(string pattern) { // Lack of slash at the end denotes the path is a path to a file, // per our validation logic. + // // Note we assume this is path to a file even if the path is invalid, // even though in such case the path might be a path to a directory. if (!pattern.EndsWith("/")) @@ -181,30 +231,26 @@ private static string SetPatternSuffix(string targetPath, string pattern) // not a substring, as we are dealing with a file. pattern += "$"; } - // If the CODEOWNERS pattern is matching only against directories, - // but the targetPath may not be a directory - // (as it doesn't have "/" at the end), we need to trim - // the "/" from the pattern to ensure match is present. - // - // To illustrate this, consider following cases: - // - // 1. 2. - // targetPath: /a , /a*/ - // pattern: /a/ , /abc - // - // Without trimming pattern to be "/a" and "/a*" respectively, - // these wouldn't match, but they should. - // - // On the other hand, trimming the suffix "/" when it is not - // necessary won't lead to issues. E.g.: - // - // targetPath: /a/b - // pattern: /a/ - // - // Here we still have a prefix match even if we trim pattern to "/a". - else if (pattern.EndsWith("/") && !targetPath.EndsWith("/")) + else // The pattern ends with "/", denoting it is a directory. { - pattern = pattern.TrimEnd('/'); + // We do not append the end-of-string symbol, "$", + // because we want to allow matching to any string suffix, + // which semantically means matching to anything (including nothing) + // in the directory represented by the path. + + // Note that if the targetPath is exactly the same as pattern, + // except that it is missing the trailing "/", then we assume + // no match. This is because The pattern is matching + // against a directory, which, due to our validation, + // should exist. The targetPath, because it doesn't have trailing "/", + // matches to a file with the same name, hence it cannot exist. + // + // For example: + // pattern: /a/ + // targetPath: /a + // result: no match + // + // There is no match because "/a" is not a substring of "/a/". } return pattern; @@ -217,7 +263,7 @@ private static string SetPatternSuffix(string targetPath, string pattern) /// paths that somehow slipped through that validation. We do so by instead treating /// such paths as if they were absolute to repository root, i.e. starting with "/". /// - private string NormalizePath(string codeownersPath) + private static string NormalizePath(string codeownersPath) { if (!codeownersPath.StartsWith("/")) codeownersPath = "/" + codeownersPath; @@ -225,22 +271,20 @@ private string NormalizePath(string codeownersPath) } /// - /// The entry is valid if it obeys following conditions: - /// - The Value was obtained with a call to - /// Azure.Sdk.Tools.CodeOwnersParser.CodeownersFile.GetCodeownersEntries(). - /// - As a consequence, in the case of no match, the entry is not valid. - /// - It does not contain unsupported characters (see "unsupportedChars"). - /// - the Value.PathExpression starts with "/". + /// The codeownersPathExpression is valid if it obeys following conditions: + /// - It starts with "/". + /// - It doesn't contain unsupported fragments, including unsupported + /// characters, character sequences and character sequence suffixes. /// /// Once the validation described in the following issue is implemented: /// https://github.com/Azure/azure-sdk-tools/issues/4859 /// to be valid, the entry will also have to obey following conditions: - /// - if the Value.PathExpression ends with "/", at least one corresponding + /// - if the path expression ends with "/" at least one matching /// directory exists in the repository. - /// - if the Value.PathExpression does not end with "/", at least one corresponding + /// - if the path expression does not end with "/", at least one matching /// file exists in the repository. /// - private bool IsCodeownersPathValid(string codeownersPath) - => codeownersPath.StartsWith("/") && !ContainsUnsupportedCharacters(codeownersPath); + private bool IsCodeownersPathValid(string codeownersPathExpression) + => codeownersPathExpression.StartsWith("/") && !ContainsUnsupportedFragments(codeownersPathExpression); } }