Add binary protocol/prepared statement support

[brianmario]

As the title says, this adds support for the binary protocol, which is used by prepared statements.

There's still a little bit to do here, though mainly just wrapping up docs and some cleanup and adding a few more tests. But I wanted to get this open for review sooner than later.

I figured wrapping this with the Ruby gem/extension could happen in a separate pull request, but if it makes sense to just go ahead and add here let me know.

Some of the things I'd like to wrap up before calling this done:

• Finish up writing client API docs
• Finish up writing blocking API docs
• Add tests for the blocking API
• Fix null bitmap builder
• Add more tests for edge cases
• Document trilogy_binary_value_t, describing which C types map to which MySQL column types.

So happy to see this project finally made open source.

Thank you!

[brianmario]

@eileencodes hadn't seen this before, but after merging in your change from #5 it requires approval for the workflow to run on this pull. Make sense I guess. But just FYI for when one of you have a moment 🙏

[composerinteralia]

Approved the workflow. We've got the "Require approval for first-time contributors" setting selected, so we shouldn't need to do that for you again.

[composerinteralia]

Looks like I need to approve the workflows every time you push to this branch, so I'll keep doing that. I think once this is merged they will run for you automatically on any future PRs.

I'm working my way through this, trying to make sure I understand everything as I go. Looks great so far!

[brianmario]

Interesting test failure. Is there a way to re-run individual actions or does it require another push?

[composerinteralia]

I've seen that one before. I triggered a re-run

[composerinteralia]

I looked through everything and it all seems to check out to me. 🎉 I'll leave it to somebody with more comfort in C than I to give a final review once you are happy with test coverage and such.

Adding this to Ruby gem makes sense to me as a separate PR, since this PR's already got plenty going on.

[brianmario]

@composerinteralia amazing thank you for giving it a look over! 🙏

I'll see if I can find someone to help review the C-specific stuff when I'm ready for it.

[composerinteralia]

I played around with some blocking tests (modeled after your non-blocking tests) as part of my review. Feel free to use or throw away as necessary!

tests

TEST test_blocking_stmt_prepare()
{
    trilogy_conn_t conn;

    connect_conn(&conn);

    const char *query = "SELECT ?";
    trilogy_stmt_t stmt;

    int err = trilogy_stmt_prepare(&conn, query, strlen(query), &stmt);
    ASSERT_OK(err);

    ASSERT_EQ(1, stmt.parameter_count);

    trilogy_column_packet_t param;
    err = trilogy_read_full_column(&conn, &param);
    ASSERT_OK(err);

    ASSERT_EQ(1, stmt.column_count);

    trilogy_column_packet_t column_def;
    err = trilogy_read_full_column(&conn, &column_def);
    ASSERT_OK(err);

    trilogy_free(&conn);
    PASS();
}

TEST test_blocking_stmt_execute()
{
    trilogy_conn_t conn;

    connect_conn(&conn);

    const char *query = "SELECT ?";
    trilogy_stmt_t stmt;

    int err = trilogy_stmt_prepare(&conn, query, strlen(query), &stmt);
    ASSERT_OK(err);

    ASSERT_EQ(1, stmt.parameter_count);

    trilogy_column_packet_t param;
    err = trilogy_read_full_column(&conn, &param);
    ASSERT_OK(err);

    ASSERT_EQ(1, stmt.column_count);

    trilogy_column_packet_t column_def;
    err = trilogy_read_full_column(&conn, &column_def);
    ASSERT_OK(err);

    uint8_t flags = 0x00;
    const char str[] = {'t', 'e', 's', 't'};
    size_t len = sizeof(str);
    trilogy_binary_value_t binds[] = {
        {.is_null = false, .type = TRILOGY_TYPE_VAR_STRING, .as.str.data = str, .as.str.len = len}};

    uint64_t column_count;

    err = trilogy_stmt_execute(&conn, &stmt, flags, binds, &column_count);
    ASSERT_OK(err);

    ASSERT_EQ(1, column_count);

    trilogy_column_packet_t columns[1];
    err = trilogy_read_full_column(&conn, &columns[0]);
    ASSERT_OK(err);

    trilogy_binary_value_t values[1];
    err = trilogy_stmt_read_full_row(&conn, &stmt, columns, values);
    ASSERT_OK(err);

    ASSERT_MEM_EQ(values[0].as.str.data, "test", values[0].as.str.len);

    err = trilogy_stmt_read_full_row(&conn, &stmt, columns, values);
    ASSERT_EOF(err);

    trilogy_free(&conn);
    PASS();
}

