feat: Allow bounded numbers to be deserialized from strings

[DrChat]

Fixes #295.

This allows libraries like serde-html-form to deserialize bounded integers from strings.

[sugyan]

I didn't know that this kind of conversion is possible.
But I don't understand why it is necessary to deserialize from string to integer. Can you tell me in what specific cases this is necessary?

[sugyan]

Oh... sorry, I didn't read the #295 one. I see, so you are saying that this kind of conversion is necessary when HTTP query parameters are received, etc.

[DrChat]

All good! And yup - axum will treat all query parameters as strings when performing deserialization, so we'd have to handle that here in order to be compatible.

I'm also open to other ways to implement this too - this just seemed to be the best way to do it based on what I saw :)

[sugyan]

I have no objection to your implementation way.
However, I am wondering if I can accept the change of being able to deserialize from strings to bounded numbers. I think this conversion is irreversible and should not be deserializable under normal circumstances.
How about not changing the current behavior and only allowing the conversion you wrote if, for example, the deserialize_numbers_from_string feature is enabled?

[DrChat]

Okay, that is a reasonable concern!

So I found an alternate mechanism to implement the functionality I needed.
I can lean on forward_parsed_value! as it is implemented in serde-html-form in order to implicitly convert the string inputs into integers. That way, we don't have to implement the conversions on our end.

The specific requirements on us are: we must implement FromStr for each bounded integer type, and we must call deserialize_$primitive when deserializing.

I've confirmed that this works via testing, but I have not checked in those tests because they would introduce a dev dependency on serde-html-form :)

[DrChat]

Hrm. So the tests I write pass in isolation, but axum's Query extractor is still exhibiting the same limitation and failing to deserialize structures with these bounded types.
The only variable I can see is that they use serde_path_to_error and my tests do not.

For posterity, here is the test I have written that passes:

    #[test]
    fn query_deserialize() {
        #[derive(Deserialize)]
        struct S {
            #[serde(skip_serializing_if = "core::option::Option::is_none")]
            pub cursor: core::option::Option<String>,
            #[serde(skip_serializing_if = "core::option::Option::is_none")]
            pub limit: core::option::Option<crate::types::LimitedNonZeroU16<1000u16>>,
        }

        let s = serde_urlencoded::from_str::<S>("cursor=abcd&limit=100").unwrap();
        assert_eq!(s.limit.unwrap().0, NonZeroU16::new(100).unwrap());
    }

Edit: Okay, it appears that we should not be wrapping query parameters in atrium_api::types::Object.
I've confirmed that if I manually unwrap the parameters (e.g. Query(input): Query<sync::list_repos::ParametersData>), axum is successfully able to deserialize them.

This is a separate issue that I'm not going to fix in this pull request :)

[sugyan]

Thank you for continuing to work on this issue.
It is great to know that serde-html-form can deserialize from a string just by implementing FromStr.
I have no problem adding serde-html-form to dev-dependencies. I have sent a pull-request to you in DrChat#1.
However, when I try to deserialize by serde_json, the deserialization does not seem to succeed even if I pass a number.
Perhaps only the implementation of FromStr is needed to fix this problem, and the change to implement deserialize by Visitor using paste is no longer needed...?
I would be glad if you could confirm this.

[DrChat]

So it's actually expected that serde-json will fail to perform the conversion from a string to an integer.
This all depends on the serialization format.

For json, everything is strongly typed enough that they do not need to implement implicit conversions.
For serde-urlencoded (which I think is the actual crate used by axum - I was wrong earlier), they use forward_parsed_value! to perform implicit conversions.

TY for the pull request btw!

[sugyan]

I'm thinking .map_err(|e| e.to_string()) would be fine, but what do you think?

[sugyan]

I've written tests only for LimitedU8, I would be glad if you could add tests for other types as well. If you decide that the current ones are sufficient, you can just delete the // TODO.