[Breaking Change Request] HttpClientResponse.autoUncompress

[tvolkert]

Intended change

This proposal is to add a bool get autoUncompress getter to HttpClientResponse, which would have the same semantics as HttpClient.autoUncompress.

Rationale

The use case is when an API method takes an HttpClientResponse parameter and needs to know:

1. Whether the response byte stream contains compressed or uncompressed bytes.
2. Whether they can trust the value of the Content-Length response header.

Expected impact

On callers

This is new API, so callers will be unaffected.

On implementations of the HttpClientResponse interface

Libraries that are implementing the HttpClientResponse interface will be broken because they will no longer be implementing the interface. A quick search yielded the following results:

Only tests will be affected within these packages, as they are mocking HttpClientResponse:

• chrome
• flutter
• flutter_test
• flutter_tools
• flutter_svg
• usage

There's only 1 case I found in a search where API would be broken, and it's an internal Google client, so we'll fix that implementation when this change rolls into Google.

Steps for mitigation

The plan is to release a new non-breaking version bump of package:http_server that adds this getter to HttpClientResponse (without annotating as @override) so that when the breaking change lands, http_server is already compliant.

[tvolkert]

Alternatives considered

An alternative that was considered was just exposing an HttpClient get client getter in HttpClientResponse.

Benefits

• This is an easier property to comprehend for the developer, and they can still get at the info they need by calling response.client.autoUncompress.
• It opens up potentially broader use cases by allowing access back to more properties of the client than just the autoUncompress property.
• Discussing with other engineers, it seems logical that a response should point back to the client that created it.

Drawbacks

• It opens up a broader API surface, and exercising tighter control over the API surface is often warranted in an SDK.

Conclusion

I actually think exposing client would be cleaner, but I tried to limit the scope of this proposal. I'm very open to opinions that exposing client would be preferable.

[aadilmaan]

Looping in @Hixie @dgrove @matanlurey for approval.

[tvolkert]

Thanks @aadilmaan

Note to reviewers: I'm particularly interested in your opinions on the proposal vis-a-vis the alternative described in #36971 (comment)

[Hixie]

"isUncompressed" doesn't describe its semantics very actually. "wasUncompressed" or indeed "wasCompressed" or "wasAutoUncompressed" all seem more accurate.

Exposing the client seems like it would essentially be giving code the capability to do more network calls; seems like if the caller wants to do that they should give the client directly themselves.

I don't have strong opinions on any of this. Happy for this to go whatever direction you want.

[tvolkert]

Actually, thinking about it more, I think just exposing the autoUncompress flag from the client directly, with the same semantics, is the best option.

It keeps the semantics very clear, it unblocks the target use cases, and it alleviates the HttpClientResponse impl's getter from having to consult the headers to see if the response bytes were compressed.

I'm going to update this issue summary to match.

[Hixie]

Same semantics except for being readonly I assume?

[tvolkert]

Same semantics except for being readonly I assume?

Yep

[tvolkert]

• Implementation change has already landed in sdk/102780
• Internal Google change to make the roll easier is in cl/248588898
• The changes to Flutter are in Prepare for API addition to HttpClientResponse flutter/flutter#32834
• Proposed API change is in sdk/102800

[matanlurey]

LGTM from moi!

[Hixie]

LGTM

[dgrove]

I'd like to see @lrhn 's view on this. I have no objections, but want to hear from him before providing an LGTM.

[lrhn]

The autoUncompress is a configuration option. It applies to a number of future responses, not a particular response.
For one single response, the setting of this configuration is probably too unspecific.
As Ian points out, wasCompressed would be more interesting. Whether the client configuration autoUncompress is true seems irrelevant if the actual response body was not compressed.

I see three states the response and body can be in:

• Not compressed.
• Compressed, but automatically decompressed.
• Compressed and not decompressed.

The user may need to distinguish all three.
In the first and third cases, you can trust the contentLength header. In the third case, you need to manually decompress.

As uses cases, I'd expect:

• client checking whether response body is still compressed. A proxy may need to propagate that information without decompressing the bod.
• client want to check whether contentLength is correct.

Maybe:

• wasCompressed and isCompressed (that's probably too confusing), or
• compressionState -> enum uncompressed/decompressed/compressed (not very usable), or
• wasResponseCompressed, isResponseDecompressed
• bodyLengthModified, isCompressed

Any better ideas?

[tvolkert]

Thanks Lasse - I agree with that analysis and the possible solutions. I actually think the compressionState enum, however, is the most comprehensible, and thus the most usable, of the options.

With the existing proposal, the caller can get the necessary information by manually checking the Content-Encoding header and the value of the autoUncompress passthrough config option. But forcing the caller to check the value of the header breaks encapsulation in that the caller is now tied to the implementation of the response (what if the sdk later adds support for non-gzip encodings?).

TLDR: 👍 to compressionState from me. Proposed enum:

enum HttpClientResponseCompressionState {
  /// The body of the HTTP response was received and remains in an uncompressed
  /// state.
  ///
  /// In this state, the value of the `Content-Length` HTTP header, if
  /// specified (non-negative), should match the number of bytes produced by
  /// the response's byte stream.
  uncompressed,

  /// The body of the HTTP response was originally compressed, but by virtue of
  /// the [HttpClient.autoUncompress] configuration option, it has been
  /// automatically uncompressed.
  ///
  /// HTTP headers are not modified, so when a response has been uncompressed
  /// in this way, the value of the `Content-Length` HTTP header cannot be
  /// trusted, as it will contain the compressed content length, whereas the
  /// stream of bytes produced by the response will contain uncompressed bytes.
  decompressed,

  /// The body of the HTTP response contains compressed bytes.
  ///
  /// In this state, the value of the `Content-Length` HTTP header, if
  /// specified (non-negative), should match the number of bytes produced by
  /// the response's byte stream.
  ///
  /// If the caller wishes to manually uncompress the body of the response,
  /// it should consult the value of the `Content-Encoding` HTTP header to see
  /// what type of compression has been applied. See
  /// https://tools.ietf.org/html/rfc2616#section-14.11 for more information.
  compressed,
}

[tvolkert]

@matanlurey @dgrove thoughts?

[lrhn]

My biggest concern is that uncompressed and decompressed are too close in meaning. They are basically synnonyms in common use (even if I personally think that's using one of them incorrectly, I probably can't prevent it).

Maybe use notCompressed instead of uncompressed.

I also worry slightly about usability, because enums are not particularly nice to use. if it's not a very common feature (probably not since it hasn't existed before now), then that's probably not an issue. Low-level code gets to have more precision at the cost of less convenience, and this is a low-level feature.

[tvolkert]

Low-level code gets to have more precision at the cost of less convenience, and this is a low-level feature.

I agree.

Maybe use notCompressed instead of uncompressed.

This SGTM.

I'll send out the change for review.

[tvolkert]

Resolving this, as this has been implemented in aa2ce7c