TEST test_blocking_stmt_reset()
{
    trilogy_conn_t conn;

    connect_conn(&conn);

    const char *query = "SELECT ?";
    trilogy_stmt_t stmt;

    int err = trilogy_stmt_prepare(&conn, query, strlen(query), &stmt);
    ASSERT_OK(err);

    ASSERT_EQ(1, stmt.parameter_count);

    trilogy_column_packet_t param;
    err = trilogy_read_full_column(&conn, &param);
    ASSERT_OK(err);

    ASSERT_EQ(1, stmt.column_count);

    trilogy_column_packet_t column_def;
    err = trilogy_read_full_column(&conn, &column_def);
    ASSERT_OK(err);

    err = trilogy_stmt_reset(&conn, &stmt);
    ASSERT_OK(err);

    trilogy_free(&conn);
    PASS();
}

TEST test_blocking_stmt_close()
{
    trilogy_conn_t conn;

    connect_conn(&conn);

    const char *query = "SELECT ?";
    trilogy_stmt_t stmt;

    int err = trilogy_stmt_prepare(&conn, query, strlen(query), &stmt);
    ASSERT_OK(err);

    ASSERT_EQ(1, stmt.parameter_count);

    trilogy_column_packet_t param;
    err = trilogy_read_full_column(&conn, &param);
    ASSERT_OK(err);

    ASSERT_EQ(1, stmt.column_count);

    trilogy_column_packet_t column_def;
    err = trilogy_read_full_column(&conn, &column_def);
    ASSERT_OK(err);

    err = trilogy_stmt_close(&conn, &stmt);
    ASSERT_OK(err);

    trilogy_free(&conn);
    PASS();
}

[brianmario]

@composerinteralia Thanks! Dropped them in. I'll go back and add some more edge-case tests for everything to try and make sure we've covered some more surface area.

[adrianna-chang-shopify]

👋 Hi folks -- I'm gonna pick this up and try to get it across the finish line. @brianmario I can either start a new PR with commits cherry-picked from yours, or if you give me write access to your fork I can push updates here.

Do either you or @composerinteralia remember what sorts of "edge cases" we were still looking to test? (I've yet to take a look at the code in depth, so it's not immediately obvious to me what gaps are left to fill). I'll take a look at the test suite for mysql2's prepared statements for inspiration as well, but figured I'd see if either of y'all could remember where this PR was at 😂

[brianmario]

Hi!

@adrianna-chang-shopify I can totally give you access to my fork, not a problem at all.

In terms of where it was left off, it should be ready to go other than my wanting there to be some more tests. I haven't had time to get back to writing them myself, but the rest of the code should be ready to go.

[composerinteralia]

I recall this looking fairly ready when I reviewed it a while back. I'd be fine to merging this as-is after a rebase or whatever and then we can fill in whatever additional test coverage we want (e.g. testing with all the different types) separately. Some of that test coverage could probably also be done via the Ruby client once we've added support there.

[adrianna-chang-shopify]

Okay, so I've added prepared statement tests for all of the MySQL types, two of which are still failing and have me a bit stumped:

Expected: TRILOGY_OK
     Got: TRILOGY_ERR
F
FAIL test_blocking_stmt_execute_year: (TRILOGY_OK) != ((err)) (test/blocking_test.c:758)

FAIL test_blocking_stmt_execute_float: values[0].as.flt != float_val (test/blocking_test.c:420)

The first one I can't manage to repro locally, which is making it difficult to debug. (I apologize for all the noise with the builds, I was trying to debug on CI 😅 ). Seems like building the statement itself is problematic -- if anyone wants to take a stab at reproing locally that would be great 🙏

The second one does fail locally, but the failure is bizarre. Basically, when Trilogy reads the column back from the resultset, it thinks that the type is TRILOGY_TYPE_DOUBLE instead of TRILOGY_TYPE_FLOAT. This is shown if print out the value as a float vs a double:

diff --git a/test/blocking_test.c b/test/blocking_test.c
index 083a132..6da78c0 100644
--- a/test/blocking_test.c
+++ b/test/blocking_test.c
@@ -411,12 +411,16 @@ TEST test_blocking_stmt_execute_float() {
 
     trilogy_column_packet_t columns[1];
     err = trilogy_read_full_column(&conn, &columns[0]);
+    printf("Column type: %d\n", columns[0].type);
     ASSERT_OK(err);
 
     trilogy_binary_value_t values[1];
     err = trilogy_stmt_read_full_row(&conn, &stmt, columns, values);
     ASSERT_OK(err);
 
+    printf("Value as float: %f\n", values[0].as.flt); // wrong
+    printf("Value as double: %lf\n", values[0].as.dbl);
+
     ASSERT_EQ(values[0].as.flt, float_val);
 
     err = trilogy_stmt_read_full_row(&conn, &stmt, columns, values);

Column type: 5 # TRILOGY_TYPE_DOUBLE
Value as float: 0.000000
Value as double: 1234.500000
F
FAIL test_blocking_stmt_execute_float: values[0].as.flt != float_val (test/blocking_test.c:424)

That value is definitely being written correctly:

diff --git a/src/protocol.c b/src/protocol.c
index 232bb97..21a0577 100644
--- a/src/protocol.c
+++ b/src/protocol.c
@@ -781,6 +781,7 @@ int trilogy_build_stmt_execute_packet(trilogy_builder_t *builder, uint32_t stmt_
         CHECKED(trilogy_builder_write_uint8(builder, 0x1));
 
         for (i = 0; i < num_binds; i++) {
+            printf("Writing type: %d\n", binds[i].type);
             CHECKED(trilogy_builder_write_uint8(builder, binds[i].type));
 
             if (binds[i].is_unsigned) {

Writing type: 4
Column type: 5 # TRILOGY_TYPE_DOUBLE
Value as float: 0.000000
Value as double: 1234.500000
F
FAIL test_blocking_stmt_execute_float: values[0].as.flt != float_val (test/blocking_test.c:424)

But when it's read in #trilogy_parse_column_packet, we think it's a double:

diff --git a/src/protocol.c b/src/protocol.c
index 232bb97..d207ba6 100644
--- a/src/protocol.c
+++ b/src/protocol.c
@@ -379,6 +379,7 @@ int trilogy_parse_column_packet(const uint8_t *buff, size_t len, bool field_list
 
     uint8_t type;
     CHECKED(trilogy_reader_get_uint8(&reader, &type));
+    printf("Read column type: %d\n", type);
     out_packet->type = type;
 
     CHECKED(trilogy_reader_get_uint16(&reader, &out_packet->flags));

Writing type: 4
Read column type: 5
Column type: 5
Value as float: 0.000000
Value as double: 1234.500000
F
FAIL test_blocking_stmt_execute_float: values[0].as.flt != float_val (test/blocking_test.c:424)

Would love input if either of you have it @brianmario @composerinteralia! Maybe I'm missing something simple.

[matthewd]

Value as float: 0.000000
Value as double: 1234.500000

It sounds like it is a double [as opposed to being a float that's mis-tagged]. Perhaps the server accepts incoming floats for compatibility, but just upcasts them to a double immediately?

Ooh, maybe relevant:

Using FLOAT might give you some unexpected problems because all calculations in MariaDB are done with double precision.

[adrianna-chang-shopify]

Oh, good find @matthewd ! I'll dig a bit more -- but yeah, if that's the behaviour on the server side, there's not much we can do there (we'll just want to adjust the test accordingly) 😄

[adrianna-chang-shopify]

Okay, this is finally ready for review 🙈 I'll rebase off main and squash the commits once we deem it merge-able.

[brianmario]

Thanks for adding the rest of these tests! 🙏

It looks good to me, but I'll wait for someone with merge access to do a review as well 😉

[brianmario]

🎉