Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extract::Path doesn't work with types using serde::Deserializer::deserialize_any #1678

Closed
paolobarbolini opened this issue Jan 5, 2023 · 5 comments · Fixed by #1693
Closed
Labels

Comments

@paolobarbolini
Copy link
Contributor

Feature Request

Motivation

PathDeserializer doesn't support deserialize_any 1 so it doesn't work with things like time::Date 2

Proposal

Add support for deserialize_any. I might be able to look into it in the following days.

Alternatives

Create a wrapper around the type and force it to use a specific deserializer

Footnotes

  1. https://github.com/tokio-rs/axum/blob/87a80ec47bfd2c39ac4ea3d1acffb5d1ff540f63/axum/src/extract/path/de.rs#L59

  2. https://github.com/time-rs/time/blob/d97594b9f5cfeab0335c16687e26a8b51c56a526/time/src/serde/mod.rs#L124

@paolobarbolini paolobarbolini changed the title extract::Path doesn't work with types using serde::de::Deserializer::deserialize_any extract::Path doesn't work with types using serde::Deserializer::deserialize_any Jan 5, 2023
@davidpdrsn
Copy link
Member

davidpdrsn commented Jan 5, 2023

What would such an implementation look like? Afaik URL params aren't a self describing format so when you encounter some key/value pair how do you know what to deserialize that into?

I check and the Path extractors in poem actix-web don't support deserialize_any either, though there is a PR for actix-web for forwards it to deserialize_str (actix/actix-web#2881). I'm not sure thats right 🤔

@jplatte
Copy link
Member

jplatte commented Jan 5, 2023

I think if we support it, deserialize_str is the right implementation.

@paolobarbolini
Copy link
Contributor Author

paolobarbolini commented Jan 5, 2023

serde_urlencoded forwards deserialize_any to deserialize_map and it seems to work fine for them, so it should make sense for us to forward it to deserialize_str
https://github.com/nox/serde_urlencoded/blob/d8bc15d16adf6b3ba6ae46d199d7109cf5079efa/src/de.rs#L102

The playground doesn't enable the right time features for it to work, but you can test it locally:

Cargo.toml

[package]
name = "test123"
version = "0.1.0"
edition = "2021"

[dependencies]
serde = { version = "1", features = ["derive"] }
time = { version = "0.3", features = ["serde-human-readable", "parsing"] }
serde_urlencoded = "0.7"

main.rs

use serde::Deserialize;
use time::Date;

fn main() {
    #[derive(Debug, Deserialize)]
    struct Stuff {
        date: Date,
    }

    let deserialized: Stuff = serde_urlencoded::from_str("date=2022-01-01").unwrap();
    println!("{deserialized:?}");
}

stdout

Stuff { date: 2022-01-01 }

@jplatte
Copy link
Member

jplatte commented Jan 5, 2023

I think the more interesting question for serde_urlencoded is, what does deserialize_any do for the individual keys and values (represented as Part<'de> internally)?

@davidpdrsn
Copy link
Member

davidpdrsn commented Jan 13, 2023

It seems serde_urlencoded also just calls deserialize_str for values (https://github.com/nox/serde_urlencoded/blob/master/src/de.rs#L194-L202), so thats probably also what axum should do (#1693)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants