Page MenuHomePhabricator
Differential D5158

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

Authored by kamil on Sep 16 2022, 1:52 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 18, 8:52 PM
Unknown Object (File)
Tue, Mar 18, 8:52 PM
Unknown Object (File)
Tue, Mar 18, 1:47 AM
Unknown Object (File)
Tue, Mar 18, 1:47 AM
Unknown Object (File)
Tue, Mar 18, 1:47 AM
Unknown Object (File)
Mon, Mar 17, 4:18 AM
Unknown Object (File)
Feb 17 2025, 7:09 PM
Unknown Object (File)
Feb 17 2025, 6:30 PM
View All Files
Subscribers
• abosh
ashoat
atul
varun

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
Branch
db-migrations-copy
Lint
No Lint Coverage
Unit
No Test Coverage

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

tomek added a comment.Sep 16 2022, 10:42 AM

@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
kamil updated this revision to Diff 16823.Sep 19 2022, 3:32 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!

atul added a comment.Sep 19 2022, 3:00 PM

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.

ashoat added a subscriber: varun.Sep 20 2022, 4:44 AM

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

atul added a comment.Sep 20 2022, 4:59 AM

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.

kamil added a comment.Sep 20 2022, 5:40 AM
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 accepted this revision.Sep 20 2022, 5:48 AM
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)

• jon resigned from this revision.Sep 20 2022, 8:27 AM

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
kamil updated this revision to Diff 17100.Sep 27 2022, 4:28 AM

throw system error on version save failure

Harbormaster completed remote builds in B12453: Diff 17100.Sep 27 2022, 4:38 AM
kamil updated this revision to Diff 17101.Sep 27 2022, 4:42 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

Revision Contents
Changeset List

PathSize
native/
cpp/
CommonCpp/
DatabaseManagers/
150 lines

Diff 17100

View Options

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp

Show First 20 Lines • Show All 98 Lines • ▼ Show 20 Linesconst bindCallKeyserverEndpointSelector = createSelector(
( (
dispatch: Dispatch, dispatch: Dispatch,
currentUserInfo: ?CurrentUserInfo, currentUserInfo: ?CurrentUserInfo,
keyserverInfos: { +[keyserverID: string]: KeyserverInfo }, keyserverInfos: { +[keyserverID: string]: KeyserverInfo },
) => { ) => {
return _memoize( return _memoize(
<Args: $ReadOnlyArray<mixed>, Return>( <Args: $ReadOnlyArray<mixed>, Return>(
keyserverCall: KeyserverCall<Args, Return>, keyserverCall: KeyserverCall<Args, Return>,
): ((...Args) => Promise<Return>) => { ): ((...Args) => Promise<Return>) => {
michalUnsubmitted
Not Done
Inline Actions

I meant that this Return is wrong, because in case of fanout it returns [Return]

michal: I meant that this `Return` is wrong, because in case of `fanout` it returns `[Return]`
const callKeyserverEndpoint = ( const callKeyserverEndpoint = (
endpoint: Endpoint, endpoint: Endpoint,
data: Object, data: Object,
args: Args, args: Args,
options?: ?CallServerEndpointOptions, options?: ?CallServerEndpointOptions,
) => { ) => {
// TODO const bindCallKeyserverEndpoint = (keyserverID: string) => {
if (keyserverCall.config.keyserverSelection === 'fanout') {
return Promise.resolve(undefined);
}
const keyserverID = keyserverCall.config.keyserverIDExtractor(
...args,
);
const { const {
cookie, cookie,
urlPrefix, urlPrefix,
sessionID, sessionID,
connection, connection,
lastCommunicatedPlatformDetails, lastCommunicatedPlatformDetails,
} = keyserverInfos[keyserverID]; } = keyserverInfos[keyserverID];
const boundCallServerEndpoint = createBoundServerCallsSelector( const boundCallServerEndpoint = createBoundServerCallsSelector(
keyserverID, keyserverID,
)({ )({
dispatch, dispatch,
currentUserInfo, currentUserInfo,
cookie, cookie,
urlPrefix, urlPrefix,
sessionID, sessionID,
connectionStatus: connection.status, connectionStatus: connection.status,
lastCommunicatedPlatformDetails, lastCommunicatedPlatformDetails,
}); });
return boundCallServerEndpoint(endpoint, data, options); return boundCallServerEndpoint(endpoint, data, options);
}; };
if (keyserverCall.config.keyserverSelection === 'fanout') {
const promises = [];
for (const keyserverID in keyserverInfos) {
promises.push(bindCallKeyserverEndpoint(keyserverID));
}
return Promise.all(promises);
}
const keyserverID = keyserverCall.config.keyserverIDExtractor(
...args,
);
return bindCallKeyserverEndpoint(keyserverID);
};
return keyserverCall.actionFunc(callKeyserverEndpoint); return keyserverCall.actionFunc(callKeyserverEndpoint);
}, },
); );
}, },
); );
function useKeyserverCall<Args: $ReadOnlyArray<mixed>, Return>( function useKeyserverCall<Args: $ReadOnlyArray<mixed>, Return>(
Show All 21 Lines