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.

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 de_num for unsigned number #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sundy-li
Copy link

No description provided.

@sundy-li sundy-li mentioned this pull request Sep 22, 2021
@blackbeam
Copy link
Owner

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
Copy link
Owner

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

@sundy-li
Copy link
Author

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:

@blackbeam
Copy link
Owner

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?

@sundy-li
Copy link
Author

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
Copy link
Author

sundy-li commented Sep 24, 2021

@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.

@blackbeam
Copy link
Owner

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

@sundy-li
Copy link
Author

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

@ovr
Copy link

ovr commented Nov 29, 2021

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

@blackbeam
Copy link
Owner

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants