GREsau / schemars

Generate JSON Schema documents from Rust code
https://graham.cool/schemars/
MIT License
826 stars 229 forks source link

Deriving JsonSchema seems to break serde's "with" field annotation #89

Open joel-wright opened 3 years ago

joel-wright commented 3 years ago

I have been using #[derive(JsonSchema)], but recently ran into issues attempting to use serde's field attributes and the serde_with crate in order to report an error on duplicate keys in a mapping.

When I add the field attribute to the struct:

#[derive(JsonSchema, Serialize, Deserialize, Debug, PartialEq)]
#[serde(deny_unknown_fields)]
pub(crate) struct MyStruct {
    ...
    #[serde(with = "::serde_with::rust::maps_duplicate_key_is_error")]
    pub mapping: BTreeMap<String, String>,
    ...
}

I get the following compilation error:

error[E0573]: expected type, found module `serde_with::rust::maps_duplicate_key_is_error`
  --> data/structs.rs:24:20
   |
24 |     #[serde(with = "::serde_with::rust::maps_duplicate_key_is_error")]
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not a type

error: aborting due to previous error

If I remove JsonSchema from the list of derives it seems to work fine.

Haifischbecken commented 3 years ago

I ran into the same problem with a custom module, when I add JsonSchema to derive #[serde(with = ...)] suddenly expects a type instead of a module. Schemars version is 0.8.3, serde is 1.0.126.

Edit: I dug around a little and it seems like in schemars_derive::attr line 87, a #[serde(with="literal")] will only be parsed to a WithAttr::Type. Maybe I am not understanding enough of what is going on but since it coincides with rust expecting a type I thought I'd mention it.

Edit2: I realized now that it would be very hard to generate a JsonSchema for the custom (de)serialization. But the way it stops working looks like a bug not like an incapability.

Igosuki commented 3 years ago

I also have trouble with remotes for some specific data structures, seems to have to do with some resolution order. And I also have a recursive enum that breaks schema generation with a stackoverflow :)

uttarayan21 commented 3 years ago

I seem to have found a workaround.

  #[derive(Debug, Insertable, Deserialize, JsonSchema)]
  #[serde(crate = "rocket::serde")]
  #[table_name = "contents"]
  pub struct InsertableContent {
      pub title: String,
      pub story: String,
      #[serde(deserialize_with = "chrono::serde::ts_seconds::deserialize")]
      #[serde(serialize_with = "chrono::serde::ts_seconds::serialize")]
      pub published: DateTime<chrono::Utc>,
      pub user_id: i32,
  }

If you break the #[serde(with = "module") to separate serialize and deserialize functions it seems to compile fine.

Igosuki commented 3 years ago

@uttarayan21 sounds like something should get patched in the code though

uttarayan21 commented 3 years ago

Definitely. I just added it here so that people can compile in the meantime.

abizjak commented 2 years ago

I came across this today as well and while I think the error when this happens should (and easily can) be better and point to the way things should be done, but there is a straightforward way to use with and this might even be the intended way.

For the original example the following would be the solution

#[derive(JsonSchema, Serialize, Deserialize, Debug, PartialEq)]
#[serde(deny_unknown_fields)]
pub(crate) struct MyStruct {
    ...
    #[serde(with = "::serde_with::rust::maps_duplicate_key_is_error")]
    #[schemars(with = BTreeMap<String, String>)]
    pub mapping: BTreeMap<String, String>,
    ...
}

or ideally a type that more specifically does the "no duplicate keys validation" and records that in the schema.

Generally the allowed attributes and their semantics need to be better documented, like they are for serde since I also found it hard to figure out what is and what is not possible.

In my code I've settled on the pattern

#[derive(JsonSchema, Serialize, Deserialize, Debug, PartialEq)]
pub(crate) struct MyStruct {
    #[serde(with = "foo")]
    #[schemars(with = MyStructSchema)]
    pub mapping: SomeType,
}

impl JsonSchema for MyStructSchema {
  ...
}

in the cases where I needed custom and precise schemas.

wackazong commented 1 year ago

I can confirm the bug and the workaround by @uttarayan21 🙏

JakkuSakura commented 6 months ago

This seems not hard to fix at first glance, but with #[serde(with)] one can do crazy things. I guess it's the reason this hasn't been picked up for years

GREsau commented 2 months ago

This occurs because the #[serde(with = "...")] attribute is respected by schemars in order to enable remote deriving. But as you've discovered, this breaks when the with value is not a type that implements JsonSchema.

The easiest way to resolve this would be using @abizjak's workaround, although you must include quotes around the with value like so:

#[derive(JsonSchema, Serialize, Deserialize, Debug, PartialEq)]
#[serde(deny_unknown_fields)]
pub(crate) struct MyStruct {
    #[serde(with = "::serde_with::rust::maps_duplicate_key_is_error")]
    #[schemars(with = "BTreeMap<String, String>")]
    pub mapping: BTreeMap<String, String>,
}

I'm afraid I don't know how this could be reliably fixed within schemars, because (as far as I know) the proc macro has no way of knowing whether the with value (in this case ::serde_with::rust::maps_duplicate_key_is_error) refers to a module or a type.

oxalica commented 3 weeks ago

I'm afraid I don't know how this could be reliably fixed within schemars, because (as far as I know) the proc macro has no way of knowing whether the with value (in this case ::serde_with::rust::maps_duplicate_key_is_error) refers to a module or a type.

Why is it necessary to accept both types and module paths? According to docs of serde, they always accept a "module", not a type, and that's the reason why they require #[serde(with = "Vec::<DurationDef>")] in remote deriving.

Is there any reason prevents us from doing the same to always parse serde(with = ...) to a path? We could require the same for schemars(with = ...) for consistency, but can also make them accept different syntax for compatibility.