0.6.0: RETURNING causes value to have an extra Option wrapped

[leshow]

I have defined a NOT NULL column:

CREATE TABLE IF NOT EXISTS foo(
    ip INTEGER NOT NULL,
    client_id BLOB,
    PRIMARY KEY(ip)
);

After the update to 0.6.0, wherever regular SELECT queries happen the type is correctly not optional:

        sqlx::query!(
            "SELECT ip 
            FROM 
                foo 
            WHERE 
                client_id = ?1
            LIMIT 1",
            id
        )
        .fetch_optional(pool)
        .await?
        .map(|cur| cur.ip as u32)

However, if I use RETURNING the type has an extra Option wrap and fails to compile now:

        sqlx::query!(
            r#"
            UPDATE foo
           ....
            RETURNING ip
            "#,
        )
        .fetch_optional(conn)
        .await?
        .map(|cur| IpAddr::V4(Ipv4Addr::from(cur.ip as u32))))

error[E0605]: non-primitive cast: `Option<i64>` as `u32`
   --> libs/ip-manager/src/sqlite.rs:700:47
    |
700 |                 ip: IpAddr::V4(Ipv4Addr::from(cur.ip as u32)),
    |                                               ^^^^^^^^^^^^^ an `as` expression can only be used to convert between primitive types or to coerce to a specific trait object

edit: using sqlite backend

[dragonnn]

I am pretty sure you should use fetch_one instead of fetch_optional in this use case

[leshow]

I don't think so, fetch_one will return an error if there is nothing to return, which is not what I want. fetch_optional returns "at most one row" (from the docs), it's not guaranteed that something will return with the second query, so I think I'm using it correctly.

What I'm trying to point out is the API has had a breaking change between 0.5.x -> 0.6 that I think was unintentional.

edit: I should mention, I'm happy to PR to fix this if someone can point me in the right direction I believe. I have identified a test that I could add an extra assert! into to validate the behavior afterwards:

sqlx/tests/sqlite/describe.rs

Line 179 in 061fdca

