Refactor: RaftError: serde derive

[drmingdrmer]

Changelog

Refactor: RaftError: serde derive

The problem is not the bound on the E generic, but rather, the fact
that NID is receiving two different Serialize/Deserialize bounds;
one from the derive annotation, and one from the NID: NodeId trait
bound.

This modification is from suggestions by @tvsfx about the serde bound:
#991 (comment)

---

This change is 

[drmingdrmer]

@tvsfx

[tvsfx]

Great!

[drmingdrmer]

Great!

Thanks again!

[tvsfx]

One more note; RPCError seems to have a similarly unnecessarily tight bound that could be split up into separate Serialize and Deserialize bounds. The trick with the bound="" does not work here, because the correct bound for the E generic is not inferred; presumably serde cannot tell how E is used inside RemoteError<NID, N, E>.

[drmingdrmer]

@tvsfx

Thanks for the heads-up! 😊

Do you know of any specific scenarios where this current declaration might run into trouble? I'd love to whip up some tests to double-check that everything works smoothly in those cases. Any insights you have would be super helpful!

[tvsfx]

The current specification is fine functionally speaking; what I tried to highlight is more of a usability issue. The issue is two-fold.

Let me illustrate by inspecting the output of cargo expand error -p openraft --lib --features serde in the openraft repo.

The derived RaftError instance (using the new bounds style) looks as follows:

      impl<NID, E> _serde::Serialize for RaftError<NID, E>
       where
           NID: NodeId,
           E: _serde::Serialize,
       {...}

The RPCError instance (using the old bounds style) looks as follows:

       impl<NID: NodeId, N: Node, E: Error> _serde::Serialize for RPCError<NID, N, E>
        where
            E: serde::Serialize + for<'d> serde::Deserialize<'d>,
        { ... }

Any wrapping struct or enum that internally uses RPCError<NID,N,E> and tries to derive Serialize (or Deserialize for that matter) will have the automatic derivation fail, because serde tries to derive an instance by naively, recursively putting Serialize(Deserialize) bounds on all generics that are used by fields. In this case, the instance it tries to derive will say something along the lines of "if NID & N & E (whichever ones of these are kept generic in the wrapping struct/enum) are serializable, then the whole wrapping struct is serializable". However, this instance does not typecheck, because what you say by specifying the bound in RPCError is "if E is serializable and deserializable, then RPCError is serializable". This precondition is not met. The wrapping type will hence need to copy this bound explicitly for the derived instance to typecheck (i.e. write a manual bound annotation).

Additionally, any location that wants to impl methods for this wrapping struct or enum and needs it to be deserializable xor serializable will need to annotate the impl block with a bound E : serialize + deserialize, whereas normally either one or the other would suffice. This can be annoying for e.g. a server implementation that does not care about deserialization.

The new derived RaftError bounds runs into neither one of those issues. The RPCError bound can be fixed by writing:

/// Error occurs when invoking a remote raft API.
#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
#[cfg_attr(
    feature = "serde",
    derive(serde::Deserialize, serde::Serialize),
    serde(bound(serialize = "E: serde::Serialize")),
    serde(bound(deserialize = "E: for <'d> serde::Deserialize<'d>"))
)]
pub enum RPCError<NID: NodeId, N: Node, E: Error> 

[tvsfx]

On a related note, I see some explicit serde bounds on structs without generics, which I do not think you need. For example:
https://github.com/datafuselabs/openraft/blob/49e8adfebd79a556d74045490a1b96d3f7cb6a5c/openraft/src/error.rs#L407
You should just be able to elide these bounds.

For some more info, see here: https://serde.rs/attr-bound.html

[drmingdrmer]

The current specification is fine functionally speaking; what I tried to highlight is more of a usability issue. The issue is two-fold.

Let me illustrate by inspecting the output of cargo expand error -p openraft --lib --features serde in the openraft repo.

Thank you for the detailed explanation.

I believe the refinement you suggested will make the openraft more approachable and its behavior more predictable.

Let me fix these issues. :D