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

Issue225 #259

Merged
merged 18 commits into from May 2, 2019
@@ -77,10 +77,13 @@ object Http4sServerGenerator {
case GetExtraRouteParams(tracing) =>
for {
_ <- Target.log.debug("Http4sServerGenerator", "server")(s"getExtraRouteParams(${tracing})")
res <- if (tracing) {
Target.pure(List(param"""trace: String => Request[F] => TraceBuilder[F]"""))
} else Target.pure(List.empty)
} yield res
mapRoute = param"""mapRoute: (String, Request[F], F[Response[F]]) => F[Response[F]] = (_: String, _: Request[F], r: F[Response[F]]) => r"""
tracing <- if (tracing) {
Target.pure(Option(param"""trace: String => Request[F] => TraceBuilder[F]"""))
} else Target.pure(Option.empty)
} yield {
tracing.toList ::: List(mapRoute)
}

case GenerateSupportDefinitions(tracing) =>
Target.pure(List.empty)
@@ -501,8 +504,17 @@ object Http4sServerGenerator {
case Nil => responseInMatch
case generators => q"for {..${generators :+ enumerator"response <- $responseInMatch"}} yield response"
}
val routeBody = entityProcessor.fold[Term](responseInMatchInFor)(_.apply(responseInMatchInFor))
val fullRoute: Case = p"case req @ $fullRouteWithTracingMatcher => $routeBody"
val routeBody = entityProcessor.fold[Term](responseInMatchInFor)(_.apply(responseInMatchInFor))
val responseName = generateSafeRouteValName("response", pathArgs)
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 2, 2019

Collaborator

This still seems like unnecessary complexity. If this was avoiding allocations or something, I'd understand it, but this is just for readability of the generated code?

Having arbitrary integer suffixes that change based on name collisions seems only slightly better, not enough to warrant the additional complexity.

That being said, I understand you've put a lot of effort into this PR at this time. I don't anticipate this causing any failures, though I expect it will eventually be unified to not require an additional binding.

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 2, 2019

Author Contributor

well i added this functionality when we were going to do F[Unit] ... with F[Response[F]] I can remove this

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 2, 2019

Collaborator

Ah, I understand -- <* with real bindings definitely would make it much easier to read. Since we've settled on mapRoute, I would appreciate simplifying this section.

No objections beyond that though.


val responseComputationDefn = Defn.Val(Nil, List(Pat.Var(name = Term.Name(responseName))), None, q"{$routeBody}")

val fullRoute: Case =
p"""case req @ $fullRouteWithTracingMatcher =>
$responseComputationDefn
mapRoute($operationId, req, ${Term.Name(responseName)})
"""

val respond: List[List[Term.Param]] = List(List(param"respond: $responseCompanionTerm.type"))

@@ -670,5 +682,14 @@ object Http4sServerGenerator {
def unapply(r: Request[F]): Option[(Request[F], TraceBuilder[F])] = Some(r -> $tracingField(r))
}
"""

def generateSafeRouteValName(suggested: String, reserved: List[ScalaParameter[ScalaLanguage]]): String = {
def iter(i: Int): String = {
val withIter = suggested + i
reserved.find(_.argName.value == withIter).map(_ => iter(i + 1)).getOrElse(withIter)
}
iter(0)
}

}
}
@@ -48,24 +48,34 @@ class Issue165 extends FunSuite with Matchers with SwaggerSpecRunner {
}
"""
val resource = q"""
class StoreResource[F[_]]()(implicit F: Async[F]) extends Http4sDsl[F] {
class StoreResource[F[_]](mapRoute: (String, Request[F], F[Response[F]]) => F[Response[F]] = (_: String, _: Request[F], r: F[Response[F]]) => r)(implicit F: Async[F]) extends Http4sDsl[F] {
def routes(handler: StoreHandler[F]): HttpRoutes[F] = HttpRoutes.of {
{
case req @ GET -> Root =>
handler.getRoot(GetRootResponse)() flatMap {
case GetRootResponse.Ok =>
Ok()
case req @ GET -> Root =>
val response0 = {
handler.getRoot(GetRootResponse)() flatMap {
case GetRootResponse.Ok =>
Ok()
}
}
mapRoute("getRoot", req, response0)
case req @ GET -> Root / "foo" =>
handler.getFoo(GetFooResponse)() flatMap {
case GetFooResponse.Ok =>
Ok()
val response0 = {
handler.getFoo(GetFooResponse)() flatMap {
case GetFooResponse.Ok =>
Ok()
}
}
mapRoute("getFoo", req, response0)
case req @ GET -> Root / "foo" / "" =>
handler.getFooDir(GetFooDirResponse)() flatMap {
case GetFooDirResponse.Ok =>
Ok()
val response0 = {
handler.getFooDir(GetFooDirResponse)() flatMap {
case GetFooDirResponse.Ok =>
Ok()
}
}
mapRoute("getFooDir", req, response0)
}
}
}
@@ -0,0 +1,65 @@
package core.issues

import com.twilio.guardrail.generators.Http4s
import com.twilio.guardrail.{ Context, Server, Servers }
import org.scalatest.{ FunSuite, Matchers }
import support.SwaggerSpecRunner

class Issue225 extends FunSuite with Matchers with SwaggerSpecRunner {

import scala.meta._

val swagger: String =
s"""
|swagger: '2.0'
|host: petstore.swagger.io
|paths:
| /{response0}:
| parameters:
| - in: path
| name: response0
| type: string
| get:
| operationId: getRoot
| responses:
| 200:
| description: description
|""".stripMargin

test("Ensure handlerWrapper is generated and conflicting name is resolved correctly (response0 param conflicts with generated val name)") {
val (_, _, Servers(Server(_, _, genHandler, genResource :: _) :: Nil, Nil)) = runSwaggerSpec(swagger)(Context.empty, Http4s)

val handler = q"""
trait Handler[F[_]] {
def getRoot(respond: GetRootResponse.type)(response0: String): F[GetRootResponse]
}
"""
val resource = q"""
class Resource[F[_]](mapRoute: (String, Request[F], F[Response[F]]) => F[Response[F]] = (_: String, _: Request[F], r: F[Response[F]]) => r)(implicit F: Async[F]) extends Http4sDsl[F] {
def routes(handler: Handler[F]): HttpRoutes[F] = HttpRoutes.of {
{
case req @ GET -> Root / response0 =>
val response1 = {
handler.getRoot(GetRootResponse)(response0) flatMap {
case GetRootResponse.Ok =>
Ok()
}
}
mapRoute("getRoot", req, response1)
}
}
}
"""

compare(genHandler, handler)

// Cause structure is slightly different but source code is the same the value converted to string and then parsed
compare(genResource.toString().parse[Stat].get, resource)
}

private def compare(actual: Tree, expected: Tree): Unit = {
println(s"actual: ${actual.syntax}")
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 2, 2019

Collaborator

No printlns during normal operation please

This comment has been minimized.

Copy link
@tomasherman

tomasherman May 2, 2019

Author Contributor

sorry, leftover from debugging :)

println(s"expected: ${expected.syntax}")
actual.structure shouldEqual expected.structure
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.