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 Trailers.apply to construct from code, message, and details #1933

Conversation

alexklibisz
Copy link
Contributor

The handler generated by akka-grpc takes an error handler in the form of eHandler: ActorSystem => PartialFunction[Throwable, Trailers].

This change just adds a method to make it easier to construct an instance of Trailers containing rich error details, so that we can enrich the error response from within the error handler.

Couple things I'm not sure about in the initial implementation:

  1. This implementation delegates to the analogous GrpcServiceException.apply method. Is there a preferred way to would abstract out the common code? I.e., create a Utils object somewhere.
  2. The runtime/test submodule does not seem to import any instances of GeneratedMessage, so I'm not sure how to test that we're encoding details correctly. Should we modify the subproject so that it imports LocalizedMessage? Should we put the tests in another submodule.

Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

A lot of the test coverage is in the akka-grpc-plugin-tester-{java,scala} modules which have more access to actual message generation etc. so maybe sneak in some coverage there in LoggingErrorHandlingGreeterServer.java and LoggingErrorHandlingGreeterServer.scala. Those also has snippets that goes in the docs which may be useful to show off the new signature. For example the custom-error-mapping snippet.

Looks like there is no actual test calling those, but since this mostly adds convenience and no real logic having it used in something that is compiled as a part of the test suite is probably enough.

@alexklibisz
Copy link
Contributor Author

I added the create method in ed7a913

And added an example to the LoggingErrorHandlingGreeterServer in 1885652.

If that example looks good, I can do the same on the Java side.

@johanandren
Copy link
Member

That example looks good, please add for Java as well 👍

@alexklibisz
Copy link
Contributor Author

Ran into an issue in the Java example. Commented inline.

@alexklibisz
Copy link
Contributor Author

I'm seeing an error in the Gradle build: https://github.com/akka/akka-grpc/actions/runs/8851059859/job/24306757026?pr=1933#step:9:131

Error:  /home/runner/work/akka-grpc/akka-grpc/plugin-tester-scala/src/main/scala/example/myapp/helloworld/LoggingErrorHandlingGreeterServer.scala:89: class com.google.rpc.LocalizedMessage is not a value

on the line:

Trailers(com.google.rpc.Code.INVALID_ARGUMENT, ex.getMessage, List(LocalizedMessage("en-US", ex.getMessage)))

I guess it's possible that gradle isn't configured to know about the scalapb-generated LocalizedMessage. 🤔

@alexklibisz
Copy link
Contributor Author

@johanandren This is ready for another look. I might need a hint on how to get the Gradle Scala build working.

@johanandren
Copy link
Member

The plugin-tester-scala is used for verifying building Scala projects with gradle and maven in addition to the "normal" sbt, so those two project files needed some ammends, I've pushed fixes that should sort it.

Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Tricker than you'd think at first.

@johanandren johanandren merged commit 9ea98ed into akka:main Apr 30, 2024
12 checks passed
@johanandren johanandren added this to the 2.4.2 milestone Apr 30, 2024
@alexklibisz alexklibisz deleted the alexklibisz-trailers-from-code-message-details branch April 30, 2024 21:57
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

Successfully merging this pull request may close these issues.

2 participants