Skip to content

Add OutFile property in WebResponseObject #24047

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.

Sign up for GitHub

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

Jump to bottom
Merged
daxian-dbw merged 5 commits into from
Jul 23, 2024
Merged

Add OutFile property in WebResponseObject #24047

daxian-dbw merged 5 commits into from
Jul 23, 2024

Conversation

@jshigetomi
Copy link
Collaborator

@jshigetomi jshigetomi commented Jul 12, 2024 •
edited
Loading

PR Summary

Fixes: #21082

Added OutFile as a property in WebResponseObject. OutFile will save whenever writing to pipe line.
Refactored one line to reduce redundancy.

PR Context

PR extends WebObjectResponse with a property that stores the installed file's path. It makes sense to have this as a built in property since we have access to it.

PR Checklist

@jshigetomi jshigetomi changed the title Issue 21082 fix Fix Invoke-WeRequest -PassThru -OutFile WebRequestObject Property .OutFile Stores Out File Path Jul 12, 2024
iSazonov
iSazonov reviewed Jul 13, 2024
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests for the scenario in WebCmdlets.Tests.ps1.

@iSazonov
Added OutFile as a property of WebResponseObject. Output file path wi…
40781e0 
…ll now be saved to the WebResponseObject when -OutFile flag is used. Added Tests for directory, file, and verbose.

Co-authored-by: Ilya <darpa@yandex.ru>
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 16, 2024

@mklement0 Please review.

@mklement0 mklement0 mentioned this pull request Jul 16, 2024
Merged
21 tasks
SteveL-MSFT
SteveL-MSFT requested changes Jul 16, 2024
View reviewed changes
...ft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebResponseObject.Common.cs Outdated
/// <summary>
/// Gets or sets the output file path.
/// </summary>
public string? OutFile { get; set; }
Copy link
Member

@SteveL-MSFT SteveL-MSFT Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be an internal set?

Copy link
Contributor

@mklement0 mklement0 Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree (public string? OutFile { get; internal set; })

@SteveL-MSFT, @iSazonov, just to confirm: Is it generally fine to extend public classes with new members?

Other than that, LGTM - thanks for taking this on, @jshigetomi.

Copy link
Collaborator

@iSazonov iSazonov Jul 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mklement0 Removing public element is a breaking change, adding is ok.

Copy link
Collaborator

@iSazonov iSazonov Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jshigetomi Please address the request.

Copy link
Collaborator Author

@jshigetomi jshigetomi Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iSazonov Thank you for bringing this to my attention. I misread this chain and thought there were no actionable items.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 16, 2024
@iSazonov iSazonov changed the title Fix Invoke-WeRequest -PassThru -OutFile WebRequestObject Property .OutFile Stores Out File Path Add OutFile property in WebResponseObject Jul 17, 2024
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 17, 2024

@jshigetomi Please resolve merge conflicts.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 17, 2024
daxian-dbw
daxian-dbw requested changes Jul 18, 2024
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the Invoke-ResetMethod -OutFile -Passthru needs to be updated accordingly to have the similar behavior, doesn't it?

PowerShell/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/InvokeRestMethodCommand.Common.cs

Line 143 in 8016daa

string outFilePath = WebResponseHelper.GetOutFilePath(response, _qualifiedOutFile);

@microsoft-github-policy-service microsoft-github-policy-service bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 18, 2024
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 19, 2024

I guess the Invoke-ResetMethod -OutFile -Passthru needs to be updated accordingly to have the similar behavior, doesn't it?

I didn't find WebResponseObject there. It is seem used only in Invoke-WebRequest.

But I found WebResponseObject and BasicHtmlWebResponseObject in PowerShellCore_format_ps1xml.cs. I believe it would be useful to add new Outfile property in the formatting.

@daxian-dbw
Copy link
Member

daxian-dbw commented Jul 22, 2024 •
edited
Loading

I guess the Invoke-ResetMethod -OutFile -Passthru needs to be updated accordingly to have the similar behavior, doesn't it?

The Invoke-RestMethod does support -OutFile and -Passthru to be specified together, so the file information should be kept in the returned object just like Invoke-WebRequest, isn't it?

@mklement0
Copy link
Contributor

mklement0 commented Jul 23, 2024 •
edited
Loading

@daxian-dbw: Invoke-RestMethod -PassThru -OutFile is currently broken:

If it did / once it does work as intended, what would be passed through is data (possibly parsed into a [pscustomobject] graph / an[xml] DOM), not a WebResponseObject instance.

@daxian-dbw
Update src/Microsoft.PowerShell.Commands.Utility/commands/utility/Web…
b7c05f6 
…Cmdlet/CoreCLR/InvokeWebRequestCommand.CoreClr.cs
@daxian-dbw
Copy link
Member

daxian-dbw commented Jul 23, 2024

@mklement0 Thanks for pointing to that issue. I didn't know it's currently broken. I re-opened it.

@daxian-dbw daxian-dbw enabled auto-merge (squash) July 23, 2024 20:30
@daxian-dbw daxian-dbw merged commit 9995125 into PowerShell:master Jul 23, 2024
@microsoft-github-policy-service
Copy link
Contributor

microsoft-github-policy-service bot commented Jul 23, 2024 •
edited by unfurl-links bot
Loading

📣 Hey @jshigetomi, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

@iSazonov iSazonov added CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Jul 24, 2024
@jshigetomi jshigetomi deleted the issue-21082-fix branch July 24, 2024 15:36
@sdwheeler sdwheeler mentioned this pull request Aug 5, 2024
Closed
2 tasks
chrisdent-de pushed a commit to chrisdent-de/PowerShell that referenced this pull request Sep 12, 2024
@jshigetomi @chrisdent-de
Add OutFile property in WebResponseObject (PowerShell#24047)
9bc6226
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

+1 more reviewer

@mklement0 mklement0 mklement0 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Invoke-WebRequest -PassThru, when combined with -OutFile, should reflect the download file path.

5 participants

@jshigetomi @iSazonov @daxian-dbw @mklement0 @SteveL-MSFT