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

tonic_web::enable won't work with grpc services because Cors from tower_http does not implement NamedService #1312

Closed
jaysonsantos opened this issue Mar 11, 2023 · 5 comments

Comments

@jaysonsantos
Copy link

Bug Report

Hi there, first thank you very much for this lib, it has been a pleasure working with it!

Version

cargo tree | grep tonic
│   │   │   │   ├── tonic v0.8.3
│   │   │   ├── tonic v0.8.3 (*)
│   │   │   │   └── tonic v0.8.3 (*)
│   │   │   │   └── tonic-build v0.8.4
│   │   │   └── tonic v0.8.3 (*)
│   └── tonic v0.8.3 (*)
│   └── tonic-build v0.8.4 (*)
├── tonic v0.8.3 (*)
├── tonic-health v0.8.0
│   └── tonic v0.8.3 (*)
├── tonic-web v0.5.0
│   ├── tonic v0.8.3 (*)

Platform

Darwin Jaysons-MBP.localdomain 22.3.0 Darwin Kernel Version 22.3.0: Mon Jan 30 20:38:37 PST 2023; root:xnu-8792.81.3~2/RELEASE_ARM64_T6000 arm64

Crates

Description

While trying to follow the docs to enable cors in my service, I found that it actually does not work because Server::add_service requires NamedService to work.

A simple way to test it is to patch tonic_web's own test with:

diff --git a/tonic-web/tests/integration/tests/grpc_web.rs b/tonic-web/tests/integration/tests/grpc_web.rs
index e062a83..452c8fa 100644
--- a/tonic-web/tests/integration/tests/grpc_web.rs
+++ b/tonic-web/tests/integration/tests/grpc_web.rs
@@ -69,7 +69,7 @@ async fn spawn() -> String {
         Server::builder()
             .accept_http1(true)
             .layer(GrpcWebLayer::new())
-            .add_service(TestServer::new(Svc))
+            .add_service(tonic_web::enable(TestServer::new(Svc)))
             .serve_with_incoming(listener_stream)
             .await
             .unwrap()

and the build output is

 cargo build --tests -p integration
   ...
   Compiling tonic-build v0.8.4 (/Users/jayson.reis/src/tonic/tonic-build)
   Compiling integration v0.1.0 (/Users/jayson.reis/src/tonic/tonic-web/tests/integration)
error[E0277]: the trait bound `tower_http::cors::Cors<GrpcWebService<TestServer<Svc>>>: NamedService` is not satisfied
   --> tonic-web/tests/integration/tests/grpc_web.rs:72:26
    |
72  |             .add_service(tonic_web::enable(TestServer::new(Svc)))
    |              ----------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `NamedService` is not implemented for `tower_http::cors::Cors<GrpcWebService<TestServer<Svc>>>`
    |              |
    |              required by a bound introduced by this call
    |
    = help: the following other types implement trait `NamedService`:
              GrpcWebService<S>
              InterceptedService<S, F>
              TestServer<T>
              tower::util::either::Either<S, T>
note: required by a bound in `tonic::transport::Server::<L>::add_service`
   --> /Users/jayson.reis/src/tonic/tonic/src/transport/server/mod.rs:347:15
    |
347 |             + NamedService
    |               ^^^^^^^^^^^^ required by this bound in `Server::<L>::add_service`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `integration` due to previous error
warning: build failed, waiting for other jobs to finish...

@jaysonsantos
Copy link
Author

jaysonsantos commented Mar 11, 2023

A way I've managed to make it compile was by building a wrapper, maybe is it worth it to generate on the codegen crate?

diff --git a/tonic-web/tests/integration/tests/grpc_web.rs b/tonic-web/tests/integration/tests/grpc_web.rs
index e062a83..dd2645d 100644
--- a/tonic-web/tests/integration/tests/grpc_web.rs
+++ b/tonic-web/tests/integration/tests/grpc_web.rs
@@ -1,9 +1,11 @@
+use std::convert::Infallible;
 use std::net::SocketAddr;
+use std::task::{Context, Poll};
 
 use base64::Engine as _;
 use bytes::{Buf, BufMut, Bytes, BytesMut};
 use hyper::http::{header, StatusCode};
-use hyper::{Body, Client, Method, Request, Uri};
+use hyper::{Body, Client, Method, Request, Response, Uri};
 use prost::Message;
 use tokio::net::TcpListener;
 use tokio_stream::wrappers::TcpListenerStream;
@@ -11,6 +13,9 @@ use tonic::transport::Server;
 
 use integration::pb::{test_server::TestServer, Input, Output};
 use integration::Svc;
+use tonic::body::BoxBody;
+use tonic::codegen::Service;
+use tonic::server::NamedService;
 use tonic_web::GrpcWebLayer;
 
 #[tokio::test]
@@ -59,6 +64,38 @@ async fn text_request() {
     assert_eq!(&trailers[..], b"grpc-status:0\r\n");
 }
 
+#[derive(Clone)]
+pub struct NamedTestServerWrapper<S>(S);
+impl<S> NamedTestServerWrapper<S> {
+    pub fn new(service: S) -> Self {
+        Self(service)
+    }
+}
+impl<S> Service<Request<Body>> for NamedTestServerWrapper<S>
+where
+    S: Service<Request<Body>, Response = Response<BoxBody>, Error = Infallible>
+        + Clone
+        + Send
+        + 'static,
+    S::Future: Send + 'static,
+{
+    type Response = S::Response;
+    type Error = S::Error;
+    type Future = S::Future;
+
+    fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
+        self.0.poll_ready(cx)
+    }
+
+    fn call(&mut self, req: Request<Body>) -> Self::Future {
+        self.0.call(req)
+    }
+}
+
+impl<S> NamedService for NamedTestServerWrapper<S> {
+    const NAME: &'static str = "test.Test";
+}
+
 async fn spawn() -> String {
     let addr = SocketAddr::from(([127, 0, 0, 1], 0));
     let listener = TcpListener::bind(addr).await.expect("listener");
@@ -69,7 +106,9 @@ async fn spawn() -> String {
         Server::builder()
             .accept_http1(true)
             .layer(GrpcWebLayer::new())
-            .add_service(TestServer::new(Svc))
+            .add_service(NamedTestServerWrapper::new(tonic_web::enable(
+                TestServer::new(Svc),
+            )))
             .serve_with_incoming(listener_stream)
             .await
             .unwrap()

@Ben2146053
Copy link

This PR might be related.
#1286

@LucioFranco
Copy link
Member

Kinda a duplicate #270 will be fixed in the next release.

@vdhanan
Copy link

vdhanan commented Mar 29, 2023

@LucioFranco thanks for the fix! When will the next release be out?

@LucioFranco
Copy link
Member

Yes, will come in the next release of tonic, I just have some tests to write before we make the release since it will contain some important/cross cutting changes.

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

No branches or pull requests

4 participants