Allow using a folder path in WebCmdlet's OutFile parameter by CarloToso · Pull Request #19007 · PowerShell/PowerShell

CarloToso · 2023-01-23T01:02:41Z

PR Summary

In Invoke-WebRequest and Invoke-RestMethod -Outfile only works if you specify a file path
After this PR:

If you specify an existing directory, the file will be placed by its server-designated name there.
If you specify a file path, that path is saved to as-is; a file path is either:
- the path to an existing file (which is then quietly replaced, as before)
- the path to an existing directory followed by the name of a new file to be created

@mklement0
The filename is taken from the last segment of the Uri

Problems:

it currently doesn't work with -Resume and I'm not sure it can
I don't know how to make this an experimental feature

PR Context

#11671

PR Checklist

iSazonov · 2023-01-23T08:50:42Z

...Shell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/InvokeWebRequestCommand.CoreClr.cs

+                    else
+                    {
+                        // Get file name from last segment of Uri
+                        OutFile = Path.Combine(OutFile, System.Net.WebUtility.UrlDecode(response.RequestMessage.RequestUri.Segments[^1]));


I don't value. It is better to generate an error.

I think you're wrong, I have tested some websites and they rarely (never) use the Content-Disposition header. So it would always generate an error.
Tested downloads: Firefox, Chrome, 7zip, Adobe Reader

In this case there is no point in analyzing Content-Disposition at all :-)

LGTM. Please update the PR description.

~~Done. What should we do about the problem with -Resume ?~~
~~I'll add a new error~~
Added error when Resume is used and outfile doesn't point to a file

Content-Disposition may not be used often (I can't speak to that), but it is definitely used, including on GitHub:

E.g., in an archive download for a given project, such as https://github.com/mklement0/fileicon/archive/stable.zip, where using the last URL segment wouldn't even tell you the name of the product, the Content-Disposition field provides a helpful file name:

# -> 'attachment; filename=fileicon-stable.zip' (Invoke-WebRequest -Method Head https://github.com/mklement0/fileicon/archive/stable.zip).Headers['Content-Disposition']

Both curl and wget support Content-Disposition, albeit on an opt-in basis (-J and --content-disposition)

@mklement0 Please describe the desired behaviour and I will implement it, I removed the check for Content-Disposition only because @iSazonov requested it

Thanks, @CarloToso.

My suggestion is as follows - though I wonder if it requires a wider discussion:

I suggest also making the feature opt-in, given the following warning from man curl with respect to its opt-in (-J / --remote-header-name):

WARNING: Exercise judicious use of this option, especially on
Windows. A rogue server could send you the name of a DLL or
other file that could be loaded automatically by Windows or some
third party software.

That requires a new switch, say -UseSuggestedFileName, which would have to report an error if the -OutFile argument targets a file.

There's also a discoverability problem - also with the use-last-URI-segment approach, given that redirections may be involved, and that the new default behavior uses the redirected-to URI's last segment:

How does the user learn what file name was ultimately used (short of examining the target directory for the most recently added file)?

At the very least, -Verbose should include this information.

-PassThru should use an ETS instance member to decorate the passed-through Microsoft.PowerShell.Commands.BasicHtmlWebResponseObject instance with a .FullName property that reflects the output file's full path.

As for how to parse Content-Disposition and fallback behavior:

Based on the previously linked description, the C# equivalent of the following is probably good enough in practice, though the recommendation is to give precedence to filename* (sic) entries:

<value-of-Content-Disposition-field(s)> -replace '^.*\bfilename\*?="?([^;"]+)?.*', '$1'

If there's no Content-Disposition field or if the file-name extraction fails, fall back to the last-URI-segment approach

Maybe you could move this discussion to issue #11671

CarloToso · 2023-01-23T20:26:13Z

The test error looks like a random failure unrelated to this PR

...erShell.Commands.Utility/commands/utility/WebCmdlet/Common/InvokeRestMethodCommand.Common.cs

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

...Shell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/InvokeWebRequestCommand.CoreClr.cs

iSazonov · 2023-01-25T18:09:18Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

            }
+
+            // OutFile must not be a directory to use Resume.
+            if (Resume.IsPresent && Directory.Exists(QualifiedOutFile))


If we resolve the path we should cache it here and use later.

It seems it is not correct check since we could get the file name from URL.

I agree with you, I'll do it later

I think it's correct, this is the logic:

if -Outfile points to a file, old logic you can use resume

if -Outfile points to a folder (Directory.Exists(QualifiedOutFile)) you can't use the resume switch (throws an error) and the filename is taken later from the url

@iSazonov ~~I think I need a hand with caching the resolved path~~

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

iSazonov · 2023-01-28T04:32:16Z

@CarloToso We need new tests for null, empty, folder.

CarloToso · 2023-01-28T21:32:19Z

