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

throw an error instead of returning empty if client fails to deserialize query results #185

Closed
qianyiwen2019 opened this issue Dec 6, 2024 · 3 comments · Fixed by #186
Closed
Labels
bug Something isn't working

Comments

@qianyiwen2019
Copy link

Example:

define a struct MyRow with a field: decimal_col: Option
create a table with a column: decimal_col Nullable(Decimal(10, 4)))

when executing a query, deserialization should fail and client should throw an error to user.
but currently client just returns empty results.

    #[derive(Row, Serialize, Deserialize)]
    struct MyRow {
        pk: u32,
        decimal_col: Option<String>,
    }

    #[tokio::test]
    #[serial]
    async fn aaaaa() {
        let client = Client::default()
            .with_url("http://localhost:8123")
            .with_user("admin")
            .with_password("123456");

        // create table
        let create_table_sql = format!(
            "CREATE TABLE test_db_1.test(
        pk UInt32, 
        decimal_col Nullable(Decimal(10, 4)))
        PRIMARY KEY pk 
        ORDER BY pk"
        );

        client.query(&create_table_sql).execute().await.unwrap();

        // insert data
        client
            .query("INSERT INTO test_db_1.test VALUES(1, 1.1),(2, 2.2),(3, 3.3)")
            .execute()
            .await
            .unwrap();

        // query
        let rows = client
            .query("SELECT ?fields FROM test_db_1.test")
            .fetch_all::<MyRow>()
            .await
            .unwrap();

        println!("rows count: {}", rows.len());
        println!("rows: {}", json!(rows));
    }

output:
rows count: 0
rows: []

@qianyiwen2019 qianyiwen2019 added the bug Something isn't working label Dec 6, 2024
@qianyiwen2019 qianyiwen2019 changed the title throw an error instead of returning empty if it failed to deserialize query results throw an error instead of returning empty if client failed to deserialize query results Dec 6, 2024
@qianyiwen2019 qianyiwen2019 changed the title throw an error instead of returning empty if client failed to deserialize query results throw an error instead of returning empty if client fails to deserialize query results Dec 6, 2024
@loyd
Copy link
Collaborator

loyd commented Dec 8, 2024

Nice catch, thank you.

The problem is that in the case of incorrect schema (here String instead of u64-based types), the length of String is expected to be significant (5496 bytes):

[src/cursor.rs:48:25] chunk.data = b"\x01\0\0\0\0\xf8*\0\0\0\0\0\0\x02\0\0\0\0\xf0U\0\0\0\0\0\0\x03\0\0\0\0\xe8\x80\0\0\0\0\0\0"
[src/rowbinary/de.rs:258:15] self.input.get_u8() = 0
[src/rowbinary/de.rs:130:9] size = 5496

However, the stream ends earlier, and the cursor doesn't check that the remaining unparsed bytes are left.

This bug was introduced in #169. Previously, the cursor checked this situation.

So, the valid error should be "NotEnoughData".

loyd added a commit that referenced this issue Dec 8, 2024
It fixes regression introduced in #169.
Previously, the cursor checked if unhandled bytes are left.
Return this behaviour to detect schema mismatch.

Closes #185
loyd added a commit that referenced this issue Dec 8, 2024
It fixes the regression introduced in #169.
Previously, the cursor checked if unhandled bytes were left.
Return this behaviour to detect schema mismatch.

Closes #185
@qianyiwen2019
Copy link
Author

@loyd thanks for your response and quick fix.
for the returning error, maybe it could provide more precise error messages indicating which field caused the failure, similar to how sqlx does.

@loyd
Copy link
Collaborator

loyd commented Dec 9, 2024

maybe it could provide more precise error messages indicating which field caused the failure, similar to how sqlx does.

It's impossible to determine precisely the place of an error (in more complex cases than this one) without #10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants