Fix https://github.com/tmenier/Flurl/issues/794

[rwasef1830]

PR containing failing test demonstrating issue #794
@tmenier

Issue is due to stream.Length accessing Length property throws on GzipStream in DefaultJsonSerializer.

[rwasef1830]

Fixed by removing the length check. It's not always possible to know the length of the HTTP response stream. If the server returns an undeserializable or empty body, it's better to just throw exception. An empty string is not the same as "null" which is a valid JSON that deserializes to null. An empty string is invalid JSON.

[rwasef1830]

whoops, will fix that.

[tmenier]

Yeah, it should be fixable, just not quite that simple.

[tmenier]

I should also look into getting the build/test workflow to run automatically when a PR is opened, I don't think it was ever my intent to require an approval there

[rwasef1830]

@tmenier fixed, the assert just needed to change from checking null response to checking for FlurlParsingException (which is the correct behavior).

This could be considered a breaking change for an edge case behavor but it's a correction of a previously incorrect behavior. Not sure if it warrants version bump or not.

[tmenier]

Previously an empty steam resulted in a null object. How is that "incorrect"? To me it makes more sense intuitively than throwing an exception. Would something like this fix your scenario and not break the other test?

public T Deserialize<T>(Stream stream) =>
    stream == null ? default :
    steam.CanRead && stream.Length == 0 : default :
    JsonSerializer.Deserialize<T>(stream, _options);

Need to step away, sorry in advance if I don't get back to this today.

[rwasef1830]

The problem is you cannot touch stream.Length and stream is never null. The problem is specifically JSON. There is no such thing as a valid JSON that's composed of an empty string.

I tried using various way of getting the stream length but the issue stems from HttpBaseResponse underneathe, it throws when you touch Length in ResponseHeadersRead mode. Apparently it's not read from the HTTP response headers, and the server could omit it altogether if it wanted.

A more sophisticated way perhaps is try to read a single byte from the stream, and if we immediately get empty, then we could return empty and preserve the existing behavior, but then if we got anything at all, we'll need to wrap the stream into another stream decorator that "rewinds" back that single character.

It would work but it would entail an extra Stream wrapper allocation. If that's a good enough solution I can implement it.

[tmenier]

Thanks for your work on this. You're right that my suggestion didn't work, but it was close. I tested with CanSeek instead of CanRead and got the desired result:

public T Deserialize<T>(Stream stream) =>
    stream.CanSeek && stream.Length == 0 ? default :
    JsonSerializer.Deserialize<T>(stream, _options);

There is no such thing as a valid JSON that's composed of an empty string.

You might be right about that, but then I suspect Newtonsoft had it wrong all along. I looked back to why I did it this way initially and it was specifically to fix that failing test on switching from Newtonsoft to STJ. I wanted to minimize the surprises people encountered when upgrading to 4.0. The recommended way to deserialize a stream in Newtonsoft will return null on an empty stream vs failing. Or at least that's my assumption, given that my test did not fail when Newtonsoft was the default.

I know the code above isn't perfect, but I think it solves your issue without breaking any existing test. You might have been able to convince me to skip the zero-length check in the 4.0 release, but doing it now could break someone in a minor release.

[rwasef1830]

@tmenier Thank you for an awesome library and for responding to my PR. I had considered using CanSeek but I was worried that it might throw but hadn't actually tested it. I agree with your considerations, we should keep surprises to a minimum in minor versions updates.

I will amend my PR shortly to the desired behavior.

[rwasef1830]

@tmenier Here you go all squashed and cleaned up.

[tmenier]

I'll get it released soon. Thanks again!