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

Decoupling API handler traits from server routes? #233

Open
dsilvasc opened this issue Apr 11, 2019 · 7 comments
Open

Decoupling API handler traits from server routes? #233

dsilvasc opened this issue Apr 11, 2019 · 7 comments
Labels
core Pertains to guardrail-core enhancement Functionality that has never existed in guardrail help wanted Easy to moderately difficult issues that don't require deep knowledge or architectural thought

Comments

@dsilvasc
Copy link
Contributor

Guardrail generates a Handler trait and akka-http server routes in the same file. Would it make sense to decouple them so that a handler implementation could be used for either akka-http, http4s, or any other HTTP server?

@dsilvasc
Copy link
Contributor Author

For context here: we tend to organize our services as a few gradle modules (subprojects). For example, given a Hello service specified in either protobuf or swagger, we would have approximately the following gradle modules:

  • :hello:swagger produces guardrail-generated code from a swagger.yml api definition.
  • :hello:proto produces a protoc-generated service interface if Hello is a protobuf service. We use a custom protoc plugin to generate scala traits similar to twirp.
  • :hello:service implements the trait/interface generated by either protoc or guardrail. Contains domain logic but doesn't care how it's served (could be akka-http, could be finagle, could be aws lambda, etc). Most of the code goes here.
  • :hello:app binds the service implementation to a server (usually akka-http), wires dependencies, and provides a main entry point (an object extending scala.App). This is what we deploy.
  • :hello:client is a scala client generated from the service spec, plus endpoint config.

I noticed today that our guardrail-based services were pulling in akka-http and friends in the service module, hence the question above.

@blast-hardcheese
Copy link
Member

No objections, especially considering that split was required to support Java classes. One challenge is that Handlers for akka-http and http4s are actually different types, as http4s handlers accept F[_]: Async whereas akka-http doesn't know anything about this. Additionally, respond is slightly different between the different libraries, as each library uses the casing (OK vs Ok) as well as available status codes per library.

Unifying these features isn't a bad idea, just explaining why it doesn't exist today.

I don't know that much about how Async[_] works with Future, but based on reading typelevel/cats-effect#169, it seems as though this is not supported by cats-effect as a feature, so this seems like a nontrivial change. Despite this, I'm intrigued, as even using akka-http with Route.asyncHandler in our tests, I've found it's a challenge to represent Gen[Client] effectively when using scalacheck.

Do you have any thoughts around this?

@dsilvasc
Copy link
Contributor Author

Having a handler that expects F[_] instead of Future might actually be better for us. Most of our code tends to use an internal implementation of a Task abstraction (sort of similar to either monix Task or cats IO) so it would be nice to keep implementing our services in terms of our tasks and separately provide an instance of whichever typeclass the handler or the route needs.

If Async isn't the right abstraction, guardrail could define its own

trait Scheduler[F[_]] {
  def run[T](asyncWork: F[T])(implicit ec: ExecutionContext): Future[T]
}

Though I don't know if Scheduler would be an appropriate name for that.

@blast-hardcheese
Copy link
Member

That seems reasonable; so, the constraint of F[_]: Async would be moved out of the Handler trait and into routes? @hanny24, will this create any concerns for idiomatic usage in http4s?

This would go a long way towards the goal.

One other hurdle that would need to be overcome is currently, the frameworks generated are http4s or akka-http, not http4s+handler and akka-http+handler, so a config change would be required to say:

"Only generate the handler and protocol, leave the server out"

... coupled with another config change to specify:

"Use the handler that was generated in this package, as well as the protocols that were generated in this package"

I'd be interested to hear how you'd like to consume this in your project configuration

@blast-hardcheese
Copy link
Member

Oh, I should have looked before I asked. I just saw that Handler in http4s is already defined as

      trait StoreHandler[F[_]] {
        def getRoot(respond: GetRootResponse.type)(): F[GetRootResponse]
        def getFoo(respond: GetFooResponse.type)(): F[GetFooResponse]
        def getFooDir(respond: GetFooDirResponse.type)(): F[GetFooDirResponse]
      }

so this is already how http4s works, we'd just need to adapt akka-http's Handler to this style.

@dsilvasc
Copy link
Contributor Author

Maybe we don't even need to run the generator twice. If the akka-specific code goes into one directory and the handler trait + definition classes go elsewhere (for instance as different java/scala packages), then gradle/sbt/bazel users can compile them as separate modules by setting different source directory patterns.

For example in gradle:

hello/swagger/build.gradle.kts

val generatedPackageName = "com.yourproject.hello.api"
val guardrail = configurations.create("guardrail")
dependencies.add("guardrail", "com.twilio:guardrail_2.12:0.46.0")
val generate = tasks.register("generate", JavaExec::class) {
  inputs.files(project.file(specPath))
  outputs.dir(generatedSrcDir)
  main = "com.twilio.guardrail.CLI"
  classpath = guardrail
  args = listOf(
    "--defaults", "--import", "scala.language.higherKinds", "--import", "akka.NotUsed", "--server",
    "--specPath", specPath, "--outputPath", generatedSrcDir,
    "--packageName", generatedPackageName)
}

hello/interface/build.gradle.kts

plugins {
  scala
  `java-library`
}
dependencies {
  api(thirdParty.circeCore)
}
// compile the handler trait and definitions
java {
  sourceSets {
    this.getByName("main") {
      // https://github.com/gradle/kotlin-dsl/issues/650
      withGroovyBuilder {
       "scala" {
          "srcDir"("../swagger/build/generated")
          "include"("**/definitions/**")
          "include"("**/*Handler.scala")
        }
      }
    }
  }
}
tasks.named("scalaCompile").configure { dependsOn(":hello:swagger:generate") }

hello/routes/build.gradle.kts

plugins {
  scala
  `java-library`
}
dependencies {
  api(thirdParty.akka_http)
}
// compile the akka-http routes, implicits, etc
java {
  sourceSets {
    this.getByName("main") {
      // https://github.com/gradle/kotlin-dsl/issues/650
      withGroovyBuilder {
       "scala" {
          "srcDir"("../swagger/build/generated")
          "include"("**/akka/**")
        }
      }
    }
  }
}
tasks.named("scalaCompile").configure { dependsOn(":hello:swagger:generate") }

@hanny24
Copy link
Contributor

hanny24 commented Apr 12, 2019

Generally speaking, I feel that separation of Handler and server itself is a good thing. On the other hand, priority of this is quite low for us.

Handler as it is generated for http4s can be adapted for akka quite easily. Your akka server would just require Handler[Future] instead of generic F. If you want want be really fancy, we could generate something like FunctorK. That would allow people to impement that domain code in whatever F they need (so monix Task, Twitter's Future, or even the internal implementation @dsilvasc mentioned). However from my experience concept of FunctorK confuses people. Also, since we are not using akka, we don't have real use case for that, so again, priority of this is very low to us.

@blast-hardcheese blast-hardcheese added core Pertains to guardrail-core enhancement Functionality that has never existed in guardrail help wanted Easy to moderately difficult issues that don't require deep knowledge or architectural thought labels May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Pertains to guardrail-core enhancement Functionality that has never existed in guardrail help wanted Easy to moderately difficult issues that don't require deep knowledge or architectural thought
Projects
None yet
Development

No branches or pull requests

3 participants