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

feat(build): Better support for custom codecs #999

Merged
merged 3 commits into from May 5, 2022

Conversation

bmwill
Copy link
Contributor

@bmwill bmwill commented May 4, 2022

@LucioFranco after we chatted briefly on discord yesterday I did a bit more thinking about how improve accessibility of the code generation functionality in tonic-build. This is one potential approach. Its in a WIP state but I wanted to get your thoughts and see if this is something you'd be interested in as a contribution to tonic. If so I can spend some time actually making it usable.

Motivation

gRPC is a general purpose RPC framework that, while generally is paired with protobuf, is agnostic to the type of serialization format used for messages and in some cases it may be desired to use a different serialization format other than protobuf.

Solution

This PR takes a look at one possible way to enable using a custom, non-protobuf codec for serializing messages to the wire. This is done via:

  • changing the CODEC_PATH constant to a method which returns a &str for better programability
  • introduces a set of builders for Services and Methods so that you can build up a service with various methods and invoke the code generation without interacting with prost or proto files. This could eventually enable developing a proc-macro that does the generation at compile time instead of needing a build script (for services which don't leverage proto files and protobuf)

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

I like this a lot! I think just need to add some docs and we can merge this. Thanks!

}
}

#[test]
Copy link
Member

Choose a reason for hiding this comment

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

cfg test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah i just whipped this together to see what you thought so now that I know you're interested i'll clean it up with docs and the like

@bmwill bmwill marked this pull request as ready for review May 5, 2022 01:42
@bmwill
Copy link
Contributor Author

bmwill commented May 5, 2022

Ok this should be ready for a serious review now. My main question is how you want to handle the module naming, right now its all just in a public module rust and nothing is reexported at the top level.

@LucioFranco
Copy link
Member

@bmwill how about manual instead of rust for the module name?

@bmwill
Copy link
Contributor Author

bmwill commented May 5, 2022

@LucioFranco yeah manual is a bit more descriptive. I've updated the PR to reflect that change.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Overall looks great left a few comments/questions.

examples/src/json-codec/common.rs Outdated Show resolved Hide resolved
examples/build.rs Outdated Show resolved Hide resolved
examples/src/json-codec/client.rs Outdated Show resolved Hide resolved
examples/src/json-codec/common.rs Show resolved Hide resolved
tonic-build/src/rust.rs Outdated Show resolved Hide resolved
tonic-build/src/manual.rs Show resolved Hide resolved
tonic-build/src/manual.rs Outdated Show resolved Hide resolved
}
}

struct ServiceGenerator {
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need this extra struct? I don't mind keeping it just seems like we could just embed the code in the compile method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was sort of following the pattern as exists in the prost.rs file. The extra struct sort of helps with the bookkeeping so it's probably worth keeping just for keeping the compile method simple.

tonic-build/src/manual.rs Show resolved Hide resolved
examples/build.rs Show resolved Hide resolved
bmwill added 2 commits May 5, 2022 09:14
This patch adds the `rust` module to tonic-build which provides
utilities for declaratively defining services in rust code instead of
via the traditional method of defining a service in a `proto` file.

It also enables setting a custom `Codec` to in order to use a custom
serialization format other than `protobuf`.
@bmwill
Copy link
Contributor Author

bmwill commented May 5, 2022

@LucioFranco Ok I've updated the PR, hopefully addressing all of your comments

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@LucioFranco LucioFranco changed the title tonic-build: better support custom codecs feat(build): Better support for custom codecs May 5, 2022
@LucioFranco LucioFranco merged commit de2e4ac into hyperium:master May 5, 2022
@bmwill bmwill deleted the codec branch May 5, 2022 17:05
@zbysir
Copy link

zbysir commented May 7, 2022

@bmwill Take a look at this: #851, can you give me some advice? First of all thank you.

What I want most is to support multiple codecs, not a specific one.

@bmwill
Copy link
Contributor Author

bmwill commented May 9, 2022

@zbysir Ah interesting! Yeah one thing this change does not handle is looking at/changing the content-type header. I did ask @LucioFranco about it and he seemed to indicate that there were some implementations out that that couldn't handle a value of application/grpc+{encoding} so the ability to change that was removed from tonic.

In order to handle multiple codecs, switching based on the content-type header I'd speculate that the best way to do that would be:

  • Change the Codec trait to be able to emit the content-type header suffix it supports, e.g for Json it can be Some("json"), for protobuf it can be None or Some("proto")
 fn content_type(&self) -> Option<&str>;
  • Tonic code can use that to append the type to application/grpc when sending a message or enable switching on which codec to use on the server side.
  • At that point you should be able to either manually construct the service code or work on changing the codegen to be able to support multiple codecs

Please note though that I am not a maintainer of tonic so this is just my view on one such way to accomplish what you want to but if you want such a change to be included in tonic you'll probably need to speak with Lucio or one of the other maintainers and see if they'd be open to such a change.

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.

None yet

3 participants