Possible unexpected behavior in bdk_chain::migrate_schema

[nymius]

Describe the bug

migrate_schema skips the first n schemas to apply where nis the current schema version. That means, if I want to independently apply a new schema k+1 to a database already using schema k I should create a list of k+1 schemas (where any string, even empty ones, could be an schema) as migrate_schema will skip the k first anyway.
If I don't do that, skip will reach the end of iterator sooner and return another empty iterator, forcing migrate_schema to do nothing.

So:

migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[schema_v0])?;

applies schema_v0.

But

migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[schema_v1])?;

applied in a schema_v0 database does nothing.

And

migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[schema_v0, schema_v1])?;

applied in a schema_v0 database applies schema_v1.

As there is a workaround for this, I don't think it is a great deal. But I think we should at least document it, if not changed.

To Reproduce

#[test]
fn schemas_can_be_applied_independently() -> anyhow::Result<()> {
    type ChangeSet = tx_graph::ChangeSet<ConfirmationBlockTime>;
    use alloc::string::String;
    let schema_v0 = "CREATE TABLE test_table_0 (id TEXT PRIMARY KEY NOT NULL) STRICT";
    let schema_v1 = "CREATE TABLE test_table_1 (id TEXT PRIMARY KEY NOT NULL) STRICT";
    let mut conn = rusqlite::Connection::open_in_memory()?;

    // Apply schema_v0
    {
        let db_tx = conn.transaction()?;
        migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[&schema_v0])?;
        db_tx.commit()?;
    }

    assert_eq!(
        conn.query_row(
            "SELECT name FROM sqlite_master WHERE type='table' AND name='test_table_0';",
            [],
            |row| row.get::<_, String>(0),
        )?,
        "test_table_0"
    );

    // Apply schema_v1 only
    {
        let db_tx = conn.transaction()?;
        migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[&schema_v1])?;
        db_tx.commit()?;
    }

    assert_eq!(
        conn.query_row(
            "SELECT name FROM sqlite_master WHERE type='table' AND name='test_table_1';",
            [],
            |row| row.get::<_, String>(0),
        ),
        Err(rusqlite::Error::QueryReturnedNoRows)
    );

    // Apply "dummy" and then schema_v1
    {
        let db_tx = conn.transaction()?;
        migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[&"dummy", &schema_v1])?;
        db_tx.commit()?;
    }

    assert_eq!(
        conn.query_row(
            "SELECT name FROM sqlite_master WHERE type='table' AND name='test_table_1';",
            [],
            |row| row.get::<_, String>(0),
        )?,
        "test_table_1"
    );
    Ok(())
}

git diff

diff --git a/crates/chain/src/rusqlite_impl.rs b/crates/chain/src/rusqlite_impl.rs
index 7b39f53c..d70f1d7b 100644
--- a/crates/chain/src/rusqlite_impl.rs
+++ b/crates/chain/src/rusqlite_impl.rs
@@ -715,7 +715,64 @@ mod test {
             db_tx.commit()?;
             assert!(changeset.anchors.contains(&(anchor, txid)));
         }
+        Ok(())
+    }
+
+    #[test]
+    fn schemas_can_be_applied_independently() -> anyhow::Result<()> {
+        type ChangeSet = tx_graph::ChangeSet<ConfirmationBlockTime>;
+        use alloc::string::String;
+        let schema_v0 = "CREATE TABLE test_table_0 (id TEXT PRIMARY KEY NOT NULL) STRICT";
+        let schema_v1 = "CREATE TABLE test_table_1 (id TEXT PRIMARY KEY NOT NULL) STRICT";
+        let mut conn = rusqlite::Connection::open_in_memory()?;
+
+        // Apply schema_v0
+        {
+            let db_tx = conn.transaction()?;
+            migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[&schema_v0])?;
+            db_tx.commit()?;
+        }
+
+        assert_eq!(
+            conn.query_row(
+                "SELECT name FROM sqlite_master WHERE type='table' AND name='test_table_0';",
+                [],
+                |row| row.get::<_, String>(0),
+            )?,
+            "test_table_0"
+        );
 
+        // Apply schema_v1 only
+        {
+            let db_tx = conn.transaction()?;
+            migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[&schema_v1])?;
+            db_tx.commit()?;
+        }
+
+        assert_eq!(
+            conn.query_row(
+                "SELECT name FROM sqlite_master WHERE type='table' AND name='test_table_1';",
+                [],
+                |row| row.get::<_, String>(0),
+            ),
+            Err(rusqlite::Error::QueryReturnedNoRows)
+        );
+
+        // Apply "dummy" and then schema_v1
+        {
+            let db_tx = conn.transaction()?;
+            migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[&"dummy", &schema_v1])?;
+            db_tx.commit()?;
+        }
+
+        assert_eq!(
+            conn.query_row(
+                "SELECT name FROM sqlite_master WHERE type='table' AND name='test_table_1';",
+                [],
+                |row| row.get::<_, String>(0),
+            )?,
+            "test_table_1"
+        );
         Ok(())
     }
 }

Expected behavior

1. To apply schema_v5 when calling migrate_schema(..., &[schema_v5]) on a database with schema v4.
2. To raise an error asking to apply schema_v4 first if calling migrate_schema(..., &[schema_v5]) on a database with schema v3.
3. To do nothing when calling migrate_schema(..., &[schema_v3]) on a database with schema v4.
4. To apply schema_v4, schema_v5 in order and ignore schema_v3 when calling migrate_schema(..., &[schema_v4, schema_v3, schema_v5]) on a database with schema v3.

[notmandatory]

Thanks for the great description of this issue. As long as BDK is implementing the DB migration for users I think this is a low risk issue. But still good to have it documented in case anyone wants to implement a better way to do it.

[evanlinjin]

migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[schema_v1])?;

But this is just wrong code right?

I don't think this ticket is a bug, but a feature request for performance reasons?

However, looking at this, I do have a new thought. If the current (persisted) schema version is greater than the defined schemas, we should probably error out. --> This is the bug.

[nymius]

migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[schema_v1])?;

But this is just wrong code right?

Understood. Maybe I'm the only one committing this error, but then, shouldn't we add to migrate_schemas's docs a note mentioning the correct way to call this function is by passing a list including the schema we want to apply and all previous ones, in order? Probably the above example of what you should not do would help too.

However, looking at this, I do have a new thought. If the current (persisted) schema version is greater than the defined schemas, we should probably error out. --> This is the bug.

But the notion of "version" for the individual schemas is given by their order in the versioned_scripts dir. It can be faked by adding empty strings until the current persisted version number is reached, and then add the schema we want to apply.
What I mean with this is incorrect behavior is still possible by relying in an array to apply the schemas, if we want to enforce just one possible behavior, I think we should add the notion of version to the scripts themselves. I don't know which kind of structure is better for this.