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

Deprecate Service and remove MaybeResponse #1424

Merged
merged 16 commits into from
Oct 2, 2017

Conversation

aeons
Copy link
Member

@aeons aeons commented Sep 26, 2017

Following discussion on Gittter, this is an experiment to

  1. deprecate the Service[F, A, B] type alias and object of the same name in favour of the raw Kleisli[F, A, B] type
  2. remove the MaybeResponse[F] and Pass[F] types and changing the HttpService[F] type to be Kleisli[OptionT[F, ?], Request[F], Response[F]]

@@ -136,4 +77,13 @@ package object http4s { // scalastyle:ignore
case Left(l) => f(l)
}
}

implicit def http4sHttpServiceSemigroup[F[_]: Monad]: Semigroup[HttpService[F]] =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just here to make stuff compile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we aiming to provide a Semigroup or a SemigroupK?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the specific case of HttpService, I guess a Semigroup works just as well.

This still needs to be manually in scope to work, I just wanted the rest to compile while we figure out what to do.

Kleisli(request => x(request).orElse(y(request)))
}

def FToOptionT[F[_]: Functor]: FunctionK[F, OptionT[F, ?]] =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where this should live. It's come in handy a couple of times.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question for the Cats room. If we can't get a satisfactory cats answer, then we look to org.http4s.util, I think.

@aeons
Copy link
Member Author

aeons commented Sep 26, 2017

There are quite a few places where MaybeResponse.orNotFound is used. That is a pure empty 404 response. The Response object has a def notFound which takes a request and renders s"${request.pathInfo} not found".

I suggest renaming that one to something like notFoundFor(request: Request[F]) and adding a notFound that returns a 404 with something like Not found.

The empty page on hitting a 404 is not always obvious what went wrong, and having the default 404 defined somewhere ensures consistency.

@rossabaker
Copy link
Member

That sounds reasonable.

@aeons aeons changed the title WIP: Deprecate Service and remove MaybeResponse Deprecate Service and remove MaybeResponse Sep 27, 2017
@bryce-anderson
Copy link
Member

Drive-by comment and haven't looked at the details in depth, but I'm cool with the general idea.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is on the right track.

}
.map(renderResponse(_))
catch serviceErrorHandler(req).andThen(_.widen[MaybeResponse[F]])
.getOrElse(Response.notFound)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not apropos of this commit, but this needs to look more like Http1ServerStage to thread shift properly. 😟

@@ -7,9 +7,12 @@ import cats.implicits._
import fs2._
import java.time.Instant
import java.time.temporal.ChronoUnit

import cats.data.Kleisli
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These imports didn't sort correctly.

import cats.arrow.FunctionK
import cats.data.OptionT

trait FunctionKInstances {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little reluctant to provide this when it's 100% cats functionality. Have we talked to the Cats project about whether this would be a welcome function there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented this with an extension method on Kleisli instead, so instead of

kleisliOfF.transform(FToOptionT)

it's now

kleisliOfF.liftOptionT


trait KleisliInstances {

implicit def http4sKleisliSemigroupK[F[_]: SemigroupK, A]: SemigroupK[Kleisli[F, A, ?]] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this is what we can eliminate with typelevel/cats#1928.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in that same awkward position as KleisliOps, but it replicates the syntax we want people to have once that cats issue moves forward. I'm sad but totally okay with this one.

@@ -87,21 +90,20 @@ object PushSupport {
service: HttpService[F],
verify: String => Boolean = _ => true): HttpService[F] = {

def gather(req: Request[F], resp: Response[F]): Response[F] =
def gather(req: Request[F])(resp: Response[F]): Response[F] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smart.

f(req, service)
}
}
def apply[F[_], A, B, C, D](f: (C, Kleisli[F, A, B]) => F[D]): Middleware[F, A, B, C, D] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anybody even use these? They're kind of Finagle-like. I have to stare at it every time I remember it's here to understand how it works.

Copy link
Member Author

@aeons aeons Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecate?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, lets get rid of funky type wrappers unless they are neccesary. Feel Similarly towards EntityBody not that anyone is asking.

@codecov-io
Copy link

codecov-io commented Sep 28, 2017

Codecov Report

Merging #1424 into master will decrease coverage by 0.35%.
The diff coverage is 70.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1424      +/-   ##
==========================================
- Coverage   73.84%   73.48%   -0.36%     
==========================================
  Files         235      238       +3     
  Lines        5050     4997      -53     
  Branches      279      270       -9     
==========================================
- Hits         3729     3672      -57     
- Misses       1321     1325       +4
Impacted Files Coverage Δ
...in/scala/org/http4s/server/middleware/Logger.scala 72.22% <ø> (ø) ⬆️
...ttp4s/client/asynchttpclient/AsyncHttpClient.scala 92% <ø> (ø) ⬆️
core/src/main/scala/org/http4s/Service.scala 0% <ø> (-80%) ⬇️
...ala/org/http4s/server/blaze/Http1ServerStage.scala 73.68% <ø> (-0.35%) ⬇️
...in/scala/org/http4s/client/blaze/BlazeClient.scala 100% <ø> (ø) ⬆️
...ala/org/http4s/server/blaze/WebSocketSupport.scala 10.71% <ø> (-3.08%) ⬇️
.../main/scala/org/http4s/server/syntax/package.scala 0% <0%> (ø) ⬆️
...main/scala/org/http4s/server/metrics/Metrics.scala 0% <0%> (ø) ⬆️
...ain/scala/org/http4s/server/middleware/Jsonp.scala 0% <0%> (ø) ⬆️
...ala/org/http4s/server/middleware/PushSupport.scala 0% <0%> (ø) ⬆️
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3c3ef7...d2e497c. Read the comment docs.

service: Kleisli[OptionT[F, ?], A, Response[F]]): KleisliResponseOps[F, A] =
new KleisliResponseOps[F, A](service)

implicit def http4sKleisliSYntax[F[_]: Functor, A, B](

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird Capitalization here. Although I thought we might be removing it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'd like to see whether Cats is interested in providing this. KleisliResponseOps is our jam, but this one has nothing to do with HTTP.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capitalization was a typo.

I don't disagree that this should be provided somewhere else, but having it in a released version of cats will not happen overnight.

Inlining the functionality is basically .mapF(OptionT.liftF(_)). Would that be an acceptable solution?

case Left(t) =>
IO(logger.error(t)("Error rendering response"))
}
executionContext.execute(new Runnable {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit mirrors how Http1 works and should do the same as #1435 did for 0.17.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cant that be done via F.shift(executionContext) >> currentRunMethodMinusUnsafeRun

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same, and no, because the routing happens outside the task.

scala> def foo(i: Int): IO[List[String]] = { val x = Thread.currentThread.getName; IO(List(x, Thread.currentThread.getName)) }
e; IO(List(x, Thread.currentThread.getName)) }Thread.currentThread.getNam
<console>:27: warning: parameter value i in method foo is never used
       def foo(i: Int): IO[List[String]] = { val x = Thread.currentThread.getName; IO(List(x, Thread.currentThread.getName)) }
               ^
foo: (i: Int)cats.effect.IO[List[String]]

scala> (Effect[IO].shift(global) >> foo(42)).unsafeRunSync
(Effect[IO].shift(global) >> foo(42)).unsafeRunSync
res7: List[String] = List(run-main-0, scala-execution-context-global-125)

I think it could be done with an F.shift if the service were called inside F and then flattened.

@aeons
Copy link
Member Author

aeons commented Oct 2, 2017

The last couple of commits inlines the F ~> OptionT[F, ?] and deprecates {Authed,Http}Service.lift.

service: Kleisli[OptionT[F, ?], A, Response[F]]): KleisliResponseOps[F, A] =
new KleisliResponseOps[F, A](service)

implicit def http4sKleisliSYntax[F[_]: Functor, A, B](
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still miscapitalized

}

final class KleisliOps[F[_]: Functor, A, B](self: Kleisli[F, A, B]) {
def liftOptionT: Kleisli[OptionT[F, ?], A, B] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still a little hesitant to expose implicit syntax for types we don't own and aren't in our domain, but I see the use case for it. Would it be better to make this private[http4s] until Cats provides a better solution?

* handle all requests it is given. If `f` is a `PartialFunction`, use
* `apply` instead.
*/
def lift[F[_]: Functor](f: Request[F] => F[Response[F]]): HttpService[F] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one I thought could be deprecated.

def lift[F[_]: Functor](f: Request[F] => F[Response[F]]): HttpService[F] =
Kleisli(f).liftOptionT

def liftF[F[_]](f: Request[F] => OptionT[F, Response[F]]): HttpService[F] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We call it liftF because it lifts a Function? I don't have a better name. Cats calls this apply, but that's already taken. lift is confusing for the existing lift and for this, because Kleisli.lift takes just the F[B].

Does this help with type inference, or could we even deprecate this one and just use Kleisli for things that are already Kleisli functions and leave our apply as the DSL for partiality?


trait KleisliInstances {

implicit def http4sKleisliSemigroupK[F[_]: SemigroupK, A]: SemigroupK[Kleisli[F, A, ?]] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in that same awkward position as KleisliOps, but it replicates the syntax we want people to have once that cats issue moves forward. I'm sad but totally okay with this one.

* handle all requests it is given. If `f` is a `PartialFunction`, use
* `apply` instead.
*/
def lift[F[_]: Functor, T](f: AuthedRequest[F, T] => F[Response[F]]): AuthedService[F, T] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another one to deprecate.

@rossabaker
Copy link
Member

Alright, I'm happy with this in current form, with the possible exception of the existence of liftF alias for Kleisli.apply. I wonder if those just add more confusion, like the old Service alias.

@aeons
Copy link
Member Author

aeons commented Oct 2, 2017

I would be perfectly fine with deprecating .liftF as well and instructing people to use Kleisli.apply.

@ChristopherDavenport
Copy link
Member

I agree with the deprecations. Let's make things explicit as this PR suggests.

@aeons
Copy link
Member Author

aeons commented Oct 2, 2017

liftF is now removed. Should be good to merge on green.

@aeons aeons merged commit 0d1ebe8 into http4s:master Oct 2, 2017
@aeons aeons deleted the kleislis-all-over branch October 2, 2017 17:58
rossabaker pushed a commit to http4s/http4s-servlet that referenced this pull request Apr 3, 2022
Deprecate Service and remove MaybeResponse
rossabaker pushed a commit to http4s/http4s-servlet that referenced this pull request Apr 4, 2022
Deprecate Service and remove MaybeResponse
armanbilge pushed a commit to http4s/http4s-async-http-client that referenced this pull request May 20, 2022
Deprecate Service and remove MaybeResponse
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants