devashishdxt / tonic-web-wasm-client

Other
104 stars 28 forks source link

Combined content-type not recognized by tonic #3

Closed jetaggart closed 2 years ago

jetaggart commented 2 years ago

I'm using tonic on the backend. The created client mergers the content-type together creating a header that tonic-web does not expect.

For example, tonic-web expects one of these 4 content-types:

https://github.com/hyperium/tonic/blob/8084f4ea26cccf9bd2d96d2a81eaea490aaf603b/tonic-web/src/service.rs#L29-L32

    //  - "application/grpc-web"
    //  - "application/grpc-web+proto"
    //  - "application/grpc-web-text"
    //  - "application/grpc-web-text+proto"

However, the header application/grpc gets combined with the headers added by this client `application/grpc-web+proto". This results in a 400:

400 Bad Request
panicked at 'called `Result::unwrap()` on an `Err` value: Status { code: Unknown, message: "missing content-type header in grpc response", source: Some(MissingContentTypeHeader) }'

The header that is generated by this client looks like this:

content-type: application/grpc-web+proto, application/grpc

If I modify these lines: https://github.com/devashishdxt/tonic-web-wasm-client/blob/main/src/client.rs#L62-L64 to be this:

    for (header_name, header_value) in req.headers().iter() {
        if header_name.as_str() != "content-type" {
            builder = builder.header(header_name.as_str(), header_value.to_str()?);
        }
    }

it all works. I'm not sure where the application/grpc is getting attached, I think it's on the tonic side. Not sure exactly what the resolution here is either.

devashishdxt commented 2 years ago

I too am not sure what the correct resolution is: grpc-web specification mentions that content-type should be application/grpc-web[+...]. https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md#protocol-differences-vs-grpc-over-http2.

devashishdxt commented 2 years ago

tonic-web response should contain content-type header. https://github.com/devashishdxt/tonic-web-wasm-client/blob/6b96030a0ad749716bcf8fdd0e0faaf505864b86/src/client.rs#L80

devashishdxt commented 2 years ago

I've tested tonic-web-wasm-client with Go based grpc-web server (which is more mature) and it works fine. Maybe there's some mismatch between tonic-web and tonic-web-wasm-client header requirements.

jetaggart commented 2 years ago

I think this client is doing the right thing, but tonic is attaching the application/grpc header somewhere. It does understand the application/grpc-web+proto just fine, just not the combination. Maybe this is an issue with tonic?

jetaggart commented 2 years ago

Although I don't think this client should ultimately emit application/grpc content-type header at all, since that's wrong.

devashishdxt commented 2 years ago

Thanks for opening this issue. I'll check this over the weekend when I get some time.

jetaggart commented 2 years ago

I've opened it on the tonic side as well: https://github.com/hyperium/tonic/issues/1041

devashishdxt commented 2 years ago

Created a new version on crates.io with a fix: https://crates.io/crates/tonic-web-wasm-client

jetaggart commented 2 years ago

Thank you! ❤️

jetaggart commented 2 years ago

@devashishdxt might be worth setting the correct content type in tower per https://github.com/hyperium/tonic/issues/1041#issuecomment-1199578481