Deprecate Client#fetch

[rossabaker]

@ChristopherDavenport has been arguing for this, and I like @SystemFw's argument about not burying use.

Steps:

1. Add @deprecated annotation to both overloads of Client.fetch.
2. Replace client.fetch(req)(f) with client.run(req).use(f) in tests and docs.

Bonus step 1.5, but more difficult: Implement a scalafix rule. Would make step 2 easier, and will help a lot of people upgrading.

[SystemFw]

@rossabaker the link to my comment points to Chris twitter as well

[Daenyth]

Semi-related to #2738 because the F[Request[F]] signature is awkward to use with run

[kevinmeredith]

Am I correct in understanding your opposition to Client#fetch is that it essentially discards the Resource[F, Response[F]] type, @rossabaker @ChristopherDavenport , @SystemFw?

In other words, it's better to deal with Resource[F, Response[F]] than F[Response[F]] given the, as I understand, guaranteed clean-up?

[rossabaker]

Yes. fetch(req)(f) is just an alias for run(req).use(f), but the transition from Resource[F, A] to F[A] allows us to make silly mistakes that the types can't save us from. We learn -- usually the hard way -- not to .use(F.pure), but hiding it behind fetch obscures that mistake. run(req).use(f) isn't significantly longer for the novice, and much more meaningful for those experienced in Resource.

[kevinmeredith]

Is another benefit, @rossabaker, in deprecating Client#fetch, that Resource doesn't "suffer" from the following problem with respect to disposal of the response body?

Imports

@ import $ivy.`org.http4s::http4s-circe:0.20.19` 
import $ivy.$                                 

@ import $ivy.`org.http4s::http4s-core:0.20.19` 
import $ivy.$  

@ import $ivy.`org.http4s::http4s-client:0.20.19` 
import $ivy.$  

@ import org.http4s._

@ import org.http4s.client._

@ import org.http4s.circe._

@ import io.circe.Json

IO

@ for {
    resp <- client.fetch(Request[IO]()) { resp: Response[IO] => IO { resp } }
    json1 <- resp.as[Json]
    json2 <- resp.as[Json]
  } yield Json.fromValues(List(json1, json2)) 
res26: IO[Json] = Bind(
  Bind(
    Map(Delay(cats.effect.concurrent.Ref$$$Lambda$3047/864102782@30bee87a), org.http4s.client.Client$$$Lambda$3048/2058221393@39cd953c, 0),
    cats.effect.Resource$$Lambda$3094/1466001486@300ce687
  ),
  ammonite.$sess.cmd26$$$Lambda$3268/181218042@5f342596
)

@ res26.unsafeRunSync 
java.io.IOException: response was disposed
...

Resource

@ val client: Client[IO] = Client.fromHttpApp[IO] { HttpApp { case _ => Response[IO](status = Status.Ok).withBody(Json.obj( ("hi", Json.fromString("world")) ) ) } }  
client: Client[IO] = org.http4s.client.Client$$anon$1@3627bfdd

@ for {
    resp <- client.run(Request[IO]())
    json1 <- Resource.liftF(resp.as[Json])
    json2 <- Resource.liftF(resp.as[Json])
  } yield Json.fromValues(List(json1, json2)) 
res20: Resource[IO, Json] = Bind(
  Suspend(Map(Delay(cats.effect.concurrent.Ref$$$Lambda$3047/864102782@6424c975), org.http4s.client.Client$$$Lambda$3048/2058221393@76604521, 0)),
  ammonite.$sess.cmd20$$$Lambda$3246/181734605@2e045d26
)

@ res20.use { json => IO(println(json.spaces2)) }.unsafeRunSync 
[
  {
    "hi" : "world"
  },
  {
    "hi" : "world"
  }
]

[satorg]

@rossabaker, btw, some time ago I asked about those Client methods which take request in form of F[Request[F]], namely:

def fetch[A](req: F[Request[F]])(f: Response[F] => F[A]): F[A]
def expectOr[A](req: F[Request[F]])(onError: Response[F] => F[Throwable])(implicit d: EntityDecoder[F, A]): F[A]
def expect[A](req: F[Request[F]])(implicit d: EntityDecoder[F, A]): F[A]
def fetchAs[A](req: F[Request[F]])(implicit d: EntityDecoder[F, A]): F[A]
def status(req: F[Request[F]]): F[Status]
def successful(req: F[Request[F]]): F[Boolean]

And you agreed that they aren't really useful anymore. Do you think it could be right time to deprecate them too as part of this effort?

[rossabaker]

@kevinmeredith The problem with your IO version is leaking the response resource outside fetch. It's equivalent to the .fetch(F.pure) or .fetch(F.delay) anti-patterns. But the Resource version is also unsafe: you're reading the response body twice. It works when the body is created in memory, but it would fail when the response body is backed by a network socket.

@satorg I'd like to remove them, but it's been pointed out that the client DSL still returns requests in F[Request[F]]. There was a conversation about evolving away from that as well. I think maybe those two should go together in their own PR.

[kevinmeredith]

Thanks for that explanation, @rossabaker

It's equivalent to the .fetch(F.pure) or .fetch(F.delay) anti-patterns.

If I understand properly, the anti-pattern of client.fetch(request)(F.delay) or client.fetch(request)(F.pure) is that the return type is F[Response[F]]. That's a problem since, by the time either expression responds with the F[Response[F]], the body has already been disposed of? Consequently, attempts to read it will fail?

[rossabaker]

Exactly that. Generically, F.pure and F.delay are generally bad functions to .use, because they expose the resource after the resource has been released. And we add to the cognitive burden with .fetch, which people need to remember is dangerous in the same ways as .use.

[satorg]

Hey guys, would you mind if I take on this issue? If no one did it already of course...

[rossabaker]

@satorg It's yours!

[satorg]

@rossabaker what's the version this fix will go to? I need some value to put into @deprecated. I'd guess since it doesn't introduce any incompatibility, it could be included even into one of the next 0.21.x, but just in case...

[rossabaker]

Great question.

• It could technically go on series/0.20 and 0.20.22, but that branch is almost EOL anyway.
• series/0.21 and 0.21.4 is the next natural place for it, but people expect those patch upgrades to be easy, and we might want a scalafix to go with it.
• master and 1.0.0-M1 is the easy way out.

I think series/0.21 and 0.21.4 is good, and we should see if we can get a volunteer to dust off the scalafix machinery.

[satorg]

So far so good. Meanwhile there're a bunch of other suspicious (for the same reason as fetch) methods in Client which we may consider to deprecate as well. I'd target these:

1. def toKleisli[A](f: Response[F] => F[A]): Kleisli[F, Request[F], A]
   The simplest one: can be replaced with Kleisli(run(_).use(f))
2. def get[A](uri: Uri)(f: Response[F] => F[A]): F[A]
   Not sure about it. Looks like there's no simple way to replace it.
   If there was a method like def get(uri: Uri): Resource[F, Response[F]] we could naturally use it as a replacement.
   But if add such method it can create ambiguity with the former one due to the parameter groups there.
   We can consider adding such method with a slightly different name though.
3. def get[A](s: String)(f: Response[F] => F[A]): F[A]
   The same as above.

Also there're a few methods that take F[Request[F]] as a parameter which can be easily replaced by flatmapping followed by the corresponding method with Request[F] (looks like related to #2738):

4. def expectOr[A](req: F[Request[F]])...
5. def expect[A](req: F[Request[F]])...
6. def fetchAs[A](req: F[Request[F]])
7. def status(req: F[Request[F]])
8. def successful(req: F[Request[F]])

@rossabaker, @ChristopherDavenport, @SystemFw, @kevinmeredith any thoughts?

[kevinmeredith]

Also there're a few methods that take F[Request[F]] as a parameter which can be easily replaced by flatmapping

I agree with your idea, @satorg. The benefit of deprecating 4-8 would be to reduce the surface area of the Client API, I argue.

[rossabaker]

Sorry, I overlooked @satorg's followup. I agree with @kevinmeredith. These were in questionable taste, but convenient, when encoding was effectful. Now that it's not, they are much less compelling.

Note that the client DSL still returns F[Request[F]], so users of that will feel some pain until we adjust that. They might go hand-in-hand.

[satorg]

@rossabaker yes, I agree that the client DSL should be changed accordingly.

[rossabaker]

fetch is done, but leaving open to discuss @satorg's proposal to go further.

[satorg]

@rossabaker would it make sense to create a separate issue for deprecating methods that take F[Request[...]] as a parameter, wdyt?

[rossabaker]

Yes, as long as we link it to this one to continue the context, that would be better.

[danicheg]

As far as I can see, this issue might be closed since the proposed work is done.

[danicheg]

If I'm not wrong, the proposed issue by @satorg wasn't created. So here we go in #6650.