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 support for gRPC-Web text encoding #150

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

emcfarlane
Copy link
Collaborator

@emcfarlane emcfarlane commented Nov 9, 2024

This adds support for gRPC-Web's text encoding to handle application/grpc-web-text content types. Only client support is added for now, for simplicity. Encoding of request and response body is handled to support text streams. Testing is added by mutating a gRPC-Web client to implement the text protocol.

The implementation is done by converting the request body and response writer to encode and decode streams of base64 encoded data. Go's std library cannot handle padding characters within the stream. For writing base64 encoded data we can simply use base64.NewEncoder, calling Close when needed to flush data to the writer. To read base64 encoded data we cannot use base64.NewDecoder as we need to split the reader on padding characters to form valid base64 chunks to pass to the decoder. Therefore this implementation buffers the stream into 4-byte tokens before decoding, splitting on padding tokens when found.

Closes #143

This adds support for gRPC-Web's text encoding. Only client support
is added. Encoding of request and response body is handled to support
text streams.

Signed-off-by: Edward McFarlane <[email protected]>
@emcfarlane emcfarlane requested a review from jhump November 9, 2024 20:37
Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

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

See comment. I think we could reimplement this as separate, dumber middleware that gets used from a Transcoder. WDYT?

// ProtocolGRPCWebText is a variant of ProtocolGRPCWeb that uses base64
// text encoding for the request and response bodies.
//
// This protocol is not supported on the server side.
Copy link
Member

@jhump jhump Dec 5, 2024

Choose a reason for hiding this comment

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

Do we even need/want an exported constant for it?

It would likely be trivial to implement as a separate, simpler middleware handler. Then we have the option to potentially move it to connect-go in the future, to enable grpc-web-text there. The Transcoder could always delegate to that middleware first since it should basically be free/pass-through for non-grpc-web-text requests. Then we don't need a protocol constant for it at all since the rest of the logic in here doesn't need to know about it.

Copy link
Member

Choose a reason for hiding this comment

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

It would also be relatively easy to test that separate middleware by copying it over into the connect conformance referenceserver and then see if the JS grpc-web-client starts passing the server stream tests that it currently opts out of. We'd have to enable grpc-web-text in that client, but that's as simple as changing the mode to "grpcwebtext" and re-generating.

(Admittedly, that's not a way to unit test this repo's implementation of it. But the tests you've added here could still be used of course.)

@@ -416,3 +419,150 @@ func TestMux_RPCxRPC(t *testing.T) {
})
}
}

func Test_grpcWebText(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func Test_grpcWebText(t *testing.T) {
func TestTranscoder_GrpcWebText(t *testing.T) {

(Aside: at some point we should change the other battery of tests from TestMux_... to TestTranscoder_..., to reflect the actual API names.)

// ProtocolGRPCWebText is a variant of ProtocolGRPCWeb that uses base64
// text encoding for the request and response bodies.
//
// This protocol is not supported on the server side.
Copy link
Member

Choose a reason for hiding this comment

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

It would also be relatively easy to test that separate middleware by copying it over into the connect conformance referenceserver and then see if the JS grpc-web-client starts passing the server stream tests that it currently opts out of. We'd have to enable grpc-web-text in that client, but that's as simple as changing the mode to "grpcwebtext" and re-generating.

(Admittedly, that's not a way to unit test this repo's implementation of it. But the tests you've added here could still be used of course.)

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.

Feature Request: grpc-web-text wire format support
2 participants