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

[Feature request] Better Akka integration #748

Closed
erikvanoosten opened this issue Sep 12, 2020 · 11 comments
Closed

[Feature request] Better Akka integration #748

erikvanoosten opened this issue Sep 12, 2020 · 11 comments

Comments

@erikvanoosten
Copy link
Contributor

The way Tapir is currently integrated with Akka leads to issues like #111 . The main problem is that we can't use Akka directives after the route has been recognized, but before the logic is executed. This is a proposal that does allow this.

The API would be used as follows:

endpoint.toDirective { completion =>
  securityDirective {
    metricDirective {
      completion(logic _)
   }
 }
}

alternative formulation:

(endpoint.toDirective & securityDirective & metricDirective) { completion =>
  completion(logic _)
}

This is the API that would allow this:

implicit class RichAkkaHttpEndpoint[I, E, O](e: Endpoint[I, E, O, AkkaStream]) {
  /**
    * Converts the endpoint to a directive that will do one of the following:
    * * `reject()` when the request does not match the endpoint
    * * `reject(ValidationRejection(errorMsg))` when the requests matches the endpoint but there are invalid parameters
    * * `provide` a directive that can be called to complete the request
    */
  def toDirective: Directive1[I => Future[Either[E, O]] => Route]
}

This is just an idea. I have not tried it out to see if type derivation plays nicely. If you like it, I can put a bit of time in it to see if it can be implemented.

@adamw
Copy link
Member

adamw commented Sep 12, 2020

Sure I think this could work. There already is a EndpointToAkkaDirective which probably would do part of the job. So the directive would check if the request matches, extract the input values, and return a function which, when provided with the server logic, would apply it to the extracted inputs?

And I think the type in toDirective should be: Directive1[(I => Future[Either[E, O]]) => Route], no?

@erikvanoosten
Copy link
Contributor Author

Yes, like that.
Thanks, indeed, there should be parenthesis around the first =>.

@erikvanoosten
Copy link
Contributor Author

erikvanoosten commented Sep 13, 2020

I also have a use case where the security directive needs access to the parsed parameters. Lets introduce a new api for that. For completeness lets give the directive the chance to change the input parameters before passing it to the logic.

implicit class RichAkkaHttpEndpoint[I, E, O](e: Endpoint[I, E, O, AkkaStream]) {
  /**
    * Converts the endpoint to a directive that will do one of the following:
    * * `reject()` when the request does not match the endpoint
    * * `reject(ValidationRejection(errorMsg))` when the requests matches the endpoint but there are invalid parameters
    * * `provide` a directive that can be called to complete (`C`) the request
    *
    * Example usage:
    * {{{
    * endpoint.toDirectiveC { completion =>
    *   securityDirective {
    *     completion(logic _)
    *   }
    * }
    * }}}
    */
  def toDirectiveC: Directive1[(I => Future[Either[E, O]]) => Route]

  /**
    * Converts the endpoint to a directive that will do one of the following:
    * * `reject()` when the request does not match the endpoint
    * * `reject(ValidationRejection(errorMsg))` when the requests matches the endpoint but there are invalid parameters
    * * `provide` the input parameters (`I`) and a directive that can be called to complete (`C`) the request
    *
    * Example usage:
    * {{{
    * endpoint.toDirectiveIC { (input, completion) =>
    *   // use `input` here
    *   securityDirective {
    *     completion(logic(input))
    *   }
    * }
    * }}}
    */
  def toDirectiveIC: Directive[(I, Future[Either[E, O]] => Route)]
}

WDYT? Keep both, or just the more powerful toDirectiveIC?

@adamw
Copy link
Member

adamw commented Sep 16, 2020

Thanks! Out of curiosity - what is missing in tapir that would allow you to replace the nested directives?

@erikvanoosten
Copy link
Contributor Author

We have a few security directives that handle authorization. They read some request headers and set a few response headers. Also they need some of the input parameters. I don't know if this can be done through tapir. If we could, I guess the endpoint definitions would be fairly complicated. Secondly, we have the directives already.

These directives need to be nested inside and not around the endpoint route. If they were outside we could get a reject(forbidden) which would eventually lead to a 403 instead of a 404 when no route matches at all.

With the proposal in #751 combining tapir endpoints and directives, nested or around, becomes trivial.

Do you think it brings too much flexibility?

@adamw
Copy link
Member

adamw commented Sep 16, 2020

No, I think the change is very interesting, was just wondering if there's something in tapir that we could add to make such situations easier. I'm gathering use-cases to see what needs to be covered, if we tried to come up with a solution

@erikvanoosten
Copy link
Contributor Author

A colleague wrote https://github.com/kolov/pepper . It describes how we tackled the same problem with Tapir and Http4s. Reality was a bit more complex still (we need to set response headers) and we refactored it a bit more afterwards. But it indicates that solving this in the realm of Tapir is very complex.

@adamw
Copy link
Member

adamw commented Sep 17, 2020

@erikvanoosten Pepper looks interesting - thanks for the pointer! I'll have to take a deeper look at it :)

@adamw
Copy link
Member

adamw commented Sep 17, 2020

Released in 0.17.0-M1, thanks!

@adamw
Copy link
Member

adamw commented Mar 16, 2022

Closing as Akka's integration design generally won't change much. If there are some features that cannot be handled by tapir's constructs (endpoints, interceptors etc.), please open separate issues

@adamw adamw closed this as completed Mar 16, 2022
@erikvanoosten
Copy link
Contributor Author

With #751 merged many features can already be handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants