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

[Breaking Change Request] HttpClientResponse.autoUncompress #36971

Open
tvolkert opened this issue May 14, 2019 · 15 comments

Comments

Projects
None yet
6 participants
@tvolkert
Copy link
Contributor

commented May 14, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Looping in @Hixie @dgrove @matanlurey for approval.

@tvolkert

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

"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 tvolkert changed the title [Breaking Change Request] HttpClientResponse.isUncompressed [Breaking Change Request] HttpClientResponse.wasAutoUncompressed May 16, 2019

@tvolkert

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

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.

@tvolkert tvolkert changed the title [Breaking Change Request] HttpClientResponse.wasAutoUncompressed [Breaking Change Request] HttpClientResponse.autoUncompress May 16, 2019

@Hixie

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Same semantics except for being readonly I assume?

@tvolkert

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Same semantics except for being readonly I assume?

Yep

tvolkert added a commit to tvolkert/flutter that referenced this issue May 16, 2019

@tvolkert

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

  • 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 flutter/flutter#32834
  • Proposed API change is in sdk/102800

tvolkert added a commit to flutter/flutter that referenced this issue May 17, 2019

@matanlurey

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

LGTM from moi!

@Hixie

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

LGTM

kevmoo added a commit to flutter/flutter_web that referenced this issue May 17, 2019

@dgrove

This comment has been minimized.

Copy link
Member

commented May 18, 2019

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

This comment has been minimized.

Copy link
Member

commented May 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

@matanlurey @dgrove thoughts?

@lrhn

This comment has been minimized.

Copy link
Member

commented May 21, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.