@iSazonov I added the tests for null and empty -OutFile to #19044

ghost · 2023-02-17T14:00:53Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

CarloToso · 2023-03-14T14:10:30Z

#firefox download
curl "https://download.mozilla.org/?product=firefox-stub&os=win&lang=it" -L -O
#--> _product=firefox-stub&os=win&lang=it
Invoke-WebRequest "https://download.mozilla.org/?product=firefox-stub&os=win&lang=it" -OutFile .\
#--> Firefox Installer.exe (BETTER)

#github download
curl "https://github.com/mklement0/ttab/archive/stable.zip" -L -O
#--> stable.zip (BETTER)
Invoke-WebRequest "https://github.com/mklement0/ttab/archive/stable.zip" -OutFile .\
#--> stable

@mklement0 It seems to me that both approaches have some issues... What should we do?

mklement0 · 2023-03-14T14:52:46Z

@CarloToso, yes, both approaches are defensible.

The curl approach has one advantage: it is obvious to the user what the output file name will be - though it may not be a meaningful one.
Your current approach may provide a more meaningful filename, but the user will have to investigate the output directory to find out what filename was used.

The curl approach is the simplest one (but note that curl also offers -J to honor a Content-Disposition filename).

If we stick with what you have now, we should offer a way to discover the filename via (a) -Verbose output and (b) via the object returned when you use -PassThru (in the simplest form with an ETS instance property containing a [System.IO.FileInfo] instance describing the output file) - this is why I asked for this aspect to be decided on first - see #11671 (comment)

The latter will also become important if and when we implement Content-Disposition support later.

(Curiously curl -JO also doesn't report the filename ultimately used, and while you can glean it from the -v (verbose) output, it's buried in a lot of other info; I think we can do better here).

mklement0 · 2023-03-14T20:10:12Z

@CarloToso, realistically speaking, given that my plea to decide on the pass-through issue before merging this PR are falling on deaf ears:

In the absence of a feature that allows discovery of the output filename via -PassThru, I suggest changing the implementation to follow curl'ls lead: use the input URI's last segment as the filename.

Also - I'm not sure how you're handling this - note that curl reports an error if the input URI lacks a server-relative path; e.g.:

PS> curl -LO http://example.org
curl: Remote file name has no length!
curl: (23) Failed writing received data to disk/application

This can notably also happen if the last URI segment happens to be invalid as a filename.
It happens with your https://download.mozilla.org/?product=firefox-stub&os=win&lang=it example, but, curiously, only non non-Windows platforms (perhaps because curl.exe actively strips out chars. such as ? on Windows.

CarloToso · 2023-03-14T20:29:22Z

@mklement0 I'm slowly working on addressing your suggestions about -PassThru, I created WebResponseHelper.GetOutFilePath as a stepping stone. I became aware of the problem you just described some hours ago and I'm working on a solution for a follow up PR. My current Roadmap is:

This PR --> first implementation, it only works for files using the last segment of the uri (fails for http://example.org), filename shown if -Verbose

One or Two other PR:

Add Content-Disposition support, choose each time between curl method and my method for filenames
Handle http://example.org case
Add warnings (like curl) each time we choose a folder as download target, discovery of the output filename via -PassThru, add more tests

I don't think I should add more commits to this PR

mklement0 · 2023-03-14T21:14:12Z

Thank you, @CarloToso, that makes sense, except I'm unclear on what you mean by:

choose each time between curl method and my method for filenames

Add warnings (like curl) each time we choose a folder as download target

CarloToso · 2023-03-14T21:29:38Z

@mklement0

Each time choose what filename works better between:

Uri.AbsoluteUri.Split("/")[^1] (sanitized) [curl]
response.RequestMessage.RequestUri.Segments[^1] [my method]

Add a warning message when the user chooses a folder path with -OutFile, you said it could be unsafe for the user

WARNING: Exercise judicious use of this option, especially on
Windows. A rogue server could send you the name of a DLL or
other file that could be loaded automatically by Windows or some
third party software.

mklement0 · 2023-03-14T21:51:19Z

Thanks for clarifying , @CarloToso.

Just food for thought, we can defer further discussion to your future PRs:

Re 1: How would you decide in cases where both methods yield a viable filename?

Re 2: Personally, I think warning in the documentation is sufficient (which is what curl does).

CarloToso · 2023-03-16T16:11:41Z

@iSazonov @SteveL-MSFT Can we merge this? So I con continue working on the name from Content-Disposition feature

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

…s.Tests.ps1 Co-authored-by: Steve Lee <slee@microsoft.com>

pull-request-quantifier-deprecated · 2023-03-20T19:41:35Z

This PR has 44 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Small
Size       : +41 -3
Percentile : 17.6%

Total files changed: 6

Change summary by file extension:
.cs : +16 -3
.resx : +3 -0
.ps1 : +22 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

ghost · 2023-04-20T17:40:01Z

🎉v7.4.0-preview.3 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

CarloToso added 2 commits January 23, 2023 01:49

Outfile without filename

78b7742

remove useless

27a7d34

CarloToso requested a review from PaulHigin as a code owner January 23, 2023 01:02

pull-request-quantifier-deprecated bot added the Extra Small label Jan 23, 2023

ghost assigned TravisEz13 Jan 23, 2023

revert

64b8011

iSazonov reviewed Jan 23, 2023

View reviewed changes

CarloToso added 3 commits January 23, 2023 14:28

remove get filename from content-disposition

e3ca008

add resume error

6758fb6

removed extra )

