diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/Azure.Sdk.Tools.CodeOwnersParser.Tests.csproj b/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/Azure.Sdk.Tools.CodeOwnersParser.Tests.csproj new file mode 100644 index 00000000000..db32d036a8c --- /dev/null +++ b/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/Azure.Sdk.Tools.CodeOwnersParser.Tests.csproj @@ -0,0 +1,23 @@ + + + + net6.0 + 10.0 + enable + Nullable + false + + + + + + + + + + + + + + + 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 new file mode 100644 index 00000000000..85d19043c48 --- /dev/null +++ b/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeOwnersFileTests.cs @@ -0,0 +1,160 @@ +using System; +using System.Collections.Generic; +using Microsoft.Extensions.FileSystemGlobbing; +using NUnit.Framework; + +namespace Azure.Sdk.Tools.CodeOwnersParser.Tests; + +[TestFixture] +public class CodeOwnersFileTests +{ + /// + /// 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 + /// + 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 ), + new( "3*" , "x/a/b" , "a/*" , false , false ), + new( "4*" , "a/b" , "a/*/*" , false , false ), + new( "5*" , "a/b/c/d" , "a/*/*/d" , false , true ), + new( "6*" , "a/b/x/c/d" , "a/*/*/d" , false , false ), + new( "7*" , "a/b/x/c/d" , "a/**/*/d" , false , true ), + new( "*1" , "a/b" , "*/b" , false , true ), + new( "*2" , "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" , "a/*" , false , true ) + // @formatter:on + }; + + /// + /// A repro for https://github.com/dotnet/runtime/issues/80076 + /// + [Test] + public void TestGlobBugRepro() + { + var globMatcher = new Matcher(StringComparison.Ordinal); + globMatcher.AddInclude("/*/"); + + var dir = new InMemoryDirectoryInfo( + rootDir: "/", + files: new List { "/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); + } + + /// + /// Exercises Azure.Sdk.Tools.CodeOwnersParser.Tests.CodeOwnersFileTests.testCases. + /// See comment on that member for details. + /// + [TestCaseSource(nameof(testCases))] + public void TestParseAndFindOwnersForClosestMatch(TestCase testCase) + { + List? 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 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); +} diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/Azure.Sdk.Tools.RetrieveCodeOwners.Tests.csproj b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/Azure.Sdk.Tools.RetrieveCodeOwners.Tests.csproj index 4f744552b70..87d23caffe9 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/Azure.Sdk.Tools.RetrieveCodeOwners.Tests.csproj +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/Azure.Sdk.Tools.RetrieveCodeOwners.Tests.csproj @@ -2,13 +2,17 @@ net6.0 + enable + Nullable false - - - + + + + + @@ -17,7 +21,7 @@ - Always + PreserveNewest diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/MainTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/MainTests.cs index 6242a046ec2..51701637e91 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/MainTests.cs +++ b/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 expectReturn, string output) { - CodeOwnerEntry codeOwnerEntry = JsonSerializer.Deserialize(output); + CodeOwnerEntry? codeOwnerEntry = JsonSerializer.Deserialize(output); List 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])); } } } diff --git a/tools/code-owners-parser/CodeOwnersParser.sln b/tools/code-owners-parser/CodeOwnersParser.sln index 260c393ac31..fc4b2530a2f 100644 --- a/tools/code-owners-parser/CodeOwnersParser.sln +++ b/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 diff --git a/tools/code-owners-parser/CodeOwnersParser/Azure.Sdk.Tools.CodeOwnersParser.csproj b/tools/code-owners-parser/CodeOwnersParser/Azure.Sdk.Tools.CodeOwnersParser.csproj index eb6d93f360b..8a4e48256af 100644 --- a/tools/code-owners-parser/CodeOwnersParser/Azure.Sdk.Tools.CodeOwnersParser.csproj +++ b/tools/code-owners-parser/CodeOwnersParser/Azure.Sdk.Tools.CodeOwnersParser.csproj @@ -7,6 +7,7 @@ + diff --git a/tools/code-owners-parser/CodeOwnersParser/CodeOwnersFile.cs b/tools/code-owners-parser/CodeOwnersParser/CodeOwnersFile.cs index 578cd531e80..0e7d4285973 100644 --- a/tools/code-owners-parser/CodeOwnersParser/CodeOwnersFile.cs +++ b/tools/code-owners-parser/CodeOwnersParser/CodeOwnersFile.cs @@ -59,13 +59,27 @@ public static List 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 codeOwnerEntries, string targetPath) + public static CodeOwnerEntry FindOwnersForClosestMatch( + List codeOwnerEntries, + string targetPath, + bool useNewFindOwnersForClosestMatchImpl = false) + { + return useNewFindOwnersForClosestMatchImpl + ? new MatchedCodeOwnerEntry(codeOwnerEntries, targetPath).Value + : FindOwnersForClosestMatchLegacyImpl(codeOwnerEntries, targetPath); + } + + private static CodeOwnerEntry FindOwnersForClosestMatchLegacyImpl(List codeOwnerEntries, + string targetPath) { // Normalize the start and end of the paths by trimming slash targetPath = targetPath.Trim('/'); diff --git a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeOwnerEntry.cs b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeOwnerEntry.cs new file mode 100644 index 00000000000..b13df0f6c7c --- /dev/null +++ b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeOwnerEntry.cs @@ -0,0 +1,184 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using Microsoft.Extensions.FileSystemGlobbing; + +namespace Azure.Sdk.Tools.CodeOwnersParser +{ + /// + /// Represents a CODEOWNERS file entry that matched to targetPath from + /// the list of entries, assumed to have been parsed from CODEOWNERS file. + /// + /// To obtain the value of the matched entry, reference "Value" member. + /// + internal class MatchedCodeOwnerEntry + { + public readonly CodeOwnerEntry Value; + + private static readonly char[] unsupportedChars = { '[', ']', '!', '?' }; + + public MatchedCodeOwnerEntry(List entries, string targetPath) + { + this.Value = FindOwnersForClosestMatch(entries, targetPath); + } + + /// + /// Returns a CodeOwnerEntry from codeOwnerEntries that matches targetPath + /// per algorithm described in: + /// https://git-scm.com/docs/gitignore#_pattern_format + /// and + /// https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#codeowners-syntax + /// + /// If there is no match, returns "new CodeOwnerEntry()". + /// + private static CodeOwnerEntry FindOwnersForClosestMatch( + List codeownersEntries, + string targetPath) + { + // targetPath is assumed to be absolute w.r.t. repository root, hence we ensure + // it starts with "/" to denote that. + if (!targetPath.StartsWith("/")) + targetPath = "/" + targetPath; + + // We do not trim or add the slash ("/") at the end of the targetPath because its + // presence influences the matching algorithm: + // Slash at the end denotes the target path is a directory, not a file, so it might + // match against a CODEOWNERS entry that matches only directories and not files. + + // Entries 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', + // - 'ServicePath' + // - and then 'PackagePath'. + // However, due to lack of validation, as of 12/29/2022 this is not always the case. + for (int i = codeownersEntries.Count - 1; i >= 0; i--) + { + string codeownersPath = codeownersEntries[i].PathExpression; + if (ContainsUnsupportedCharacters(codeownersPath)) + { + continue; + } + + List globPatterns = ConvertToGlobPatterns(codeownersPath); + PatternMatchingResult patternMatchingResult = MatchGlobPatterns(targetPath, globPatterns); + if (patternMatchingResult.HasMatches) + { + return codeownersEntries[i]; + } + } + // assert: none of the codeownersEntries matched targetPath + return new CodeOwnerEntry(); + } + + private static bool ContainsUnsupportedCharacters(string codeownersPath) + => unsupportedChars.Any(codeownersPath.Contains); + + /// + /// Converts codeownersPath to a set of glob patterns to include in + /// glob matching. The conversion is a translation from codeowners and .gitignore + /// spec into glob. That is, it reduces the spec to glob rules, + /// which then can be checked against using glob matcher. + /// + /// + /// Usually 1 glob pattern to include in matching. In one special case + /// returns 2 patterns, which happens when the path needs to be interpreted + /// both as-is file, or as a directory prefix. + /// + private static List ConvertToGlobPatterns(string codeownersPath) + { + codeownersPath = ConvertPrefix(codeownersPath); + var patternsToInclude = PatternsToInclude(codeownersPath); + return patternsToInclude; + } + + private static string ConvertPrefix(string codeownersPath) + { + // Codeowners entry path starting with "/*" is equivalent to it starting with "*". + // Note this also covers cases when it starts with "/**". + if (codeownersPath.StartsWith("/*")) + codeownersPath = codeownersPath.Substring("/".Length); + + // If the codeownersPath doesn't have any slash at the beginning or in the middle, + // then it means its start is relative to any directory in the repository, + // hence we prepend "**/" to reflect this as a glob pattern. + if (!codeownersPath.TrimEnd('/').Contains("/")) + { + codeownersPath = "**/" + codeownersPath; + } + // If, on the other hand, codeownersPath has to start at the root, we ensure + // it starts with slash to reflect that. + else + { + if (!codeownersPath.StartsWith("/")) + { + codeownersPath = "/" + codeownersPath; + } + else + { + // codeownersPath already starts with "/", so nothing to prepend. + } + } + + return codeownersPath; + } + + private static List PatternsToInclude(string codeownersPath) + { + List patternsToInclude = new List(); + + if (codeownersPath.EndsWith("/")) + { + patternsToInclude.Add(ConvertDirectorySuffix(codeownersPath)); + } + else + { + patternsToInclude.Add(ConvertDirectorySuffix(codeownersPath + "/")); + patternsToInclude.Add(codeownersPath); + } + + return patternsToInclude; + } + + private static string ConvertDirectorySuffix(string codeownersPath) + { + // If the codeownersPath doesn't already end with "*", + // we need to append "**", to denote that codeownersPath has to match + // a prefix of the targetPath, not the entire path. + if (!codeownersPath.TrimEnd('/').EndsWith("*")) + { + codeownersPath += "**"; + } + else + { + // codeownersPath directory already has stars in the suffix, so nothing to do. + // Example paths: + // apps/*/ + // apps/**/ + } + + return codeownersPath; + } + + private static PatternMatchingResult MatchGlobPatterns( + string targetPath, + List patterns) + { + // Note we use StringComparison.Ordinal, not StringComparison.OrdinalIgnoreCase, + // as CODEOWNERS paths are case-sensitive. + var globMatcher = new Matcher(StringComparison.Ordinal); + + foreach (var pattern in patterns) + { + globMatcher.AddInclude(pattern); + } + + var dir = new InMemoryDirectoryInfo( + // This 'rootDir: "/"' is used here only because the globMatcher API requires it. + rootDir: "/", + files: new List { targetPath }); + + var patternMatchingResult = globMatcher.Execute(dir); + return patternMatchingResult; + } + } +}