Skip to content
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

Invoke-RestMethod parameter combination of OutFile/PassThru does not write to file as intended #15409

Open
sdwheeler opened this issue May 14, 2021 · 15 comments
Labels
Issue-Bug Needs-Triage WG-Cmdlets-Utility

Comments

@sdwheeler
Copy link
Collaborator

@sdwheeler sdwheeler commented May 14, 2021

Problem description

In Invoke-RestMethod, the PassThru parameter must be used with the OutFile parameter. The intent is that you can write the results to the file and get the output to the pipeline. But the cmdlets only write to the pipeline, not the file.

The problem code starts here:

if (ShouldWriteToPipeline)
{
   ...
}
else if (ShouldSaveToOutFile)
{
   ...
}

Notice that the logic outputs pipeline and skips the output to the file. Both ShouldWriteToPipeline and ShouldSaveToOutFile can be true so need to remove the else.

Steps to reproduce

Invoke-RestMethod https://taxonomyservice.azurefd.net/taxonomies/product-uri -PassThru -OutFile C:\temp\irm.json

Expected behavior

Output to console and to the file.

Actual behavior

Output to console only. The output file is not created.

Environment data

This affects all versions 5.1 and higher.

@sdwheeler sdwheeler added the Needs-Triage label May 14, 2021
@iSazonov iSazonov added the Issue-Bug label May 18, 2021
@sdwheeler sdwheeler changed the title IRM/IWR parameter combination of OutFile/PassThru does not write to file as intended Invoke-RestMethod parameter combination of OutFile/PassThru does not write to file as intended May 27, 2021
@sdwheeler
Copy link
Collaborator Author

@sdwheeler sdwheeler commented May 27, 2021

After further investigation, removing the else does not fix the problem. When the else is removed, the outfile is created but empty. The problem is that the stream is consumed by writing to the pipeline. There is no way to rewind the stream so that it can be written to the file as well.

The only "fix" would be to disallow the parameter combination. That would be a breaking change.

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented May 27, 2021

Why can't we implement this if we can have Tee-Object?

@sdwheeler
Copy link
Collaborator Author

@sdwheeler sdwheeler commented May 27, 2021

@iSazonov I suppose we could, but I think that would be a large change to the architecture of the cmdlet. @markekraus may have a better answer.

@vexx32
Copy link
Collaborator

@vexx32 vexx32 commented May 27, 2021

My inclination would be to write the file and then read the data, perhaps 🤔

Or, alternatively, can we reset the stream's position after writing to the pipeline and then use it to write the file?

@TravisEz13 TravisEz13 added the Review - Maintainer label May 27, 2021
@sdwheeler
Copy link
Collaborator Author

@sdwheeler sdwheeler commented May 27, 2021

@vexx32 Swapping the order does not change the problem. The problem is that the StreamHelper class does not provide a way to rewind the stream. So the helper class needs to change, which is potentially a lot riskier.

EDIT: Oh, I just realized you are suggesting we write the file first then read the file to output to the pipeline. I still prefer having a way to rewind the stream pointer.

@vexx32
Copy link
Collaborator

@vexx32 vexx32 commented May 28, 2021

Yeah. For small files, we could cache the file in memory as a memory stream before saving/outputting, since those allow us to rewind the stream.

To account for large files, it might make sense to use a MemoryStream (or similar) as a buffer (make it say 1024 bytes or whatever) and:

  • copy X bytes from the StreamHelper into the MemoryStream
  • write all those to the pipeline from the stream
  • rewind the MemoryStream
  • write the bytes to the file
  • repeat the process until there's no more data from the StreamHelper

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented May 28, 2021

@iSazonov I suppose we could, but I think that would be a large change to the architecture of the cmdlet. @markekraus may have a better answer.

I don't see any complicity to add this - there is only two places there we output.

And we read all in buffer :-) - BufferingStreamReader - and rewind it with responseStream.Seek(0, SeekOrigin.Begin);

@vexx32
Copy link
Collaborator

@vexx32 vexx32 commented May 28, 2021

If we have a large file download -- say, 10gb -- does that still work? Or will the buffering mean that one or both modes of output will only get partial data? Can we afford to completely rewind a 10gb stream or similar and force it to be all kept in memory?

Seems to me that we should avoid having the entire stream in memory as much as possible.

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented May 28, 2021

If we have a large file download -- say, 10gb -- does that still work?

10 Gb for REST? This is not a realistic scenario as far as I'm concerned. Invoke-WebRequest is for downloading large files.

@sdwheeler
Copy link
Collaborator Author

@sdwheeler sdwheeler commented May 28, 2021

I don't think it will work to try to mix handling of the output together. When you write to the file, it writes the raw data stream. When you write to the pipeline it does a lot of conversion from XML or JSON and creates a PSCustomObject. So pipeline case needs the complete stream before it can transform the data.

The real fix to so be able to rewind the stream pointer. When I talked to @markekraus, he didn't seem to think that was possible with the current StreamHelper class.

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented May 28, 2021

When you write to the file, it writes the raw data stream.

Why would we need a raw data? I'd expect we write to file end objects converted to strings.

@sdwheeler
Copy link
Collaborator Author

@sdwheeler sdwheeler commented May 28, 2021

When we write to a file we write the raw xml or raw json. It does not get converted to objects like when it is written to the pipeline. Look at the code. You will see the difference.

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented May 30, 2021

It doesn't seem to make sense to write decoded object to pipeline and raw data to output file.

Then still the question is what kind of behavior do we want:

  1. Write decoded object to pipeline and write decoded object to output file
  2. Write decoded object to pipeline and write raw data to output file
  3. Write raw data to pipeline and write raw data to output file
  4. Write raw data to pipeline and write decoded object to output file

Since there is already buffered stream in one code path any scenario could be implemented.

@anmenaga anmenaga added WG-Cmdlets-Utility and removed Review - Maintainer labels Jun 22, 2021
@anmenaga
Copy link
Contributor

@anmenaga anmenaga commented Jun 22, 2021

Maintainer review: this is a cmdlet definition/functionality issue - reassigned to Area-Cmdlets-Utility.

@JustinWebDev
Copy link

@JustinWebDev JustinWebDev commented Mar 24, 2022

The behavior is indeed strange. If you pipe it, you get one string that is unusable. The only way to use the return object is to capture it in a variable then pipe the variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug Needs-Triage WG-Cmdlets-Utility
Projects
None yet
Development

No branches or pull requests

7 participants
@TravisEz13 @anmenaga @sdwheeler @iSazonov @vexx32 @JustinWebDev and others