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

gRPC - ContentSubType Support #7321

Closed
CH3CHO opened this issue Aug 12, 2020 · 10 comments
Closed

gRPC - ContentSubType Support #7321

CH3CHO opened this issue Aug 12, 2020 · 10 comments

Comments

@CH3CHO
Copy link

CH3CHO commented Aug 12, 2020

I find that grpc-go support registering a custom codec and specifying a contentSubType when making a call.

Is it possible we support this feature in grpc-java so users can use custom serializers?

@sanjaypujare
Copy link
Contributor

There is a blog about using a custom serializer/deserializer (JSON) https://grpc.io/blog/grpc-with-json/ which might be helpful? It doesn't mention registering using the contentSubType though.

@CH3CHO
Copy link
Author

CH3CHO commented Aug 13, 2020

Yes, I've read that blog. It does help a bit. But it is a static solution for customizing serialization. And it doesn't change the fact that gRPC doesn't support multiple serialization methods at the same time in Java. But if we introduce in contentSubType, we can make multiple serializers available for clients choose, and server will response accordingly if it supports the same contentSubType. This solution would be much better than the current custom marshaller one.

@sanjaypujare
Copy link
Contributor

Java has Codec, CompressorRegistry and DecompressorRegistry which doesn't use contentSubType but rather the "grpc-encoding" header and is mainly used for compression/decompression rather than custom ser-deser. Would that work for you?

@CH3CHO
Copy link
Author

CH3CHO commented Aug 13, 2020

Thanks, @sanjaypujare . I will look into them.

@CH3CHO
Copy link
Author

CH3CHO commented Aug 13, 2020

I'm afraid Codec doesn't meet my requirement. We do need a custom ser-deser support. And are we going to align the features of gRPC in all platforms?

@sanjaypujare
Copy link
Contributor

I am not aware of any efforts. CC @ejona86 for more info.

@ejona86
Copy link
Member

ejona86 commented Aug 13, 2020

@CH3CHO, grpc-java does not support the subtype thing; it is currently ignored and never sent. grpc-go added support for it thinking other languages supported it, when they don't. It would be possible to add, but would require additions to how MethodDescriptor.Marshallers work and would be quite different to set up than Go's RegisterCodec as marshallers are not global in Java (and I think it was a mistake to make them so in Go). It is only necessary if you want one method to support two encoding types, which has been very uncommon.

If all you want to do is use a different codec type then you would change the MethodDescriptors to have the marshaller you want. That is similar to ForceCodec(). If you need multiple codecs simultaneously then I'd be interested in your use-case.

@marsqing
Copy link

marsqing commented Sep 9, 2020

@ejona86 Our services have two types of client. One prefers performance and uses protobuf. The other prefers ease of use and uses json. The latter ones are usually QAs who have lots of Postman related test cases which are basically inputs and outputs encoded in json.

Postman is heavily used in our company and Postman do not support protobuf yet😥.
Feature Request : Support for google protobuf

So it would really help a lot if we can support multiple codecs simultaneously.

@ejona86 ejona86 added this to the Next milestone Sep 9, 2020
@ejona86
Copy link
Member

ejona86 commented Sep 9, 2020

Okay. That's exactly the expected use-case (not the Postman not supporting protobuf, but some clients you want to be simpler/user-friendly). It will need API designs for both sending and receiving. Receiving is probably easier and is what you need.

My thought-about-it-30-seconds design for receiving would be a subinterface of MethodDescriptor.Marshaller with one method that is provided the subtype string and returns a Marshaller. If no subtype is specified on the content-type, then the interface is ignored and the marshaller is used like normal. We'd need to consider what should happen in the case that the subtype is unsupported. The interface method itself could return null in such a case.

@marsqing
Copy link

IMHO we should respond with HTTP status of 415 (Unsupported Media Type) as specified in the gRPC spec.

I am not very familiar with the code. After some debugging, I think the core part to enable ContentSubType is in ServerCallImpl.java, specifically this line

listener.onMessage(call.method.parseRequest(message));
// call is instance of  ServerCallImpl

where we parse the message with Marshaller. Actually in ServerCallImpl's constructor, we do have all the HTTP headers.

ServerCallImpl(..., Metadata inboundHeaders, ...) {
    //...
    this.messageAcceptEncoding = inboundHeaders.get(MESSAGE_ACCEPT_ENCODING_KEY);
    //...
}

So we can save the content-type header in ServerCallImpl

ServerCallImpl(..., Metadata inboundHeaders, ...) {
    //...
    this.messageAcceptEncoding = inboundHeaders.get(MESSAGE_ACCEPT_ENCODING_KEY);
    this.messageContentType = inboundHeaders.get(CONTENT_TYPE_KEY);
    //...
}

and pass the content-type header to Marshaller.

// replace this line with the following
//listener.onMessage(call.method.parseRequest(message));

MethodDescriptor.Marshaller<ReqT> marshaller = call.method.getRequestMarshaller();
String contentType = call.messageContentType;  // the newly added field
if(marshaller instanceof MethodDescriptor.SubTypeAwareMarshaller && hasSubType(contentType)) {
    // SubTypeAwareMarshaller is a subinterface of Marshaller
    marshaller = ((MethodDescriptor.SubTypeAwareMarshaller)marshaller).getMarshallerForSubType(contentType);
}
listener.onMessage(marshaller.parse(message));

@ejona86 is the rough idea ok and can ServerCallImpl cover all the cases?

xerial added a commit to wvlet/airframe that referenced this issue Jan 22, 2021
* Move internal only classes to wvlet.airframe.http.grpc.internal
* Add Metadata helper
* Rename marshaller classes
* Use Accept header instead as grpc-java doesn't support content-subtype
* Add x-airframe-xxx-version header
* Wrap generated clients with the default client interceptor
* extract server factory
* Wrap with GrpcResponse for setting JSON encoding
* Add note about the missing support of content sub type: grpc/grpc-java#7321
@CH3CHO CH3CHO closed this as completed Feb 4, 2023
@ejona86 ejona86 removed this from the Next milestone Feb 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants