feat: Add `IntoRequest` and `IntoStreamingRequest` traits #66

alce · 2019-10-10T18:11:09Z

Motivation

This PR implements IntoRequest and IntoStreamingRequest traits, as outlined in #34. At the moment, both unary and streaming requests work but interop crate needs to be updated for this PR to be complete. I'll wait for feedback and implement what is remaining if it's decided we should move forward with this.

Solution

Implementation is almost exactly what is discussed in #34.

LucioFranco · 2019-10-13T16:07:59Z

tonic/src/request.rs

+use futures_core::Stream;
+
+/// A marker trait that should be implemented for all input messages.
+pub trait RequestMessage {}


I don't think we need this marker trait since there is only ever one generic?

You are right, it is not strictly necessary and a previous implementation did not have it. I decided to include it because:

It sort of helps us constrain the other trait implementations. It just feels weird for IntoRequest or IntoStreamingRequest impl to exist for any type that is not an input to a rpc method.

I am toying around with an implementation that would let us call the streaming methods with iterators and having this bound sort of helps. Not sure if this will work of if it even makes sense though.

It makes code generation extremely simple, basically 1 line per input type.

Triggered by the breakage in tonic-interop, I initially thought that it was going to be necessary to expose this marker for users to implement in certain scenarios, but that seems not to be necessary. I found the reason why interop is broken and has nothing to do with this.

None of these reasons hold much water though. In the end, however, this should be an internal implementation detail and as long as we are not changing the public api willy-nilly we should be able to iterate on the actual implementation until we find the optimal one.

Yeah, those points make sense, my real motivation for removing it is that it's not actually required and just simplifies things. Happy either way.

My 2¢ is that since the Request type itself is unconstrained, this shouldn't be added; if we are going to constrain the Message type on IntoRequest, we should also constrain Request, and I'd consider adding that in a separate branch.

Also, I think it's potentially very surprising to generate implementations of a doc(hidden) trait in a user's codebase — I'd find it weird to run cargo doc on my project and see implementations of a trait that doesn't appear to exist.

I removed this trait as suggested. IntoRequest for T is generated now instead, please let me know what you think.

alce · 2019-10-15T01:33:34Z

I've fixed the issue with interop. This implementation seems to work well with different .proto setups.

The only way I found to make it work without the RequestMessage trait is to generate IntoRequest implementations for all input types. I could not find a way to make a blanket implementation for T without the bound.

I'm happy to make that change if you prefer, it's easy.

LucioFranco · 2019-10-18T21:29:28Z

@hawkw what do you think about having a RequestMessage trait to enforce the type that can be intoed?

LucioFranco · 2019-10-18T21:29:49Z

and @alce sorry for the delay on this I just wanna see what eliza has to say since she came up with the original spec other wise this looks good!

tonic-build/src/client.rs

hawkw · 2019-10-18T21:45:42Z

tonic/src/request.rs

+use futures_core::Stream;
+
+/// A marker trait that should be implemented for all input messages.
+pub trait RequestMessage {}


My 2¢ is that since the Request type itself is unconstrained, this shouldn't be added; if we are going to constrain the Message type on IntoRequest, we should also constrain Request, and I'd consider adding that in a separate branch.

hawkw · 2019-10-18T21:46:56Z

tonic/src/request.rs

+use futures_core::Stream;
+
+/// A marker trait that should be implemented for all input messages.
+pub trait RequestMessage {}


Also, I think it's potentially very surprising to generate implementations of a doc(hidden) trait in a user's codebase — I'd find it weird to run cargo doc on my project and see implementations of a trait that doesn't appear to exist.

LucioFranco · 2019-10-19T01:28:01Z

@alce can you resolve the conflicts? I think CI may not be running because of that?

alce · 2019-10-19T02:22:55Z

Goofed. Fixing...

tonic/src/request.rs

alce · 2019-10-23T00:42:26Z

I am having second thoughts about this PR.

On the one hand, as Carl and Eliza pointed out, it makes writing client calls a lot nicer. Every time I write client code I moan about having to write Request::new() explicitly.

