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

add monad syntax and tests #1925

Closed
wants to merge 12 commits into from

Conversation

winitzki
Copy link

@winitzki winitzki commented Mar 9, 2018

Support the Scala for/yield syntax in route directives, simplify route code.

Writing routes in for/yield syntax

The default Akka-HTTP style is to use nested functions for defining routes, for example:

val myRoute: Route = get {
  path("test" / Segment) { userId =>
    val user = userById(userId)
    extractLog { log =>
      extractUri { uri =>
        parameter('x, 'y, 'z) { (x, y, z) =>
          log.info(s"Got URI $uri with parameters $x, $y, $z") // log this before parsing JSON
          entity(as[MyCaseClass]) { input =>
            // `computeAsFuture` is our business logic function returning a `Future`
            onSuccess(computeAsFuture(input, user, x, y, z)) { result =>
              complete(MyResultCaseClass(result))
            }
          }
        }
      }
    }
  }
}

Each of the functions get, path, extractLog, entity etc. are defined by the Akka-HTTP library as values of type Directive[L].
A Directive[L] has a tapply() method that takes as an argument a function of type L => Route and returns a Route.
Therefore, each directive in a route requires you to write one more nested function of type L => Route.

It also follows that a Directive[L] is equivalent to a value of type (L => Route) => Route, which is known in functional programming as the "continuation monad".
Being a monad, this type can be used in for/yield syntax, which is idiomatic in Scala, and avoids having to write several nested functions.
The only missing part is the methods map, flatMap, and withFilter as required by the for/yield syntax.

The class Directive[L] already has methods tmap, tflatMap, and tfilter but their type signatures are not suitable because of the type class constraint on L and because of using magnets.

This PR adds syntax helpers that enable the for/yield syntax for routes, so that the route above can be rewritten like this:

import DirectiveMonad._

