Page MenuHomePhabricator

[native/sqlite] add `NOT NULL` constraint to `metadata` table
ClosedPublic

Authored by kamil on Sep 15 2022, 6:46 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 11:40 PM
Unknown Object (File)
Sat, Nov 2, 11:40 PM
Unknown Object (File)
Sat, Nov 2, 11:40 PM
Unknown Object (File)
Sat, Nov 2, 11:39 PM
Unknown Object (File)
Sat, Nov 2, 11:38 PM
Unknown Object (File)
Oct 23 2024, 9:46 AM
Unknown Object (File)
Oct 9 2024, 3:20 AM
Unknown Object (File)
Oct 8 2024, 8:19 AM

Details

Summary

Motivation

  1. Purpose is to enhance DB schema and add NOT NULL everywhere where NULL is not expected since SQLite by default accepts null values. task
  2. Instead of running all migrations to create database even on a fresh install (task) there is possibility to infer model from sqlite_orm, while orm itself deduce where types are not nullish there will be inconsistency (orm will generate NOT NULL clause in places where it should be).

Implementation
There is no ALTER COLUMN or any more straightforward method to do it, this solution is suggested by SQLite docs: https://www.sqlite.org/lang_altertable.html#making_other_kinds_of_table_schema_changes.

Why this change

  1. data field is std::string and as a result it can not be assigned to null (it's not the same as empty string), so NULL should not appear in DB.
  2. In CommCoreModule while adding something do metadata table we use utf() method which should return c++ string.
Test Plan
  1. Use decoded SQLite (e.g. comment encryption code and re-install app)
  2. Connect to database (path should be logged) by sqlite3 and check data in metadata table.
  3. Run this code and restart app to run migration
  4. Connect to database and run .schema metadata to check if the constraint is added and data is unchanged.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil edited the test plan for this revision. (Show Details)
kamil added reviewers: atul, karol.
kamil published this revision for review.Sep 15 2022, 8:45 AM
tomek requested changes to this revision.Sep 16 2022, 10:37 AM
tomek added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
298 ↗(On Diff #16695)

The same as in D5144, should we add a where condition?

This revision now requires changes to proceed.Sep 16 2022, 10:37 AM
tomek added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
300 ↗(On Diff #16815)

Can we also add a check for name?

This revision is now accepted and ready to land.Sep 19 2022, 6:22 AM
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
300 ↗(On Diff #16815)

Sorry I didn't comment on why I omitted name.
There was a NOT NULL constraint for it: https://github.com/CommE2E/comm/blob/master/native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp#L260
I've added it only for data so I think there is no chance to get NULL here.