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

Copying storage_t causes all subsequent operations to fail with "no such table" SQL logic error #517

Closed
mrkline opened this issue May 20, 2020 · 6 comments
Labels

Comments

@mrkline
Copy link
Contributor

mrkline commented May 20, 2020

Consider the following snippet:

#include <sqlite_orm/sqlite_orm.h>

#include <iostream>

using namespace sqlite_orm;

struct Row {
    std::string foo;
    int bar;
};

auto open(const char* dbPath)
{
    auto storage = make_storage(dbPath,
            make_table(
                "Table",
                make_column("foo", &Row::foo),
                make_column("bar", &Row::bar)));
    return storage;
}

using Orm = decltype(open(""));

struct OpenAndSyncResult {
#ifdef COPIED
    Orm database;
#else
    std::unique_ptr<Orm> database;
#endif
    /// A map of table names and actions taken to sync
    std::map<std::string, sqlite_orm::sync_schema_result> syncResults;
};

OpenAndSyncResult openAndSync(const char* dbPath)
{
#ifdef COPIED
    Orm db = open(dbPath);
    std::map<std::string, sqlite_orm::sync_schema_result> syncResult = db.sync_schema();
#else
    auto db = std::make_unique<Orm>(open(dbPath));
    std::map<std::string, sqlite_orm::sync_schema_result> syncResult = db->sync_schema();
#endif
    return { std::move(db), std::move(syncResult) };
}

int main()
{
    try {
        auto sunk = openAndSync("");
#ifdef COPIED
        auto& db = sunk.database;
#else
        auto& db = *sunk.database;
#endif
        // Should already be sunk:
        for (auto& p : sunk.syncResults) {
            std::cerr << "synchronized table '" << p.first << "': " << p.second << '\n';
        }
        // Makes things work again - see below
        // db.sync_schema();

        db.insert(Row{ .foo = "I am row 1", .bar = 42});
    }
    catch (const std::exception& e) {
        std::cerr << e.what() << '\n';
    }
    return 0;
}

If the code is built with -DCOPIED (i.e., the storage object is moved or copied out of openAndSync(), the insert (and any other operations) fail with:

no such table: Table: SQL logic error

Interestingly, if sync_schema() is called on the returned copy, everything works again - it's as if copying the storage object forgets that it was synchronized.

This behavior seems new to v1.4 and v1.5; 1.3 works fine when built either way.
If we're not meant to copy storage_t, its copy constructor should be disabled.

@mrkline mrkline changed the title Copying the storage object causes all subesquent operations to fail with "no such table" SQL logic error Copying storage_t causes all subesquent operations to fail with "no such table" SQL logic error May 21, 2020
@mrkline mrkline changed the title Copying storage_t causes all subesquent operations to fail with "no such table" SQL logic error Copying storage_t causes all subsequent operations to fail with "no such table" SQL logic error May 21, 2020
@fnc12
Copy link
Owner

fnc12 commented May 21, 2020

It is ok until you specify empty storage path. Empty storage path tells SQLite that you want a memory database. Memory database drops all data on destruction and shares data with no one. Try to specify real file path (doesn't matter existing or not) and it will work. This is not a but cause SQLite3 works this way.

@fnc12 fnc12 added the notabug label May 21, 2020
@fnc12
Copy link
Owner

fnc12 commented May 21, 2020

If you need to copy data from one database to another (you actually have two databases in your case) please use backup API created for it:
storage.backup_to(storage2); or call sync_schema after copying.

@fnc12
Copy link
Owner

fnc12 commented May 23, 2020

@mrkline is the issue actual?

@fnc12
Copy link
Owner

fnc12 commented May 27, 2020

closing due to inactivity

@fnc12 fnc12 closed this as completed May 27, 2020
@mrkline
Copy link
Contributor Author

mrkline commented May 27, 2020

Sorry for the delay.

If you need to copy data from one database to another (you actually have two databases in your case) please use backup API created for it:
storage.backup_to(storage2); or call sync_schema after copying.

The concern is with the semantics here - if storage_t has a copy constructor, I'd expect the new copy to be in the same state (synchronized to the same backing database, etc.) as the original. If not, I'd expect the copy constructor to be disabled.

I understand that a memory database is a special case, but it still seems like surprising behavior.

@fnc12
Copy link
Owner

fnc12 commented May 27, 2020

No, copy ctor makes a copy of sqlite3 db handle. Different in-memory handles mean different databases. It is documented in SQLite docs so disabling it in sqlite_orm just because someone doesn't know some SQLite rules seems very strange. I can advice you to use move ctor instead - it must move db handle and preserve sync state.

@fnc12 fnc12 reopened this May 27, 2020
@fnc12 fnc12 closed this as completed May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants