P3KI / bendy

A rust library for encoding and decoding bencode with enforced cannonicalization rules.
BSD 3-Clause "New" or "Revised" License
70 stars 13 forks source link

Serde rename not working #53

Closed morr0ne closed 9 months ago

morr0ne commented 3 years ago

Trying to use serde rename in a filed causes an UnexpectedToken error

#[derive(Debug, Serialize, Deserialize)]
pub struct MetaInfo {
    #[serde(rename = "creation date")]
    creation_date: Option<u64>,
}

let t = fs::read("temp/manjaro.torrent").unwrap();
let torrent: MetaInfo = bendy::serde::from_bytes(&t).unwrap();
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Decode(Error { context: None, error: UnexpectedToken("List", "Num") })', src/main.rs:70:58
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This makes it impossible to deserialize certain fields

0ndorio commented 3 years ago

Hey @morr0ne, I'm not sure about rename as I haven't used it yet, but the error message looks like your data structure and your torrent file do not match.

I just checked the implementation of the Option deserialization used inside the serde feature (https://github.com/P3KI/bendy/blob/master/src/serde/de.rs#L285-L297) and the code seems to expect a single element list, but based on your error message the torrent file contains an integer instead.

@casey @thequux Is it expected that an optional value has to be encapsulated in a list element to be compatible with the serde feature? As far as I remember this isn't the case for non serde use cases, e.g. the old torrent decoding example https://github.com/P3KI/bendy/blob/master/examples/decode_torrent.rs#L106-L109.

casey commented 3 years ago

Hey @morr0one, sorry you're running in to this.

I made some choices that might seem strange when writing Bendy's Serde support.

I tried not emitting optional fields when the value of those fields was None, however, this prevented using the #[serde(flatten)] attribute.

I also wanted a round-trip through serde to be lossless, and serializing and deserializing Some values just as the contained values, is if I recall correctly, not lossless.

You can use a combination of serde attributes to get the desired behavior. This should work:

pub(crate) use serde_with::rust::unwrap_or_skip; // serde_with from crates.io

#[derive(Debug, Serialize, Deserialize)]
pub struct MetaInfo {
    #[serde(
      skip_serializing_if = "Option::is_none",
      default,
      with = "unwrap_or_skip",
       rename = "creation date"
    )]
    creation_date: Option<u64>,
}
morr0ne commented 3 years ago

I wanted to migrate from serde_bencode since it has some weirdness, this crates seems to fit all my needs but without being able to use standard serde features it will be hard to implement serde_with in hundreds of struct.

Is there a particular reason bendy doesn't return optional values? It makes using it really hard since torrent files are full of extensions that might or might not be present

casey commented 3 years ago

I wanted to migrate from [serde_bencode[(https://github.com/toby/serde-bencode) since it has some weirdenes, this crates seems to fit all my needs but without being able to use standard serde features it will be hard to implement serde_with in hundreds of struct.

I believe all standard serde features are working correctly. The issue is not that rename isn't working, it's that if you have a struct like this:

#[derive(Debug, Serialize, Deserialize)]
pub struct Foo {
    bar: Option<u64>,
}

Then bendy expects either d3:barle if bar is None, or d3:barli0ee if bar is Some, which makes it necessary to use annotations when processing torrent files. I agree that this is unfortunate. I'm not sure if I know a solution, however.

There are a few things that bendy is doing that you're running into:

  1. When serializing a struct, if the struct has a field with an option value, it will include that field in the emitted bencode map.

  2. When deserializing a struct, unless fields are annotated with #[serde(default)], it will treat a missing field as an error.

  3. Option::None is represented as the empty list, and Option::Some is length one list containing the item.

There are a bunch of trade offs here that are worth considering.

First, concerning 1. and 2., I initially tried to make bendy skip optional fields, and deserialize them as None if they were missing. However, this led to a few other serde features not working. I wish I could remember the details, but I believe that #[serde(flatten)] didn't work, possibly at all, or possibly only in certain configurations.

Concerning 3., If Option::None is not emitted by the serializer at all, there are a few weird situations. If you have Vec<Option<u32>>, and you serialize a list with a bunch of None values, does the list have a different length when deserializing? Also, I think serializing and deserializing values wouldn't be lossless, i.e. values like Option<Option<Option<()>>> would be tricky.

So, given all the above, the best solution I could find would be to have an unambiguous representation of Option, and always expect that fields are present. These limitations can be worked around with annotations, however.

If I had skipped fields and had an ambiguous representation of Option values, I think that there would be different limitations, but those limitations would be impossible to work around.

I definitely think that something could be done to make this more user friendly, or cut down on the number of annotations.

One idea is to have bendy provide a attribute that expands to all the serde attributes, so you don't need another dependency and as many attributes. I'm not sure if this is possible, but ideally it could look like this:

#[derive(Debug, Serialize, Deserialize)]
pub struct MetaInfo {
    #[serde(rename = "creation date")]
    #[bendy::optional]
    creation_date: Option<u64>,
}
morr0ne commented 3 years ago

I don't know enough of serdes internals to know if a bendy attribute is possible but I was thinking if maybe to solve 1 and 2 bendy could add features with various tradeoff, for example I don't use serde flatten so I would mind it

morr0ne commented 9 months ago

Closing as this as gotten stale and I have no longer interest in this crate. Feel free to reopen if necessary