Details

Reviewers

tomek
atul
marcin
ashoat
• jon

Commits

rCOMMccc4e5f12d6c: [native/sqlite] infer db schema from `sqlite_orm`

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Sep 16 2022, 1:52 AM

kamil held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptSep 16 2022, 1:52 AM

Herald added subscribers: • abosh, atul, ashoat. · View Herald Transcript

kamil added a parent revision: D5150: [native/sqlite] add missing indexes to `sqlite_orm` structure.Sep 16 2022, 1:53 AM

Harbormaster completed remote builds in B12200: Diff 16735.Sep 16 2022, 2:02 AM

kamil edited the summary of this revision. (Show Details)Sep 16 2022, 2:07 AM

kamil edited the test plan for this revision. (Show Details)

kamil edited reviewers, added: atul, • karol, marcin; removed: • jon.

Herald added a reviewer: • jon. · View Herald TranscriptSep 16 2022, 2:07 AM

kamil published this revision for review.Sep 16 2022, 2:17 AM

marcin added inline comments.Sep 16 2022, 6:00 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.

ashoat added inline comments.Sep 16 2022, 7:23 AM

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

Harbormaster completed remote builds in B12262: Diff 16823.Sep 19 2022, 3:42 AM

kamil added inline comments.Sep 19 2022, 3:50 AM

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.

ashoat added inline comments.Sep 19 2022, 10:00 AM

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

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.

tomek resigned from this revision.Sep 21 2022, 4:30 AM

atul accepted this revision.Sep 21 2022, 10:37 AM

marcin accepted this revision.Sep 26 2022, 6:17 AM

This revision is now accepted and ready to land.Sep 26 2022, 6:17 AM

marcin added inline comments.Sep 26 2022, 6:20 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.

marcin added 1 blocking reviewer(s): tomek.Sep 27 2022, 4:15 AM

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

throw system error on version save failure

Harbormaster completed remote builds in B12453: Diff 17100.Sep 27 2022, 4:38 AM

update error type

Harbormaster completed remote builds in B12454: Diff 17101.Sep 27 2022, 4:51 AM

tomek accepted this revision.Sep 27 2022, 5:53 AM

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

Closed by commit rCOMMccc4e5f12d6c: [native/sqlite] infer db schema from `sqlite_orm` (authored by kamil). · Explain WhySep 28 2022, 2:10 AM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMMccc4e5f12d6c: [native/sqlite] infer db schema from `sqlite_orm`.

ashoat added a reverting change: rCOMM147f0455e29e: Revert "[native/sqlite] infer db schema from `sqlite_orm`".Oct 7 2022, 6:45 AM

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 17133

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 17133

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp

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

Revision Contents
Changeset List