seanmonstar
I think `Either` should implement `Serialize` with `#[serde(untagged)]`
Let's say I have several routes, and the handler for each route returns something that I want to then serialize into json for the body of my response.
use warp::*;
fn main() {
// Option 1: put a `.map(|res| reply::json(&res))` in every route
let filter = path("foo")
.map(foo)
.map(|res| reply::json(&res))
.or(
path("bar")
.map(bar)
.map(|res| reply::json(&res)));
// when every route has something in common, it would be nice if we could
// factor out that common filter to before / after the `or`s, so that we're not repeating ourselves.
// Option 2: use `.unify()`
let filter = path("foo")
.map(foo)
.or(
path("bar")
.map(bar))
.unify()
.map(|res| reply::json(&res));
// now my handlers have to all return the same type. I could define an enum with a variant
// for each handler, or I could return `serde_json::Value`s, but both these solutions feel kinda forced
// Option 3: have my handlers return `reply::Json` directly
let filter = path("foo")
.map(foo)
.or(
path("bar")
.map(bar));
// this makes my code cleaner, but now my handler functions are tied to `warp` types. This
// is fine most of the time, but I'd prefer to have clean separation between my internal logic and my api.
// My suggestion: throw a `#[derive(Serialize)] #[serde(untagged)]` on `Either`, so that we can do this:
let filter = path("foo")
.map(foo)
.or(
path("bar")
.map(bar))
.map(|res| reply::json(&res));
}
I think that #[derive(Serialize)] #[serde(untagged)] makes sense for Either in terms of what it represents. An Either<A, B> should behave like an A when it's an A and like a B when it's a B. With #[serde(untagged)], it will be serialized the same way as the type it contains: serde docs
Actually, now that I think about it, I think we wouldn't be able to use the derive macro, because we only want Either<A, B>: Serialize when A: Serialize, B: Serialize. However, it wouldn't be hard to implement Serialize manually, to behave the same as it would with #[serde(untagged)].
One last thing, and maybe this should be a separate issue: you'll notice that i've been using .map(|res| reply::json(&res)) instead of .map(reply::json). If we take a look at the signature of reply::json:
pub fn json<T>(val: &T) -> Json
where
T: Serialize,
{ ... }
reply::json only takes references. I think I understand why this was done, because reply::json only needs read access, so it shouldn't have to take ownership of it's argument. However, serde provides a blanket impl Serialize for &T where T: Serialize, so we could change the signature to take T directly. This way we can pass owned types to reply::json when it's more convenient, and we still have the option to pass a reference when we need to keep ownership.
Do people agree that we should make these changes? I don't think the first change is breaking. The second change might affect the behavior of existing code where people pass a boxed value to reply::json, but I'm not sure. I'm happy to put together a pull request if people are interested.
I agree with this. The desired behavior of Either is to do one thing or another, and having a tagged variant seems to interfere with that goal.
I agree with this. The desired behavior of
Eitheris to do one thing or another, and having a tagged variant seems to interfere with that goal.
To be clear, Either doesn't implement Serialize at all at the moment. What I'm really suggesting is that Either should implement Serialize. I'm glad you agree that untagged would be the clear way to do that.
