Add request decompression to server GZip middleware

[matkob]

Fixes #7072.
Relates to #4981 but doesn't cover client side middleware.

[isomarcte]

I don't love running this effect like this, though I see that it is status quo.

Perhaps in 1.x we should move the logging out to a separate wrapper method. Something like,

object Gzip extends GzipPlatform {

  def withLogging[F[_]: Functor, G[_]: Compression: Sync](...):` Http[F, G]

[armanbilge]

Instead of asking for Sync we should ask for a LoggerFactory. There are old mistakes but let's not repeat them in new code 😅

[armanbilge]

See:

• Expunge all unsafe logging #6645

[isomarcte]

Ah I see. Makes sense.

[matkob]

This seems like out of scope of this PR, doesn't it?

[armanbilge]

Changing existing code is out-of-scope. Avoiding writing new code with anti-pattterns (established as they may be) is in-scope :)

[matkob]

Got it! I'll change it a little

[matkob]

This change seems to have some repercussions. Biggest one being breaking compatibility but also when working on it I had an issue with GZipPlatform I couldn't resolve - I couldn't provide enough evidence to create GZip middleware.
How about removing those trace logs?

[armanbilge]

I couldn't provide enough evidence to create GZip middleware.

What's the error?

[matkob]

If I change the signature of Gzip::apply to something like def apply[F[_]: LoggerFactory: Monad, G[_]: Compression](...) then the arguments of GZipPlatform::apply must change as well. Currently it's

def apply[F[_], G[_]](
      http: Http[F, G],
      bufferSize: Int,
      level: DeflateParams.Level,
      isZippable: Response[G] => Boolean,
      F: Functor[F],
      G: Sync[G],
  ): Http[F, G]

But F: Functor[F] is not enough. I tried various approaches but can't find anything that would work.

[isomarcte]

@matkob you are going to need to add an overload to not break bincompat, e.g.

 def apply[F[_]: Functor, G[_]: Compression](
      http: Http[F, G],
      bufferSize: Int = 32 * 1024,
      level: DeflateParams.Level = DeflateParams.Level.DEFAULT,
      isZippable: Response[G] => Boolean = defaultIsZippable[G](_: Response[G]),
      isZipped: Request[G] => Boolean = defaultIsZipped[G](_: Request[G]),
  ): Http[F, G] = apply(http, bufferSize, level, isZippable, defaultIsZipped[G])

 def apply[F[_]: Functor, G[_]: Compression](
      http: Http[F, G],
      bufferSize: Int = 32 * 1024,
      level: DeflateParams.Level = DeflateParams.Level.DEFAULT,
      isZippable: Response[G] => Boolean = defaultIsZippable[G](_: Response[G]),
      isZipped: Request[G] => Boolean = defaultIsZipped[G](_: Request[G]),
  ): Http[F, G] = ...

[armanbilge]

So actually, I am not sure if it makes sense to put "Gzip" and "Gunzip" together. Shouldn't they be exposed as different middlewares?

I know there's the issue on the client side, but this PR is about the server.

[isomarcte]

@armanbilge Yeah, I think that makes sense.

[matkob]

Coming from the user perspective here. I assumed that adding GZip middleware would cover everything related to gzip in my http4s server, so both incoming requests and outgoing responses. I was really surprised it did not.

In #4981 I saw a discussion about that but I'm not convinced there should be separate middlewares for decompressing and compressing, especially because this requires some further renaming and deprecations.

[armanbilge]

That's a fair point. So for logging we have RequestLogger and ResponseLogger and then Logger which does both. Maybe we need something similar here?

[matkob]

you are going to need to add an overload to not break bincompat

I wanted to add that overload but it didn't play well with its optional arguments - compiler was complaining about the ambiguity of the signatures. Also, after I removed this additional isZipped argument, bincompat shouldn't be an issue.

[matkob]

So for logging we have RequestLogger and ResponseLogger and then Logger which does both. Maybe we need something similar here?

Time will tell, I guess. And if there's need, anyone can contribute such enhanced granularity.

[wjoel]

I would strongly suggest adding a "max uncompressed body size" parameter to this middleware to protect it from gzip bombs.

Adding it as a separate request-only middleware would make the trace thing easy to fix as bincompat wouldn't be an issue.

[wjoel]

I think the .handleErrorWith in #5546 (comment) is a good idea to throw MalformedMessageBodyFailure for streams that aren't valid gzip streams, but maybe it could be more specific (and I should have added a test for it).