Details

Reviewers

atul
tomek
kamil

Commits

rCOMMd20ac9605749: [native] Add pinned_count column to SQLite threads table

Summary

We need to add the pinned_count column to SQLite threads table, so when we (in the next diff) try to store pinnedCount in ThreadInfos, the ClientDB will have the pinned_count.

As mentioned in the test plan:

I'm not entirely sure if we really even need to consider a case where a new client will already have the column besides me in development work, so I can also just remove that check entirely and just add the column. I ran into this inconsistency while working off of my modified schema and running a migration on top of that

Part of https://linear.app/comm/issue/ENG-3396/store-the-number-of-pinned-messages-in-threadinfo

Depends on D7175

Test Plan

Confirm the migration works

git checkout master
Build the app from xcode
Connect to the SQLite database and confirm there is no pinned_count column
git checkout pinned_messages
Rebuild the app from xcode without deleting it
Connect to the SQLite database and confirm the existence of a pinned_count column

Confirm the schema works

git checkout pinned_messages
Delete the app from the simulator
Build the app from xcode
Connect to the SQLite database and confirm the existence of a pinned_count column

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Mar 27 2023, 12:01 PM

Herald added a subscriber: ashoat. · View Herald TranscriptMar 27 2023, 12:01 PM

rohan edited the summary of this revision. (Show Details)Mar 27 2023, 12:07 PM

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

rohan edited the summary of this revision. (Show Details)

rohan attached a referenced file: F448096: Screenshot 2023-03-27 at 12.50.47 PM.png. (Show Details)

rohan attached a referenced file: F448095: Screenshot 2023-03-27 at 2.56.41 PM.png. (Show Details)

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

rohan attached a referenced file: F448100: Screenshot 2023-03-27 at 1.42.00 PM.png. (Show Details)

rohan edited the summary of this revision. (Show Details)Mar 27 2023, 12:16 PM

Harbormaster completed remote builds in B17702: Diff 24230.Mar 27 2023, 12:18 PM

rohan requested review of this revision.Mar 27 2023, 12:18 PM

rohan edited the summary of this revision. (Show Details)Mar 27 2023, 12:21 PM

When you're modifying SQLite database on native you should do two things:

