automerge / autosurgeon

MIT License
273 stars 17 forks source link

Unexpected uint error with newtype wrapper #45

Open wg opened 5 months ago

wg commented 5 months ago

Hello!

I have a newtype wrapper around a u64 like so:

#[derive(Debug, Hydrate, Reconcile)]
pub struct Item {
    pub name: String,
    pub date: Option<Date>,
}

#[derive(Debug, Hydrate, Reconcile)]
pub struct Date(u64);

When I attempt to hydrate it from a doc with a value present for that key, I get the error unexpected uint.

I think this is because Option::hydrate calls Date::hydrate_uint but the derive impl for a newtype struct only generates Hydrate::hydrate. Is there a better way to accomplish what I'm trying to do here?

And as a larger meta question, why isn't the Hydrate impl for Option simply something like this that handles both missing and explicitly null values?

Ok(match doc.get(obj, &prop)? {
    Some((Scalar(v), _)) if *v == Null => None,
    Some(_) => Some(T::hydrate(doc, obj, prop)?),
    None    => None,
})
alexjg commented 5 months ago

The reason Option<T>::hydrate returns an error if the value is not present at all is that sometimes you need to know that there wasn't a value in the document. Schema migrations are actually a good example of this, you might introduce a field which is optional but which you wish to have a default value for when the document was created before the field was introduced; in this case you would need to distinguish between the value being Null vs not present at all.

We have two tools for managing this. First there's MaybeMissing which is a wrapper type which allows you to detect the missing value. Second there is the missing= attribute which allows you to specify a function which will be used to populate the missing value (in your case you can use Default::default() to get the None).

wg commented 5 months ago

Having a mechanism to detect and handle missing values makes sense, thanks! However this default behavior for Option is very surprising, Rust (serde)^1, Swift^2, and Go^3 all support deserializing both { "name": "test" } and { "name": "test", "date": null } into a struct with optional date field, where the date will be None/nil. TypeScript with zod's nullish behaves similarly.

I think optionality is orthogonal to defaults, when I want a default value in Rust I use the Default trait, not Option. Wouldn't it be better to make the missing= attribute an opt-in feature similar to serde's default attribute? As it stands right now I'd have to add #[autosurgeon(missing="Default::default")] to every single optional field in my schema which adds a lot of noise.