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
Use HttpApp instead of HttpRoutes in Http4sServlet #3012
Conversation
36d7d65
to
fcebca0
Compare
fcebca0
to
fb43c0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, with that name tweak.
Pinging @nigredo-tori, who has worked a lot on the servlets and may have commentary on the breaking change, but I think it's a good idea.
@@ -120,6 +121,9 @@ sealed class TomcatBuilder[F[_]] private ( | |||
}) | |||
|
|||
def mountService(service: HttpRoutes[F], prefix: String): Self = | |||
mountServiceHttpApp(http4s.httpRoutesToApp(service), prefix) | |||
|
|||
def mountServiceHttpApp(service: HttpApp[F], prefix: String): Self = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would just call this mountHttpApp
.
On blaze, it's called witHttpApp
, but the servlet model is a little different: we have more than one. So I think the different name is justified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
breaking change
The only breaking change I see here is that the parameters for the servlet constructors and apply
methods have changed. Ideally, we should provide additional @deprecated
constructors and methods with old signatures to avoid breaking packages, compiled against previous 0.21 milestones - although I don't think many libraries use those directly.
.suspend(serviceFn(request)) | ||
.getOrElse(Response.notFound) | ||
.recover({ case _: scala.MatchError => Response.notFound[F] }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need this. serviceFn
is a total function, so any MatchError
s are implementation errors, and shouldn't be handled (and especially swallowed) here. This also means we don't need to test this.
Same in BlockingHttp4sServlet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to be able to handle NotFound
in a single place, that's why I used a PartialFunction
and then handle the NotFound
in the recover
. Otherwise, when you have multiple services, you have to add case _ => handleNotFound()
at the end of each of them.
Furthermore, if you don't mount a service on the prefix /
, then a request on anything caught by /
will return an HTML that doesn't seem to be programmable (even though fixing this would require to modify the code in JettyBuilder.resource()
which I haven't done here but could be factorized in a single place with what I describe in the above paragraph).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm not quite sure what you're getting at here. I'll try and answer some of the points you are trying to make...
First, it looks like you've added this recover
as a way to solve some issue in JettyBuilder
, and it handles the situation that should not happen during any normal use of the AsyncHttp4sServer
. This smells.
Second, in the Cats world we make a lot of assumptions, one of which is that all the functions are total except for PartialFunction
s. We can only get a MatchError
by breaking this rule via a non-exhaustive match - which, by the way, would lead to a compiler warning. Generally, any MatchError
thrown signals broken code, and we should never try specifically handling it for an arbitrary function provided by the user code, never mind swallowing it without reporting. Basically, if you want to support a non-exhaustive service
- use HttpRoutes
instead. This is exactly what it is there for.
Third, regarding case _ => handleNotFound()
- we can do .orNotFound
when mounting HttpRoutes
in JettyBuilder
. I'm not sure why that would be a problem.
Finally, regarding /
. As I understand the problem (if you can call it that) is that Jetty catches any requests that were not handled, and shows a standard 404 error page. I'm not sure it's our place to change that by default, but even if it were - as I understand it, this can be solved by mounting a servlet to /
. I'm still not sure what all of this has to do with the servlet implementation, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thank you for your comments. I think I understand your points. I will try to update my PR accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nigredo-tori I added def orNotFound(notFoundHandler: A => Response[F]): Kleisli[F, A, Response[F]]
to KleisliSyntax
. Also I updated NotFoundHttpAppSpec
. It describes what I am trying to achieve here: having a simple, single way of handling custom NotFound
responses accross the server. Do you think this would be reasonable ?
(also I agree with your latest comment about simply adding
def mountHttpApp(service: HttpApp[F], prefix: String): Self =
mountService(service.mapK(OptionT.liftK[F])), prefix)
to the builders)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried and summarized the important points of this below, and I suggest we continue this discussion there, since the scope of it went far beyond the single line of code I want gone. 😄
@@ -120,6 +121,9 @@ sealed class TomcatBuilder[F[_]] private ( | |||
}) | |||
|
|||
def mountService(service: HttpRoutes[F], prefix: String): Self = | |||
mountServiceHttpApp(http4s.httpRoutesToApp(service), prefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're in package org.http4s
, so we can just use httpRoutesToApp(service)
.
@@ -44,7 +44,7 @@ object Issue454 { | |||
} | |||
|
|||
val servlet = new AsyncHttp4sServlet[IO]( | |||
service = HttpRoutes.of[IO] { | |||
service = HttpApp { | |||
case GET -> Root => Ok(insanelyHugeData) | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try and avoid non-exhaustive matches. This can be done with httpRoutesToApp
, or by adding a catch-all clause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the .orNotFound
syntax in as described below
@@ -77,4 +78,7 @@ package object http4s { // scalastyle:ignore | |||
type Http4sSyntax = syntax.AllSyntax | |||
@deprecated("Moved to org.http4s.syntax.all", "0.16") | |||
val Http4sSyntax = syntax.all | |||
|
|||
def httpRoutesToApp[F[_]: ConcurrentEffect](httpRoutes: HttpRoutes[F]): HttpApp[F] = | |||
HttpApp[F](req => httpRoutes(req).getOrElse[Response[F]](Response.notFound[F])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a syntax for this. With import org.http4s.syntax.kleisli._
we can do httpRoutes.orNotFound
. So I don't think we need this as a separate function.
Actually, if the motivation behind this PR is to just add def mountHttpApp(service: HttpApp[F], prefix: String): Self =
mountService(service.mapK(OptionT.liftK[F])), prefix) This way we wouldn't have to break anything. Unless, of course, the change to the servlets is the main goal - in which case I don't see the point of it, since we can always get a |
So, to summarize the discussion (and to get my thoughts in order):
|
@@ -30,6 +30,9 @@ trait KleisliSyntaxBinCompat1 { | |||
final class KleisliResponseOps[F[_]: Functor, A](self: Kleisli[OptionT[F, ?], A, Response[F]]) { | |||
def orNotFound: Kleisli[F, A, Response[F]] = | |||
Kleisli(a => self.run(a).getOrElse(Response.notFound)) | |||
|
|||
def orNotFound(notFoundHandler: A => Response[F]): Kleisli[F, A, Response[F]] = | |||
Kleisli(a => self.run(a).getOrElse(notFoundHandler(a))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this use case (a custom fall-through handler) is common enough to warrant special syntax. I can also think of a few other ways to implement similar functionality - instead of A => Response[F]
we can use Response[F]
, F[Response[F]]
, A => F[Response[F]]
, HttpApp[F]
... All of those potential functions are equally valid, and would have their uses - I don't think the one you've chosen is the only one that should have a syntax.
If you insist, I'd like you to at least choose another name. Overloading should be avoided in general, and orNotFound
is misleading here, since it has no relation to NotFound
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to be able to do:
JettyBuilder[IO]
.bindAny()
.mountServiceHttpApp(
httpRoutes0.orNotFound(myCustomNotFoundHandler),
"/"
)
.mountServiceHttpApp(
httpRoutes1.orNotFound(myCustomNotFoundHandler),
"/prefix1"
)
(...)
.mountServiceHttpApp(
httpRoutesN.orNotFound(myCustomNotFoundHandler),
"/prefixN"
)
So that you can see in one place how all non-matched endpoints
are handled and that there are no leaks. The method name could be .orHandleNonMatch
. I agree with all your other points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that you can see in one place how all non-matched endpoints are handled and that there are no leaks.
You don't need a (library) syntax for that - it works just as well with a custom function:
def orMyHandler(x: HttpRoutes[F]): HttpApp[F] =
Kleisli { req => x.run(req).getOrElse(myCustomNotFoundHandler(req)) }
//...
.mountServiceHttpApp(
orMyHandler(httpRoutes0),
"/"
)
// ...
As an aside, I've never seen someone mount a lot of services onto one builder like this. I'd rewrite your example like this instead:
JettyBuilder[IO]
.bindAny()
.mountServiceHttpApp(
myOrHandler(
Router(
"" -> httpRoutes0,
"prefix1" -> httpRoutes1,
...
"prefixN" -> httpRoutesN
)
),
"/"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is exactly what I have been looking for !
More generally, do you think there is a need for a JettyBuilder.mountServiceHttpApp
? I would say yes because as .mountServiceHttpApp
expects a total
function then it is clearer to see how NotFound
for instance are handled. Also it would make sense to unify the APIs between Jetty
and Blaze
. What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added clarity is not as important here; AFAICT, people tend to separate their routing logic from the server itself (http4s is great in that), so whoever wants to be sure they have matched all the possible options is probably already using HttpApp
anyway, and just rewraps that as HttpRoutes
. I think mountServiceHttpApp
(or, rather, mountHttpApp
) will be useful chiefly because it would allow us to avoid that annoying rewrapping.
Also it would make sense to unify the APIs between Jetty and Blaze.
Not really. As it was mentioned here, Jetty and Blaze builders have fundamentally different capabilities for mounting routes, so I don't see an attempt to unify them leading to anything other than extra complexity and confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think mountServiceHttpApp (or, rather, mountHttpApp) will be useful chiefly because it would allow us to avoid that annoying rewrapping.
Avoiding useless rewrapping on the library-user side is always good, I will simplify this PR and follow your advices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 once these two minor things are addressed.
@@ -14,7 +14,7 @@ import scala.concurrent.duration._ | |||
|
|||
class BlockingHttp4sServletSpec extends Http4sSpec { | |||
|
|||
lazy val service = HttpRoutes.of[IO] { | |||
lazy val service = HttpApp[IO] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This match is now non-exhaustive! Please add .orNotFound
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was fixed in my latest commit
import scala.io.Source | ||
import scala.util.Try | ||
|
||
class NotFoundHttpAppSpec extends Http4sSpec { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need this spec - it doesn't test anything that isn't covered elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I removed it
Hello @nigredo-tori , I fixed the PR following your advice. For some reason Travis CI keeps failing on 1 of the 3 test machines:
but it is successful on the 2 others. Please let me know what you think and thank you for your help ! |
We already test it in
This by itself is not a sufficient reason for adding tests. Also, the most common case for making a |
👍 from me. @hamnis, @rossabaker, please take a look again - this PR has gotten a lot lighter. |
I cleared the travis cache and restarted the build, can be merged on build success. |
Same
NotFound
behavior as withHttpRoutes
for unspecified endpoints (seeNotFoundHttpAppSpec
)Preserve retrocompatibility in
JettyBuilder
,ServletContextSyntax
,TomcatBuilder