Page MenuHomePhabricator

[native/sqlite] infer db schema from `sqlite_orm`
ClosedPublic

Authored by kamil on Sep 16 2022, 1:52 AM.
Tags
None
Referenced Files
F3112018: D5158.diff
Thu, Oct 31, 4:03 PM
Unknown Object (File)
Wed, Oct 9, 8:41 PM
Unknown Object (File)
Wed, Oct 9, 8:41 PM
Unknown Object (File)
Wed, Oct 9, 2:28 AM
Unknown Object (File)
Wed, Oct 9, 2:28 AM
Unknown Object (File)
Wed, Oct 9, 2:28 AM
Unknown Object (File)
Tue, Oct 8, 8:20 AM
Unknown Object (File)
Sun, Oct 6, 11:21 PM

Details

Summary

Instead of running all migrations to create a fresh database, we can synchronize schema with the structure declared in make_storage (task).

According to sqlite_orm docs "sync_schema function that takes responsibility of comparing actual db file schema with one you specified in make_storage call and if something is not equal it alters or drops/creates schema." which suits to our needs.

Note: we used this method previously and was removed in https://phab.comm.dev/D1765, more details in this notion note.
However, this time we use this in a different context, not for migration but for creating a clean database schema, so mentioned in previous links problems with deleting data should not appear.

Test Plan

Logout user (it'll recreate database) or clean install app on the device to make sure current db version is 0 - migration should not start and schema should be created (can be checked without encryption using eg. sqlite3).
In any other scenario when db version is different from 0 migration should run (or no if the version is equal to the highest migration index) as it was previously

Diff Detail

Repository
rCOMM Comm
Branch
db-migrations-copy
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 edited reviewers, added: atul, karol, marcin; removed: jon.
kamil published this revision for review.Sep 16 2022, 2:17 AM
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
605 ↗(On Diff #16735)

We usually use string stream for logging errors in which case const char* representing error message is inserted into the stream. In this case we can probably just directly pass string literal into log method.

614 ↗(On Diff #16735)

Shouldn't we check for failure here? sqlite3_exec can return error due to SQL execution failure. Are we sure we can't get error here or even if we do we can ignore it? If neither of those two then we should check for it.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
602 ↗(On Diff #16735)

I think this is fairly dangerous because it can drop and recreate the table. Makes sense that it is safe when current_user_version == 0, but I'm confused why it's necessary now given it wasn't necessary before

@marcin These are valid comments! Feel free to request changes in cases like this.

marcin requested changes to this revision.Sep 19 2022, 2:29 AM
marcin added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
614 ↗(On Diff #16735)

We should always check for such calls failing. If we don't the data we get when app crashes are hard to decipher.

This revision now requires changes to proceed.Sep 19 2022, 2:29 AM

simplify logging, handle sqlite3_exec error

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
602 ↗(On Diff #16735)

It wasn't necessary because the database was created by running all migration from 0 (eg. it creates drafts table with the wrong name and after this renaming one columm) - we don't have separate code for creating schema.

I think creating schema using sync_schema() in this context (current_user_version == 0) will be good because we will reuse already created ORM model.

There is one alternative solution to that - writing new proper SQL queries to create schema and run them when current_user_version == 0 - which will be basically the same as running all migrations with a little simplification.

614 ↗(On Diff #16735)

Thanks for the suggestion, I agree that this should be done - that being said we should think about adding a check to this line which does basically the same.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
602 ↗(On Diff #16735)

Makes sense, thanks for explaining!

There is one alternative solution to that - writing new proper SQL queries to create schema and run them when current_user_version == 0 - which will be basically the same as running all migrations with a little simplification.

I think this approach makes the most sense. I'd prefer to to avoid sqlite_orm and match the existing migrations approach as much as possible.

It's fairly straightforward to read SQLite's dialect of SQL and understand what's going on (with help from the comprehensive docs). On the other hand, reading through sqlite_orm docs and source has been pretty frustrating (for me) in the past.

I'd actually like to remove sqlite_orm altogether, personally... not sure we get much value from it. I'd support reopening ENG-1595 if people agree. Perhaps something we could talk about in office hours, cc @varun

not sure we get much value from it

I think we get a decent amount of value from it for the implementations of all the functions listed in DatabaseQueryExecutor.h. Not sure we want to sign up for writing a bunch of SQL queries when we have something that works at the moment.

That said, I agree that it would be good to move off of sqlite_orm at some point so I think it makes sense to move from Canceled to Backlog.

In D5158#151949, @atul wrote:

There is one alternative solution to that - writing new proper SQL queries to create schema and run them when current_user_version == 0 - which will be basically the same as running all migrations with a little simplification.

I think this approach makes the most sense. I'd prefer to to avoid sqlite_orm and match the existing migrations approach as much as possible.

It's fairly straightforward to read SQLite's dialect of SQL and understand what's going on (with help from the comprehensive docs). On the other hand, reading through sqlite_orm docs and source has been pretty frustrating (for me) in the past.

but it's tough to make it compatible with ORM model, more details I wrote here: https://phab.comm.dev/D5148#152210

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

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

not familiar enough with the sqlite_orm to have meaningful feedback.

This revision is now accepted and ready to land.Sep 26 2022, 6:17 AM
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
620 ↗(On Diff #16823)

How is application going to function if this error occurs? In most places we the runtime error to kill the app if database operation fails since application is not going to work correctly. I just want to know whether it is also the case here.

This revision now requires review to proceed.Sep 27 2022, 4:15 AM

throw system error on version save failure

tomek added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
603 ↗(On Diff #17101)

I can imagine one edge case here: if a user that has an existing db with current_user_version == 0 decides to upgrade the app, we would call sync_schema instead of running the migrations. So the code of the user would contain a db, but would be old enough to not have any migration. This shouldn't be an issue though, because version 0 doesn't contain anything meaningful.

This revision is now accepted and ready to land.Sep 27 2022, 5:53 AM