#203: Wrap POST body as in wiro (closes #203) #212
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tpetrucciani
commented
Mar 31, 2020
| @@ -9,33 +9,42 @@ import cats.data.NonEmptyList | |||
| object Meta { | |||
| val codecsImplicits = (routes: List[TapiroRoute]) => { | |||
There was a problem hiding this comment.
This function is getting quite messy...
Part of the problem is that we're delaying the conversion to scalameta type for each type of codec because we want to call .distinct while we still have a list of metarpheus types, because it wouldn't work on scalameta types because == is by reference
Maybe we could use structural equality from scala.meta.contrib (https://scalameta.org/docs/trees/guide.html#compare-trees-for-equality) to clean this up a bit?
There was a problem hiding this comment.
I think using isEqual is a good idea
There was a problem hiding this comment.
I've tried to rewrite that function and I think it reads a bit better now
gabro
reviewed
Mar 31, 2020
Comment on lines
53
to
55
| case 0 => q"_ => $controllersName()" | ||
| case 1 => controllersName | ||
| case _ => q"($controllersName _).tupled" |
| } | ||
| case RouteMethod.POST => | ||
| val fields = route.route.params | ||
| .filterNot(_.tpe == MetarpheusType.Name("AuthToken")) |
There was a problem hiding this comment.
Is it correct that we hardcode AuthToken here?
There was a problem hiding this comment.
I think it's sort-of-correct right now because we're hardcoding it everywhere anyway, so we expect the type parameter in the controller trait to be always named exactly AuthToken. As I understand it, we should fix that in #204 when we change metarpheus to return the real name of the type parameter
| q"x => $controllersName(..${fields.map(f => q"x.$f")})" | ||
| } | ||
| val toRoute = q"$endpointsName.toRoute($controllerContent)" | ||
| q"val ${Pat.Var(name)} = $toRoute" |
There was a problem hiding this comment.
minor, but I think this can be
Suggested change
| q"val ${Pat.Var(name)} = $toRoute" | |
| q"val $name = $toRoute" |
There was a problem hiding this comment.
I can't get this to work 🙁 it expects a Pat there and $name is a Term.Name, which doesn't seem to work (using a String instead compiles but generates a literal string pattern)
Comment on lines
48
to
68
| val endpoints = (routes: List[TapiroRoute]) => | ||
| routes.map { route => | ||
| val name = Term.Name(route.route.name.last) | ||
| val endpointsName = q"endpoints.$name" | ||
| val controllersName = q"controller.$name" | ||
| val controllerContent = | ||
| if (route.params.length <= 1) Some(controllersName) | ||
| else Some(Term.Select(Term.Eta(controllersName), Term.Name("tupled"))) | ||
| controllerContent.map { content => | ||
| val toRoutes = Term.Apply(Term.Select(endpointsName, Term.Name("toRoutes")), List(content)) | ||
| q"val ${Pat.Var(name)} = $toRoutes" | ||
| } | ||
| route.method match { | ||
| case RouteMethod.GET => | ||
| route.route.params.length match { | ||
| case 0 => q"_ => $controllersName()" | ||
| case 1 => controllersName | ||
| case _ => q"($controllersName _).tupled" | ||
| } | ||
| case RouteMethod.POST => | ||
| val fields = route.route.params | ||
| .filterNot(_.tpe == MetarpheusType.Name("AuthToken")) | ||
| .map(p => Term.Name(p.name.getOrElse(Meta.typeNameString(p.tpe)))) | ||
| q"x => $controllersName(..${fields.map(f => q"x.$f")})" | ||
| } | ||
| val toRoutes = q"$endpointsName.toRoutes($controllerContent)" | ||
| q"val ${Pat.Var(name)} = $toRoutes" |
There was a problem hiding this comment.
this seems identical to the same code in AkkaHttpMeta. Can we deduplicate it? (Not necessarily in this PR)
There was a problem hiding this comment.
I've factored out most of the duplication while fixing bugs
There's still some duplicated code here and there that processes route parameters (filtering out the auth param, computing default param names from their types if missing, etc.) which I think we should factor out, but it's best done in another PR I think
In particular, I think we could include in TapiroRoute a "cleaned" version of the route parameters, already processed so we don't have to do the same processing in different functions
| @@ -9,33 +9,42 @@ import cats.data.NonEmptyList | |||
| object Meta { | |||
| val codecsImplicits = (routes: List[TapiroRoute]) => { | |||
There was a problem hiding this comment.
I think using isEqual is a good idea
|
This generates wrong I launched tapiro via where Main is then compiled the target service (called and got the following |
|
Many thanks for the test @giuscri! I had indeed messed things up with with auth tokens 🙈 The version I just pushed (a8ecd98) should fix that. I still get an error on your code, but it seems unrelated ( Adding some unit tests will surely be useful 😄 |
|
I removed myself as an assignee enough people is working on this :D |
Merged
(reverting 0a856a8)
tpetrucciani
force-pushed
the
203-wrap_post_body_as
branch
from
a8ecd98 to
fad617e
Compare
gabro
previously approved these changes
Apr 16, 2020
| @@ -5,7 +5,7 @@ import java.nio.file.Files | |||
| class TapiroSuite extends munit.FunSuite { | |||
|
|
|||
| check( | |||
| "http4s", | |||
| "tapir and http4s endpoints", | |||
There was a problem hiding this comment.
minor: let's try to keep test names short and without spaces, so that they're easily selectable for testOnly.
E.g.
tapiroCore/testOnly -- io.buildo.tapiro.TapiroSuite.tapir-http4s-endpoints| @@ -44,17 +45,10 @@ object Http4sMeta { | |||
| q"Router($route -> NonEmptyList($first, List(..$rest)).reduceK)" | |||
There was a problem hiding this comment.
not related to this PR, but this can be:
Suggested change
| q"Router($route -> NonEmptyList($first, List(..$rest)).reduceK)" | |
| q"Router($route -> NonEmptyList.of(first, rest: _*).reduceK)" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2520577
Closes #203
Stringoutput (see tapiro generated code will need a JsonCodec for _primitive_ types (String, Int, ...) #180 (comment) and later comments)Notes:
JsonCodec, I've added an import ofimport sttp.tapir.json.circe._to that file (so it can generateJsonCodecinstances given CirceEncoderandDecoder) and added the Circe semiauto derivation of theEncoderandDecoder, adding implicit parameters ofEncoderandDecoderfor field names.JsonCodec/PlainCodecwith circeEncoder/Decoder, it would be maybe nicer to always requireJsonCodecand be able to derive theJsonCodecfor the case class from those of the fields, but I couldn't find a way to do it.Encoder, but tapir always expects both to be able to describe clients I think.For example, for the controller
we generate the tapir endpoints
and the http4s endpoints
Test Plan
Tested on a small example project