Bump deps

[PsiACE]

• bump deps
• make test work
• make clippy happy

[codecov]

Codecov Report

Merging #22 (684a2b4) into master (e77fd27) will decrease coverage by 10.64%.
The diff coverage is 47.61%.

❗ Current head 684a2b4 differs from pull request most recent head a4ca72f. Consider uploading reports for the commit a4ca72f to get more accurate results

@@             Coverage Diff             @@
##           master      #22       +/-   ##
===========================================
- Coverage   81.19%   70.55%   -10.65%     
===========================================
  Files          10       10               
  Lines        2159     2007      -152     
===========================================
- Hits         1753     1416      -337     
- Misses        406      591      +185     

Impacted Files	Coverage Δ
src/commands.rs	88.70% <ø> (ø)
src/packet.rs	88.88% <ø> (+1.58%)	⬆️
src/value/encode.rs	6.56% <0.00%> (-55.37%)	⬇️
src/lib.rs	63.43% <11.11%> (-1.46%)	⬇️
src/value/decode.rs	30.08% <33.33%> (-49.23%)	⬇️
src/params.rs	97.61% <100.00%> (ø)
src/resultset.rs	79.67% <100.00%> (ø)
tests/async.rs	91.98% <100.00%> (+0.01%)	⬆️
tests/main.rs	85.63% <100.00%> (ø)
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e77fd27...a4ca72f. Read the comment docs.

[PsiACE]

Hi @jonhoo , would you like to take some time to review this PR?

[jonhoo]

Sorry, I've been away on vacation :)

Overall this looks great, thanks for taking the time! I left some notes inline.

[jonhoo]

Why does this need spawn_blocking?

[PsiACE]

I will remove the dependency on tokio. I checked it and we don't need tokio.

[jonhoo]

Oh, that's weird. I wasn't actually proposing you remove tokio. Instead, I was just questioning the use of spawn_blocking specifically. I would think that rt.block_on would be sufficient if you remove the call to .wait(). The .wait() is just a utility function inside the mysql library that does that blocking for you. But using .wait() is also okay if you prefer that, it's just a bit less idiomatic.

[PsiACE]

Sorry, I will fix it later :( I was not thinking that much.

[PsiACE]

I guess we'll have to rely on wait() for now. Is it okay to change it to this?

        let rt = tokio::runtime::Runtime::new().unwrap();

        rt.block_on(async move {
            mysql_async::Conn::new(format!("mysql://127.0.0.1:{}", port))
                .and_then(|conn| c(conn))
                .wait()
                .unwrap()
        });

Or would you like to commit a patch telling me what to do? cc @jonhoo

[jonhoo]

I was thinking:

rt.block_on(async move {
    mysql_async::Conn::new(format!("mysql://127.0.0.1:{}", port))
        .and_then(|conn| c(conn))
        .await.unwrap()
});

The wait method is just a helper method that mysql_async provides, but since we actually execute in async context here, we can do it properly and use await.

[jonhoo]

I'm not sure I follow this file — what is it testing? It seems to also contain a bunch of implementation, but that's only used for tests? If this is an orthogonal change, would you mind making it in a separate PR?

[PsiACE]

Here's some code to help the tests run correctly, migrated from the old mysql_common. I don't think this is considered orthogonal :(

[jonhoo]

If this comes from mysql_common, why move it in here? Does it test anything that actually relates to this crate? Why was it removed from mysql_common in the first place?

[PsiACE]

This is because mysql_common removed functions like write_bin_value. In fact, this is just to ensure that the existing tests work properly.

It looks like mysql_common is now updated with serialization/deserialization.

[jonhoo]

Hmm, I don't think they actually removed those functions, they just renamed them. For example, decimal decoding is now in Decimal::parse_bytes. I think we should just switch to those methods instead of inlining the old implementation (which may have been replaced because it was buggy).

[jonhoo]

Also note that the Postgres version was already bumped by #24, so you may need to merge from master :)

[PsiACE]

@jonhoo , thank you for your work, in fact I really like them. Our company's project datafuse is using msql-srv and the team is happy to participate in the maintenance.

[jonhoo]

By the way — it's easier to review the PR as it changes over time if you avoid force-pushing. We can always squash at the end to clean up the history :)

[sundy-li]

After blackbeam/rust_mysql_common#43 is merged.

[jonhoo]

I'll just add that I don't think we should need to have tests for mysql_common in this crate. We should assume that it does "the right thing", and simply rely on that. Errors in its logic should be tested for in that crate instead, so maybe contribute these tests there?

[jonhoo]

Do we still then need to wait for blackbeam/rust_mysql_common#43? I don't see a failing test, which suggests either that we do not, or that we should add a test to catch the issue represented by blackbeam/rust_mysql_common#43.

[jonhoo]

Ah, no, I think we've talked past each other. What I was object to originally was the code introduced here. The rt! tests should remain, since they test that our encoding and decoding matches the MySQL client's decoding and encoding respectively. Those tests are not tests of mysql_common, they're tests of our code under the assumption that mysql_common does the right thing.

[sundy-li]

Do we still then need to wait for blackbeam/rust_mysql_common#43?

Yes, I've added the rt! tests and these are not passed.

There are 2 problems with rust_mysql_common

• ValueDeserializer<TextValue> only return Value::Null and Value::Bytes, codes, this is reasonable since it's not enough to determine the type only with texts.
• ValueDeserializer<TextValue> did not respect the ColumnFlags::UNSIGNED_FLAG

The author did not think it's buggy, I still did not figure out why.

[jonhoo]

Thinking some more about this, I wonder if what we should do is change rt! so that instead of directly serializing and deserializing the values, it constructs a full server and client, and then checks that if the server sends a value v in a column, the client produces a row with a value v of that same type. And also that if the client sends a value v, as, say, a prepared statement value, the server receives a value v of that same type. That way we should be testing the "whole" machinery, rather than whether the low-level interface matches (which it sounds like it doesn't any more).

What do you think?

[jonhoo]

This landed in #31.