Server-side power API #179 #429

ctoomey · 2018-10-07T01:42:33Z

Here's a draft, scala-only implementation of both #179 and #420.

In AkkaGrpcPlugin.scala I added a new setting akkaGrpcGeneratorOptions with 2 options: ServerPowerApis to generate server power APIs, and UsePlayActions to generate a handler and router that implement Play's RequestHeader => EssentialAction instead of HttpRequest => Future[HttpResponse].

The server power APIs are defined by a power API trait that extends the base trait and adds a MetadataMap parameter to each method.

If this looks good I’ll work on the rest of the changes (Java implementation, etc.).

akka-ci · 2018-10-07T01:42:38Z

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

johanandren · 2018-10-09T07:29:19Z

I think it would be easier to review and understand what is going on if the PRs were split up and there was one with the power-user API and one with the Play-nativeness changes, you think you could do like that instead @ctoomey ? (Would be fine to base one PR branch off the other if you have changes that both need. )

raboof · 2018-10-09T09:12:12Z

OK TO TEST

raboof

Hi @ctoomey, thanks for sharing your progress! This is definitely interesting, much appreciated.

I agree with @johanandren that doing both the 'provide power api' and the 'support play api' in one PR might make this a rather large one, that thus will take more time/effort/cycles to review and get to a point where it can be merged. On the other hand I can see how those 2 are rather intertwined in the current PR.

My main concern with the approach in this PR is that you have basically duplicated the implementation of the Router and Handler with another model. That can be fine, but in akka-grpc we're already also duplicating the generated code across Scala and Java implementations/models - so this would eventually lead to 4 variations of each component.

I wonder if we could achieve the same by 'wrapping' the Akka HTTP-based infrastructure with a Play API 'conversion': this has the advantage that this 'wrapping' logic could eventually be moved over to Play itself and be generally used to serve Akka HTTP routes in Play applications. This might also untangle things a bit, so for example the 'provide power api' feature could indeed be more independent from the 'support play api' feature, and presented as independent PR's.

What do you think?

sbt-plugin/src/main/scala/akka/grpc/sbt/AkkaGrpcPlugin.scala

raboof · 2018-10-09T08:59:18Z

codegen/src/main/twirl/templates/PlayScala/HandlerUsingActions.scala.txt

+      import @{service.name}.Serializers._
+
+      def handle(request: RequestHeader, method: String): EssentialAction = method match {
+        @for(method <- service.methods) {


This seems to sort of duplicate the logic in the @{serviceName}Handler. Would it be possible to implement this as a 'wrapper around' @{serviceName}Handler instead?

As discussed in #420, I'm fine with adding some code to akka-grpc to handle this 'for the time being', but for the longer term I'd like this 'wrapping' to happen in Play rather than in Akka gRPC (on 2.x by adding the 'wrapping' there, and hopefully this can 'evaporate' for Play 3).

Implementing this 'by wrapping' in akka-grpc for now will make it easier to transition that code to Play eventually.

Perhaps if we do the 'wrapping' in the Router we don't need a separate Hander implementation anymore?

I'll investigate this, but given the Akka gRPC handler returns completed HttpResponses whereas the Play one returns EssentialActions that need further processing including from filters before they generate a response, not sure wrapping is gonna work.

Right, so the wrapping would take the HttpResponse generated by the generic handler, and convert it to an EssentialAction to be passed through further filters. Could that work?

Yep it looks like something along these lines is doable.

raboof · 2018-10-09T09:04:26Z

codegen/src/main/scala/akka/grpc/gen/scaladsl/play/PlayScalaServerCodeGenerator.scala

+  private val generateRouterUsingActions: (Logger, Service) => CodeGeneratorResponse.File = (logger, service) => {
+    val b = CodeGeneratorResponse.File.newBuilder()
+    b.setContent(RouterUsingActions(service, powerApis).body)
+    b.setName(s"${service.packageDir}/Abstract${service.name}RouterUsingActions.scala")


Since we're already generating either the router implemented with Actions or the router implemented with Akka HTTP models, wouldn't it make sense to generate it with the same name? That way for the end user they should be able to switch between 'backends' without further code changes, right?

Sure, it'd be less work, but as of now the Action router takes an extra constructor parameter.

raboof · 2018-10-09T09:10:45Z

runtime/src/main/scala/akka/grpc/scaladsl/scaladsl.scala

+import scala.collection.immutable
+
+package object scaladsl {
+  type MetadataMap = immutable.Map[String, Seq[MetadataEntry]]


Hmm, so you're promoting the previously-internal MetadataEntry types to the DSL. Why? Wouldn't it be OK to keep the existing Metadata facade?

The existing Metadata facade is a super limited interface that is inadequate for "real" use by service implementers. It only provides getText() and getBinary() methods returning optional values if given keys exist. It doesn't provide a means for getting all the keys nor for getting multi-valued keys. So some kind of richer interface is needed, and per discussion on #179 I kept the new-code surface small by using a Map and reusing MetadataEntry. If you want the latter to be kept internal, how would you suggest providing this richer API?

Instead of exposing MetadataEntry we could also consider adding additional methods to the Metadata API. I don't have a strong preference at this point, though.

Keeping a simple facade gives us more freedom to change implementation though and optimize for performance etc. I think having the facade makes sense, having an explicit Metadata.toSeq to get the collection capabilities for example would perhaps allow for the more advanced use cases?

raboof · 2018-10-16T09:35:59Z

codegen/src/main/twirl/templates/ScalaServer/PowerApiTrait.scala.txt

+
+import io.grpc.Status
+
+trait @{service.name}PowerApi extends @{service.name} {


In the future we'd want to allow finer-grained configuration to determine which methods should have the power API and which methods have the simple API. In that case perhaps PowerApi is not a great name - though I'm not sure about alternatives. This is OK for now.

…apis2

…ettings

ctoomey · 2018-10-29T06:43:23Z

I've updated AkkaGrpcPlugin to include the additional server-side code generation options into akkaGrpcCodeGeneratorSettings as requested.

ctoomey · 2018-11-06T18:11:44Z

@raboof @johanandren: I've made the requested changes so am ready to look at supporting java and the other build tools.

Please confirm you're good with this moving forward.

johanandren

I think the general approach is good, but added some comments on details.

codegen/src/main/scala/akka/grpc/gen/scaladsl/ScalaServerCodeGenerator.scala

runtime/src/main/scala/akka/grpc/internal/PlayRouterUsingActions.scala

johanandren · 2018-11-07T14:25:17Z

codegen/src/main/twirl/templates/ScalaServer/Handler.scala.txt

+        @for(method <- service.methods) {
+        case "@method.grpcName" =>
+          val responseCodec = Codecs.negotiate(request)
+          @{if(powerApis) { "val metadataMap = MetadataMap(request.headers)" } else { "" }}


Now this is eager for all power APIs even if they don't actually read the metadata. An actual type inbetween could lessen that to just one object allocation, and then parse the headers into our datatypes only if they are accessed.

Reverted to using an enhanced version of the Metadata trait, see runtime/src/main/scala/akka/grpc/scaladsl/Metadata.scala.

johanandren · 2018-11-07T14:29:36Z

runtime/src/main/scala/akka/grpc/scaladsl/scaladsl.scala

+import scala.collection.immutable
+
+package object scaladsl {
+  type MetadataMap = immutable.Map[String, Seq[MetadataEntry]]


Keeping a simple facade gives us more freedom to change implementation though and optimize for performance etc. I think having the facade makes sense, having an explicit Metadata.toSeq to get the collection capabilities for example would perhaps allow for the more advanced use cases?

johanandren

Good, added some detail feedback.

johanandren · 2018-11-12T08:29:06Z

runtime/src/main/scala/akka/grpc/scaladsl/Metadata.scala

+    entries
+  }
+
+  override def getText(key: String): Option[String] = headers.find(_.name == key).map(_.value)


I guess these may be from the original implementation, however collectFirst { case header if header.name == key => header.value } would avoid an extra option and function allocation here

johanandren · 2018-11-12T08:32:24Z

runtime/src/main/scala/akka/grpc/scaladsl/scaladsl.scala

-      entries
-    }
-  }
+  type MetadataMap = Map[String, List[MetadataEntry]]


I don't think we should have a type alias as public API, and the signature isn't that messy, so let's just use that instead.

codegen/src/main/scala/akka/grpc/gen/scaladsl/ScalaServerCodeGenerator.scala

johanandren · 2018-11-12T08:36:06Z

runtime/src/main/scala/akka/grpc/scaladsl/Metadata.scala

+        entries += (key -> (entry :: Nil))
+    }
+    entries
+  }


I wish we could do this with the mutable Map.newBuilder instead of creating a new map for each header entry. Not sure that is doable though.

Ya, not with having to group by header name.

I think we could create the map by folding over the headers, but I guess that's an implementation detail, OK for now.

That wouldn't help because we'd still pass a map along and update, so it would just be a different way of doing the same.

johanandren · 2018-11-12T08:38:22Z

runtime/src/main/scala/akka/grpc/scaladsl/Metadata.scala

+}
+
+class MetadataImpl(headers: immutable.Seq[HttpHeader] = immutable.Seq.empty) extends Metadata {
+  lazy private val b64Decoder = Base64.getDecoder


This one is just returning a static final field in java.util.Encoder no need to introduce a lazy val for that.

ctoomey · 2019-01-22T07:05:39Z

Note: I've finished the Java implementation of the power APIs and the maven plugin update, so AFAIK the only things left are the gradle plugin and updating the doc.

johanandren · 2019-01-25T10:15:15Z

I haven't done a thorough review (traveling) but just wanted to say that this is shaping up nicely!

ctoomey · 2019-02-05T03:19:00Z

Hi all, I've added documentation for the server power APIs and that completes everything that was left to do AFAIK. Please review so we can finally get it merged and I can move on to similarly finishing #447 .

johanandren

Some minor comments added, looks good in general.

codegen/src/main/scala/akka/grpc/gen/Main.scala

codegen/src/main/scala/akka/grpc/gen/scaladsl/ScalaTraitCodeGenerator.scala

codegen/src/main/scala/akka/grpc/gen/scaladsl/play/PlayScalaServerCodeGenerator.scala

docs/src/main/paradox/server/walkthrough.md

johanandren

LGTM, thanks for all the work on this!

johanandren · 2019-02-07T12:34:59Z

We'll want a final LGTM from @raboof as well but he is on vacation right now.

ctoomey · 2019-02-07T19:31:34Z

Thanks @johanandren, I hope @raboof comes back from vacation in a merging mood :-).

I'm working on getting #447 finished up now.

raboof

Very cool, sorry to have been a bottleneck here ;).

I think this looks really good! Seems complete in terms of API parity, maven/gradle/sbt support, docs and examples!

@ctoomey many thanks for your perseverance adding this feature to the project!

codegen/src/main/scala/akka/grpc/gen/Main.scala

ctoomey · 2019-02-25T18:58:11Z

Yay, thanks @raboof! Happy to be able to contribute back our enhancements to this important project.

Draft implementation

6d90c85

ctoomey added 2 commits October 8, 2018 14:06

Fix missing content-type header in Play response

b983c53

Fixed missing Content-Type header

ff4f0d1

ctoomey mentioned this pull request Oct 8, 2018

Set Content-Type response header #428

Closed

raboof reviewed Oct 9, 2018

View reviewed changes

ctoomey added 6 commits October 12, 2018 22:15

Ripped out Play changes

4a9515c

Cleanup

2d8872f

Remove PowerAPI from generated names

42a9431

Remove PowerAPI from generated names

502072b

Merge power API handling into handler template

fe75c41

Merge power API handling into handler template

73e9397

raboof reviewed Oct 16, 2018

View reviewed changes

ctoomey added 5 commits October 27, 2018 16:23

Merge remote-tracking branch 'upstream/master' into 179-server-power-…

fb86f38

…apis2

Integrate extended code generator options into akkaGrpcCodeGeneratorS…

