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

sync_schema ignores the addition of foreign keys, and incorrect insert statement #790

Open
khgreav opened this issue Sep 13, 2021 · 11 comments

Comments

@khgreav
Copy link

khgreav commented Sep 13, 2021

Hi, first and foremost, thanks for the library as I haven't seen too many usable ones around.

Now to get my issue, I've been playing with the library and testing it for a bit on and off. Yesterday, I came across 2 issues.
To illustrate, I've put together a small example below.

I tend to use class-based entities, but that shouldn't be an issue, at least I never found one up until now.

#include <sqlite_orm/sqlite_orm.h>
#include <iostream>

using namespace sqlite_orm;

class A {
public:
	A() = default;
	A(const uint8_t &a) : a(a) {};

	const uint32_t& getId() const {
		return this->id;
	}

	void setId(const uint32_t &id) {
		this->id = id;
	}

	const uint8_t& getA() const {
		return this->a;
	}

	void setA(const uint8_t &a) {
		this->a = a;
	}
private:
	uint32_t id;
	uint8_t a;
};

class B {
public:
	B() = default;
	B(const uint8_t &b) : b(b) {};

	const uint32_t& getId() const {
		return this->id;
	}

	void setId(const uint32_t &id) {
		this->id = id;
	}

	const uint8_t& getB() const {
		return this->b;
	}

	void setB(const uint8_t &b) {
		this->b = b;
	}
private:
	uint32_t id;
	uint8_t b;
};

class C {
public:
	C() = default;
	C(const uint32_t &aId, const uint32_t &bId) : aId(aId), bId(bId) {};

	const uint32_t& getAId() const {
		return this->aId;
	}

	void setAId(const uint32_t &aId) {
		this->aId = aId;
	}

	const uint32_t& getBId() const {
		return this->bId;
	}

	void setBId(const uint32_t &bId) {
		this->bId = bId;
	}
private:
	uint32_t aId;
	uint32_t bId;
};

static auto initExample(const std::string &file) {
	return make_storage(file,
		make_table("A",
			make_column("id", &A::getId, &A::setId, primary_key(), autoincrement()),
			make_column("a", &A::getA, &A::setA)
		),
		make_table("B",
			make_column("id", &B::getId, &B::setId, primary_key(), autoincrement()),
			make_column("b", &B::getB, &B::setB)
		),
		make_table("C",
			make_column("aId", &C::getAId, &C::setAId),
			make_column("bId", &C::getBId, &C::setBId),
			primary_key(&C::getAId, &C::getBId),
			foreign_key(&C::getAId).references(&A::getId), // added later once the table was already created
			foreign_key(&C::getBId).references(&B::getId) //added later once the table was already created
		)
	);
}

using Db = decltype(initExample(""));

static void example(const std::string &path) {
	std::unique_ptr<Db> db = std::make_unique<Db>(initExample(path));
	auto res = db->sync_schema();
	A a(5);
	B b(10);
	uint32_t aId, bId;
	try {
		aId = db->insert(a);
		bId = db->insert(b);
	} catch (const std::system_error &e) {
		//
	}
	try {
		C c(aId, bId);
		db->insert(c);
	} catch (const std::system_error &e) {
		//
	}
}
  1. It seems that sync_schema() ignores addition of foreign keys once a table has been created previously. When I first created entity and table C, the foreign keys were not included, after adding foreign keys and calling sync_schema(), the return value reports all tables as already_in_sync. Removing the database file and letting it be created again from scratch results in the desired composite and foreign keys.

  2. Table C exists to match records from A and B, however, the insert statement fails. After debugging the code, I found out that the serialized statement to be executed is this: "INSERT INTO 'C' DEFAULT VALUES ".

Am I doing this wrong? Thanks for the help in advance.

@fnc12
Copy link
Owner

fnc12 commented Sep 13, 2021

Hi. Thank you for using this lib and saying such nice words about it.

  1. Actually yes. sync_schema was created to sync column equality first and it has not FOREIGN KEY support. There is a TODO entry in TODO list which is related to foreign keys. I (or someone) need(s) to add this feature. If you can help it would be nice to know how to know foreign key existence using raw SQL queries that I can use inside sync_schema function later.
  2. What did you expect instead of INSERT INTO 'C' DEFAULT VALUES? If you want to INSERT primary keys explicitly then use storage.replace instead cause storage.insert was designed to ignore primary key columns for cases and return new valid for from SQLite engine. To make it clear I'd like to add a raw insert function to be able to do something just like this:
storage.insert_into<C>(columns(&C::getAId, &C::getBId), values(5, 10));

I'll add raw insert into TODO list right now.

BTW can you share a project you use this lib for? I collect a list of projects to publish it inside this repo. Thanks

@fnc12
Copy link
Owner

fnc12 commented Sep 13, 2021

Hey. I've added raw insert in TODO list a moment age

@fnc12 fnc12 added the bug label Sep 13, 2021
@khgreav
Copy link
Author

khgreav commented Sep 13, 2021

Thanks for the quick reply.

  1. Alright, I'd love to help, I'll look into it in my spare time. It should definitely be possible. I'm using sqlitebrowser to visualize contents of the database and it can display foreign keys, should I contribute here if I figure something out?
  2. That's a mistake on my part then, I tried using storage.replace in the example and it seemed to work. So probably fine.

For the project, sure! I'm doing some testing on my own, but if everything goes well, we'd love to use it here.

@fnc12
Copy link
Owner

fnc12 commented Sep 13, 2021

should I contribute here if I figure something out?

Yes

That's a mistake on my part then, I tried using storage.replace in the example and it seemed to work. So probably fine.

Nice!

Thank you! I'll place it right into #783 just yet.

@khgreav
Copy link
Author

khgreav commented Sep 13, 2021

Thank you! I'll place it right into #783 just yet.

Not sure how things work out, but like I said, if I don't find any major missing features, I definitely want to use the library for our project. C/C++ ORM is hard to come by. Requirements aside, I love projects like these and I want to see them get more traction. Every bit of exposure helps bring people in and contribute to make them even better.

@fnc12
Copy link
Owner

fnc12 commented Sep 13, 2021

Oh that is cool. Please report every missing feature. It is very important. The goal if this lib is replacing raw string queries with strong typed and typos free with C++

@khgreav
Copy link
Author

khgreav commented Sep 15, 2021

Hi, after a bit of googling, it seems that you can just use a pragma.

Using PRAGMA foreign_key_list("table_name") should do the trick, see the image below.

Result on the same database from the original post.

image

From there, the handling should be similar to your get_table_info function.

from -> table.to

@fnc12
Copy link
Owner

fnc12 commented Sep 17, 2021

Thank you. I'll make an improvement of sync_schema soon

@fnc12
Copy link
Owner

fnc12 commented Dec 30, 2021

@khgreav how do you expect to alter table and add foreign key? Is there a query type for this? Cause I don't know how to do this

@fnc12
Copy link
Owner

fnc12 commented Dec 31, 2021

looks like there is no way of doing this out of box. Prove. So the feature will work like this:

  1. if you call sync_schema and it is expected to alter foreign key constraint then it will drop and recreate the whole table if preserve argument is false and backup a table if preserve is true

@fnc12 fnc12 added enhancement and removed bug labels Dec 31, 2021
@khgreav
Copy link
Author

khgreav commented Dec 31, 2021

I can have a look and let ya know if I figure something fancy out, the standard procedure for sqlite is to rename the old table, create a new table with the original name and then copy the data over and delete the old table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants