Add support for gRPC transcoding

[zZHorizonZz]

Motivation:

This pull request introduces gRPC transcoding capabilities to the gRPC server. The most important changes include the addition of new interfaces for HTTP templates and variable bindings, as well as updates to existing classes to support these new features.

The code for path lookup and checking is based on code from grpc-transcoding-repo. I think, we shouldn't try to reinvent the wheel when a solution already exists. While there are some changes since the original code is in C++, the logic for matching and extracting HTTP requests remains largely similar. I have also converted several tests from the original repository for these classes.

Regarding the actual transcoding, I'm currently facing two issues. First, by definition, requests can have an empty body while the input gRPC message can still have fields. What I mean is that values can be passed in requests via path or query parameters. However, when I test this, the code gets stuck, my guess is that it's possibly waiting for a body. I need toinvestigate this further but would appreciate any help. The second issue is that in the generator, the code seems unable to read method options despite having them as dependencies. But, overall the transcoding seems to be working except for these two issues, though obviously, this pr will need additional work.

Also currently there isn't really support for streaming that would require websockets which is an entirely new thing in my opinion.

The code for the transcoding is reusing the wire format infrastructure by putting length and compression data before the json data, so that we don't need to create entirely new thing for this.

I appreciate any feedback and help with integrating this feature into the codebase.

Closes: #95

[zZHorizonZz]

Well, I cannot really create an account on eclipse foundation as it's throwing error when I try to create one, I will wait a couple of days with that and see If it fixes itself or something.

[vietj]

I need to read more about it, however I think this feature should be implemented in its own module instead of being in grpc common

[zZHorizonZz]

Yeah, I can do that. I have been wondering about that as well.

[vietj]

I am wondering if we should not make some work on the server to provide an invocation API to decouple the delivery of gRPC invocations from the actual transport.

[zZHorizonZz]

Are you talking about implementing a middleware layer where you could have daisy chained middlewares with buffers or something, so you could have compression middleware, transcoding middleware, etc.? Or am I thinking of something completely different?

[vietj]

I'm thinking of a way to community between a gRPC server request/response and an invocation layer using grpc messages, this way anything could invoke a gRPC service by crafting the proper messages, very much like HTTP messages in Netty

[zZHorizonZz]

I think I'm starting to understand it, but I'm not quite sure yet, I might be overthinking it.

So, if I understand correctly, it would be something like this:

public interface MethodRegistry {

<Req, Resp> MethodRegistry callHandler(ServiceMethod<Req, Resp> serviceMethod, Handler<GrpcRequest<Req, Resp>> handler);

}

public interface MethodCallHandler<Req, Resp> implements Handler<GrpcRequest<Req, Resp>> {

void handle(GrpcRequest<Req, Resp> grpcRequest);

}

OR

public interface MethodCallHandler<Req> implements Handler<Req> {

void handle(Req);

// The default implementation could basically be MethodCallHandler<HttpServerRequest> so the code for handling of stuff could be basically partially or completely moved to this implementation.

So here default implementation of method registry would contain basically logic from server:

if (handler != null) {
      MethodCallHandler<Req, Resp> p = new MethodCallHandler<>(serviceMethod.decoder(), serviceMethod.encoder(), handler);
      methodCallHandlers.compute(serviceMethod.fullMethodName(), (key, prev) -> {
        if (prev == null) {
          prev = new ArrayList<>();
        }
        for (int i = 0; i < prev.size(); i++) {
          MethodCallHandler<?, ?> a = prev.get(i);
          if (a.messageDecoder.format() == serviceMethod.decoder().format() && a.messageEncoder.format() == serviceMethod.encoder().format()) {
            prev.set(i, p);
            return prev;
          }
        }
        prev.add(p);
        return prev;
      });
    } else {
      methodCallHandlers.compute(serviceMethod.fullMethodName(), (key, prev) -> {
        if (prev != null) {
          for (int i = 0; i < prev.size(); i++) {
            MethodCallHandler<?, ?> a = prev.get(i);
            if (a.messageDecoder.format() == serviceMethod.decoder().format() && a.messageEncoder.format() == serviceMethod.encoder().format()) {
              prev.remove(i);
              break;
            }
          }
          if (prev.isEmpty()) {
            prev = null;
          }
        }
        return prev;
      });
    }
    return this;

And what you could do is provide the registry to the server. Or, it might create its own, which you could then get and pass to the call handler. The call handler would then look up the method from the registry and invoke it.

[vietj]

I'll handle the internal refactoring (decoupling) myself after we merge this PR

[vietj]

Most objects in the transcoding package should be annotated with @DataObject and should be plain classes instead of having an interface/implementation

[zZHorizonZz]

I'll handle the internal refactoring (decoupling) myself after we merge this PR

I wish I could help, but I think I'm imagining something completely different from what you're thinking, so that will probably be best.

Most objects in the transcoding package should be annotated with @DataObject and should be plain classes instead of having an interface/implementation

Yeah, I have refactored it so now there are just classes. I did it mostly to fit grpc-common module style and also because I had some issues compiling, as I was getting errors about @VertxGen usage, etc., as I've never really used these annotations. But now since it's in its own package, there are no issues.

[vietj]

link to grpc transcoding

[zZHorizonZz]

Most of the time when I need to read about transcoding, I go to Google Cloud gRPC docs, as I haven't found any official transcoding documentation. So is this link ok or do you have something else in mind.

[vietj]

My point is that the documentation should do more and explain what transcoding does and when it is useful for users.

[zZHorizonZz]

I have added documentation. While writing docs for the generator, I have been thinking and this is more or less for discussion if we should add configuration options to enable/disable the generation of wire format and transcoding options. I think in large codebases where these features are not needed, it might make a significant difference in terms of final file size.

[vietj]

I have added documentation. While writing docs for the generator, I have been thinking and this is more or less for discussion if we should add configuration options to enable/disable the generation of wire format and transcoding options. I think in large codebases where these features are not needed, it might make a significant difference in terms of final file size.

we should only perform transcoding generation when the service is annotated with google.api.http so the intent is clear

[vietj]

is grpc.io server generation capable of doing transcoding ? I don't see any the service options metadata available in the generated code

[zZHorizonZz]

we should only perform transcoding generation when the service is annotated with google.api.http so the intent is clear

So basically, currently the generator is generating MethodTranscodingOptions for every method and is using default values in these. For example, for the HTTP method it falls back to POST, and for the path it falls back to a path like this:

if (methodContext.transcodingContext.path == null) {
        methodContext.transcodingContext.method = "POST";
        if (serviceContext.packageName == null || serviceContext.packageName.isEmpty()) {
          methodContext.transcodingContext.path = "/" + serviceContext.serviceName + "/" + methodContext.methodName;
        } else {
          methodContext.transcodingContext.path = "/" + serviceContext.packageName + "/" + serviceContext.serviceName + "/" + methodContext.methodName;
        }
      }

I think I have an idea on how to generate the code only when there are transcoding annotations available though, so I will try to fix that, so that we are generating code only for method with option, but I have a issue with that where I need a little help from somebody who understands it more, basically I have annotated the some methods in IT test protos with google.api.http option but the generator doesn't really seems to be registering them:

if (methodProto.getOptions().hasExtension(AnnotationsProto.http)) {
      HttpRule httpRule = methodProto.getOptions().getExtension(AnnotationsProto.http);
      methodContext.transcodingContext = buildTranscodingContext(httpRule);
    }

is grpc.io server generation capable of doing transcoding ? I don't see any the service options metadata available in the generated code

If we are talking about server from for example vertx-grpc-it then there should be generated code on top of the class something like this:

public static final MethodTranscodingOptions SayHello_TRANSCODING = new MethodTranscodingOptions(
    "",
    HttpMethod.valueOf("POST"),
    "/Greeter/SayHello",
    "",
    "",
    List.of(
    ));

And then some binding methods for transcoding.

[vietj]

about grpc.io, I am merely wondering whether the transcoding information is available from the grpc-java generator, because I cannot see anything here, so there is no possibility to take a grpc-java service descriptor and obtain the oogle.api.http information for honnoring the declared mapping.

[vietj]

for the vertx generator, we need to honour the google.api.http to implement it, that is what users will expect to use

[vietj]

@zZHorizonZz can you do a clean rebase on latest repo main branch ? that makes it easier to merge and review

Should I also squash the commits? I don't know what the view on that is in Eclipse projects, but in Quarkus they want that to be done so the git history is more or less clean.

we don't mind having commits not squashed if they are properly separated, there is value in the history we might want to preserve sometimes.

we can check when we merge the contribution

[vietj]

your commit history looks clean, so we might preserve it

[vietj]

I can see many code reformatted by your IDE which makes the PR harder to review, can you remove those un-necessary changes ?

[zZHorizonZz]

I can see many code reformatted by your IDE which makes the PR harder to review, can you remove those un-necessary changes ?

Yeah, sorry about that. IDE is doing its thing where it seems like it's able to adapt to the code style, but at the same time, it's not. I'll fix that.

[vietj]

@zZHorizonZz I'll make another review soonish, hopefully we can get that in time for 5.0

[vietj]

do we have a test for sub query parameters like here https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L109 ?

[zZHorizonZz]

do we have a test for sub query parameters like here https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L109 ?

There is a test in MessageWeaverTest, but not in the actual request testing. I have added a test for that and fixed an issue I found along the way.

[vietj]

thanks for the contribution I plan to merge it soon

[zZHorizonZz]

I remembered that I never implemented the HTTP error code mapping, so I have added that. Since there isn't much documentation, it's done on a best effort basis. I followed the mapping from the .NET implementation of transcoding.