Refactor AutoInflate/Deflate into generic features

[paul]

This makes HTTP::Client, Request and Response unaware of the implementation details behind inflating and deflating stream bodies. Instead, the client iterates over all enabled features on request and response, calling #wrap_request or #wrap_response, which is expected to return the Request or Response as-is if nothing should be done, or returns a new Request or Response object after having done whatever transformation the feature requires.

Related to #478 (and all the issues linked therein), this also provides a clean way to implement more features, like logging/instrumentation, caching, etc... In fact, several existing http.rb features could be probably be refactored into this style and clean up the codebase (proxy, json parsing, cookies, redirects...)

This pattern also avoids callbacks and chained-middleware (but it is a little similar), two things I know @tarcieri wasn't a fan of. Using #inject to iterate over the enabled features means that the default behavior of just returning the original object is a no-op, and doesn't allocate any additional objects.

 >> o = Object.new
=> #<Object:0x000055e0c6e69888>
>> [1, 2, 3, 4].inject(o) { |obj, i| obj }
=> #<Object:0x000055e0c6e69888>

I'd also like to nuke the "available features" whitelist in place of some sort of registry, or some other flexible way to add additional features to the list, but I wanted to get feedback on this approach first.

[tarcieri]

This pattern also avoids callbacks and chained-middleware (but it is a little similar), two things I know @tarcieri wasn't a fan of. Using #inject to iterate over the enabled features means that the default behavior or just returning the original object is a no-op, and doesn't allocate any additional objects.

🎉 yes, thank you!

[janko]

LGTM, thank you! 👍

[tarcieri]

Nice refactoring

[janko]

Actually, what is the reason for this method being added?

[paul]

I was trying to get the CompressedBody to quack like a Request::Body so that this branch wouldn't fail: https://github.com/httprb/http/blob/master/lib/http/request/body.rb#L20

There's probably a better approach, but I wasn't really sure how this code in Request::Body was for, so I wanted my change to remain low-impact.

[janko]

I see. But isn't CompressedBody wrapping Request::Body, not the other way around?

If I'm wrong, then in Request::Body we can modify the branch where it checks if body is Enumerable to just checking if it responds to #each instead (we don't actually need the object to be Enumerable, we're just using #each anyway).

[paul]

That was the first thing I tried, actually, but several other tests failed, and when I started digging in I decided that would be a larger change. Also, since Request::Body#size gets used to set the Content-Length header, I was worried that if it was passed an array-like enumerable like ["foo", "bar"], then size would be 2 instead of 6. I don't know if its ever possible for Body to ever have an array source, but it seemed easier to make AutoDeflate quack like a normal body instead.

[tarcieri]

The arrangement in this PR makes sense to me except for Request::Body being the actual compressed body (i.e. on the wire) and CompressedBody actually being the decompressed body, though that feels like a silly bikeshed/nit

[janko]

Ok, I see now what's the problem. The Request::Body is being wrapped by DeflatedBody and passed as :body to Request.new, which then wraps Request::Body around it again. This causes an issue when trying to determine size, because the outer Request::Body isn't able to determine the size, even though it's CompressedBody source responds to #size. We should somehow try to avoid it, that Request#body ends up being CompressedBody.

I'm reluctant to have this PR merged as is because it loses on-the-fly compression, as CompressedBody#each is never called since Request::Body will pick the new CompressedBody#read.

[paul]

@janko-m Ok, I tweaked it a bit so that CompressedBody inherits from Request::Body, and the Request doesn't try to wrap the body in a body if its already a body.

[janko]

Do you think it would make more sense for the Inflater to wrap the Response::Body object instead of Connection? That would be symmetrical to CompressedBody wrapping the Request::Body object. And it makes more sense to me that Response::Body object reads the response body as is, and then the Inflater wrapper changes it.

[paul]

Probably. In fact, I think the entire Inflater methods could be moved into this class, similar to how AutoDeflate does it. But, again, I wanted this PR to remain low-impact.

Personally, I'm more interested in being able to add more "features" like these as plugins, like logging/instrumentation or caching. I was reading through the PR history and debating how to monkeypatch it for my own use, when I noticed that with a few minor changes to how the AutoInflate/Deflate features were implemented, then #use could become much more powerful, and enable hooks that I need without moneypatching.

I'd like for this to get merged, and then perhaps used as a jumping-off point for further refactorings and more features. There's also some ergonomic low-hanging-fruit here too (like implementing a Request#merge(opts) method, rather than each feature implementation having to reconstruct the hash by hand. For example, in this file L8-15 could be replaced with response.merge(body: stream_for(response.connection)).

[janko]

Yeah, that makes total sense. I do like the gradual approach to refactoring, with keeping the same functionality 👍

[paul]

@janko-m Ok, I tweaked it a bit so that CompressedBody inherits from Request::Body, and the Request doesn't try to wrap the body in a body if its already a body. Is that more what you had in mind? @tarcieri should probably resolve your valid bikeshed concern, too

[tarcieri]

Looks good to me

[ixti]

Thank you!

[janko]

@paul Thank you!

[tarcieri]

Indeed, thanks @paul! And I hope this is just the start of refactoring more features this way 😄