Page MenuHomePhabricator

[native/sqlite] add missing unique and primary_key constraints to `sqlite_orm` structure
ClosedPublic

Authored by kamil on Sep 15 2022, 7:00 AM.
Tags
None
Referenced Files
F3113819: D5148.diff
Thu, Oct 31, 7:27 PM
Unknown Object (File)
Wed, Oct 9, 2:25 AM
Unknown Object (File)
Tue, Oct 8, 8:19 AM
Unknown Object (File)
Tue, Oct 8, 8:19 AM
Unknown Object (File)
Tue, Oct 8, 1:17 AM
Unknown Object (File)
Sun, Oct 6, 11:21 PM
Unknown Object (File)
Sep 24 2024, 3:36 AM
Unknown Object (File)
Sep 11 2024, 4:15 PM

Details

Summary

Add missing constraints to sqlite_orm storage which are present in sql creation code for olm_persist_account and olm_persist_sessions_table.

Test Plan
  1. Use decoded SQLite (e.g. comment encryption code and re-install app)
  2. Instead of running migrations run SQLiteQueryExecutor::getStorage().sync_schema(); which will generate database.
  3. Connect do database using sqlite3 and call .schema;
  4. Check if constraints for mentioned table match previous one, created by migrations.

Diff Detail

Repository
rCOMM Comm
Branch
db-migrations
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil edited the test plan for this revision. (Show Details)
kamil added reviewers: karol, atul.
kamil published this revision for review.Sep 15 2022, 8:45 AM

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?

not familiar enough with this yet to give good feedback

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.

  1. 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.

ashoat removed a reviewer: karol. ashoat added 2 blocking reviewer(s): atul, marcin.

This diff looks good to me, but would love if @atul and @marcin could take a look. (And then @tomek on a second pass.)

@marcin please give it a review and add me as a blocking reviewer when accepted

marcin added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
605

Are we 100% percent sure we do not need to explicitly provide not_null() constraint here? Is it enough that declaring a field as std::string or int in C++ db entity is enough for ORM to generate NOT NULL constraint for us? I am asking since I can imagine that even though we cannot instantiate an entity in C++ with null variable the corresponding SQL generated by ORM might still not contain not null constraint which would lead discrepancies you described in other differentials in this stack.
You can either link docs or describe result of experimentation where you examined schema of unencrypted db and NOT null constraint was there. Accepting the revision since if answer to any of those questions is YES then feel free to land this diff without waiting for me to read the answer and respond.

This revision is now accepted and ready to land.Sep 26 2022, 7:04 AM
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
605

Are we 100% percent sure we do not need to explicitly provide not_null() constraint here? Is it enough that declaring a field as std::string or int in C++ db entity is enough for ORM to generate NOT NULL constraint for us?

Yes and yes.

According to docs: "not_null is deduced from type automatically". (end of linked section).

This revision now requires review to proceed.Sep 27 2022, 4:16 AM
This revision is now accepted and ready to land.Sep 27 2022, 5:35 AM