rust_mysql_common icon indicating copy to clipboard operation
rust_mysql_common copied to clipboard

Fix de_num for unsigned number

Open sundy-li opened this issue 4 years ago • 10 comments

sundy-li avatar Sep 22 '21 14:09 sundy-li

Hi, thanks for the PR, but this part of the code isn't buggy.

The reason here is to always emit a single representation for any integer. The rule is the following:

  1. library should emit the UInt variant for some integer x if it satisfies i64::MAX < x ≤ u64::MAX
  2. library should emit the Int variant for any other integer.

As you can see the de_num macro is used for u8 i8 u16 i16 u32 and i32. This usage satisfies the rule.

blackbeam avatar Sep 22 '21 14:09 blackbeam

I'll add this to the Value::UInt documentation.

blackbeam avatar Sep 22 '21 14:09 blackbeam

Thanks, but what's the proper way to make a unit test for UInt number serde?

Eg in the code: https://github.com/jonhoo/msql-srv/blob/1c112267ce568ac83088e13861bf4911f8df9793/src/value/decode.rs#L292-L329

I serialize a u8 number 1 to binary and then deserialize it into a Value with ColumnFlags::UNSIGNED_FLAG, but the result is i8.

running 1 test
thread 'value::encode::tests::roundtrip_bin::u8_one' panicked at 'assertion failed: `(left == right)`
  left: `Int(1)`,
 right: `UInt(1)`', src/value/encode.rs:778:9
stack backtrace:

sundy-li avatar Sep 23 '21 01:09 sundy-li

You can compare the normalized values, where "normalized" means that it follows the rule stated above.

Btw, why do you need to test mysql_common's ser/de within the msql-srv?

blackbeam avatar Sep 23 '21 06:09 blackbeam

Btw, why do you need to test mysql_common's ser/de within the msql-srv?

I bump up the version of mysql_common in msql-srv, but some functions like write_bin_value are changed, so I have to be compatible with old unit tests.

sundy-li avatar Sep 23 '21 06:09 sundy-li

@blackbeam

I still take the code as buggy, since I tell the MyDeserialize that I want to deserialize binary into an unsigned Value with UNSIGNED_FLAG, the Deserialize already has all information to return unsigned number.

I don't understand why it force return signed number.

sundy-li avatar Sep 24 '21 09:09 sundy-li

May I ask you why did you ignore the Text Protocol in you tests?

https://github.com/jonhoo/msql-srv/blob/1c112267ce568ac83088e13861bf4911f8df9793/src/value/decode.rs#L317

blackbeam avatar Sep 24 '21 21:09 blackbeam

Because MySerialize.serialize the Value into binary bytes, so I use ValueDeserializer<BinValue> to deserialize the bytes into Value in binary protocol.

sundy-li avatar Sep 25 '21 02:09 sundy-li

+1, I am using mysql_common & msql-srv. I've spent a few hours to find a problem, lol.

ovr avatar Nov 29 '21 13:11 ovr

Hmm.. Seems like I need to look at this from wider perspective, but still don't want to encode all of u8, i8, u16, i16, ...

@ovr, could you please describe how exactly this problem hit you?

blackbeam avatar Nov 30 '21 05:11 blackbeam