async fn it_describes_insert_with_returning() -> anyhow::Result<()> {

[dragonnn]

Ok so a simple question, what should fetch_optional in your opinion return when no row was created? You want to return an i64 when no row was created? From where should fetch_optional get then the i64 from?
fetch_optional will return a Some(T) or a None if no row was created, as the name says optional. It can not return T when no row was created.
And yes, fetch_one will error when they is nothing to return, because you can either throw an error or return an Option, you can not magically just return something when they is nothing to return.
In my opinion if 0.5.X was returning T on fetch_optional that was a bug.

[leshow]

I believe the return type of fetch_optional is and should be unchanged:

    pub async fn fetch_optional<'e, 'c: 'e, E>(mut self, executor: E) -> Result<Option<O>, Error>

In my opinion if 0.5.X was returning T on fetch_optional that was a bug.

It always returned Option

The field on Record appears to be wrapped in an Option now internally with 0.6, if you use RETURNING, so the column has an extra Option wrap. Notice the ip field is defined as NOT NULL so it should not be optional, unless I'm missing something.

[dragonnn]

Ah ok, right Result<Option<Option<X>>, _> doesn't make sense in this case I agree. Didn't get that it returns a double option

[leshow]

And just to explain this further, even if I use fetch_one the Record's field is internally still Record { ip: Option<i64> }. I believe there is an issue with RETURNING in the sqlite backend, as it looks like the postgres example does not have this issue

sqlx/examples/postgres/todos/src/main.rs

Line 45 in 061fdca

async fn add_todo(pool: &PgPool, description: String) -> anyhow::Result<i64> {

Again, I would like to PR this, just point me in the right direction.

[sweepline]

I can confirm having this issue with an insert statement.

[liningpan]

I think the problem is that sqlx now cannot infer nullability correctly. To be fair, the sqlite byte code is not considered as part of the API, and not meant to be depended on by external applications.

The bytecode engine is not an API of SQLite. Details about the bytecode engine change from one release of SQLite to the next. Applications that use SQLite should not depend on any of the details found in this document.
https://www.sqlite.org/opcode.html

I also don't know if this is related to the CTE feature #1816, which made extensive change to the module parsing the sqlite byte code.

For this specific issue I think you might be able to force the column to be not null. https://docs.rs/sqlx/latest/sqlx/macro.query.html#type-overrides-output-columns

        sqlx::query!(
            r#"
            UPDATE foo
           ....
            RETURNING ip as "ip!"
            "#,
        )
        .fetch_optional(conn)
        .await?
        .map(|cur| IpAddr::V4(Ipv4Addr::from(cur.ip as u32))))

[tyrelr]

@leshow

Again, I would like to PR this, just point me in the right direction.

Unfortunately I can't dig into the root cause right now. But if you want to dig deeper, this is the workflow I had settled into for most of my prior changes:

• add a test case to tests/sqlite/describe.rs reproducing the wrong nullability for that 'shape' of query (probably possible even with the existing tables)

• run sqlx's sqlite unit tests to reproduce.

  • If you enable logging, you should be able to see every 'execution path' leading to a result. I recommend running ONLY the test function you add if you use the logging. It is a lot to read through if you run every single test function.

• Combine the following 3 pieces of information to try to step backwards into where the query analyzer went wrong:

1. the logging output above (which includes the bytecode, and each possible result (column types/nullability, plus the order of operations which produced it)
2. the sqlite opcode reference page
3. the output for "EXPLAIN {the query}", using something like DB Browser. (technically this is part of the log output... but it's easier to read with formatting)

I typically end up adding temporary debug statements into where it looks like the issue is (unless it's obvious), before actually fixing anything. The actual code to fix will be somewhere within: sqlx-core/src/sqlite/connection/explain.rs

Hopefully this actually helps you...

[leshow]

Thanks @tyrelr . Ive got some time tomorrow, will see if I can make progress on this.

edit: I haven't been having much luck getting the describe tests to fail. It looks like nullable() is returning false on columns with RETURNING.

[tyrelr]

Thanks for the update, @leshow. I gave this a look this afternoon and was able to construct an update query with incorrect nullibility. I believe I found the root-cause and a fix within PR #1960 (or at least, the start of one).

[leshow]

Did nullable() return Some(true) for you before the changes? Weird. I modified the same test but it always returned false. Anyway, thanks for looking at this.

[tyrelr]

Yep, I just double checked that after todays rebase to get a fix for an unrelated CI failure.

Thanks for the good reproduction steps. Those make life so much easier.

[serzhshakur]

I can confirm this has been fixed in 0.7-dev

[leshow]

It appears to still be broken in some cases on 0.7.0 @serzhshakur

I have a reproduce case, I think it has something to do with the WHERE clause:

pub async fn update_ip<'a, E>(
    conn: E,
    ip: i64,
    client_id: Option<&[u8]>,
) -> Result<Option<IpAddr>, sqlx::Error>
where
    E: sqlx::Executor<'a, Database = Sqlite>,
{
    Ok(sqlx::query!(
        r#"
            UPDATE leases
            SET
                client_id = ?2
            WHERE 
                ip = ?1
            RETURNING *
            "#,
        ip,
        client_id,
    )
    .fetch_optional(conn)
    .await?
    .map(|cur| IpAddr::V4(Ipv4Addr::from(cur.ip as u32))))
}

CREATE TABLE IF NOT EXISTS leases(
    ip INTEGER NOT NULL,
    client_id BLOB,
    PRIMARY KEY(ip)
);

produces:

  Compiling sqlx-test v0.1.0 (/home/leshow/dev/rust/sqlx-test)
error[E0605]: non-primitive cast: `Option<i64>` as `u32`
  --> src/lib.rs:31:42
   |
31 |     .map(|cur| IpAddr::V4(Ipv4Addr::from(cur.ip as u32))))
   |                                          ^^^^^^^^^^^^^ an `as` expression can only be used to convert between primitive types or to coerce to a specific trait object

If I change WHERE ip = ?1 to WHERE client_id = ?2 the error goes away. Pretty weird.

[leshow]

@tyrelr pinging you in case you haven't seen this. I have the reproduce case above, I've been trying to create a test that fails but I can't seem to satisfy the DATABASE_URL for the sqlite tests

edit:

I think I got the tests to run properly and a test case that fails when it should pass. Ran tests with:

 DATABASE_URL=sqlite:tests/sqlite/sqlite.db cargo test --no-default-features --features any,macros,sqlite,_unstable-all-types,runtime-tokio 

test case:

#[sqlx_macros::test]
async fn it_describes_update_id_returning() -> anyhow::Result<()> {
    let mut conn = new::<Sqlite>().await?;

    let d = conn
        .describe("UPDATE accounts SET is_active=true WHERE id=?1 RETURNING *")
        .await?;

    assert_eq!(d.columns().len(), 3);
    assert_eq!(d.column(0).type_info().name(), "INTEGER");
    assert_eq!(d.nullable(0), Some(false)); // <--- fails, reports `true`

    Ok(())
}

here column 0 is INTEGER NOT NULL but sqlx still thinks it is nullable.

[tyrelr]

That test looks good, but it might be quite a while before I get spare time to poke at it.

If you want a quick fix, you probably will want to use the type alias override functionality to work around the bug. Any behavioural change in the analyzer is considered a breaking change, so wouldn't be included in a bugfix release.

If you want to try poking the bug itself, run the test with environment variable RUST_LOG=trace to get a listing of all the execution paths from. Warning/Apology in advance... the logs are not easy to read.

But the idea is basically to search through to find find an execution path which resulted in null, then reason through where sqlx logic handled something wrong. It's often due to some command or control flow having implicit/explicit null handling which isn't recognized/implemented by the logic in sqlx-sqlite/src/connection/explain.rs.

[leshow]

Thanks, I appreciate the tips, I will post here if I have any luck next week.

[joao-conde]

Any updates on this?
I have the following code working:

#[derive(Deserialize, Serialize, FromRow)]
pub struct Todo {
    pub id: i64,
    pub title: String,
    pub description: String,
}

pub async fn create_todo(pool: &SqlitePool, todo: CreateTodo) -> Result<Todo, InternalError> {
    let todo = sqlx::query_as!(
        Todo,
        "INSERT INTO todos (title, description) VALUES (?, ?) RETURNING *",
        todo.title,
        todo.description
    )
    .fetch_one(pool)
    .await?;
    Ok(todo)
}

This works too:

pub async fn update_todo(
    pool: &SqlitePool,
    id: i64,
    todo: CreateTodo,
) -> Result<(), InternalError> {
    sqlx::query_as!(
        Todo,
        "UPDATE todos SET title = ?, description = ? WHERE id = ?",
        todo.title,
        todo.description,
        id,
    )
    .fetch_one(pool)
    .await?;
    Ok(())
}

But this fails:

pub async fn update_todo(
    pool: &SqlitePool,
    id: i64,
    todo: CreateTodo,
) -> Result<Todo, InternalError> {
    let todo = sqlx::query_as!(
        Todo,
        // notice the difference is the RETURNING clause
        "UPDATE todos SET title = ?, description = ? WHERE id = ? RETURNING *",
        todo.title,
        todo.description,
        id,
    )
    .fetch_one(pool)
    .await?;
    Ok(todo)
}

with:

error[E0277]: the trait bound `i64: std::convert::From<std::option::Option<i64>>` is not satisfied
  --> src\db.rs:49:16
   |
49 |       let todo = sqlx::query_as!(
   |  ________________^
50 | |         Todo,
51 | |         "UPDATE todos SET title = ?, description = ? WHERE id = ? RETURNING *",
52 | |         todo.title,
53 | |         todo.description,
54 | |         id,
55 | |     )
   | |_____^ the trait `std::convert::From<std::option::Option<i64>>` is not implemented for `i64`, which is required by `std::option::Option<i64>: Into<_>`

using TOML:

sqlx = { version = "0.7", default-features = false, features = [
    "macros",
    "sqlite",
] }

and lock:

[[package]]
name = "sqlx"
version = "0.7.4"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "c9a2ccff1a000a5a59cd33da541d9f2fdcd9e6e8229cc200565942bff36d0aaa"
dependencies = [
 "sqlx-core",
 "sqlx-macros",
 "sqlx-sqlite",
]

[Ddystopia]

I have the same issue as @joao-conde