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 ac7cb54677d..46eac380324 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 @@ -1,6 +1,4 @@ -using System; using System.Collections.Generic; -using Microsoft.Extensions.FileSystemGlobbing; using NUnit.Framework; namespace Azure.Sdk.Tools.CodeOwnersParser.Tests; @@ -10,129 +8,132 @@ 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. + /// 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. + /// The logic that has changed is located in CodeOwnersFile.FindOwnersForClosestMatch. /// /// 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. - /// + /// matcher expected matches that doesn't pertain to wildcard matching denotes + /// a potential backward compatibility and/or existing defect in the legacy matcher. + /// /// For further details, please see: - /// https://github.com/Azure/azure-sdk-tools/issues/2770 + /// - Class comment for Azure.Sdk.Tools.CodeOwnersParser.MatchedCodeOwnerEntry + /// - https://github.com/Azure/azure-sdk-tools/issues/2770 + /// - https://github.com/Azure/azure-sdk-tools/issues/4859 /// 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 ) + // Path: Expected match: + // Codeowners , Target , Legacy , New + new( "/a" , "a" , true , true ), + new( "/a" , "A" , true , false ), + new( "/a" , "/a" , true , true ), + new( "/a" , "a/" , true , false ), + new( "/a" , "/a/" , true , false ), + 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 , false ), + new( "a" , "/a" , true , true ), + new( "a" , "a/" , true , false ), + new( "a" , "/a/" , true , false ), + 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 , 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/" , "/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 , true ), + new( "/a/b/" , "/a/b/c" , true , true ), + new( "/a/b/" , "/a/b/c/" , true , true ), + new( "/a/b/" , "/a/b/c/d" , true , true ), + 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/c" , true , false ), + new( "/a/b" , "/a/b/c/" , true , false ), + new( "/a/b" , "/a/b/c/d" , true , false ), + new( "/!a" , "!a" , true , false ), + new( "/!a" , "b" , false , false ), + new( "/a[b" , "a[b" , true , false ), + new( "/a]b" , "a]b" , true , false ), + new( "/a?b" , "a?b" , true , false ), + new( "/a?b" , "axb" , false , false ), + new( "/*" , "a" , false , true ), + new( "/*" , "a/" , false , false ), + new( "/*" , "a/b" , false , false ), + 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/*" , "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 , 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( "/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/c" , 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( "/**/*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/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 , 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/[]/!!/CD/h" , 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. @@ -147,7 +148,8 @@ public void TestParseAndFindOwnersForClosestMatch(TestCase testCase) VerifyFindOwnersForClosestMatch(testCase, codeownersEntries, useNewImpl: true, testCase.ExpectedNewMatch); } - private static void VerifyFindOwnersForClosestMatch(TestCase testCase, + private static void VerifyFindOwnersForClosestMatch( + TestCase testCase, List codeownersEntries, bool useNewImpl, bool expectedMatch) @@ -162,7 +164,9 @@ private static void VerifyFindOwnersForClosestMatch(TestCase testCase, 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); + public record TestCase( + string CodeownersPath, + string TargetPath, + bool ExpectedLegacyMatch, + bool ExpectedNewMatch); } 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 cb16e53bd3b..efc105654b7 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs @@ -23,7 +23,7 @@ public static int Main( bool filterOutNonUserAliases = false ) { - var target = targetDirectory.ToLower().Trim(); + var target = targetDirectory.Trim(); try { var codeOwnerEntry = CodeOwnersFile.ParseAndFindOwnersForClosestMatch(codeOwnerFilePath, target); if (filterOutNonUserAliases) 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 8a4e48256af..03e676e6387 100644 --- a/tools/code-owners-parser/CodeOwnersParser/Azure.Sdk.Tools.CodeOwnersParser.csproj +++ b/tools/code-owners-parser/CodeOwnersParser/Azure.Sdk.Tools.CodeOwnersParser.csproj @@ -7,7 +7,7 @@ - + diff --git a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeOwnerEntry.cs b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeOwnerEntry.cs index b13df0f6c7c..76f3cd297ee 100644 --- a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeOwnerEntry.cs +++ b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeOwnerEntry.cs @@ -1,7 +1,7 @@ -using System; using System.Collections.Generic; using System.Linq; -using Microsoft.Extensions.FileSystemGlobbing; +using System.Text.RegularExpressions; +using Microsoft.Extensions.Logging; namespace Azure.Sdk.Tools.CodeOwnersParser { @@ -9,29 +9,71 @@ 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. /// + /// This is a new matcher, compared to the old one, located in: + /// + /// CodeOwnersFile.FindOwnersForClosestMatchLegacyImpl() + /// + /// This new matcher supports matching against wildcards, 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. + /// + /// Reference: + /// https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#codeowners-syntax + /// https://git-scm.com/docs/gitignore#_pattern_format /// internal class MatchedCodeOwnerEntry { + /// + /// Token for temporarily substituting "**" in regex, to avoid it being escaped when + /// Regex.Escape is called. + /// + private const string DoubleStar = "_DOUBLE_STAR_"; + /// + /// Token for temporarily substituting "*" in regex, to avoid it being escaped when + /// Regex.Escape is called. + /// + private const string SingleStar = "_SINGLE_STAR_"; + public readonly CodeOwnerEntry Value; + /// + /// See comment on IsCodeOwnersPathValid + /// + public bool IsValid => IsCodeOwnersPathValid(this.Value.PathExpression); + + /// + /// Any CODEOWNERS path with these characters will be skipped. + /// Note these are valid parts of file paths, but we are not supporting + /// them to simplify the matcher logic. + /// private static readonly char[] unsupportedChars = { '[', ']', '!', '?' }; + private readonly ILogger log; + public MatchedCodeOwnerEntry(List entries, string targetPath) { + this.log = CreateLog(); this.Value = FindOwnersForClosestMatch(entries, targetPath); } + private ILogger CreateLog() + { + var loggerFactory = LoggerFactory.Create(builder => { builder.AddSimpleConsole(); }); + return loggerFactory.CreateLogger(); + } + /// /// 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 + /// per algorithm described in the GitHub CODEOWNERS reference, + /// as linked to in this class comment. /// /// If there is no match, returns "new CodeOwnerEntry()". /// - private static CodeOwnerEntry FindOwnersForClosestMatch( + private CodeOwnerEntry FindOwnersForClosestMatch( List codeownersEntries, string targetPath) { @@ -40,145 +82,164 @@ private static CodeOwnerEntry FindOwnersForClosestMatch( 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(); + // Note we cannot add or trim the slash at the end of targetPath. + // Slash at the end of target path denotes it is a directory, not a file, + // so it can not match against a CODEOWNERS entry that is guaranteed to be a file, + // by the virtue of not ending with "/". + + CodeOwnerEntry matchedEntry = codeownersEntries + .Where(entry => !ContainsUnsupportedCharacters(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', + // - 'ServicePath' + // - and then 'PackagePath'. + // However, due to lack of validation, as of 12/29/2022 this is not always the case. + .Reverse() + .FirstOrDefault( + entry => Matches(targetPath, entry), + // assert: none of the codeownersEntries matched targetPath + new CodeOwnerEntry()); + + return matchedEntry; } - 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. + /// See the comment on unsupportedChars. /// - /// - /// 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) + private bool ContainsUnsupportedCharacters(string codeownersPath) { - codeownersPath = ConvertPrefix(codeownersPath); - var patternsToInclude = PatternsToInclude(codeownersPath); - return patternsToInclude; + var contains = unsupportedChars.Any(codeownersPath.Contains); + if (contains) + { + log.LogWarning( + $"CODEOWNERS path \"{codeownersPath}\" contains unsupported characters: " + + string.Join(' ', unsupportedChars) + + " Because of that this path will never match."); + } + return contains; } - private static string ConvertPrefix(string codeownersPath) + /// + /// Returns true if the regex expression representing the PathExpression + /// of CODEOWNERS entry matches a prefix of targetPath. + /// + private bool Matches(string targetPath, CodeOwnerEntry entry) { - // 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. - } - } + string codeownersPath = entry.PathExpression; - return codeownersPath; + Regex regex = ConvertToRegex(targetPath, codeownersPath); + // Is prefix match. I.e. it will return true if the regex matches + // a prefix of targetPath. + return regex.IsMatch(targetPath); } - private static List PatternsToInclude(string codeownersPath) + private Regex ConvertToRegex(string targetPath, string codeownersPath) { - List patternsToInclude = new List(); + codeownersPath = NormalizePath(codeownersPath); - if (codeownersPath.EndsWith("/")) - { - patternsToInclude.Add(ConvertDirectorySuffix(codeownersPath)); - } - else + string pattern = codeownersPath; + + if (codeownersPath.Contains(DoubleStar) || pattern.Contains(SingleStar)) { - patternsToInclude.Add(ConvertDirectorySuffix(codeownersPath + "/")); - patternsToInclude.Add(codeownersPath); + log.LogWarning( + $"CODEOWNERS path \"{codeownersPath}\" contains reserved phrases: " + + $"\"{DoubleStar}\" or \"{SingleStar}\""); } - return patternsToInclude; + pattern = pattern.Replace("**", DoubleStar); + pattern = pattern.Replace("*", SingleStar); + + pattern = Regex.Escape(pattern); + + // Denote that all paths are absolute by pre-pending "beginning of string" symbol. + pattern = "^" + pattern; + + pattern = SetPatternSuffix(targetPath, 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(SingleStar, "([^/]*)"); + + return new Regex(pattern); } - private static string ConvertDirectorySuffix(string codeownersPath) + private static string SetPatternSuffix(string targetPath, string pattern) { - // 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("*")) + // 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("/")) { - codeownersPath += "**"; + // Append "end of string" symbol, denoting the match has to be exact, + // not a substring, as we are dealing with a file. + pattern += "$"; } - else + // 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("/")) { - // codeownersPath directory already has stars in the suffix, so nothing to do. - // Example paths: - // apps/*/ - // apps/**/ + pattern = pattern.TrimEnd('/'); } - return codeownersPath; + return pattern; } - private static PatternMatchingResult MatchGlobPatterns( - string targetPath, - List patterns) + /// + /// CODEOWNERS paths that do not start with "/" are relative and considered invalid, + /// See comment on "IsCodeOwnersPathValid" for definition of "valid". + /// However, here we handle such cases to accomodate for parsing CODEOWNERS file + /// 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) { - // 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; + if (!codeownersPath.StartsWith("/")) + codeownersPath = "/" + codeownersPath; + return codeownersPath; } + + /// + /// The entry is valid if it obeys following conditions: + /// - The Value was obtained with a call to + /// Azure.Sdk.Tools.CodeOwnersParser.CodeOwnersFile.ParseContent(). + /// - 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 "/". + /// + /// 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 + /// directory exists in the repository. + /// - if the Value.PathExpression does not end with "/", at least one corresponding + /// file exists in the repository. + /// + private bool IsCodeOwnersPathValid(string codeownersPath) + => codeownersPath.StartsWith("/") && !ContainsUnsupportedCharacters(codeownersPath); } } diff --git a/tools/identity-resolution/identity-resolution.csproj b/tools/identity-resolution/identity-resolution.csproj index 42deeddf898..f8666f676aa 100644 --- a/tools/identity-resolution/identity-resolution.csproj +++ b/tools/identity-resolution/identity-resolution.csproj @@ -6,7 +6,7 @@ - + diff --git a/tools/notification-configuration/notification-creator/Azure.Sdk.Tools.NotificationConfiguration.csproj b/tools/notification-configuration/notification-creator/Azure.Sdk.Tools.NotificationConfiguration.csproj index 7acff793635..5b76741d34a 100644 --- a/tools/notification-configuration/notification-creator/Azure.Sdk.Tools.NotificationConfiguration.csproj +++ b/tools/notification-configuration/notification-creator/Azure.Sdk.Tools.NotificationConfiguration.csproj @@ -9,7 +9,7 @@ - + diff --git a/tools/notification-configuration/notification-creator/Program.cs b/tools/notification-configuration/notification-creator/Program.cs index 1212581ed2f..bee812834f4 100644 --- a/tools/notification-configuration/notification-creator/Program.cs +++ b/tools/notification-configuration/notification-creator/Program.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.VisualStudio.Services.Common; @@ -39,12 +39,10 @@ static async Task Main( var devOpsCreds = new VssBasicCredential("nobody", devOpsToken); var devOpsConnection = new VssConnection(new Uri($"https://dev.azure.com/{organization}/"), devOpsCreds); -#pragma warning disable CS0618 // Type or member is obsolete var loggerFactory = LoggerFactory.Create(builder => { - builder.AddConsole(config => { config.IncludeScopes = true; }); + builder.AddSimpleConsole(config => { config.IncludeScopes = true; }); }); -#pragma warning restore CS0618 // Type or member is obsolete var devOpsServiceLogger = loggerFactory.CreateLogger(); var notificationConfiguratorLogger = loggerFactory.CreateLogger();