Implement Serialize for Errors

[radix]

When I'm implementing an API server using JSON for message passing, I want to be able to send the client the details of the error so that client developers can better debug their implementations. It'd be nice if I could send those details in a structured form, instead of just the Debug representation.

[dtolnay]

Seems reasonable. To clarify - you mean specifically the serde_json::Error type and not std::error::Error types in general, right?

[radix]

@dtolnay yep, just the serde_json one. I can have a crack at implementing this. Do you think it will be as simple as deriving the Serializable trait on the various error types?

[dtolnay]

That would tend to give you a pretty verbose representation like:

{
    "Syntax": [
        {
            "MissingField": "f"
        },
        10,
        20
    ]
}

I think we should instead implement Serialize to serialize just the Display representation:

"missing field 'f' at line 10 column 20"

and implement Deserialize to go through serde::de::Error::custom to convert the string back to an Error. In #184 I intend to hide the internal representation anyway so it becomes just an implementation detail / performance knob.

[radix]

That representation is actually exactly what I wanted :) (although maybe in an object instead of an array, so 10 and 20 are associated with some descriptive keys).

If I just wanted the display, I could just to_string(format!("{}", e)).

[radix]

But indeed, any structured serialization would have to wait on #184.

[dtolnay]

Can you explain the use case where you need the structured representation? As far as I could tell from some GitHub searches, nobody is currently pattern-matching on our error type ("if it is a MissingField, do this, if it is an InvalidType, do this, etc"). I can understand if you need just the position information; would this be okay?

{
  "message": "missing field 'f'",
  "line": 10,
  "column": 20
}

The thing I would like to avoid is committing to a particular error representation in our public API if nobody is using it. We have had problems with that in the past in terms of not being able to add error variants for new error situations because that would be a breaking change.

[PSeitz]

Some text snippet from the failed position would also be nice, that's what you probably need at the end

[svenstaro]

@dtolnay Hey, any chance you could pick this up? This would be quite neat to allow for proper error feedback for users when something fails to deserialize when it's sent to one's API. I'm not really sure how to work around this since I can't implement traits for foreign crates so it would ideally be in serde_json itself.

[dtolnay]

Due to https://github.com/serde-rs/serde/issues/1070 I'm pretty sure a Deserialize impl for serde_json::Error would not be a good idea — that would make Result<T, serde_json::Error> automatically implement Deserialize which is a mess when someone writes too many or too few ? or unwrap and the resulting failure only occurs at runtime.

Implementing only Serialize would perhaps be justifiable, but at that point I think I'd recommend using .to_string() explicitly on the error before serializing it, or equivalently something like DisplayFromStr, and keep it clear that the other end is expected to deserialize it as a string not an Error.

[kangalio]

https://github.com/serde-rs/serde/issues/1070 is locked so I can't post it there, but if https://github.com/rust-lang/rust/issues/39935 goes through, the Result impl can be deprecated. That way, someone cannot use that impl accidentally anymore

[dtolnay]

I am not confident about https://github.com/rust-lang/rust/issues/39935 happening, or being sufficient mitigation if it does happen in some form.

From the discussion above, it's pretty clear to me there aren't compelling use cases for this that would outweigh the downsides, so I'll go ahead and close the issue.