val myRoute: Route = for {
  _         <- get
  userId    <- path("test" / Segment)
  user      =  userById(userId)
  log       <- extractLog
  uri       <- extractUri
  (x, y, z) <- parameter('x, 'y, 'z)
  _         =  log.info(s"Got URI $uri with parameters $x, $y, $z") // log this before parsing JSON
  input     <- entity(as[MyCaseClass])
  result    <- onSuccess(computeAsFuture(input, user, x, y, z))
} yield MyResultCaseClass(result) // no need for `complete()`

All Akka-HTTP directives can be used without any changes in this syntax.
Directive operations such as & can be used as well, in the right-hand side of <-.

The new syntax is added using a zero-allocation wrapper, WrapDirective[L], which can be created out of a Directive[L] or a Directive1[L] automatically via an implicit conversion.
There are also implicit conversions from WrapDirective[L] back to Directive[L] and Directive1[L] for convenience.

Some comments:

  • Directives that appear to have no arguments (such as get) need to be used with the syntax _ <- get (these directives are of type Directive0 and have an argument of type Unit).
  • Ordinary (non-directive) computations within the for block are written as a = b instead of val a = b.
  • Computations returning Unit, such as log.info(...), are written as _ = log.info(...).
  • Filtering conditions such as if x.length > 0 are permitted within the for block as usual; the route will be rejected if the condition fails.
  • To return a complete(xyz), just write yield xyz, where xyz must be a marshallable value (a string, a case class, or an http response.)
  • For convenience, yield can be also used on a Route, so that yield complete(xyz), yield reject(...), etc. are also supported.
  • Directive methods, such as recover(), & etc., can be used on a WrapDirective (i.e. on the result of for/yield) as well.

The advantages of this syntax:

  • the for/yield syntax is a standard, idiomatic Scala syntax for sequential computations
  • code is easier to read and refactor because it is not deeply nested
  • directives become composable in the usual way, as monadic values
  • further enrichment can be done using monad transformers

Example of refactoring several routes with a common prefix:

val commonDirectives = for {
  _ <- get
  _ <- pathPrefix("my_app")
} yield ()

val route1: Route = ???
val route2: Route = ???

val route: Route = for {
  _ <- commonDirectives
} yield route1 ~ route2

@akka-ci
Copy link

akka-ci commented Mar 9, 2018

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!

@ktoso ktoso self-requested a review March 9, 2018 01:26
@ktoso
Copy link
Member

ktoso commented Mar 9, 2018

Hmm, very interesting proposal, thanks!

I may not get to review it today but will soon

@ktoso
Copy link
Member

ktoso commented Mar 9, 2018

OK TO TEST

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR that is currently being validated by Jenkins labels Mar 9, 2018
@akka-ci
Copy link

akka-ci commented Mar 9, 2018

Test FAILed.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Mar 9, 2018
@akka-ci
Copy link

akka-ci commented Mar 9, 2018

Test FAILed.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Mar 9, 2018
@akka-ci
Copy link

akka-ci commented Mar 9, 2018

Test FAILed.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Mar 9, 2018
@akka-ci
Copy link

akka-ci commented Mar 9, 2018

Test PASSed.

@pocorall
Copy link

pocorall commented Apr 3, 2018

As I wrote in #1970, I hope to use for / yield in the middle of Directive1 chaining. For example:

def optionalCookieSession: Directive1[Option[(Session, User)]] =
  (for {
    sessionId <- cookie(SessionCookieName).map(_.value)
    if sessionId != ""
    SessionSearchResult(Some(session)) <- onSuccess(loginService ? FindSessionById(sessionId))
    UserSearchResult(Some(user)) <- onSuccess(userService ? FindUserById(session.userId))
  } yield Some(session -> user))
    .recover(_ => provide(None))

I tested the code above with this PR. But it shows the following error:

value recover is not a member of akka.http.scaladsl.server.directives.WrapDirective[Some[(com.xxx.Session, com.xxx.User)]]
 possible cause: maybe a semicolon is missing before `value recover'?
       .recover(_ => provide(None))

, clearly because of the return type of the for / yield is WrapDirective, not Directive1. Then, how about implement map and withFilter in akka.http.scaladsl.server.Directive directly?

Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think our 'nested' DSL is actually quite nice for this use case, also supporting for comprehensions as introduced here seems quite nice and not too heavy - thanks for the proposal!

I'm curious if it could be extended to support the example given by @pocorall

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Apr 9, 2018
@akka-ci
Copy link

akka-ci commented May 9, 2018

Test PASSed.

@lightbend-cla-validator

Hi @winitzki,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels May 10, 2018
@akka-ci
Copy link

akka-ci commented May 10, 2018

Test PASSed.

@winitzki
Copy link
Author

winitzki commented May 10, 2018

I agreed to the CLA. Not sure why this was not being checked before.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels May 14, 2018
@akka-ci
Copy link

akka-ci commented May 14, 2018

Test PASSed.

@johanandren
Copy link
Member

Not speaking for the entire team, but just to voice my opinion:

I think I'd prefer this to be something maintained outside of Akka HTTP. I don't think yet another way of doing things in vanilla Akka HTTP is a good idea, it is complicated enough as it is for newcomers (and intermediate users).

(I also don't personally find it a super good match, the routes for anything but super simple example cases are more of a tree than a sequential computation which doesn't match the sequential style well, but that is more my opinion so if this is what you want in your project, by all means, feel free to pursue it)

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels May 15, 2018
@akka-ci
Copy link

akka-ci commented May 15, 2018

Test PASSed.

@winitzki
Copy link
Author

winitzki commented May 15, 2018

In my view, routes should not necessarily be trees. The logic of a route is more or less linear: extract most of the code should be extracting various data from the request, then there might be some filtering based on validation, some logging or performance metrics, then there are calls to some functions to compute responses, and finally a response is returned. I think the API for routes and the code would have been a lot simpler, had Akka-HTTP used the monadic for/yield from the beginning. There would have been no need for Directive0, Directive1, Directive`, and no need for special-casing tuples of 1,2,3,..., elements.

Another advantage of a monad is that it is a standard and well-understood design pattern. Refactorings are straightforward. Also, for instance, you can apply a monad transformer and enrich the route with extra effects. For example, swagger annotations could be generated automatically by enriching the route directives with swagger meta-information.

@ktoso
Copy link
Member

ktoso commented May 16, 2018

Hi @winitzki,
I don't mean to be too dismisive of the PR, I think it's a great idea and showcase, but we have to weight it against the, what I call, "confusion factor" (for new people) i.e. "whoa there's so many APIs and styles, which one should I use".

In my view, routes should not necessarily be trees. The logic of a route is more or less linear: extract most of the code should be extracting various data from the request, then there might be some filtering based on validation, some logging or performance metrics, then there are calls to some functions to compute responses, and finally a response is returned.

They are trees however, and what you explained is a single traversal of such tree -- an depth-first-ish (-ish since rejections) traversal of the routes tree is indeed what routes basically do at runtime. However, in face of rejections, the traversal is not as simple as you painted it here. Even a path(...) { get {} ~ post {} } relies on this mechanism, same as do top level multiple pathPrefix directives.

had Akka-HTTP used the monadic for/yield from the beginning. There would have been no need for Directive0, Directive1, Directive`, and no need for special-casing tuples of 1,2,3,..., elements.

That could not have been the solution -- the reason we did so is in order to have feature parity with Java which is a hard, non negotiable, requirement for all of our APIs. An abstracted, using hlists, version of directives existed in Spray which became Akka HTTP, and one of the tradeoffs we had to make was this abstraction, among other things.

As for the general monad argument -- yes and no, I don't think it enchances composability over how routes already are able to compose. I do see that the API proposal can be nice, however the tradeoff is that we'd confuse newcomers even more, by providing yet another style inside the core library. This is not to say that the PR isn't cool and a great idea, I think it is, however I'd wage it against the increased "choices" issue which we continue to face on a day to day basis.

Having all that said... I wonder if you would be okey with releasing this directive as a separate library, and we would link to it in the official docs -- would this be a nice compromise? You'd also get exposure in the community that it's your cool monadic akka routes library etc :-)

@winitzki
Copy link
Author

winitzki commented May 16, 2018

I understand about the Java compatibility requirement. So perhaps another person in your team could look over this and weigh in, and if there are 3 people all saying that this functionality belongs outside the core library, so be it, let that person close the PR. I will make a small add-on library then.

@patriknw
Copy link
Member

Hi @winitzki

This is really impressive work and I understand that some people will find it attractive to define the routes with for comprehension. However, we wouldn't like to add confusion about how to use the DSL by adding another way to define the same thing. Questions like "when to use what?". Therefore I suggest that you release this as a small external library and we would be happy to include a reference to it from the Akka HTTP documentation.

@ktoso
Copy link
Member

ktoso commented May 30, 2018

Hi there, seems we're in agreement that this is very cool but best not in the main library -- however we're happy to link to your project once you send us a link or PR such PR that would add such docs :-)

Closing for now, thanks and I hope we can get that separate mini project running :-)

@ktoso ktoso closed this May 30, 2018
@winitzki
Copy link
Author

Thank you for the useful input that helped me understand where this API should be going. We are already using this for our production code at my day job. Perhaps once we see a few more use cases that require more development, I'll make this into a separate library. Especially, I would like to figure out whether monad transformers could be harnessed to generate swagger annotations or other stuff automatically from the route code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants