1.0 better internal serde usage

[TroyKomodo]

Hello!
I noticed you are in the process of a 1.0 stabilization.

While going through some of the changes I noticed a few things I wanted to raise attention of which may seem insignificant but I find quite important when writing a serde::de::Deserializer

1. If the format we are implementing has a direct representation for the type they are requesting then we forward the call todeserialize_any .

   For example, if we can represent all integer types i8 i16 i32 i64 and they request an i8 and we have an i32 we should return the i32 do not try and coerce the type.

2. If we cannot represent the type try coerce the value we have into the value they expect, and if we cant do that forward to deserialize_any

   For example if we only have f64 as the only number type and they request an i32 we try convert our f64 into an i32 or if we don’t have a number type and numbers are typically represented as strings then try parse the number from a string.

3. The first two rules imply this, however for the case of being explicit. Avoid returning errors from the deserializer at all costs. Always forward to the visitor. Of course if the serialized content is invalid then that is an acceptable error, but never return an error for mismatched type vs expectations. (ie, they called deserialize_string and we have map just call visit_map instead of throwing an error.

-- Actual Examples from the code

serde_qs/src/de.rs

Lines 473 to 490 in f24f916

	fn deserialize_tuple<V>(
	self,
	len: usize,
	visitor: V,
	) -> std::result::Result<V::Value, Self::Error>
	where
	V: de::Visitor<'de>,
	{
	if let Some(seq_len) = self.seq_length() {
	if seq_len != len {
	return Err(Error::custom(
	format!("expected a tuple of length {len}, got length {seq_len}"),
	&self.0,
	));
	}
	}
	self.deserialize_seq(visitor)
	}

During deserialize_tuple we should not do a bounds check since this isnt a "real error" (such as an invalid state or deserialization from the underlying text buffer), this is just a check that is done in code. We should just forward what we have to the visitor and then let the visitor return the error for us. Who knows, maybe the visitor is able to handle this less then full length tuple situation.

serde_qs/src/de.rs

Lines 577 to 588 in f24f916

	fn deserialize_unit<V>(self, visitor: V) -> Result<V::Value>
	where
	V: de::Visitor<'de>,
	{
	if matches!(self.0, ParsedValue::NoValue | ParsedValue::Null)
	|| matches!(self.0, ParsedValue::String(ref s) if s.is_empty())
	{
	visitor.visit_unit()
	} else {
	Err(Error::custom("expected unit".to_owned(), &self.0))
	}
	}

During deserialize_unit like in the tuple function always forward to deserialize_any (rule 1) if we dont have the type the visitor expects and we have a way of representing it in our state.

In some places this rule is upheld like such

serde_qs/src/de.rs

Lines 613 to 630 in f24f916

	fn deserialize_enum<V>(
	self,
	_name: &'static str,
	_variants: &'static [&'static str],
	visitor: V,
	) -> Result<V::Value>
	where
	V: de::Visitor<'de>,
	{
	match self.0 {
	ParsedValue::Map(mut parsed) => visitor.visit_enum(MapDeserializer {
	parsed: &mut parsed,
	field_order: None,
	popped_value: None,
	}),
	ParsedValue::String(s) => visitor.visit_enum(StringParsingDeserializer::new(s)?),
	_ => self.deserialize_any(visitor),
	}

I propose this should always be the case since from the Deserializer's perspective the deserialize_x functions are mealy "hints" as to what the visitor would like us to return, if we cannot convert to that type we should just give the visitor what we have and hope they can figure it out, in most cases they likely wont be able to and they will return an error, but we shouldn't impose hard limits on visitor implementations.

[samscott89]

Thanks for the feedback @TroyKomodo!

Out of curiosity, do you have a source/reference for these principles? I'd love to be able to link them in the module documentation if possible as future implementation guidelines.

I think I mostly understand/agree with the rationale. The reason I put the length check in place for the tuple one is that it's a way of signalling that we have values left in the sequence that weren't read. I was curious what serde_json does here, which just returns a fairly unhelpful error:

use serde::{Deserialize, Serialize};
use serde_json; // 1.0.140

fn main() {
    let json = serde_json::to_string(&(1u8, 2u8)).unwrap();
    println!("{json}");
    
    let ret: (u8,) = serde_json::from_str(&json).unwrap();
    assert_eq!(ret, (1,));
}

returns

thread 'main' panicked at src/main.rs:10:50:
called `Result::unwrap()` on an `Err` value: Error("trailing characters", line: 1, column: 4)

which has a strong assumption that once the sequence is done it's done.

In this case though, I'm okay with being more tolerant with the deserialization logic here, because I agree we can leave it up to the Deserialize impl.

On:

If the format we are implementing has a direct representation for the type they are requesting then we forward the call to deserialize_any.

For example, if we can represent all integer types i8 i16 i32 i64 and they request an i8 and we have an i32 we should return the i32 do not try and coerce the type.

I'm struggling to see the logic here. And especially not sure how to apply to serde_qs. One problem we face is that querystrings are an ad hoc format and not formally specified. Furthermore, it's not self-describing in any meaningful way and almost everything is encoded as a string. Due to this, I think it's really important for us to rely on the deserialize_X hint to allow us to (e.g.) coerce a string into a integer. Are you saying that we shouldn't try?

Or is this more like -- don't be too clever and try and convert a map to a sequence? If we've deserialized a map and someone calls deserialize_seq then just visit the map still?

If we cannot represent the type try coerce the value we have into the value they expect, and if we cant do that forward to deserialize_any

Okay, I like this a lot! I think the error messages will be nice for the user too

[samscott89]

Okay, this is my understanding of what's proposed: https://github.com/samscott89/serde_qs/pull/147/files#diff-4a0f582df1b71f5f2f747348960351a7dbd4e5e8fb3c5c7b2a8f74b55ef27fc5 how does that look @TroyKomodo ?

[TroyKomodo]

Out of curiosity, do you have a source/reference for these principles? I'd love to be able to link them in the module documentation if possible as future implementation guidelines.

These are mostly mine that I've come up with when working on my projects, however it seems that serde-rs/serde#2811 is an interesting read if you are curious.

The major reason I advocate for always deferring to the visitor is because of error handling. I recently wrote a crate tinc which is a gRPC to REST transcoding system, as part of that I wanted to be able to return all errors (both in validation, and deserialization) that were encountered when handling a REST request. I realized that if the deserializer follows those principals we can very easily collect all errors instead of stopping on the first one.

I'm struggling to see the logic here. And especially not sure how to apply to serde_qs. One problem we face is that querystrings are an ad hoc format and not formally specified. Furthermore, it's not self-describing in any meaningful way and almost everything is encoded as a string. Due to this, I think it's really important for us to rely on the deserialize_X hint to allow us to (e.g.) coerce a string into a integer. Are you saying that we shouldn't try?

No not at all, I think in the case of serde_qs since everything is essentially

type Item = String | Map<String, Item> | Array<Item>;

you should try to parse everything that isnt either a string / map / array because those have direct representations in the format. ie, we should parse numbers / enums / tuples / structs / etc... For calls to deserialize_seq if we have a map, we shouldnt convert it into a sequence because if it was a sequence it would have been represented like that. If that makes sense?

[samscott89]

For calls to deserialize_seq if we have a map, we shouldnt convert it into a sequence because if it was a sequence it would have been represented like that. If that makes sense?

Okay yep, that example makes sense.

What I'm taking away from this: we're effectively responsible for choosing how tolerant to be when the Deserialize implementation asks for a specific type via the deserialize_X hint.

In many cases the decision is not too hard, for example, I would expect most types should roundtrip cleanly, which means if you want to support it, you should be prepared to deserialize it. For serde_qs that comes into play mostly with numerics, since querystrings don't inherently differentiate numbers vs strings (in the same way that, e.g. json does).

But there are other cases where it's less obvious. I think the map -> seq one is a good example. And what makes that one hard in serde_qs is that we represent sequences as x[0]=1&x[1]=2&... which is equivalent to a map with integer keys. Given this, I think I was tempted to be overly clever "well, any map can be made a sequence if you just take the values of the map". But, yeah, it seems like if someone passes in x[a]=1&x[b]=2 and asks for a sequence. We're much better off passing them back the map and asking them to handle it themselves. If someone only actually needs the values, then they're free to implement that themselves. This feels like a version of principle of least surprise to me.

[samscott89]

Thanks again for the feedback, this is great