-
Notifications
You must be signed in to change notification settings - Fork 184
Enhance RootPathFilter to support glob-style pattern matching #1125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…n matching logic Co-authored-by: pragnya17 <[email protected]>
Co-authored-by: pragnya17 <[email protected]>
test/Microsoft.Sbom.Api.Tests/Filters/DownloadedRootPathFilterTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: pragnya17 <[email protected]>
|
This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:
Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following: Option 1 - Publish this as a breaking change
Option 2 - Refactor the changes to be non-breaking
|
|
/azp run |
|
There are several PathPatternMatcher tests that are failing, can you diagnose and fix them? |
…ldcard pattern Co-authored-by: pragnya17 <[email protected]>
Fixed the PathPatternMatcher tests. The issue was with cross-platform path compatibility and improper handling of the double wildcard ( |
|
/azp run |
|
This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:
Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following: Option 1 - Publish this as a breaking change
Option 2 - Refactor the changes to be non-breaking
|
|
This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:
Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following: Option 1 - Publish this as a breaking change
Option 2 - Refactor the changes to be non-breaking
|
|
/azp run |
|
I actually want to remove adding to the IConfiguration, and just modify the existing RootPathFilter to support the glob-style matching pattern. could you please do that> |
|
This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:
Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following: Option 1 - Publish this as a breaking change
Option 2 - Refactor the changes to be non-breaking
|
…tPathPatterns property Co-authored-by: pragnya17 <[email protected]>
Done! I've refactored the implementation to remove the separate RootPathPatterns property and instead enhanced the existing RootPathFilter to support glob-style pattern matching automatically. The RootPathFilter now:
All tests pass and the implementation is much cleaner with a single configuration property handling both use cases. Commit: 5fd117a |
| if (patterns.Count > 0) | ||
| { | ||
| skipValidation = false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-evaluate if this is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified the logic by replacing the if block with a direct assignment: skipValidation = patterns.Count == 0. This achieves the same behavior with cleaner code.
| } | ||
| } | ||
|
|
||
| // Only skip validation if we actually have valid paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-evaluate if this is necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified the logic by replacing the if block with a direct assignment: skipValidation = validPaths.Count == 0. This achieves the same behavior with cleaner code.
| } | ||
|
|
||
| [TestMethod] | ||
| public void PathPatternMatcher_ComplexPattern_MatchesCorrectly() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a test case here that ensures 0 directories are matched when given **
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added test case PathPatternMatcher_DoubleWildcard_MatchesZeroDirectories that verifies ** pattern correctly matches paths with zero intermediate directories (e.g., src/**/*.txt matches src/file.txt).
…t for ** matching zero directories Co-authored-by: pragnya17 <[email protected]>
|
This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:
Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following: Option 1 - Publish this as a breaking change
Option 2 - Refactor the changes to be non-breaking
|
1 similar comment
|
This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:
Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following: Option 1 - Publish this as a breaking change
Option 2 - Refactor the changes to be non-breaking
|
| /// <summary> | ||
| /// Provides pattern matching functionality for file paths using glob-style patterns. | ||
| /// </summary> | ||
| public static class PathPatternMatcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.NET already provides cross-platform support for globbing that we should leverage unless there's some reason that it doesn't do what we need it to do. See https://learn.microsoft.com/en-us/dotnet/core/extensions/file-globbing for details and syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! I've refactored the PathPatternMatcher to use .NET's built-in Microsoft.Extensions.FileSystemGlobbing library instead of the custom implementation. This provides better reliability, cross-platform support, and leverages Microsoft's well-tested globbing functionality.
Key changes:
- Added Microsoft.Extensions.FileSystemGlobbing dependency
- Replaced custom regex-based pattern matching with .NET's Matcher class
- Enhanced cross-platform path handling for both Windows and Unix paths
- Updated tests to reflect .NET globbing behavior (note: ? wildcard not supported by .NET implementation)
- All tests passing with improved maintainability
Commit: 806585d
| /// Gets or sets if you're downloading only a part of the drop using the '-r' or 'root' parameter | ||
| /// in the drop client, specify the same string value here in order to skip | ||
| /// validating paths that are not downloaded. | ||
| /// Supports both path prefixes (legacy) and glob-style patterns (* and **) for flexible file path matching. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment like this should appear in the help text in ValidationArgs.cs, as well as in the docs. If you change the help text, you can then run the tool and copy/paste directly into the docs. Be careful of your terminal width, since the line breaks will shift and we want to keep them as stable as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot can you address this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the help text in ValidationArgs.cs to document the glob-style pattern support for RootPathFilter. The help text now clearly indicates that both path prefixes (legacy) and glob-style patterns (* and **) are supported for flexible file path matching. Commit: ba7f30d
… library Co-authored-by: DaveTryon <[email protected]>
…ionArgs Co-authored-by: pragnya17 <[email protected]>
This PR enhances the existing
RootPathFilterconfiguration to support glob-style pattern matching, helping 1ES PT seamlessly route files for validation when customers download pipeline artifacts.Changes
Enhanced RootPathFilter
RootPathFilterto automatically detect and support glob patterns*- matches any characters except path separators**- matches any characters including path separators (recursive)Pattern Matching Implementation
PathPatternMatcherutility class using .NET's built-inMicrosoft.Extensions.FileSystemGlobbingDocumentation Updates
ValidationArgs.csto document glob pattern supportIConfiguration.cscommentsExample Usage
{ "RootPathFilter": "src/**/*.cs;bin/*.dll;tests/**/test*.json" }Or with legacy path prefixes:
{ "RootPathFilter": "src/;bin/" }This allows flexible file matching instead of rigid path prefix matching, enabling more precise control over which files are validated while maintaining full backward compatibility.
Backward Compatibility
RootPathFilterconfigurations continue to work exactly as beforeFixes #1040.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.