Page MenuHomePhabricator

[native/sqlite] introduce function to create database
ClosedPublic

Authored by kamil on Oct 17 2022, 5:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 17, 8:39 AM
Unknown Object (File)
Tue, Dec 17, 8:39 AM
Unknown Object (File)
Tue, Dec 17, 8:39 AM
Unknown Object (File)
Tue, Dec 17, 8:39 AM
Unknown Object (File)
Tue, Dec 17, 8:39 AM
Unknown Object (File)
Tue, Dec 17, 8:39 AM
Unknown Object (File)
Tue, Dec 17, 8:38 AM
Unknown Object (File)
Sat, Dec 14, 10:46 PM

Details

Summary

Code to create entire db schema on a fresh install (creates both tables and indexes).

Task ENG-1592

Code is the same which is generated by an existing database created by the previous method.

Added newlines to divide query into logical blocks but not sure if it is a good pattern here.

Test Plan

Call this function and compare created schema with schema created previously (tables, columns, types, indexes)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil retitled this revision from [native/sqlite] introduce function to set up database to [native/sqlite] introduce function to create database.Oct 17 2022, 6:05 AM
kamil edited the summary of this revision. (Show Details)
kamil edited the summary of this revision. (Show Details)
kamil published this revision for review.Oct 17 2022, 6:42 AM
kamil edited the summary of this revision. (Show Details)
atul requested changes to this revision.Oct 20 2022, 8:24 AM

I tried the following on my local machine

  1. Created a SQLite3 DB file
  2. Ran the query

and got the following:

Query 1 ERROR: no such table: main.messages

After running it again, I got the following:

Query 1 OK

I tried this twice to make sure the issue was reproducible. It looks like this query will always fail the first time it's run?

Feel free to re-request review if there's something I'm missing or the results I'm seeing are inaccurate.

This revision now requires changes to proceed.Oct 20 2022, 8:24 AM
kamil requested review of this revision.Oct 21 2022, 1:39 AM
In D5378#160333, @atul wrote:

I tried the following on my local machine

  1. Created a SQLite3 DB file
  2. Ran the query

and got the following:

Query 1 ERROR: no such table: main.messages

After running it again, I got the following:

Query 1 OK

I tried this twice to make sure the issue was reproducible. It looks like this query will always fail the first time it's run?

Feel free to re-request review if there's something I'm missing or the results I'm seeing are inaccurate.

Thanks for testing this @atul!

Unfortunately, I wasn't able to reproduce this, could you share the exact steps of what did you do?

Here is what I did:
Test on the native app:
Same as in test plan

Test using plain sqlite3`

  1. Create a database sqlite3 test-for-atul-1.db
  2. Type in entire query and check if no errors are generated
  3. Check both scheme and indexes if they where generate properly
sqlite> .schema
CREATE TABLE drafts ( key TEXT UNIQUE PRIMARY KEY NOT NULL, text TEXT NOT NULL);
CREATE TABLE messages ( id TEXT UNIQUE PRIMARY KEY NOT NULL, local_id TEXT, thread TEXT NOT NULL, user TEXT NOT NULL, type INTEGER NOT NULL, future_type INTEGER, content TEXT, time INTEGER NOT NULL);
CREATE TABLE olm_persist_account ( id INTEGER UNIQUE PRIMARY KEY NOT NULL, account_data TEXT NOT NULL);
CREATE TABLE olm_persist_sessions ( target_user_id TEXT UNIQUE PRIMARY KEY NOT NULL, session_data TEXT NOT NULL);
CREATE TABLE media ( id TEXT UNIQUE PRIMARY KEY NOT NULL, container TEXT NOT NULL, thread TEXT NOT NULL, uri TEXT NOT NULL, type TEXT NOT NULL, extras TEXT NOT NULL);
CREATE TABLE threads ( id TEXT UNIQUE PRIMARY KEY NOT NULL, type INTEGER NOT NULL, name TEXT, description TEXT, color TEXT NOT NULL, creation_time BIGINT NOT NULL, parent_thread_id TEXT, containing_thread_id TEXT, community TEXT, members TEXT NOT NULL, roles TEXT NOT NULL, current_user TEXT NOT NULL, source_message_id TEXT, replies_count INTEGER NOT NULL);
CREATE TABLE metadata ( name TEXT UNIQUE PRIMARY KEY NOT NULL, data TEXT NOT NULL);
CREATE INDEX media_idx_container ON media (container);
CREATE INDEX messages_idx_thread_time ON messages (thread, time);
sqlite> .indexes
media_idx_container
messages_idx_thread_time
sqlite_autoindex_drafts_1
sqlite_autoindex_media_1
sqlite_autoindex_messages_1
sqlite_autoindex_metadata_1
sqlite_autoindex_olm_persist_account_1
sqlite_autoindex_olm_persist_sessions_1
sqlite_autoindex_threads_1

Test using database manager (TablePlus)

  1. Create a database file sqlite3 test-for-atul-2.db
  2. Check file location
sqlite> .databases
main: /Users/kamil/comm/comm/test-for-atul-2.db r/w
  1. Connect to the database and run the query (It's important to select the entire query while hitting Run query - otherwise table plus behave very weird)
  2. All queries are ran successfully

image.png (1×2 px, 345 KB)

All three cases worked for me so re-requesting a review.

marcin added 1 blocking reviewer(s): tomek.
marcin added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
311 ↗(On Diff #17610)

Perhaps create_schema would be better but up to you.

Ah super sorry, I totally messed up with my testing.

bbd374.png (1×1 px, 628 KB)

It looks like I hit Run current and it only ran the CREATE INDEX query because that's where my cursor was... which obviously failed because the messages table wasn't created.

After hitting Run all, all 9 queries completed successfully without issue.

I guess I've only ever used TablePlus with a single query before, so I just hit "Run current" without thinking.

Thanks for detailing the steps you took to test things both via sqlite3 CLI and TablePlus. Apologies again for blocking this diff for no reason...

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

Agree, we're creating tables and indices so schema is more accurate.

315–317 ↗(On Diff #17610)

We can consider making indentation more consistent with keyserver. Ideally, we could have a tool that does that for us.

This revision is now accepted and ready to land.Oct 25 2022, 4:02 AM

rename function, update identation

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
315–317 ↗(On Diff #17610)

I've just updated it, but after I discovered it, it looks weird - it matches the indentation from keyserver but is really different from other queries in SQLiteQueryExecutor. Do we want to change style only for this one query @tomek?

not: I see this different tab before key, my IDE tricked me, will update this

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
315–317 ↗(On Diff #17610)

I think this style improves the readability so it's a good idea to update the whole code. Other queries should be updated in a separate diff - could you create a task for it?

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
315–317 ↗(On Diff #17610)