On the other hand, it just doesn't feel entirely right to me. I can't really put my finger on it but it makes the API sort of asymmetrical. Even if we were to implement something similar for Response::new() (which btw doesn't really bother me much).

Maybe if we had to call .into_request() and .into_streaming_request() explicitly it wouldn't feel so weird?.

let message = HelloRequest{name: "hello".into()}
let response = client.say_hello(message.into_request())

Thoughts?

LucioFranco · 2019-10-23T00:56:04Z

@alce I kinda agree with you, that actually might be better. What do you think @hawkw @carllerche ?

hawkw · 2019-10-23T21:49:53Z

Maybe if we had to call .into_request() and .into_streaming_request() explicitly it wouldn't feel so weird?.
let message = HelloRequest{name: "hello".into()}
let response = client.say_hello(message.into_request())
Thoughts?

I don't really see much of a difference between making users write out into_request explicitly, and making them write Request::new? If we're going to require users to call the into_request method, I think it would be better to just close this PR and continue with the current state of things.

With that said, I don't think we should close this PR. I'd rather merge the code as-is, and call into_request implicitly. While having to call Request::new is not a major issue, not having to does make the new user experience and basic examples much simpler, which I think is worth something. If the concern is that

[...] it makes the API sort of asymmetrical.

I think we should just make the same change on the server side and add an IntoResponse trait as well.

Just my two cents.

alce · 2019-10-23T23:20:42Z

The comment about asymmetry was not so much about request vs response. The weirdness I get is more about client vs server.

This client code

let point = Point { lat: 1, lon: 2 };
let response = client.get_feature(point).await?;

and this server's signature

async fn get_feature(&self, request: Request<Point>) -> Result<Response<...>;

are surprising, particularly when the client's function signature is not easily available, as is the case when following the default getting started.

But then, as I was writing this, I realized that:

As things are now, you kinda need to hunt around for the generated code to get anything going beyond hello world anyway, in which case people will see the sneaky IntoRequest in the client's signature.
My gripe is, and always has been, with how deeply hidden the generated code is.

hawkw · 2019-10-23T23:54:07Z

As things are now, you kinda need to hunt around for the generated code to get anything going beyond hello world anyway, in which case people will see the sneaky IntoRequest in the client's signature.

My gripe is, and always has been, with how deeply hidden the generated code is.

@alce do you think we can replace the generated IntoRequest impls with blanket impls, like the ones @carllerche proposed in #34 (comment)? Then, the impl IntoRequest for ... would be in tonic's docs, rather than in the generated code.

alce · 2019-10-24T00:15:25Z

I couldn't figure out how to do it without a bound. That's one of the reasons why I had the RequestMessage marker trait before, which was akin to Carl's Message trait in the comment.

I will give it another go tomorrow to see if I can find another way.

LucioFranco · 2019-10-24T15:04:28Z

I'm still not convinced that this indirection is worth it honestly. I think the Request type is a good line between too verbose and being just explicit enough.

alce · 2019-10-24T15:26:28Z

@LucioFranco I think I've found a more natural way to do it, I'll push the changes later today.

With regards to the so-called asymmetry I speak of, I think I'm splitting hairs and considering only my own workflow and bias. In a larger, more realistic context, this is almost irrelevant.

alce · 2019-10-24T19:46:48Z

Here's a cleaner implementation that combines approaches discussed before. It introduces only one new trait and does not generate any code.

hawkw

This is great. I like that the trait implementations are no longer generated; I think this will be much less surprising for users. Left a couple of minor suggestions.

tonic/src/request.rs

tonic-build/src/client.rs

tonic/src/request.rs

LucioFranco · 2019-10-29T16:57:48Z

tonic/src/request.rs

+    fn into_streaming_request(self) -> Request<Self::Stream>;
+}
+
+/// Conversion into a unary `Request`.


Can we add a bit more docs here to explain how this works? I am thinking maybe an example that shows constructing a request and passing it and one with the message explaining the trait?

Could use https://doc.rust-lang.org/stable/std/convert/trait.Into.html as inspiration

Done. Docs are slightly better but I'm not sure they are clear enough.

LucioFranco

Wonderful! Thank you @alce for fighting through this! <3

LucioFranco · 2019-10-29T20:08:10Z

tonic/src/request.rs

+/// use tonic::Request;
+///
+/// client.get_feature(Point {});
+/// client.get_feature(Request::new(Point {}));


alce added 16 commits September 24, 2019 22:46

small tweaks, typos

d6dd4ed

Merge branch 'master' of github.com:LucioFranco/tonic

dc7daca

Merge branch 'master' of github.com:alce/tonic

96c9519

pin all alpha dependencies

d11d7b6

Merge branch 'master' of github.com:LucioFranco/tonic

cb84a13

Merge branch 'master' of github.com:hyperium/tonic

481c3a9

Merge branch 'master' of github.com:hyperium/tonic

0ad4de1

Merge branch 'master' of github.com:hyperium/tonic

30142bd

Merge branch 'master' of github.com:hyperium/tonic

8313341

Merge branch 'master' of github.com:alce/tonic

78e58bf

Merge branch 'master' of https://github.com/hyperium/tonic

0bbd595

add IntoRequest and IntoStreamingRequest traits

2d86038

Merge branch 'master' of github.com:hyperium/tonic

8a5096b

RequestMessage marker trait. generate more compact code

58d4238

improve naming

bfa3e9b

fmt

d23ec04

LucioFranco reviewed Oct 13, 2019

View reviewed changes

alce added 2 commits October 14, 2019 20:07

fix interop crate

405b5e3

hide RequestMessage trait from docs

d40ac7e

