Page MenuHomePhabricator

[CommCoreModule] Remove `last[NavigatedTo/Pruned]` from `MessageStoreThread` and `message_store_threads` table
ClosedPublic

Authored by atul on Oct 17 2023, 1:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 21, 3:21 AM
Unknown Object (File)
Tue, Nov 19, 11:46 PM
Unknown Object (File)
Fri, Nov 1, 9:56 AM
Unknown Object (File)
Oct 28 2024, 8:17 PM
Unknown Object (File)
Oct 25 2024, 1:14 AM
Unknown Object (File)
Oct 2 2024, 1:05 PM
Unknown Object (File)
Oct 2 2024, 1:05 PM
Unknown Object (File)
Oct 2 2024, 6:01 AM
Subscribers

Details

Summary
IMPORTANT: While I tested this migration a few times and read through things as carefully as I could, making changes to C++ codebase and SQLite store can be risky. Adding @ashoat and @kamil as blocking to take a look... would appreciate careful review to ensure everything is correct.

In this diff we:

  1. Remove lastPruned/lastNavigatedTo from MessageStoreThread struct and remove usages on C++ side.
  2. Introduce SQLite migration that drops lastPruned and lastNavigatedTo columns.

As of D9475, we've started ignoring what's persisted here in SQLite entirely... so this is effectively just dead code that we're cleaning up.


Depends on

D9475

Test Plan
  1. C++ codebase continues to build/run without issue.
  2. recreate_message_store_threads_table migration succeeded and new message_store_threads table is as expected:

c45df9.png (402×1 px, 81 KB)

Before:

7f7378.png (1×1 px, 276 KB)

After:

86e4f2.png (1×1 px, 262 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 17 2023, 1:59 PM
Harbormaster failed remote builds in B23311: Diff 32106!
atul requested review of this revision.Oct 17 2023, 1:59 PM
atul added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
399 ↗(On Diff #32106)

IMO The migrations at the top of SQLiteQueryExecutor are super long and take up a lot of the file. Would be good to move these to a separate migrations file.. or even a migrations folder where each migration gets its own file.

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

include updated comm_query_executor.wasm

thanks @kamil for adding Emscripten job to CI that caught this

I don't really know c++ so I won't be much use here, but feel free to re-add me if you think otherwise at some point

This looks right to me

This revision is now accepted and ready to land.Oct 19 2023, 2:38 AM