concat helper method to chain together multiple service implementations, #59 #256
Conversation
There was a problem hiding this comment.
I think the utility looks useful.
Looking at it again I wonder if it wouldn't make sense to have it in akka-http-core even.
As for naming, though we started with it perhaps 'concat' (as we've called it before) isn't great since unlike Route.concat it also ends with returning a 404 (which I think is useful). WDYT?
| import akka.http.javadsl.model.HttpRequest | ||
| import akka.http.javadsl.model.HttpResponse | ||
| import akka.http.javadsl.model.StatusCodes | ||
| import akka.japi.{Function => JFunction} |
There was a problem hiding this comment.
Did you find a reason not to use java.util.function.Function here?
There was a problem hiding this comment.
I'll change that occurrence also together with the other one
There was a problem hiding this comment.
I'll do that in this PR
| result.completeExceptionally(new UnsupportedOperationException("Unexpected path")); | ||
| return result; | ||
| } else { | ||
| return notFound; |
There was a problem hiding this comment.
changed this to NOT_FOUND because I guess that's a more valid response if a single handler is used, and felt better to use that as the signal to continue with next handler in concat rather than using exception
| val service: HttpRequest => Future[HttpResponse] = GreeterServiceHandler(new GreeterServiceImpl(mat)) | ||
| .orElse { case _ => Future.successful(HttpResponse(StatusCodes.NotFound)) } | ||
| val service: HttpRequest => Future[HttpResponse] = | ||
| ServiceHandler(GreeterServiceHandler(new GreeterServiceImpl(mat))) |
There was a problem hiding this comment.
would a single implementation be "wrong" if used without the NotFound?
val service: HttpRequest => Future[HttpResponse] = GreeterServiceHandler(new GreeterServiceImpl(mat))
It might be easy to miss that?
Should we change GreeterServiceHandler.apply to be a total function appending the NotFound by default, and make the partial function another method? GreeterServiceHandler.partial?
There was a problem hiding this comment.
Then the ServiceHandler.apply that I added wouldn't be needed
| private val notFound = Future.successful(HttpResponse.create().withStatus(StatusCodes.NOT_FOUND)) | ||
|
|
||
| /** | ||
| * Creates a `HttpRequest` to `HttpResponse` handler that can be used in for example `Http.get(system).bindAndHandleAsync` |
There was a problem hiding this comment.
maybe as [[]] links? (the response/request)
I think that for javadsl it's essential, "impossible" to compose several otherwise |
|
| @@ -24,18 +24,17 @@ import akka.grpc.javadsl.GrpcExceptionHandler; | |||
| import static @{service.packageName}.@{service.name}.Serializers.*; | |||
|
|
|||
| public class @{service.name}HandlerFactory { | |||
| private static CompletionStage<HttpResponse> notFound = CompletableFuture.completedFuture( | |||
| HttpResponse.create().withStatus(StatusCodes.NOT_FOUND)); | |||
There was a problem hiding this comment.
Responding with an error immediately instead of going through the GrpcExceptionHandler seems nice, but we'd want to return a GrpcResponseHelpers.status(Status.UNIMPLEMENTED) at least for missing methods
There was a problem hiding this comment.
yes, this is for the path segments, the method match is still using UnsupportedOperationException
| @@ -24,18 +24,17 @@ import akka.grpc.javadsl.GrpcExceptionHandler; | |||
| import static @{service.packageName}.@{service.name}.Serializers.*; | |||
|
|
|||
| public class @{service.name}HandlerFactory { | |||
| private static CompletionStage<HttpResponse> notFound = CompletableFuture.completedFuture( | |||
| new JFunction[HttpRequest, CompletionStage[HttpResponse]] { | ||
| override def apply(req: HttpRequest): CompletionStage[HttpResponse] = | ||
| cont(req, notFound, handlers.toList).toJava | ||
| } |
There was a problem hiding this comment.
I see, since we generate functions and not javadsl.Route, looks good
Sounds good to me (from an consistency with akka-http naming style perspective) |
|
Updated with the things we discussed so far. Will continue with test and docs |
|
Added documentation and test. This should be ready for final review now. |
|
and the reason for the japi.Function was that bindAndHandleAsync expected that, so I reverted that change |
| @@ -0,0 +1,49 @@ | |||
| /** | |||
| * Copyright (C) 2018 Lightbend Inc. <http://www.lightbend.com> | |||
| */ | |||
There was a problem hiding this comment.
I think sbt-headers expects a newline after this
| val greeterHandler = GreeterServiceHandler(new GreeterServiceImpl) | ||
| val echoHandler = EchoServiceHandler.partial(new EchoServiceImpl) | ||
| val greeterHandler = GreeterServiceHandler.partial(new GreeterServiceImpl) | ||
| val serviceHandler = ServiceHandler.concatOrNotFound(echoHandler, greeterHandler) |
| GreeterServiceHandlerFactory.create(new GreeterServiceImpl(mat), mat); | ||
| Function<HttpRequest, CompletionStage<HttpResponse>> echoService = | ||
| EchoServiceHandlerFactory.create(new EchoServiceImpl(), mat); | ||
| Function<HttpRequest, CompletionStage<HttpResponse>> serviceHandlers = |
| }); | ||
| } | ||
|
|
||
| private static HttpsConnectionContext serverHttpContext() throws Exception { |
There was a problem hiding this comment.
Yes, finding a nicer alternative for that is part of #89
There was a problem hiding this comment.
yes, it should hurt your eyes so that we make it happen :)
| * | ||
| * Use `@{service.name}Handler.apply` if the server is only handling one service. | ||
| */ | ||
| def partial(implementation: @service.name)(implicit mat: Materializer): PartialFunction[HttpRequest, Future[HttpResponse]] = { |
patriknw
force-pushed
the
wip-59-concat-patriknw
branch
from
9a8a900 to
1f14611
Compare
|
Fixed headerCheck, rebased and squashed |
I have to test and add documentation, but wanted to see if you agree with the approach first.
Refs #59