chinedufn / dipa

dipa makes it easy to efficiently delta encode large Rust data structures.
https://chinedufn.github.io/dipa
Apache License 2.0
264 stars 7 forks source link

`BTreeMap<u16, Vec<u8>>` type broken in apply scenario #4

Closed da-x closed 3 years ago

da-x commented 3 years ago

Hi,

In the example below quick-check created values that found an issue in applying a delta. This kind of test can be extended to more complex types for arbitrary-generated values.

thread 'main' panicked at 'slice index starts at 23 but ends at 3', /home/dan/.cargo/git/checkouts/dipa-eb7bc35c193e0264/cfc0a48852df/src/sequence/sequence_delta_patch_towards.rs:70:31
use dipa::{DiffPatch, Diffable, Patchable};
use quickcheck::quickcheck;
use quickcheck_derive::Arbitrary;
use serde_derive::{Deserialize, Serialize};
use bincode::Options;
use std::collections::BTreeMap;

#[derive(Clone, Eq, PartialEq, PartialOrd, Ord, Debug, DiffPatch, Serialize, Deserialize, Arbitrary)]
pub struct Basic {
    pub items: BTreeMap<u16, Vec<u8>>,
}

fn tester(mut a: Basic, b: Basic) -> bool {
    let bin = bincode::options().with_varint_encoding();
    let created = a.create_delta_towards(&b);

    let serialized = bin.serialize(&created.delta).unwrap();
    let deserialized: <Basic as dipa::Diffable<'_, '_, Basic>>::DeltaOwned =
        bin.deserialize(&serialized).unwrap();

    a.apply_patch(deserialized);
    return a == b;
}

fn main() {
    quickcheck(tester as fn(Basic, Basic) -> bool);
}

Example dependencies:

dipa = { git = "https://github.com/chinedufn/dipa", features = [ "derive"] }
serde_derive = "1"
serde = "1"
quickcheck = "0.8"
quickcheck_macros = "0.8"
quickcheck_derive = "0.3"
rand = "0.6"
bincode = "1.3"
chinedufn commented 3 years ago

Hey. Thanks so much for putting all of this detail into the issue!

https://github.com/chinedufn/dipa/blob/9229f40f3737d37e26873247a9decfcd5d1425e5/src/map.rs#L6-L10

And the map tests live here https://github.com/chinedufn/dipa/blob/9229f40f3737d37e26873247a9decfcd5d1425e5/src/map.rs#L160-L182

If you're up for it, feel free to add and pass a minimal failing test case.

da-x commented 3 years ago

Thanks for looking into this. Turns it fails even with Vec<u8> as type, the map is not needed.

This new test fails at the apply compare stage (disregarding expected_delta).

    #[test]
    fn vec_empty_to_one() {
        DipaImplTester {
            label: None,
            start: &mut Vec::<u8>::new(),
            end: &vec![0],
            expected_delta: vec![],
            // No change, none variant is one byte
            expected_serialized_patch_size: 1,
            expected_did_change: false,
        }
        .test();
    }
chinedufn commented 3 years ago

Thanks for narrowing this down, fixed!

https://github.com/chinedufn/dipa/commit/5a258aeed0410e47c15026956f9df4aee9dc20c7