When specifying GuzzleHttp\Psr7\Stream as ReturnType, resultant stream contents is empty

[tdondich]

When attempting to download a DriveItem by utilizing a Stream object as the Return Type, the resultant Stream contents is empty.

This is due to the fact that the GraphRequest execute method first wraps the response into a new GraphResponse. The constructor for GraphResponse attempts to decode the contents of the response into a json object by using a json_decode function call. This function call takes the Stream as the first parameter, thus calling the Stream's __toString method which will read the entire contents of the stream and return as a string, therefore consuming the entire string.

After the constructor for the GraphResponse is called, then GraphRequest checks to see if $this->returnType is set, if so, then calls the GraphResponse's getResponseAsObject which indeed returns the Stream object, but at this point the Stream object is exhausted.

Proposed solution:
Modify the execute (and executeAsync) method to check if $this->returnType is "GuzzleHttp\Psr7\Stream". If so, do not wrap the response into a GraphResponse but instead immediately return the Stream object from thje Guzzle result. This can be similarly done in the async version.

This also will allow removing the code from GetResponse inside getResponseAsObject to check for returnType being a Stream and then returning $this->_body, since it's already exhausted and who cares.

Pull request to come.

[tdondich]

See PR: #131

[MIchaelMainer]

@tdondich Thank you for bringing this up.

If I understand correctly, getResponseAsObject is inefficient/ineffective as it converts a stream into a string, which if the file is sufficiently large, it will exhaust memory making the stream inaccessible.

Ideally, we we support reading directly from the stream into the GraphResponse object. Perhaps a GraphStreamResponse object is created when $this->returnType is "GuzzleHttp\Psr7\Stream". It writes the stream into streamBody.

[tdondich]

@MIchaelMainer I think that idea might be a bit overkill and the current setReturnType already has this support of specifying a stream as the return type. Ideally the resolution keeps the current style.

Please mind my PR. I'm resolving issues that your TravisCI env picks up. It should settle down momentarily. I don't have a solid local test environment to test the library independently. We're using this in https://github.com/ProcessMaker/flysystem-msgraph

[MIchaelMainer]

@tdondich Returning just a stream would break existing workflows where the stream was sufficiently small enough to be returned as a string, correct?

Without returning a *Response object, we effectively would be swallowing response headers making them inaccessible. This has caused customers and us headaches in other clients.

[tdondich]

@MIchaelMainer Nope, this fix doesn't break any existing functionality (passes test suites). Remember, this resolution still returns the GraphResponse in all cases except when the client explicitly states they want a GuzzleHttp PSR7 Stream. This type of style is in your existing test cases. See OneDrive functional test in latest release. It checks to see if the returnerd value is not null, but it never checks to see if the content of that returned stream is actually populated with something (it won't be. It'll be exhausted because of GraphResponse's decodeBody behavior in the constructor).

The other thing to note is, if the client specifies the PSR7 stream as the return type, the GraphResponse object is created, but thrown away anyways after the getResponseAsObject method is called.

BTW, no the getResponseAsObject does not convert the stream to a string. It returns the stream handle, but the stream points to a stream already exhausted because of the json_decode call in the constructor.

I agree about without returning a response object. In that situation, you'd have to return a *Response object, but NOT have a decodedBody. Which makes sense because if you're returning a stream, you are assuming the client is going to deal with the data. But this requires dealing with an entirely different flow and something that should be considered for a next major release instead of bugfix.

My recommendation, as biased as it would be, is to support the PR as it still supports all the current existing behavior but resolves the issue of specifying a stream as the return type and not getting any content back (and also supporting redirect responses such as the Files Download API call does).

Just checked, PR looks to pass TravisCI tests.

[tdondich]

@MIchaelMainer Any thoughts on merging this and a potential release? Until then, our package will reference our fork which is less than ideal. Let me know if there are changes you'd like done to the PR.

Thanks!

[MIchaelMainer]

Okay, I'm following you now. Thank you for the explanation. I've created a backlog item to provide a response object for stream responses. This would be breaking so we will consider this as part of a batch of breaking changes.