-
Notifications
You must be signed in to change notification settings - Fork 100
Fix ArgumentException when analysis target URI contains characters that are illegal in the file system #2860
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?
Changes from 5 commits
7c33875
9c9d50c
4175e1a
3926882
c47ed20
617c84d
e761588
9f251f5
660ffff
90df443
80c829b
7355bee
ae3facc
c42794f
afef455
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -679,7 +679,26 @@ private async Task<bool> EnumerateArtifact(IEnumeratedArtifact artifact, TContex | |
| return false; | ||
| } | ||
|
|
||
| string extension = Path.GetExtension(filePath); | ||
| string extension; | ||
| int lastDotIndex = filePath.LastIndexOf('.'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you have added lots of code and you didn't add unit tests for this. if you're going to inline Path.GetExtension, perhaps just grab the actual code (and get rid of the invalid path chars check). can you please create a helper for this? also, did you look for other occurrences of this issue? i see other uses of Path.GetExtension in the code. https://referencesource.microsoft.com/#mscorlib/system/io/path.cs,f424e433705aeb09
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. None of the other uses of GetExtension looked like they were problematic to me and none of the telemetry points to any other code.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not yet convinced. The export rules and export config commands look like they could problematic to me as well. By inlining this change here and here only, you have left a non-obvious fix which compromises maintainability (i.e., it's very likely that if this problem is encountered again, a dev might not be aware of what you've done, add a duplicate helper or another inlined fix, etc.). What I'd suggest is to create a static helper for this rewrite and please add unit tests. Then, add a new entry to IFileSystem for GetExtension and call this helper through IFileSystem everywhere in the code base. |
||
|
|
||
| if (lastDotIndex < 0) | ||
| { | ||
| extension = string.Empty; | ||
| } | ||
| else | ||
| { | ||
| int lastSeparatorIndex = filePath.LastIndexOf(Path.PathSeparator); | ||
| if (lastSeparatorIndex > lastDotIndex) | ||
| { | ||
| extension = string.Empty; | ||
| } | ||
| else | ||
| { | ||
| extension = filePath.Substring(lastDotIndex); | ||
| } | ||
| } | ||
|
|
||
| if (artifact.Uri.IsAbsoluteUri && | ||
| string.IsNullOrEmpty(artifact.Uri.Query) && | ||
| globalContext.OpcFileExtensions.Contains(extension)) | ||
|
|
||
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.
This release note doesn't describe the code location where the fix was made.
Uh oh!
There was an error while loading. Please reload this page.
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.
You haven't described the method that raises the exception. This means that customers who have a stack frame in an exception might not easily find your note.
MultithreadedAnalyzeCommandBase.Runwould provide this information. Although you've documented this specific fix, you haven't described the other code locations you changed, which you could add. e.g.ExportRulesMetadataCommandBase.RunandExportConfigurationCommandBase.Run