Great catch!

Two questions:

1. Will this potentially result in the table being dropped and recreated? I seem to recall that if the schema changes, sqlite_orm can be very aggressive about this. (We might already disable this behavior, though... I don't recall)
2. Do we need some sort of migration to create these indices for existing tables?

Will this potentially result in the table being dropped and recreated? I seem to recall that if the schema changes, sqlite_orm can be very aggressive about this. (We might already disable this behavior, though... I don't recall)

Previous discussion:
https://phab.comm.dev/D1765
https://phab.comm.dev/D1113
https://www.notion.so/commapp/Investigate-sync_schema-issues-in-sqlite_orm-9d7421b1fdc34ed7b4e7054b1643b8c5

1. Will this potentially result in the table being dropped and recreated? I seem to recall that if the schema changes, sqlite_orm can be very aggressive about this. (We might already disable this behavior, though... I don't recall)

I don't think so, I don't see any reason for that because we don't call sync_schema() on the existing database, what is more, I think that sqlite_orm drop table where there is some inconsistency between ORM model and DB schema - which this diff should fix, make those two consistent.

2. Do we need some sort of migration to create these indices for existing tables?

Migrations are needed when we are changing DB schema - but those constraints are already there:

sqlite> .schema olm_persist_sessions
CREATE TABLE olm_persist_sessions(target_user_id TEXT UNIQUE PRIMARY KEY NOT NULL, session_data TEXT NOT NULL);

what we do here is adding a constraint to ORM which will result that the next time when this code is executed ORM model and DB will be consistent - not changed.

Previous discussion:
https://phab.comm.dev/D1765
https://phab.comm.dev/D1113

TBH I don't see how those two are related.

https://www.notion.so/commapp/Investigate-sync_schema-issues-in-sqlite_orm-9d7421b1fdc34ed7b4e7054b1643b8c5

Also, here (we don't use sync_schema()) - but I get your point that sqlite_orm lib might cause problems.

However, I double-checked check facts you wrote on the linked notion note and I found some incompatibility.

1. There is no schema provided or code to create a database so it’s hard to analyze this because there is no database structure I can refer to.
2. For adding you are using this object User user_two { -2, "John", "Doe", "john@doe.com" }; so I assume login as well as user_login fields are type of std::string - because string can not be null in c++ sync_schema() will generate this column (for user_string because that is specified in make_storage) as NOT NULL. Now, according to docs: "if there are columns in storage that do not exist in db they will be added using 'ALTER TABLE ... ADD COLUMN ...' command and table data will not be dropped but if any of added columns is null but has not default value table will be dropped and recreated", there is no default value, sqlite_orm doesn't have anything what can be put in user_email column that is why table is dropped.
3. Incopatibilities like this leads to a problems with dropping tables, lost data etc. and this is what I try fix in this diff.
4. Any other incompatibility will lead to dropping data - docs: "if there is any column existing in both db and storage but differs by any of properties (type, pk, notnull) table will be dropped and recreated (dflt_value isn't checked cause there can be ambiguity in default values, please beware)."
5. I was able to get code similar to yours running as you expected by using proper types.

That's why my point was to use sync_schema() on fresh install in later diff to make sure those two things (ORM model and db schema) are equals to avoid problems like this in the future.
Right now we have two separate sources and there is some discrepancy between them and it makes grow in future.

You are more experienced in this so I will follow your opinion but I want to share my point of view.
cc. @ashoat @atul

My strong belief is that we should avoid using sync_schema except when creating the database for the first time. It is too "magical" and can result in dangerous, unexpected behavior. (I think @kamil's changes are fully compatible with this, just noting it down.)

My loosely held belief is that sqlite_orm does not deliver much value, and it would take ~1 day to replace it with SQLite queries, which will reduce an unnecessary layer of abstraction and simplify our code and our assumptions.