f74c92c

CarloToso closed this Jan 25, 2023

CarloToso reopened this Jan 25, 2023

iSazonov reviewed Jan 25, 2023

View reviewed changes

...erShell.Commands.Utility/commands/utility/WebCmdlet/Common/InvokeRestMethodCommand.Common.cs Outdated Show resolved Hide resolved

OutFile -> QualifiedOutFile

0a6684e

iSazonov reviewed Jan 25, 2023

View reviewed changes

...erShell.Commands.Utility/commands/utility/WebCmdlet/Common/InvokeRestMethodCommand.Common.cs Outdated Show resolved Hide resolved

Follow Suggestion

ce97583

iSazonov reviewed Jan 25, 2023

View reviewed changes

remove comments, use QualifiedOutFile

78dca27

iSazonov reviewed Jan 25, 2023

View reviewed changes

CarloToso added 4 commits January 25, 2023 21:35

cache QualifiedOutFile

2bee51b

cache stuff; ShouldSaveToOutFile; fix

6c319ff

remove forgotten line

eda8be1

try removing _shouldSaveToOutFile

a58a77e

iSazonov reviewed Jan 27, 2023

View reviewed changes

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Outdated Show resolved Hide resolved

CarloToso added 2 commits January 27, 2023 11:43

remove commented out strings

38aa845

small adjustments

f00ecb6

CarloToso marked this pull request as draft January 29, 2023 17:04

pull-request-quantifier-deprecated bot added Extra Small and removed Extra Large labels Feb 10, 2023

ghost added the Review - Needed The PR is being reviewed label Feb 17, 2023

Merge branch 'master' into WebCmdlets-Outfile-filename

2932d00

mklement0 mentioned this pull request Feb 26, 2023

Invoke-WebRequest should save the file name with the name in Content-Disposition #11671

Open

Add -Verbose FileName

39cfff2

CarloToso added 2 commits March 14, 2023 19:30

Add and use WebResponseHelper.GetOutFilePath

ea7f69f

Url -> Uri

01ee69a

SteveL-MSFT requested changes Mar 20, 2023

View reviewed changes

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

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Mar 20, 2023

Update test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlet…

6a17610

…s.Tests.ps1 Co-authored-by: Steve Lee <slee@microsoft.com>

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Mar 20, 2023

Update test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlet…

43e82dc

…s.Tests.ps1 Co-authored-by: Steve Lee <slee@microsoft.com>

SteveL-MSFT approved these changes Mar 20, 2023

View reviewed changes

iSazonov merged commit 5b1deda into PowerShell:master Mar 21, 2023

iSazonov unassigned TravisEz13 Mar 21, 2023

CarloToso mentioned this pull request Mar 21, 2023

Use Content-Disposition for output file name in Webcmdlets #19385

Open

22 tasks

CarloToso mentioned this pull request Jul 22, 2023

Invoke-WebRequest -OutLeaf #20010

Open

Conversation

CarloToso commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarloToso Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklement0 Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarloToso commented Jan 23, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarloToso Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarloToso Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

Uh oh!

Uh oh!

iSazonov commented Jan 28, 2023

Uh oh!

CarloToso commented Jan 28, 2023

Uh oh!

ghost commented Feb 17, 2023

Uh oh!

CarloToso commented Mar 14, 2023

Uh oh!

mklement0 commented Mar 14, 2023

Uh oh!

mklement0 commented Mar 14, 2023

Uh oh!

CarloToso commented Mar 14, 2023

Uh oh!

mklement0 commented Mar 14, 2023

Uh oh!

CarloToso commented Mar 14, 2023

Uh oh!

mklement0 commented Mar 14, 2023

Uh oh!

CarloToso commented Mar 16, 2023

Uh oh!

Uh oh!

Uh oh!

pull-request-quantifier-deprecated bot commented Mar 20, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

ghost commented Apr 20, 2023

CarloToso commented Jan 23, 2023 •

edited

Loading

CarloToso Jan 23, 2023 •

edited

Loading

mklement0 Feb 24, 2023 •

edited

Loading

CarloToso Jan 25, 2023 •

edited

Loading

CarloToso Jan 26, 2023 •

edited

Loading