Page MenuHomePhabricator

[SQLite] add table to persist decrypted messages
ClosedPublic

Authored by kamil on Mon, Apr 22, 4:11 PM.

Details

Summary

Schema:

  • id: this is a field working as autoincrement, in queries we'll pass it as NULL, and SQLite engine will automatically assign it to track messages order (docs)
  • message_id: message ID needed to later confirm to the other peer that the message was processed/backed up
  • sender_device_id: messages sender
  • plaintext: decrypted messages
  • status: messages status to distinguish in what phase this message is, it will be something like received | processed | backup-done
Test Plan
  1. Migration
  2. Creating DB from scratch

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Tue, Apr 23, 6:39 AM
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
638 ↗(On Diff #39373)

We could use CHECK to enforce this field to behave as an ENUM at the SQL level.
https://www.sqlite.org/lang_createtable.html#ckconst
https://stackoverflow.com/questions/5299267/how-to-create-enum-type-in-sqlite

Alternatively I am wondering if having two boolean columns: processed and backed_up wouldn't be better. It would be more idiomatic to to check in code - w wouldn't have to compare against some hardcoded strings.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
638 ↗(On Diff #39373)

Alternatively I am wondering if having two boolean columns: processed and backed_up wouldn't be better. It would be more idiomatic to to check in code - w wouldn't have to compare against some hardcoded strings.

That is a bad idea - we would risk some data anomalies such as processed as false and backed_up as true. Single columns is better. However make sure that it is not returned to C++ code as string but at some point it is converted to enum or constant defined in a class.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
638 ↗(On Diff #39373)

We could use CHECK to enforce this field to behave as an ENUM at the SQL level.

I feel like leaving this as TEXT gives us more flexibility. This is something we might want to adjust in the future to our needs and then it'll require migration/updating DB schema.
I think forcing ENUM on a different level, in C++ type definition or JS should be enough, I'll add a diff with the definition of statuses on both C++ and JS when needed.

This revision is now accepted and ready to land.Fri, Apr 26, 7:13 AM