747744e

…ettings

Integrate extended code generator options into akkaGrpcCodeGeneratorS…

54c863c

…ettings

Fix enum

f7ac227

Make GeneratorOption accessible

8a346af

ctoomey changed the title ~~Server-side power API #179, More idiomatic Play integration #420~~ Server-side power API #179 Oct 29, 2018

This was referenced Oct 29, 2018

More idiomatic play integration 420 livongo/akka-grpc#1

Closed

More idiomatic play integration #420 #447

Closed

johanandren reviewed Nov 7, 2018

View reviewed changes

ctoomey added 2 commits November 11, 2018 23:04

Latest changes requested on PR

d06be06

Latest changes requested on PR

2522ccc

johanandren reviewed Nov 12, 2018

View reviewed changes

Latest PR-requested updates

3a1a1b4

ctoomey added 9 commits January 22, 2019 11:47

Fix missing headers warning

190964f

Fix missing headers warning

fc1d9be

Gradle

0af4c9b

Build tool fixes

6a7b9a2

Merge branch 'master' into 179-server-power-apis2

b6684eb

Power API test for scala

ac44100

Power API test for java

18fd974

Power API test for java

6444b55

Power API test fixes for maven, gradle

707a5ae

amsjavan approved these changes Jan 25, 2019

View reviewed changes

RashadAnsari approved these changes Jan 26, 2019

View reviewed changes

ctoomey added 2 commits February 4, 2019 17:24

Power API documentation

e7481d1

Merge branch 'master' into 179-server-power-apis2

e2d5b68

johanandren reviewed Feb 6, 2019

View reviewed changes

ctoomey added 2 commits February 6, 2019 14:24

Merge branch 'master' into 179-server-power-apis2

e87129e

Power API tweaks

686aa5d

johanandren approved these changes Feb 7, 2019

View reviewed changes

ctoomey added 2 commits February 12, 2019 13:38

Updated Play router templates

cbfd17b

sbt Play generators moved back to akkaGrpcExtraGenerators

c8dba56

ctoomey added a commit to livongo/akka-grpc that referenced this pull request Feb 12, 2019

Latest changes from akka#429; Java, maven, gradle implementation

2b67abb

raboof approved these changes Feb 25, 2019

View reviewed changes

codegen/src/main/scala/akka/grpc/gen/Main.scala Show resolved Hide resolved

raboof merged commit 4449442 into akka:master Feb 25, 2019

raboof mentioned this pull request Feb 26, 2019

[WIP]: Fix code generation problems for Play #524

Closed

ctoomey mentioned this pull request Apr 1, 2019

Server-side power API #179

Closed


		import io.grpc.Status

		trait @{service.name}PowerApi extends @{service.name} {

Server-side power API #179 #429

Server-side power API #179 #429

Conversation

ctoomey commented Oct 7, 2018

akka-ci commented Oct 7, 2018

johanandren commented Oct 9, 2018

raboof commented Oct 9, 2018

raboof left a 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

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

ctoomey commented Oct 29, 2018

ctoomey commented Nov 6, 2018

johanandren left a 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

johanandren left a 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

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

ctoomey commented Jan 22, 2019

johanandren commented Jan 25, 2019

ctoomey commented Feb 5, 2019

johanandren left a comment

Choose a reason for hiding this comment

johanandren left a comment

Choose a reason for hiding this comment

johanandren commented Feb 7, 2019

ctoomey commented Feb 7, 2019 • edited Loading

raboof left a comment

Choose a reason for hiding this comment

ctoomey commented Feb 25, 2019

ctoomey commented Feb 7, 2019 •

edited

Loading