Details

Reviewers

ashoat
ginsu
kamil

Commits

rCOMMe614d0e05c6a: [native] Add `avatar` column to clientDB `threads` table

Summary

We are adding an avatar column to the threads table in the clientDB and including an avatar field in the Thread struct. At the moment, the value of this field will always be NULL until we make some necessary changes. These changes involve modifying:

The translation functions that convert between ClientDBThreadInfo and ThreadInfo.
The C++ code responsible for constructing JSI ClientDBThreadInfo objects from the threads table rows retrieved from the database on the "JS side."

Once these changes have been made, we will be able to update the avatar field in the threads table and properly persist data in the new avatar column.

Test Plan

Verify that the migration was successful:

Before:

After:

Ensure that freshly created clientDB includes avatar column in the threads table:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Mar 30 2023, 12:30 PM

Herald added a subscriber: tomek. · View Herald TranscriptMar 30 2023, 12:30 PM

atul published this revision for review.Mar 30 2023, 12:31 PM

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

Harbormaster completed remote builds in B17842: Diff 24438.Mar 30 2023, 12:46 PM

update

atul edited the test plan for this revision. (Show Details)Mar 30 2023, 1:13 PM

update again

Harbormaster completed remote builds in B17846: Diff 24442.Mar 30 2023, 1:29 PM

Harbormaster completed remote builds in B17847: Diff 24443.Mar 30 2023, 1:38 PM

ashoat added a reviewer: kamil.Mar 30 2023, 4:45 PM

ashoat added inline comments.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
382 ↗	(On Diff #24443)	Please line it up with the others

Seems right

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

atul added inline comments.Mar 30 2023, 6:59 PM

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
382 ↗	(On Diff #24443)	weird, it looked right in my editor

atul added inline comments.Mar 30 2023, 7:00 PM

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
382 ↗	(On Diff #24443)	editor being Xcode will update nonetheless

atul attached a referenced file: F456315: 45bb10.png. (Show Details)Mar 30 2023, 7:00 PM

formatting?

fix formatting (I guess it was a tab and space instead of two spaces? thought we stuck to spaces generally but made it consistent)

Harbormaster completed remote builds in B17852: Diff 24448.Mar 30 2023, 7:19 PM

Harbormaster completed remote builds in B17853: Diff 24449.Mar 30 2023, 7:30 PM

Requesting changes to push it back to you, but if I am missing something re-request review and I will unblock this as soon as possible.

native/cpp/CommonCpp/DatabaseManagers/entities/Thread.h
23 ↗	(On Diff #24449)	I think this is unsafe and might crash or cause undefined behavior. If I understand this correctly, and looking at the current pattern here you should use `std::string` and add a `NOT NULL` constraint to the table definition or in other cases use a pointer that is safer for assigning nullable values. When `sqlite_orm` creates `storage` the docs say "not_null is deduced from type automatically", and the non-pointer value will be deduced as `NOT NULL`.

This revision now requires changes to proceed.Mar 30 2023, 11:38 PM

ashoat added inline comments.Mar 31 2023, 5:38 AM

native/cpp/CommonCpp/DatabaseManagers/entities/Thread.h
23 ↗	(On Diff #24449)	Good catch!

atul added inline comments.Mar 31 2023, 1:17 PM

native/cpp/CommonCpp/DatabaseManagers/entities/Thread.h
23 ↗	(On Diff #24449)	Thanks for pointing that out! Totally forgot about this subtlety so 1000x appreciate that you flagged this before I broke something. Updating to: std::unique_ptr<std::string> avatar; Which seems like what we're doing with `messages` table: