-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Use Content-Disposition for output file name in Webcmdlets #19385
base: master
Are you sure you want to change the base?
Conversation
....PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebResponseHelper.CoreClr.cs
Outdated
Show resolved
Hide resolved
....PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebResponseHelper.CoreClr.cs
Outdated
Show resolved
Hide resolved
This PR works correctly, the new tests don't In HttpBin this code works as expected: $ContentDisposition = [System.Net.Http.Headers.ContentDispositionHeaderValue]::new("attachment")
$ContentDisposition.FileName = 'DownloadedFile.txt'
$x = Invoke-WebRequest https://httpbin.org/response-headers?Content-Disposition=$ContentDisposition -OutFile .\ -PassThru -Verbose
#--> DownloadedFile.txt |
Though it named the file correctly, the Verbose message was |
....PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebResponseHelper.CoreClr.cs
Outdated
Show resolved
Hide resolved
....PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebResponseHelper.CoreClr.cs
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1
Outdated
Show resolved
Hide resolved
…s.Tests.ps1 Co-authored-by: Ilya <darpa@yandex.ru>
…s.Tests.ps1 Co-authored-by: Ilya <darpa@yandex.ru>
return Path.Join(_qualifiedOutFile, contentDisposition); | ||
} | ||
|
||
if (response.RequestMessage.RequestUri.PathAndQuery != "/") |
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.
Could you please clarify why we need PathAndQuery. Maybe add a comment in the code.
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.
We need it to handle this case:
Invoke-WebRequest https://www.google.com/ -OutFile .\ #PathAndQuery == "/"
#--> OutFile: www_google_com
|
||
return Directory.Exists(_qualifiedOutFile) ? Path.Join(_qualifiedOutFile, lastUriSegment) : _qualifiedOutFile; | ||
// File name not found use sanitized Host name instead | ||
return Path.Join(qualifiedOutFile, response.RequestMessage.RequestUri.Host.Replace('.', '_')); |
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.
I want we explicitly confirm this fallback.
/cc @SteveL-MSFT @mklement0
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.
{ | ||
// Get file name from last segment of Uri | ||
string lastUriSegment = System.Net.WebUtility.UrlDecode(response.RequestMessage.RequestUri.Segments[^1]); | ||
internal static string GetOutFilePath(HttpResponseMessage response, string qualifiedOutFile) |
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.
We should be more smart - check File.Exists for result file name and if there is a conflict add (1)
or (2)
and so on.
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.
curl doesn't check for existing files and just overwrites
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.
We should discuss this too. Perhaps NoClobber should be added.
TODO in follow up PR: add sanitation such as #11671 (comment) |
// Resume requires OutFile and can't be used with OutFolder.. | ||
if (Resume.IsPresent && OutFile is null) |
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.
Why Resume can't be used with OutFolder? I'd expect we get the same file name from the request.
And typo.
// Resume requires OutFile and can't be used with OutFolder.. | |
if (Resume.IsPresent && OutFile is null) | |
// Resume requires OutFile and can't be used with OutFolder. | |
if (Resume.IsPresent && OutFile is null) |
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.
It can't be used because the filename for OutFolder is calculated after the response and Resume is before the request
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.
Maybe not for the PR. We could make first request with range 0-0 (first byte) and see if we got the filename. It is not expensive.
Unrelated test failures |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
@SteveL-MSFT, the suggested logic in the WG decision makes sense, but introducing a new parameter Wouldn't it make more sense to add the functionality to |
@mklement0 maybe it would be better to have |
Why not add a -ContentDisposition switch? ( And I agree with Steve that making |
I can see the appeal of the additional But to me the question is whether this level of control is needed in practice; perhaps the suggested implied logic (use content-disposition filename, if present, fall back to the last URL segment) is enough? The appeal is that you'll get the server-suggested filename by default, if available. That said, as noted, this would technically be a breaking change to the current |
The WG discussed this. Looks like in 7.4 if
Order of precedence:
We believe this is a bucket 3 breaking change only for the case where |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
PR Summary
PR Context
Contributes to #11671 follow up to #19007
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).