Adding support for custom headers #577

[drmontgomery]

References #577

Hi there!

I took a stab at adding support for returning custom HttpHeaders from a GrpcExceptionHandler.

I modified the GrpcExceptionHandler to return a case class (GrpcErrorResponse) that pairs an io.grpc.Status with an optional Seq of headers. These headers are then added to the trailing chunk by GrpcResponseHelpers.

Most of the complexity stems from needing to provide Java and Scala versions. I tried to follow the existing patterns, but I haven't done a lot of combined Java/Scala library development, so it's entirely possible I've missed something.

I also added HttpHeaders to GrpcServiceException. Unfortunately, because HttpHeader is defined in a language-specific DSL for akka.http, this required moving GrpcServiceException from akka.grpc into the language-specific DSLs as well. That feels like a pretty big API change for feature like this. Is there a better way to handle this?

Anyway, definitely open to feedback, but wanted to try to contribute to such a useful project.

Thanks,
-David

[lightbend-cla-validator]

Hi @drmontgomery,

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]

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!

[raboof]

OK TO TEST

[raboof]

I took a stab at adding support for returning custom HttpHeaders from a GrpcExceptionHandler. definitely open to feedback, but wanted to try to contribute to such a useful project.

Thanks a lot!

I modified the GrpcExceptionHandler to return a case class (GrpcErrorResponse) that pairs an io.grpc.Status with an optional Seq of headers. These headers are then added to the trailing chunk by GrpcResponseHelpers.

I'm on the fence here. The alternative would be to have the exception handler to return a collection of headers. Since the status also turns into a header, this would avoid introducing the GrpcErrorResponse concept. On the other hand, introducing GrpcErrorResponse makes it more clear that the status is required. Since bringing in custom error handling is a bit of a poweruser feature, I'm not sure what's best.

I also added HttpHeaders to GrpcServiceException. Unfortunately, because HttpHeader is defined in a language-specific DSL for akka.http, this required moving GrpcServiceException from akka.grpc into the language-specific DSLs as well. Is there a better way to handle this?

The alternative would be to keep GrpcServiceException language-agnostic, by giving it both Scala and Java API's. That is possible because of 2 facts: 1) the scaladsl HttpHeader extends from the javadsl HttpHeader (so each scaladsl HttpHeader can be safely used as javadsl header) and 2) it is impossible to construct a javadsl HttpHeader that is not also a scaladsl HttpHeader. While '2' is not something we want to expose to the end-user API, it does mean we can convert between the two in our implementation.

That feels like a pretty big API change for feature like this.

I'm not so concerned about the size of the API change (we are pre-1.0.0, after all), I think we should choose the best solution. I'm leaning towards keeping one GrpcServiceException and giving it both a Java and a Scala API. What do you think?

[drmontgomery]

Thanks for the quick feedback!

Re GrpcErrorResponse:

I agree that this is a bit of a poweruser feature, but returning a collection of headers feels a bit too low-level to me, for two reasons. First, it requires the library user to understand and reimplement the gRPC status encoding logic, which seems bad in terms of code duplication and overall library ergonomics. Second, it provides a mechanism by which the user could accidentally produce responses that don't follow the gRPC-HTTP2 spec (i.e. if they accidentally omit the grpc-status header), which seems like a core library responsibility.

I actually wonder if returning HttpHeaders at all is too low-level. From the perspective of a gRPC user, everything is metadata, not http headers. Using HttpHeaders requires them to understand the details of binary metadata encoding, etc. Should the exception handler be returning a status and a collection of (String, MetadataEntry) tuples instead?

Re GrpcServiceException:

I'm not sure I completely agree with the assertion that it is impossible to construct a javadsl HttpHeader that is not also a scaladsl HttpHeader.

The javadsl HttpHeader is an abstract class with no base class, which scaladsl HttpHeader subclasses. In practice, most of the useful javadsl header classes (like CustomHeader or RawHeader) derive from the scaladsl HttpHeader, but I don't see anything (like a DoNotInherit annotation) to prevent users from creating their own javadsl HttpHeader subclasses with no relationship to the scaladsl HttpHeader. I tried to take advantage of this in the GrpcExceptionHelper.asScala method, by attempting to cast the incoming javadsl HttpHeader as a scaladsl HttpHeader, and falling back to wrapping it in a RawHeader, but I don't think the cast is guaranteed to succeed.

When you envision the language-agnostic GrpcServiceException, are you thinking of something like the following?

class GrpcServiceException(val status: Status, val headers: Seq[sHttpHeader] = Nil) {
  def this(status: Status, headers: jIterable[jHttpHeader]) {
    this(status, headers.asScala.map(asScala).toSeq)
  }
}

Looking a bit more closely at Akka HTTP, it looks like the standard pattern for Java/Scala interop is to define a Java interface with a Scala implementation that derives it, and then use factory methods in the javadsl to create instances from the scaladsl. I'm violating that pattern with this change, since there is no relationship between the two GrpcServiceExceptions (or GrpcErrorResponse) for that matter. Would it make sense to move in that direction? The main quirk from a user perspective is that we'd have to disallow calling the constructor directly for javadsl classes, but it would simplify the internals slightly.

[drmontgomery]

I went ahead and made GrpcServiceException language-agnostic. Please take a look and confirm that my change matches your intent.

[raboof]

GrpcErrorResponse

returning a collection of headers feels a bit too low-level to me. it requires the library user to understand and reimplement the gRPC status encoding logic, which seems bad in terms of code duplication and overall library ergonomics

I agree we should not force users to reimplement that encoding - instead we could provide convenient helpers that produce headers based on a Status and further custom headers.

I actually wonder if returning HttpHeaders at all is too low-level. From the perspective of a gRPC user, everything is metadata, not http headers. Using HttpHeaders requires them to understand the details of binary metadata encoding, etc. Should the exception handler be returning a status and a collection of (String, MetadataEntry) tuples instead?

This could also largely be solved with helpers that take either String or ByteString data and produce the corresponding HttpHeader. I see the appeal of using the MetadataEntry concept since that is already introduced on the client side (though only in a 'corner' of the API there, as well...)

I'm not completely against introducing something like GrpcErrorResponse(Status, Map[String, MetadataEntry]), but having convenient helper methods that create plain HttpHeaders seems quite a bit simpler, and acceptable for an 'advanced' feature like this. I'm not sure the benefits of adding a GrpcErrorResponse runtime concept justify the added complexity.

GrpcServiceException

I don't see anything (like a DoNotInherit annotation) to prevent users from creating their own javadsl HttpHeader subclasses with no relationship to the scaladsl HttpHeader

It is suggested in the javadoc ("All actual header values will be instances of one of the subtypes defined in the headers packages. Unknown headers will be subtypes of RawHeader."), though I agree it would be helpful to clarify. Added akka/akka-http#2982.

I went ahead and made GrpcServiceException language-agnostic. Please take a look and confirm that my change matches your intent.

Yes, I think this looks good.

[raboof]

(some smaller code comments, generally looking healthy)

[drmontgomery]

I've also created two new branches in my repo to explore some of the options we've been discussing.

In the wip-rich-error-response-headers branch, I removed GrpcErrorResponse and replaced it with a list of http headers.

In the wip-rich-error-response-metadata branch, I modified GrpcErrorResponse to include a String -> MetadataEntry map. I also moved GrpcErrorResponse from the language-specific packages to the akka.grpc package, along with MetadataEntry, StringEntry and BytesEntry.

Is there a good way for me to package these up for you to review, or is mentioning them here enough? (I could create a pull request, I suppose, but that seems like it risks fragmenting the discussion.)

Edit: Links, for convenience:
https://github.com/drmontgomery/akka-grpc/tree/wip-rich-error-response-headers
https://github.com/drmontgomery/akka-grpc/tree/wip-rich-error-response-metadata

[ignasi35]

GrpcErrorResponse

(not quoting everything)

I think there's room for a new type but instead of GrpcErrorResponse(...) we could call it Trailers(Status, Metadata) (where Metadata is the [already available Metadata type including a collection of headers(https://github.com/akka/akka-grpc/blob/992f19f213fb1c267c091d261ac59e419f994ea7/runtime/src/main/scala/akka/grpc/scaladsl/Metadata.scala#L20)). Then, the errorHandler would be PartialFunction[Throwable, Trailers]. I'm in favor of a more strongly typed approach.

Still, I think a collection of helpers for users to easily build the headers for that Trailers instance is more valuable (or even necessary) since Trailers encapsulates Metadata which encapsulates List[HttpHeader] after all.

[drmontgomery]

I've updated the branch based on our discussion. (I also rebased it onto the current master branch.)

I introduced a MetadataBuilder class to handle constructing immutable Metadata instances on the server side. (The client-side metadata creation code was coupled to the RequestBuilder interface.)

I ended up refactoring the Metadata implementation classes pretty heavily. I think it ended up in a good place, but I'd be happy to walk it back a bit if you think the changes are too much.

[raboof]

I ended up refactoring the Metadata implementation classes pretty heavily. I think it ended up in a good place, but I'd be happy to walk it back a bit if you think the changes are too much.

I see what you mean - I think it's OK, much of the 'tricky' stuff is internal anyway so that's OK.

I think this is pretty close to being mergable. mimaReportBinaryIssues is reporting some incompatibilities - since we are still before 1.0.0 that's fine and you can just add the ProblemFilter entries to a backwards-excludes file as described in CONTRIBUTING.md (https://travis-ci.org/akka/akka-grpc/jobs/657617901#L531). I also suspect we need to run sbt scalafmtAll to make sure all the formatting is consistent.

Let me know if you want any help with that!

[drmontgomery]

Thanks for info about mimaReportBinaryIssues!

I also ran scalafmtAll, but nothing changed.

[raboof]

Thanks for info about mimaReportBinaryIssues!

Sure!

I also ran scalafmtAll, but nothing changed

Then everything was formatted correctly already 👍

[raboof]

you can even use wildcards here but this is 👍

[raboof]

Great contribution, thanks!