alce marked this pull request as ready for review October 15, 2019 01:24

hawkw reviewed Oct 18, 2019

View reviewed changes

alce added 2 commits October 18, 2019 18:03

use impl Trait instead of type parameter

1acf426

remove RequestMessage trait

0e9fe70

alce requested a review from LucioFranco October 18, 2019 23:38

hawkw approved these changes Oct 19, 2019

View reviewed changes

Merge branch 'master' of github.com:hyperium/tonic

3f6ff67

alce added 3 commits October 18, 2019 21:24

conflicts

03d7a16

Merge branch 'intorequest' of github.com:alce/tonic into intorequest

4439002

document IntoRequest

878120c

LucioFranco reviewed Oct 22, 2019

View reviewed changes

tonic/src/request.rs Outdated Show resolved Hide resolved

alce added 2 commits October 22, 2019 16:56

document IntoStreamingRequest

2f93f33

Merge branch 'master' into intorequest

0528352

alce added 2 commits October 24, 2019 14:39

remove IntoRequest in favor of From impl

adbbe8c

Merge branch 'intorequest' of github.com:alce/tonic into intorequest

b32b4e9

hawkw approved these changes Oct 24, 2019

View reviewed changes

tonic/src/request.rs Outdated Show resolved Hide resolved

tonic-build/src/client.rs Outdated Show resolved Hide resolved

tonic/src/request.rs Outdated Show resolved Hide resolved

seal IntoStreamingRequest

9a9ee93

alce requested a review from LucioFranco October 26, 2019 15:46

remove From<T> for Request, implement IntoRequest instead

8802192

LucioFranco reviewed Oct 29, 2019

View reviewed changes

improve docs

7017f97

LucioFranco approved these changes Oct 29, 2019

View reviewed changes

LucioFranco changed the title ~~IntoRequest and IntoStreamingRequest traits~~ feat: Add IntoRequest and IntoStreamingRequest traits Oct 29, 2019

LucioFranco merged commit 4bb087b into hyperium:master Oct 29, 2019

rabbitinspace pushed a commit to satelit-project/tonic that referenced this pull request Jan 1, 2020

feat: Add IntoRequest and IntoStreamingRequest traits (hyperium#66)

227e348

davidli2010 mentioned this pull request Jul 15, 2021

helloworld example report unimplemented error #724

Closed

dae mentioned this pull request Jun 20, 2023

Adding an IntoResponse trait #1416

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `IntoRequest` and `IntoStreamingRequest` traits #66

feat: Add `IntoRequest` and `IntoStreamingRequest` traits #66

alce commented Oct 10, 2019

LucioFranco Oct 13, 2019

alce Oct 13, 2019 •

edited

Loading

LucioFranco Oct 14, 2019

hawkw Oct 18, 2019

hawkw Oct 18, 2019

alce Oct 18, 2019

alce commented Oct 15, 2019

LucioFranco commented Oct 18, 2019

LucioFranco commented Oct 18, 2019

hawkw Oct 18, 2019

hawkw Oct 18, 2019

LucioFranco commented Oct 19, 2019

alce commented Oct 19, 2019

alce commented Oct 23, 2019 •

edited

Loading

LucioFranco commented Oct 23, 2019

hawkw commented Oct 23, 2019

alce commented Oct 23, 2019 •

edited

Loading

hawkw commented Oct 23, 2019

alce commented Oct 24, 2019

LucioFranco commented Oct 24, 2019

alce commented Oct 24, 2019

alce commented Oct 24, 2019

hawkw left a comment

LucioFranco Oct 29, 2019

LucioFranco Oct 29, 2019

alce Oct 29, 2019

LucioFranco left a comment

LucioFranco Oct 29, 2019

feat: Add IntoRequest and IntoStreamingRequest traits #66

feat: Add IntoRequest and IntoStreamingRequest traits #66

Conversation

alce commented Oct 10, 2019

Motivation

Solution

Choose a reason for hiding this comment

alce Oct 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alce commented Oct 15, 2019

LucioFranco commented Oct 18, 2019

LucioFranco commented Oct 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucioFranco commented Oct 19, 2019

alce commented Oct 19, 2019

alce commented Oct 23, 2019 • edited Loading

LucioFranco commented Oct 23, 2019

hawkw commented Oct 23, 2019

alce commented Oct 23, 2019 • edited Loading

hawkw commented Oct 23, 2019

alce commented Oct 24, 2019

LucioFranco commented Oct 24, 2019

alce commented Oct 24, 2019

alce commented Oct 24, 2019

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: Add `IntoRequest` and `IntoStreamingRequest` traits #66

feat: Add `IntoRequest` and `IntoStreamingRequest` traits #66

alce Oct 13, 2019 •

edited

Loading

alce commented Oct 23, 2019 •

edited

Loading

alce commented Oct 23, 2019 •

edited

Loading