feat: custom failure status code support #847
Conversation
There was a problem hiding this comment.
Nice! Refs #830
Limitation of this implementation is that you can only specify an error code, but not add a customer 'message' or additional protobuf-serialized 'details' objects. Seems fine to me.
Some comments for consideration and one probably accidentally-committed change, otherwise LGTM!
sdk/java-sdk/src/main/java/com/akkaserverless/javasdk/GrpcStatus.java
Outdated
Show resolved
Hide resolved
| * @return An error reply. | ||
| * @param <T> The type of the message that must be returned by this call. | ||
| */ | ||
| <T> Effect<T> error(String description, GrpcStatus status); |
There was a problem hiding this comment.
I guess in the future we might introduce yet another variant to also be able to control the HTTP status code (when transcoding), but this is 👍.
| * @return | ||
| * An error reply. | ||
| * @tparam T | ||
| * The type of the message that must be returned by this call. | ||
| */ | ||
| def error[T](description: String): Effect[T] | ||
| def error[T](description: String, status: Option[GrpcStatus] = None): Effect[T] |
There was a problem hiding this comment.
If we want to support controlling HTTP status codes through this API as well, that default parameter might make it harder to evolve in a binary compatible way - but I guess for the SDK we care mainly about source compatibility and not so much about bincompat?
There was a problem hiding this comment.
For the most commonly used HTTP status codes, there's a corresponding gRPC status code that we map to it in the HTTP transcoding.
But yes, my thinking here was that source compatibility is the most important thing.
The existing |
Of course, forgot about that. In that case it might be neat to have an
Yeah, there's a convention to add a |
|
@jroper Do you intend to finish this, or should someone else take over? |
|
Yes I'll finish it. |
Added support for custom status codes to both Java and Scala SDKs.
| def error[S](description: String): Action.Effect[S] = ErrorEffect(description, Nil) | ||
| def error[S](description: String): Action.Effect[S] = ErrorEffect(description, None, Nil) | ||
| def error[S](description: String, statusCode: Status.Code): Action.Effect[S] = { | ||
| if (statusCode.toStatus.isOk) throw new IllegalArgumentException("Cannot fail with a success status") |
| Future.successful( | ||
| ActionResponse( | ||
| ActionResponse.Response.Failure(Failure(description = description)), | ||
| ActionResponse.Response.Failure( | ||
| Failure(description = description, grpcStatusCode = status.map(_.value()).getOrElse(0))), |
There was a problem hiding this comment.
ok, 0 does not mean SUCCESS but 'unspecified' here, an will be converted to UNKNOWN in the proxy. Not super obvious but 👍
Added support for custom status codes to both Java and Scala SDKs.
This is to match feature parity with lightbend/kalix-javascript-sdk#253.