0.19 #253

chuwy · 2018-10-06T07:23:46Z

This is based on #236 and leaves @Igosuki's work almost untouched, but:

bumps http4s to 0.19.0 (which is still not final, I guess?)
rebased on top of current master
makes all tests green
Bumps SBT to latest version
Makes json4s a test-only dependency for swagger

TODO

Fix HListToFuncSpec
Fix docstring in AuthedContext
Rename few more "service" to "routes"
Add a comment in AuthedContext that middleware is mandatory
Should we rename RhoService to RhoRoutes?
Bump to final 0.20.0 when available

core/src/main/scala/org/http4s/rho/Result.scala

zarthross · 2018-10-06T13:35:29Z

core/src/test/scala/org/http4s/rho/bits/HListToFuncSpec.scala

      new RhoService[IO] {
        // `.pure[IO]` used to require the cats.implicits under test
-        GET / "route1" |>> { () => Ok("foo".pure[IO]) }
+        GET / "route1" |>> { () => Ok("foo") }


This needs to get changed back, otherwise it doesn't properly test for regressions of issue #218

zarthross · 2018-10-06T13:36:51Z

examples/project/build.properties

@@ -0,0 +1 @@
+sbt.version=1.1.5


Probably set this to 1.2.3 like the main project.

zarthross · 2018-10-06T13:49:25Z

examples/src/main/scala/com/http4s/rho/hal/plus/swagger/demo/Main.scala


    val routes =
      new Routes(businessLayer)

    BlazeBuilder[IO]
      .mountService(routes.staticContent combineK routes.dynamicContent, "")
      .bindLocal(port)
-      .serve
+      .serve.compile.toList.map(_.head)


Would it be better to use head.compile.last instead of toList here? No need to build a full list here? (I realize it will likely never emit more than a single Exit code, but if it does, shouldn't we just take the head exit code and stop?

Right, thanks. We don't want toList for sure. But I belive compile.drain.as(ExitCode.Success) is a preferred option for streaming applications inside IOApp.

Yeah, I saw that in the Http4s docs, wasn't 100% sure thats the right choice either honestly... but I'm ok with using what the http4s docs say.

zarthross · 2018-10-06T13:56:50Z

examples/src/main/scala/com/http4s/rho/swagger/demo/Main.scala

      .bindLocal(port)
-      .serve
+      .serve.compile.toList.map(_.head)


Same as the above comment on head.compile.last

zarthross · 2018-10-06T14:09:22Z

swagger/src/main/scala/org/http4s/rho/swagger/SwaggerModelsBuilder.scala

-      consumes    = rr.validMedia.toList.map(_.renderString),
-      produces    = rr.responseEncodings.toList.map(_.renderString),
+      consumes    = rr.validMedia.toList.map(_.toString),
+      produces    = rr.responseEncodings.toList.map(_.toString),


I feel like the old renderString function was not the same as toString on a MediaRange. (Possibly not on an responseEncoding)... toString's results have a MediaRange and some parens in the string that 'renderString' did not put there...

I'm looking at this diff in http4s to see the change: http4s/http4s@f2ca175

zarthross · 2018-10-06T14:10:58Z

swagger/src/test/scala/org/http4s/rho/swagger/Arbitraries.scala

+      audio.midi,
+      image.gif,
+      text.html
+    ).map(_.toString)


Again, not sure toString should be the substitute for renderString on a MediaRange

It seems that we have Show instance for MediaType. I believe it will be the correct substitution.

I believe you are correct.

chuwy · 2018-10-06T16:29:22Z

Thank you very much for the feedback, @zarthross. I'm still a little bit puzzled by HListToFuncSpec as it cannot resolve EntityEncoder[IO, IO[String]], but probably I'm missing something super obvious as it is quite late in where I am - I'll try to fix it tomorrow.

chuwy · 2018-10-06T17:00:21Z

Aha! So, the problem is that indeed EntityDecoder[F, F[A]] is missing since this commit. Wondering what was the reasoning.

core/src/main/scala/org/http4s/rho/CompileService.scala

zarthross · 2018-10-06T17:36:01Z

core/src/test/scala/org/http4s/rho/CodecRouterSpec.scala

@@ -14,7 +14,7 @@ class CodecRouterSpec extends Specification {

  "A CodecRouter in a RhoService" should {

-    val service = new RhoService[IO] {
+    val routes = new RhoService[IO] {


This is still a RhoService, so i don't know that the rename is appropriate here.

But we're immediately converting it to HttpRoutes[IO] below, so I think all these are justified.

¯\_(ツ)_/¯
Not really that big of deal. Not a deal breaker for me either way.

(Removed other similar comments for brevity.)

zarthross

Looks good to me.

rossabaker · 2018-10-06T20:28:38Z

Yeah, sorry about 0.19.0 not being final. I messed up the tagging.

Would you like to do a milestone release on this so people can try it?

zarthross · 2018-10-06T20:54:39Z

I think there is a few more changes @chuwy would like to make before we merge/release, but once he finishes those check boxes, I would like to get this released. I have some projects I would like to get a head start updating before the 0.19.1 release.

chuwy · 2018-10-07T07:15:18Z

@rossabaker, @zarthross I made some more "service -> routes" renamings to be consistent with core and updated AuthedContext (I believe unsafe getAuth was a bug - I stumbled upon it when first used rho). It would be nice to get M1 when you have time.

Last question I have is whether we should rename RhoService to RhoRoutes. I vote for yes, e.g. it is just a wrapper around list of individual RhoRoutes.

zarthross · 2018-10-07T11:54:04Z

core/src/main/scala/org/http4s/rho/ServiceBuilder.scala

@@ -7,15 +7,15 @@ import shapeless.HList
 import scala.collection.immutable.VectorBuilder

 /** CompileService which accumulates routes and can build a `HttpService` */
-final class ServiceBuilder[F[_]: Monad] private(internalRoutes: VectorBuilder[RhoRoute.Tpe[F]]) extends CompileService[F, RhoRoute.Tpe[F]] {
+final class ServiceBuilder[F[_]: Monad] private(internalRoutes: VectorBuilder[RhoRoute.Tpe[F]]) extends CompileRoutes[F, RhoRoute.Tpe[F]] {

  /** Turn the accumulated routes into an `HttpService`
    *
    * @param filter [[RhoMiddleware]] to apply to the collection of routes.
    * @return An `HttpService` which can be mounted by http4s servers.
    */
  def toService(filter: RhoMiddleware[F] = identity): HttpRoutes[F] =


ServiceBuilder and its toService function also need a rename.

Agree, but I think we should handle this along with RhoService

Yeah, I agree with the RhoService rename. It makes sense in context with what http4s is trying to do.

The only worry is that it'll get confusing with RhoRhoute vs RhoRhoutes?

My worry is similar: we're getting too many "route" in different places that clash with each other and core's. We're ending up with RouteFoo.toRoutes(routes).asRoutes roughly speaking. But still I believe this is a right choice.

zarthross · 2018-10-07T11:57:32Z

core/src/test/scala/org/http4s/rho/AuthedContextSpec.scala

@@ -26,13 +26,22 @@ object MyAuth extends AuthedContext[IO, User]
 object MyService extends RhoService[IO] {
  import MyAuth._

-  GET +? param("foo", "bar") >>> auth |>> { (req: Request[IO], foo: String, user: User) =>
+  GET +? param("foo", "bar") >>> auth |>> { (_: Request[IO], foo: String, user: User) =>


You can remove the _: Request[IO] all together here. If Request isn't the first parameter of the HList its just not provided.

core/src/main/scala/org/http4s/rho/AuthedContext.scala

zarthross · 2018-10-07T12:16:51Z

core/src/test/scala/org/http4s/rho/AuthedContextSpec.scala

+  GET / "private" / 'place |>> { (req: Request[IO], path: String) =>
+    getAuth(req) match {
+      case Some(user) => Ok(s"${user.name} at $path")
+      case None => Forbidden(s"not authenticated at $path")


Unfortunately, if getAuth returns None it doesn't mean the user was not authenticated, it means the AuthMiddleware wasn't applied.

Our authenticated middleware always returns Some, but in case where it returns None (which is still valid) - it will be indistinguishable from not applied middleware. So we have plenty of slightly broken APIs, but this one from my POV is less broken. We just need to mention in comments that AuthMiddleware always should be applied. Until we come up with best strategy.

zarthross · 2018-10-15T17:00:25Z

@chuwy How many of those checkboxes do you want to complete before this gets merged in?

chuwy · 2018-10-15T17:02:29Z

@zarthross I think two at least. Should be done in next couple of days.

zarthross · 2018-10-15T17:04:37Z

I think it would probably be wise to go through with the RhoService to RhoRoutes rename to follow suite with the Http4s rename.

chuwy · 2018-10-15T17:05:57Z

Sure. Totally agree. I meant "all except Bump to final 0.19.1", so this renaming will be included for sure.

chuwy · 2018-10-15T17:06:42Z

Also, I assume you're fine with Option in AuthedContext. I believe that it is a not an ideal solution, but still better than runtime exception.

ChristopherDavenport · 2018-10-15T17:08:43Z

project/Dependencies.scala

@@ -2,7 +2,7 @@ import sbt._
 import Keys._

 object Dependencies {
-  lazy val http4sVersion = "0.18.19"
+  lazy val http4sVersion = "0.19.0"


The currently version is 0.19.0-M4, 0.19.0 was mistagged and should be avoided if possible.

chuwy · 2018-10-29T23:40:25Z

Hey @zarthross, sorry for delay. This has been updated to http4s 0.20.0-M1. I also believe I addressed all your feedback and everything I wanted to change.

I assume we're not pursuing version-parity with http4s, so I left it as 0.19, not 0.20.

If everything is correct, it should be ready for M1.

rossabaker · 2018-10-29T23:58:50Z

Version parity has always been coincidental, but maybe it's nice since it has recently been in lockstep. I guess it might get more complicated when both projects hit 1.0. I don't have a strong opinion there.

zarthross

I'm still not comfortable with the way the Auth has played out yet. So lets release this as M1 and I'll work on something before we get to M2.

core/src/main/scala/org/http4s/rho/AuthedContext.scala

zarthross requested changes Oct 6, 2018

View reviewed changes

chuwy force-pushed the 0.19 branch from 43f2f8d to 2dee2a5 Compare October 6, 2018 16:25

chuwy commented Oct 6, 2018

View reviewed changes

core/src/main/scala/org/http4s/rho/CompileService.scala Outdated Show resolved Hide resolved

zarthross reviewed Oct 6, 2018

View reviewed changes

zarthross approved these changes Oct 6, 2018

View reviewed changes

zarthross mentioned this pull request Oct 6, 2018

Upgrade to http4s 0.19 and friends #236

Closed

chuwy force-pushed the 0.19 branch from 0a731d6 to ec2ba9e Compare October 7, 2018 07:08

chuwy force-pushed the 0.19 branch from ec2ba9e to 46d3787 Compare October 7, 2018 07:30

zarthross reviewed Oct 7, 2018

View reviewed changes

zarthross requested changes Oct 7, 2018

View reviewed changes

chuwy mentioned this pull request Oct 7, 2018

Get rid of unsafe function in AuthedContext #256

Open

ChristopherDavenport reviewed Oct 15, 2018

View reviewed changes

scala-steward and others added 8 commits October 30, 2018 06:08

Update http4s-server to 0.18.19

f379d67

Update scala-xml to 1.1.1

8c91e1e

Update /swagger to http4s 0.19

9fb03aa

Update /core to http4s 0.19

be54b70

Update examples to http4s 0.19.0-M1

676e499

use a specific version of http4s (forked)

a8d99a6

Fix compilation after updating http4s and fs2(1.0.0-M4)

2563328

Use http4s 0.19.0-M1

2f8f5c6

chuwy added 14 commits October 30, 2018 06:08

Bump to 0.19.0

417094f

Bump SBT to 1.2.3

fc29c08

Get rid of runtime json4s dependency

fc9e19d

Minor renamings

7773468

First round of feedback

da8420c

Fix HListToFuncSpec cats.implicits case

fc6311c

Minor renamings

83dab6b

Rename CompileService to CompileRoutes

8f4ee99

Rename foldService to foldRoutes and others

2501b46

Fix docstring example in AuthedContext and get rid of unsafe getAuth

c62b4f1

Bump http4s to 0.20.0-M1

5f8c926

Rename RhoService to RhoRoutes

bf440e5

Rename RhoService to RhoRoutes second pass

f67eed3

Mention AuthMiddleware requirement (http4s#256)

945a4aa

chuwy force-pushed the 0.19 branch from 9486603 to 945a4aa Compare October 29, 2018 23:34

Remove unnecessary request argument

3b96baf

Merge branch 'master' into 0.19

00b17d2

zarthross approved these changes Oct 30, 2018

View reviewed changes

core/src/main/scala/org/http4s/rho/AuthedContext.scala Show resolved Hide resolved

zarthross added this to the v0.19.0-M1 milestone Oct 30, 2018

zarthross merged commit a12cbe1 into http4s:master Nov 4, 2018

		@@ -0,0 +1 @@
		sbt.version=1.1.5

0.19 #253

0.19 #253

Conversation

chuwy commented Oct 6, 2018 • edited Loading

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarthross Oct 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarthross Oct 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuwy commented Oct 6, 2018 • edited Loading

chuwy commented Oct 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarthross Oct 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarthross left a comment

Choose a reason for hiding this comment

rossabaker commented Oct 6, 2018

zarthross commented Oct 6, 2018

chuwy commented Oct 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuwy Oct 7, 2018 • edited Loading

Choose a reason for hiding this comment

zarthross commented Oct 15, 2018

chuwy commented Oct 15, 2018

zarthross commented Oct 15, 2018

chuwy commented Oct 15, 2018

chuwy commented Oct 15, 2018

Choose a reason for hiding this comment

chuwy commented Oct 29, 2018

rossabaker commented Oct 29, 2018

zarthross left a comment

Choose a reason for hiding this comment

chuwy commented Oct 6, 2018 •

edited

Loading

zarthross Oct 6, 2018 •

edited

Loading

zarthross Oct 6, 2018 •

edited

Loading

chuwy commented Oct 6, 2018 •

edited

Loading

zarthross Oct 6, 2018 •

edited

Loading

chuwy commented Oct 7, 2018 •

edited

Loading

chuwy Oct 7, 2018 •

edited

Loading