Database connections are cached by name

[lgrapenthin]

It appears that both the IOS and Android implementation cache database connections, by name. This causes #109, but also surprises users who rely on working with more than one connection.

A tyical usecase for having multiple connections is to operate on the same database from different parts of the program that BEGIN and COMMIT their own (deferred) transactions. By operating on their own connections, they can rely on concurrent transactions being serialized by sqlite. WIth the connection caching implemented in this library, the cordova predecessor, and other libraries that copied from this one, the BEGIN and COMMIT statements will become interleaved and it results in unexpected errors such as "No transaction active."

It should be reconsidered whether this caching of connections is necessary, what problem it is supposed to solve, and if any, whether it should be enabled by default. At the very least, users should have an option to bypass it.

[miallo]

The following test will work because it circumvents the caching of the database connections.

const exec = (db, sql, dbnr) =>
    new Promise((resolve, reject) =>
        db._db.exec(
            [
                {
                    sql,
                    args: [],
                },
            ],
            false,
            (e, result) => {
                if (result[0].error) {
                    console.error({ dbnr, sql, e, result, row: result[0].rows });
                    reject(result)
                } else {
                    console.log({ dbnr, sql, e, result, row: result[0].rows });
                    resolve(result);
                }
            }
        )
    );

const rnd = Math.random() // don't accidentally use a previous DB
const db1 = SQLite.openDatabase(`./test${rnd}.db`); // since the caching does not do path sanitisation, but the `stringByAppendingPathComponent` does, we can get around the caching with putting a second slash in the path
const db2 = SQLite.openDatabase(`.//test${rnd}.db`); // Removing second slash will fail the test on iOS, because then it uses the cached database transaction
await exec(
    db1,
    "CREATE TABLE IF NOT EXISTS schema_version(" +
        "version_id INTEGER PRIMARY KEY NOT NULL);"
);
await exec(db1, "BEGIN;", 1);
await exec(db2, "BEGIN;", 2); // the native iOS SQLite does not allow two BEGINs on the same (cached) connection, whereas the shipped Android version will ignore it
await exec(db1, "SELECT * FROM schema_version;", 1);
await exec(db2, "SELECT * FROM schema_version;", 2);
await exec(db1, "COMMIT;", 1);
await exec(db2, "COMMIT;", 2);

Removing one of the slashes in the path of db2 will create the "cannot start a transaction within a transaction." error on iOS, because it is stricter than the shipped SQLite version on Android. This explains why the error is only visible on iOS. But on Android this can lead to unexpected behaviour, since transactions are interleaved without the knowledge of the user

[craftzdog]

The native module needs to open the database on every query:

• iOS: https://github.com/craftzdog/react-native-sqlite-2/blob/master/ios/RNSqlite2.m#L106
• Android: https://github.com/craftzdog/react-native-sqlite-2/blob/master/android/src/main/java/dog/craftz/sqlite_2/RNSqlite2Module.java#L83

So, it has to be cached in the native side by design.

[miallo]

Couldn't one split up the function openDatabase into two: one that is there for opening a new connection (and putting that into the cache, but e.g. with an ever increasing numerical identifier instead of the DB-name) and one for getting a cached one. The first is then exposed to JS in the openDatabase and the second one is only used in the native module where we want to work on the already opened database. Or what am I missing?

[lgrapenthin]

@craftzdog Fair enough, I have changed the issue title from "Database connections are cached" to "Database connections are cached by name" - To make it clear: Instead of being cached by name, they should be cached by connection identity.

[miallo]

@craftzdog First of all thanks a lot for your work in this project!

Should I create a MR for this suggestion?

[craftzdog]

Please hold on. I'm working on restructuring the library now.

[craftzdog]

It's ready for accepting your PR: https://github.com/craftzdog/react-native-sqlite-2/pull/116 Added an example project so you can easily test your patch: https://github.com/craftzdog/react-native-sqlite-2/blob/master/CONTRIBUTING.md

[miallo]

I thought a bit about it and it might be a breaking change if we change the API (e.g. if it is called many times). @craftzdog What do you think about having a useDatabase hook in addition to the functional-wise untouched openDatabase?

[craftzdog]

I guess it wouldn't be a breaking change because you could issue an ID for each database connection instead of using database names for referring the connections.

I don't understand why having a useDatabase hook solves that. It's React-specific but the issue doesn't have anything to do with it.

[miallo]

Thanks a lot for your feedback! You right, my explanation was very bad of what I had in mind and now I think that my suggestion of the hook is a bad idea. And I also completely agree with you that having a new API is generally bad, because the users would need to adapt it.

My fears are that (theoretically) one could rely on the caching the same database connection with openDatabase:

class MyComponent extends React.Component {
  render() {
    const db = openDatabase('my.db') // currently only opens one connection and then does the lookup in the cached Databases
    // With the changed behaviour this would open a connection on every render
    const onSave = () => {
      // store to database
    }
    return <Something onSave={onSave}/>;
  }
}

Do you think that this is a realistic scenario?

My (now probably discarded) thoughts about the useDatabase went roughly like:

const useDatabase = (name) => {
    const db = React.useRef()
    if (!db) {
        db.current = openNewConnectionById(name) // the `openDatabase` without caching is only called once
    }

    return db.current
}

but of course this would only work for functional components and also limits the usage there quite a lot. Everyone who needs it could implement it if the openNewConnectionById (with a better name ^^) was the function that is exposed instead.

[craftzdog]

It is the native-side issue.