Fix #861 serde::Deserialize impl for Url prints confusing error message

[justy777]

Fixes #861

[denschub]

I think a better implementation would be

E::invalid_value(Unexpected::Other(&err_s), &self)

which then would keep the original "why it's broken" info intact:

invalid value: invalid port number, expected a string representing an URL

or even simpler:

Url::parse(s).map_err(E::custom)?;

which still would be fine:

invalid port number

[justy777]

@denschub Thanks for your attention on my pull request.

I respectfully disagree with your implementation as it does not pass the parameter intended in serde's API and causes a different problem.

https://docs.rs/serde/latest/serde/de/trait.Error.html#method.invalid_value

"The unexp argument provides information about what value was received. This is the value that was present in the input file or other source data of the Deserializer."

"err_s" is not the invalid value, and the resulting message will not print the invalid value.

In a real world you could have several fields of type Url, if you don't print the invalid value, you'd have no idea which of the fields is wrong.

I understand that the reason it's invalid is relevant information, but it doesn't fit into serde's API. serde is meant to serialize and deserialize, not to validate. validation should be done separately, and can be accomplished with the validator crate instead.

[denschub]

The documentation for Unxpected::Other describe the parameter to it as

A message stating what uncategorized thing the input contained that was not expected.

an invalid port number is "a thing", it was not expected, and the input contained it. Your PR changes the implementation in a way that's even less helpful. Seeing a

invalid value: string "http://localhost:804211/", expected a string representing an URL

does not help anyone figure out why the parsing failed. "serde is meant to parse, not to validate" is a valid statement, but you need to parse before you can validate, and nobody is helped if parsing fails for reasons not clear to anyone, and for reasons that are not communicated. Also, what is being returned here is the result of Url::parse, so it's kinda by definition a parse-error, not a validation-error. And yes, it's a strict parser, but that's not bad either, and a strict parser should at least communicate why it's refusing to parse something.

Ultimately, though, I'm not a maintainer of this create. And I don't really mind either way, because I have ways to work around both solutions.

[justy777]

Those are good points. serde could use an API that allows you to better communicates why it refuses to parse something.

Though in my opinion, if you want to provide a feature to provide an implementation for a particular trait it should do so in a way that's predictable and consistent with standard/core implementations for that trait.

At the end of the day, I ignore the "serde" feature and just use the NewType pattern over Url to implement serde::Serialize and serde::Deserialize or whatever functionality I happen to need. After almost a year with no comment or response from anyone, I don't see that changing anytime soon.

Thanks again for being the first to comment.