Update create_schema - this code is running on a fresh install/logout user (you did it properly)
Add new migration which will update this table when existing users will update the app to a newer version. We were adding NOT EXISTS everywhere because of multiple crashes at that time, but I think it should work without this constraint (a lot of refactors were done to avoid problems). If you really want to make this code resistant you can use PRAGMA table_info() to check if the column exists and then execute ALTER TABLE.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
201 ↗	(On Diff #24230)	This code is an old migration, which means that this code will be executed only when the user has db version < 20 (see here) and should not be changed

This revision now requires changes to proceed.Mar 27 2023, 12:59 PM

Address feedback and add_pinned_count_column_to_threads after speaking with @kamil offline

rohan edited the test plan for this revision. (Show Details)Mar 27 2023, 2:53 PM

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

rohan edited the summary of this revision. (Show Details)Mar 27 2023, 2:55 PM

rohan edited the summary of this revision. (Show Details)Mar 27 2023, 2:58 PM

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

Harbormaster completed remote builds in B17709: Diff 24240.Mar 27 2023, 3:06 PM

rohan added a child revision: D7211: [keyserver/lib/native] Store the number of pinned messages in ThreadInfo.Mar 27 2023, 5:04 PM

In the test plan for the Confirm migration for new clients already with the column works

I'm not entirely sure if we really even need to consider a case where a new client will already have the column besides me in development work, so I can also just remove that check entirely and just add the column. I ran into this inconsistency while working off of my modified schema and running a migration on top of that

This scenario should never happen, if you have not tested code that is not working properly - we added those checks (IF NOT EXISTS, checking if columns are in the table) because there were some problems in the past and we wanted to avoid crashes as soon as possible.

I think this additional check is okay, like in here but if this code fails on your side without this it means something bad is going on.

Looks like the test flow is not fully correct:

Delete the app

Add the schema changes

Install the app (so the db is constructed from the schema)

And that's how this column is added - it's in create_schema but you don't add migration which means the database version is set to 25. Database version is max of migration indexes.

Add the migration

DB version is 25 so migration 26 executes and fails.

Rebuild the app

Confirm that no error for 'duplicate column' is thrown since we detected the column before adding it

When modifying schema you should always modify and test both create_schema and migrations at once. In this test plan you first modified the schema, then built the app and after this, you added migration.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
311–323 ↗	(On Diff #24240)	I probably mislead you previously, I just used `PRAGMA table_info` in the past and I know it was a solution, I think this is correct but looks like we already do something very similar thing and checking whether the column exists is done differently, see this: link, sorry for that. I think it'll be best if you can follow that pattern I linked.
403 ↗	(On Diff #24240)	You added `DEFAULT 0` in migration but not here - those two must be strictly consistent
848 ↗	(On Diff #24240)	Based on the migration code you have written - you should add `default_value()` to `sqlite_orm`'s `storage`. See this example.

This revision now requires changes to proceed.Mar 28 2023, 3:15 AM

In D7202#214418, @kamil wrote:

In the test plan for the Confirm migration for new clients already with the column works

I'm not entirely sure if we really even need to consider a case where a new client will already have the column besides me in development work, so I can also just remove that check entirely and just add the column. I ran into this inconsistency while working off of my modified schema and running a migration on top of that

This scenario should never happen, if you have not tested code that is not working properly - we added those checks (IF NOT EXISTS, checking if columns are in the table) because there were some problems in the past and we wanted to avoid crashes as soon as possible.

I think this additional check is okay, like in here but if this code fails on your side without this it means something bad is going on.

Looks like the test flow is not fully correct:

Delete the app

Add the schema changes

Install the app (so the db is constructed from the schema)

And that's how this column is added - it's in create_schema but you don't add migration which means the database version is set to 25. Database version is max of migration indexes.

Add the migration

DB version is 25 so migration 26 executes and fails.

Rebuild the app

Confirm that no error for 'duplicate column' is thrown since we detected the column before adding it

When modifying schema you should always modify and test both create_schema and migrations at once. In this test plan you first modified the schema, then built the app and after this, you added migration.

Thanks for pointing out the alternate way for performing the additional check before executing the query, I probably did overcomplicate it a little by iterating through each column and checking it's value.

I agree - the check realistically should not be needed in a case where 1) both the schema and 2) the migration code is added at the same time, since clients will either have one or the other, but in my malformed state I ended up having both and that's what caused the problems. I'll update the diff based on your feedback to change the way the check is performed, but if you feel strongly about not needing the additional check let me know and I can remove it.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
311–323 ↗	(On Diff #24240)	Got it, I'll update to follow that pattern you linked
403 ↗	(On Diff #24240)	I'll add the `DEFAULT 0` to the schema and test it to confirm
848 ↗	(On Diff #24240)	Thanks for the reference! Makes sense, I'll add this

rohan changed 2 blocking reviewer(s), added 1: kamil; removed 1: atul.Mar 28 2023, 5:37 AM

This revision now requires review to proceed.Mar 28 2023, 5:37 AM

rohan planned changes to this revision.Mar 28 2023, 5:38 AM

Add default_value() to sqlite_orm's storage, add default value to schema, and rewrite the check for a pinned_count column to match convention

Fix syntax

rohan added inline comments.Mar 28 2023, 6:24 AM

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
845–846 ↗	(On Diff #24282)	This formatting seems to happen when I commit the changes, moving it from one line down to two. I'm not too familiar with the formatting settings for the c++ code

rohan edited the test plan for this revision. (Show Details)Mar 28 2023, 6:27 AM

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

Harbormaster completed remote builds in B17741: Diff 24281.Mar 28 2023, 6:37 AM

Harbormaster completed remote builds in B17742: Diff 24282.

Looks good!!

One note to test plan:

Confirm the schema works

git checkout pinned_messages

Build the app from xcode

Connect to the SQLite database and confirm the existence of a pinned_count column

This is not a full test, between steps 2 and 3 you should logout user or make sure it's a fresh install - otherwise, you can not be sure that create_schema was called. Could you verify if it works and amend this to the test plan?

Also, this is a fragile part of the codebase, and a small mistake can make a mess and I will feel more comfortable if one of senior reviewers will take a look - pick who you think will suit most and add as blocking.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
845–846 ↗	(On Diff #24282)	It's okay, caused by a clang formatter triggered by commit hooks, I think

This revision is now accepted and ready to land.Mar 28 2023, 8:58 AM

In D7202#214607, @kamil wrote:

Looks good!!

One note to test plan:

Confirm the schema works

git checkout pinned_messages

Build the app from xcode

Connect to the SQLite database and confirm the existence of a pinned_count column

This is not a full test, between steps 2 and 3 you should logout user or make sure it's a fresh install - otherwise, you can not be sure that create_schema was called. Could you verify if it works and amend this to the test plan?

Yeah I had deleted the app and it was the first install, let me fix the test plan to include that so it's clear

This revision now requires review to proceed.Mar 28 2023, 9:03 AM

rohan edited the test plan for this revision. (Show Details)Mar 28 2023, 9:04 AM

Thanks @kamil for taking a thorough look at this diff!

Agree with "this is a fragile part of the codebase, and a small mistake can make a mess," so @rohan please make sure you've tested this thoroughly and are confident in the changes before landing.

This revision is now accepted and ready to land.Mar 28 2023, 4:23 PM

Rebase

Harbormaster completed remote builds in B17884: Diff 24499.Mar 31 2023, 4:45 AM

rohan added inline comments.Apr 5 2023, 5:57 AM