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.

Sign up for GitHub

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

fix for incorrect implementation of KeyDeserialize for (T, U) #34

Merged
merged 4 commits into from
Apr 17, 2023
Merged

fix for incorrect implementation of KeyDeserialize for (T, U) #34

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cde9dac
fix for incorrect implementation of KeyDeserialize for (T, U)
0xForerunner Apr 13, 2023
86040d3
changed KEY_LEN to KEY_ELEMS
0xForerunner Apr 17, 2023
238b4e1
Add more tuple / triple de tests
maurolacy Apr 17, 2023
403eff6
Slightly better version of first key split
maurolacy Apr 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 129 additions & 16 deletions src/de.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ use crate::int_key::IntKey;
pub trait KeyDeserialize {
type Output: Sized;

/// The number of key elements is used for the deserialization of compound keys.
/// It should be equal to PrimaryKey::key().len()
const KEY_ELEMS: u16;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change since the trait is public

Copy link
Contributor Author

@0xForerunner 0xForerunner Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is. Unfortunately there is no solution to this bug that does not introduce breaking changes. If you look at one of my earlier suggestions I mentioned that creating a new trait called KeyElems, which contains the const, and then using that as a trait bound for KeyDeserialize for (T, U) would make this change less braking for most users. I still think that's a good solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! We'll check next week how we can ensure the 1.x release series remains free of breaking changes. It might take a while until this can be released. So I recommend using the main branch from GitHub as a dependency to unblock you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. If you go with the new trait solution, then the changes will only be braking specifically to users who are directly exposed to the bug. Isn't it a good thing that they will now receive a compiler error, forcing them to address the issue? That is definitely the route I would take.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it might be the best option. I need to look into it with a fresh head again. For now I just want to make sure this does not break happy users when released.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks happy users, but the fix is straightforward. I think if we document this in the MIGRATING file, we should be fine.


fn from_vec(value: Vec<u8>) -> StdResult<Self::Output>;

fn from_slice(value: &[u8]) -> StdResult<Self::Output> {
Expand All @@ -18,6 +22,8 @@ pub trait KeyDeserialize {
impl KeyDeserialize for () {
type Output = ();

const KEY_ELEMS: u16 = 0;

#[inline(always)]
fn from_vec(_value: Vec<u8>) -> StdResult<Self::Output> {
Ok(())
Expand All @@ -27,6 +33,8 @@ impl KeyDeserialize for () {
impl KeyDeserialize for Vec<u8> {
type Output = Vec<u8>;

const KEY_ELEMS: u16 = 1;

#[inline(always)]
fn from_vec(value: Vec<u8>) -> StdResult<Self::Output> {
Ok(value)
Expand All @@ -36,6 +44,8 @@ impl KeyDeserialize for Vec<u8> {
impl KeyDeserialize for &Vec<u8> {
type Output = Vec<u8>;

const KEY_ELEMS: u16 = 1;

#[inline(always)]
fn from_vec(value: Vec<u8>) -> StdResult<Self::Output> {
Ok(value)
Expand All @@ -45,6 +55,8 @@ impl KeyDeserialize for &Vec<u8> {
impl KeyDeserialize for &[u8] {
type Output = Vec<u8>;

const KEY_ELEMS: u16 = 1;

#[inline(always)]
fn from_vec(value: Vec<u8>) -> StdResult<Self::Output> {
Ok(value)
Expand All @@ -54,6 +66,8 @@ impl KeyDeserialize for &[u8] {
impl KeyDeserialize for String {
type Output = String;

const KEY_ELEMS: u16 = 1;

#[inline(always)]
fn from_vec(value: Vec<u8>) -> StdResult<Self::Output> {
String::from_utf8(value).map_err(StdError::invalid_utf8)
Expand All @@ -63,6 +77,8 @@ impl KeyDeserialize for String {
impl KeyDeserialize for &String {
type Output = String;

const KEY_ELEMS: u16 = 1;

#[inline(always)]
fn from_vec(value: Vec<u8>) -> StdResult<Self::Output> {
Self::Output::from_vec(value)
Expand All @@ -72,6 +88,8 @@ impl KeyDeserialize for &String {
impl KeyDeserialize for &str {
type Output = String;

const KEY_ELEMS: u16 = 1;

#[inline(always)]
fn from_vec(value: Vec<u8>) -> StdResult<Self::Output> {
Self::Output::from_vec(value)
Expand All @@ -81,6 +99,8 @@ impl KeyDeserialize for &str {
impl KeyDeserialize for Addr {
type Output = Addr;

const KEY_ELEMS: u16 = 1;

#[inline(always)]
fn from_vec(value: Vec<u8>) -> StdResult<Self::Output> {
Ok(Addr::unchecked(String::from_vec(value)?))
Expand All @@ -90,6 +110,8 @@ impl KeyDeserialize for Addr {
impl KeyDeserialize for &Addr {
type Output = Addr;

const KEY_ELEMS: u16 = 1;

#[inline(always)]
fn from_vec(value: Vec<u8>) -> StdResult<Self::Output> {
Self::Output::from_vec(value)
Expand All @@ -101,6 +123,8 @@ macro_rules! integer_de {
$(impl KeyDeserialize for $t {
type Output = $t;

const KEY_ELEMS: u16 = 1;

#[inline(always)]
fn from_vec(value: Vec<u8>) -> StdResult<Self::Output> {
Ok(<$t>::from_cw_bytes(value.as_slice().try_into()
Expand All @@ -121,33 +145,54 @@ fn parse_length(value: &[u8]) -> StdResult<usize> {
.into())
}

/// Splits the first key from the value based on the provided number of key elements.
/// The return value is ordered as (first_key, remainder).
///
fn split_first_key(key_elems: u16, value: &[u8]) -> StdResult<(Vec<u8>, &[u8])> {
let mut index = 0;
let mut first_key = Vec::new();

// Iterate over the sub keys
for i in 0..key_elems {
let len_slice = &value[index..index + 2];
index += 2;
let is_last_key = i == key_elems - 1;

if !is_last_key {
first_key.extend_from_slice(len_slice);
}

let subkey_len = parse_length(len_slice)?;
first_key.extend_from_slice(&value[index..index + subkey_len]);
index += subkey_len;
}

let remainder = &value[index..];
Ok((first_key, remainder))
}

impl<T: KeyDeserialize, U: KeyDeserialize> KeyDeserialize for (T, U) {
type Output = (T::Output, U::Output);

#[inline(always)]
fn from_vec(mut value: Vec<u8>) -> StdResult<Self::Output> {
let mut tu = value.split_off(2);
let t_len = parse_length(&value)?;
let u = tu.split_off(t_len);
const KEY_ELEMS: u16 = T::KEY_ELEMS + U::KEY_ELEMS;

Ok((T::from_vec(tu)?, U::from_vec(u)?))
#[inline(always)]
fn from_vec(value: Vec<u8>) -> StdResult<Self::Output> {
let (t, u) = split_first_key(T::KEY_ELEMS, value.as_ref())?;
Ok((T::from_vec(t)?, U::from_vec(u.to_vec())?))
}
}

impl<T: KeyDeserialize, U: KeyDeserialize, V: KeyDeserialize> KeyDeserialize for (T, U, V) {
type Output = (T::Output, U::Output, V::Output);

#[inline(always)]
fn from_vec(mut value: Vec<u8>) -> StdResult<Self::Output> {
let mut tuv = value.split_off(2);
let t_len = parse_length(&value)?;
let mut len_uv = tuv.split_off(t_len);

let mut uv = len_uv.split_off(2);
let u_len = parse_length(&len_uv)?;
let v = uv.split_off(u_len);
const KEY_ELEMS: u16 = T::KEY_ELEMS + U::KEY_ELEMS + V::KEY_ELEMS;

Ok((T::from_vec(tuv)?, U::from_vec(uv)?, V::from_vec(v)?))
#[inline(always)]
fn from_vec(value: Vec<u8>) -> StdResult<Self::Output> {
let (t, remainder) = split_first_key(T::KEY_ELEMS, value.as_ref())?;
let (u, v) = split_first_key(U::KEY_ELEMS, remainder)?;
Ok((T::from_vec(t)?, U::from_vec(u)?, V::from_vec(v.to_vec())?))
}
}

Expand Down Expand Up @@ -257,6 +302,74 @@ mod test {
);
}

#[test]
fn deserialize_tuple_of_tuples_works() {
assert_eq!(
<((&[u8], &str), (&[u8], &str))>::from_slice(
((BYTES, STRING), (BYTES, STRING)).joined_key().as_slice()
)
.unwrap(),
(
(BYTES.to_vec(), STRING.to_string()),
(BYTES.to_vec(), STRING.to_string())
)
);
}

#[test]
fn deserialize_tuple_of_triples_works() {
assert_eq!(
<((&[u8], &str, u32), (&[u8], &str, u16))>::from_slice(
((BYTES, STRING, 1234u32), (BYTES, STRING, 567u16))
.joined_key()
.as_slice()
)
.unwrap(),
(
(BYTES.to_vec(), STRING.to_string(), 1234),
(BYTES.to_vec(), STRING.to_string(), 567)
)
);
}

#[test]
fn deserialize_triple_of_tuples_works() {
assert_eq!(
<((u32, &str), (&str, &[u8]), (i32, i32))>::from_slice(
((1234u32, STRING), (STRING, BYTES), (1234i32, 567i32))
.joined_key()
.as_slice()
)
.unwrap(),
(
(1234, STRING.to_string()),
(STRING.to_string(), BYTES.to_vec()),
(1234, 567)
)
);
}

#[test]
fn deserialize_triple_of_triples_works() {
assert_eq!(
<((u32, &str, &str), (&str, &[u8], u8), (i32, u8, i32))>::from_slice(
(
(1234u32, STRING, STRING),
(STRING, BYTES, 123u8),
(4567i32, 89u8, 10i32)
)
.joined_key()
.as_slice()
)
.unwrap(),
(
(1234, STRING.to_string(), STRING.to_string()),
(STRING.to_string(), BYTES.to_vec(), 123),
(4567, 89, 10)
)
);
}

#[test]
fn deserialize_triple_works() {
assert_eq!(
Expand Down