Some ideas to make command interface simpler and uniform

[Retamogordo]

Hi,
I would like to share some thoughts on how the command rpc interface could be further developed to make it simpler and uniform.
I'm afraid this will be a long read so hopefully you have time and patience for this.
Consider this typical use case:

        let mut cmd = crate::project::ShowCommand {
            // omitted
        };
        let mut rpc = RpcBuilder::new(ctx, opts, &nc.name).tcp(tcp).build()?;
        rpc.request(api::project::show(&cmd)).await?;
        let project = rpc.parse_response::<Project>()?;

The rpc request instance is type-parametherized by the type of the command, while the request method has additional parametherization of the return type of api::project::show(&cmd).
Yet another type-parametherization is applied on the response method.
Meanwhile the command type defines one-to-one relationship between the two latter types.
This allows to simplify the above code to roughly this:

        let mut cmd = crate::project::ShowCommand {
            // omitted
        };
        let mut rpc = RpcBuilder::new(ctx, opts, &nc.name).tcp(tcp).build()?;
        rpc.request(&mut cmd).await?;
        let project = rpc.parse_response(&mut cmd)?;

Now, having type signatures uniform we can add a method that covers the common use case:

        let mut cmd = crate::project::ShowCommand {
            // omitted
        };
        let mut rpc = RpcBuilder::new(ctx, opts, &nc.name).tcp(tcp).build()?;
        let project = rpc.request_then_response(&mut cmd).await?;

To do so a very simple trait can be introduced:

// commands implements this
pub trait RpcCaller<'a> {
    type Req: Encode<()>;
    type Resp: Decode<'a, ()> + Output + serde::Serialize;

    fn req(&'a mut self) -> RequestBuilder<'_, Self::Req>;
}

and the implementation example:

impl<'a> RpcCaller<'a> for ShowCommand {
    type Req = BareCloudRequestWrapper<'a>;
    type Resp = Project<'a>;

    fn req(&'a mut self) -> ockam_core::api::RequestBuilder<'_, Self::Req> {
        api::project::show(self)
    }
}

Let's now move on to another thing and consider the current implementation of the Rpc struct:

pub struct Rpc<'a> {
    ctx: &'a ockam::Context,
    buf: Vec<u8>,
    opts: &'a CommandGlobalOpts,
    cfg: NodeConfig,
    to: Route,
    /// Needed for when we want to call multiple Rpc's from a single command.
    tcp: Option<&'a TcpTransport>,
}

which holds references with a lifetime 'a.
On the other hand Rpc holds buf field which is referenced by an instance of Decoder, which is returned to a caller by an Rpc method (parse_response).
The instance of Decoder is also marked with the lifetime 'a'.
I would think that those lifetimes are not necesarilly identical and some kind of coercion occurs there, namely in this signature:

    pub fn parse_response<T>(&'a self) -> crate::Result<T>

This means that whenever a Decoder instance is somewhere in scope, the Rpc is borrowed and,
transitively, all the references it holds, too.
For the sake of example, let's mark the lifetime of &ctx by 'c and the lifetime of the Decoder by 'd. So 'c is not necessarily narrower than 'd.
This could be expressed as 'c: 'd.
I tried to play and split these lifetimes like so, and it compiles:

// some details are omitted here
pub struct Rpc<'c: 'd, 'd> {
    ctx: &'c ockam::Context,
    cmd: T,
    buf: Vec<u8>,
    opts: &'c CommandGlobalOpts,
    cfg: NodeConfig,
    to: Route,
    /// Needed for when we want to call multiple Rpc's from a single command.
    tcp: Option<&'c TcpTransport>,
    _marker: &'d PhantomData<T>,
}

But after all, this was to little avail, because although more relaxed, the entanglement is still there.
So, the next step I tried was to yield the ownership to the caller, so the Rpc instance can peacefully die after its job of retrieving response is done and the parse job is completed by the RpcCaller implementor, after all it defines the type of the response and knows how to parse it.
To do so a very slim wrapper struct is introduced:

pub struct RpcResponse<'a, T: RpcCaller<'a>>(Vec<u8>, &'a PhantomData<T>);

It just holds the raw data and implements all parse methods which are actually held by Rpc struct.

For instance:

    pub fn parse_body(&'a self) -> crate::Result<T::Resp> {
        let mut dec = self.parse_response_impl()?;
        Ok(dec.decode().context("Failed to decode response body")?)
    }

This allows to end up with this variation of the original example:

        let mut cmd = crate::project::ShowCommand {
            // omitted
        };

        let project = RpcBuilder::new(ctx, opts, &nc.name)
            .tcp(tcp).build()?
            .request_then_response(&mut cmd).await?.parse_body()?.to_owned();

resulting in a uniform interface for common use cases.
On the command implementation side things also get simpler.

I have a branch where I played around with these things, I implemented a couple of commands just to see it compiles, and even tested one of them.
For further testing I lack comprehension of the system, but if someone is interested in these things I could publish it.

[mrinalwadhwa]

@Retamogordo thank you for that exploration. Please publish the branch - maybe in a Work In Progress - draft PR. I'll read and share thoughts soon.

I'm about to push a bats test file, with some common usages, so that may help with some testing along the way.

[mrinalwadhwa]

cc @adrianbenavides @twittner @SanjoDeundiak @antoinevg

[Retamogordo]

Okay, I'll publish this in the next days, I still want to have a bit of time to polish things, of course if you guys think it worth the time.
I just suddenly had another idea of implementing response parsing as one way state machine at compile time, so if a programmer forces wrong sequence of decoding, it would not compile. What do you think ?

[antoinevg]

In my experience type states are usually well worth the time to implement with the caveat that rust docs should always include a verbose example of how the resultant API is used!

[Retamogordo]

Yes, otherwise it becomes tricky to use :)

[adrianbenavides]

Some initial thoughts:

1. You don't always want to create the rpc request out of a command. We have some cases where we build the request out of simple variables.
2. The RpcCaller trait would add a layer of complexity just to avoid the response parsing parameterization. On top of that, and related to the previous point, it makes the Rpc abstraction less flexible, which might get in the way.
3. A slight modification on the request method to make it return &Rpc would make the end result very similar to what you proposed:

RpcBuilder::new(ctx, opts, &nc.name)
    .tcp(tcp).build()?
    .request(api::project::create(&cmd)).await?.parse_response::<Project>()?.to_owned()

Looking forward to that PR!

[Retamogordo]

Hi !
I think the main point in this story is that response holds the Rpc (and whatever Rpc instance refers to) borrowed during its lifetime.
But actually the response does not need them. Maybe in current use this is not an issue, but in an imaginary situation that for some reason you need to send the response further to a wider scope (e.g. for some custom parsing), it will still drag all those borrowed things with it.
I suspect that this is why the Rpc struct needs to hold these methods of parsing and printing.
So whatever functionality one wants to add to the response they will need to do it through Rpc struct.
I saw that to_owned() function usage, frankly I didn't look at it inside, but maybe it is intended to do this explicit ownership take ?
Actually I started to look at this code because I was playing with blocking version of this interface (node_rpc) and I faced certain difficulties to bring my code to compile because of borrowed things, I don't remember exactly though what the trouble was.
As for RpcCaller, yes, anything that wants to do rpc, would need the request and response type, roughly like so:

impl RpcCaller for () {
    type Req = ();
    type Resp = ();

    fn req(&mut self) -> ockam_core::api::RequestBuilder<'_, Self::Req> {
/// ....
    }
}

Possibly it could amended for more ergonomics, but the idea behind is to take the ownership of the response just after the raw data is ready