Enable foreign key support

sssoleileraaa commented 4 years ago

Description

Right now we do not have foreign key support enabled for db.sqlite so we're not getting foreign key protections to ensure referential integrity, see https://docs.sqlalchemy.org/en/13/dialects/sqlite.html#foreign-key-support.

Steps to Reproduce

sqlite> select * from sources;
1|db9d0ad6-b45c-4293-819f-6f181b87be91|5LL6MCTO4HJRVPRTHOEUHEV7PLLCKYET2OIL3PEB2NI27EADJZENCZCECXGDKDCZRS3IZLYR44EI5KHVCOWI2WYAV65OGPG4776O4XY=|neither carelessness|0|2020-09-16 07:11:19.777508|0|4|
2|bcc07e32-a366-4335-8db2-eda19601cc3d|HAMMETVY2ZO23ACL3YBNZS47ZFYVHNSI7EFQ63LGL52WKSMXMOGYLEYIHQZ7XE6B6XU2EVRC6JDILXKPJPCPOJMONDRMDMSMEMNIZPI=|featured down|0|2020-09-16 07:11:20.087783|0|4|
sqlite> insert into source_stars (source_id, starred) values ('a', 1);
sqlite> insert into source_stars (source_id, starred) values (99999999, 1);
sqlite> select * from source_stars;
1|a|1
2|99999999|1

To enable foreign keys and see them working:

sqlite> PRAGMA foreign_keys = ON;
sqlite> insert into source_stars (source_id, starred) values ('b', 1);
Error: FOREIGN KEY constraint failed
sqlite> insert into source_stars (source_id, starred) values (66666666, 1);
Error: FOREIGN KEY constraint failed

Expected Behavior

For foreign keys to be enforced.

Actual Behavior

Foreign keys are not enforced.

eloquence commented 4 years ago

As part of the 9/17-10/1 sprint, we agreed during sprint planning to have a preliminary discussion and investigation focused on enabling foreign key support, e.g.:

What do we gain by enforcing foreign key constraints?
What changes do we need to make to existing code to do so?
What migrations would have to be applied?
What referential integrity violations could exist in long-running instances?
How would we guard against unexpected side effects of making this change?

We'll set up an initial tech chat about this (probably a small group), and go from there, aiming to limit our commitment for this sprint to about about 4-8 hours total.

eloquence commented 4 years ago

We discussed this a bit more in the context of #5467 in the last sprint, and will likely continue that discussion at the next tech meeting. Keeping the issue on the sprint, but only to capture emerging consensus.

eloquence commented 4 years ago

We'll likely revisit this after we've completed the complex template consolidation effort (in the SecureDrop Workstation project, see https://github.com/freedomofpress/securedrop-workstation/issues/471), and after #5467 has landed, which is one important prerequisite.

cfm commented 2 years ago

Just to close the loop on my own (pedantic) point of confusion during yesterday's discussion of https://github.com/freedomofpress/securedrop/issues/5467#issuecomment-988290412: A SQLite FOREIGNKEY constraint, enforced by PRAGMA foreign_keys = ON, will indeed reject invalid foreign-key values but accept null foreign-key values without an additional NOT NULL constraint:

sqlite> PRAGMA foreign_keys = ON;  -- as above
sqlite> insert into source_stars (source_id, starred) values ('b', 1);  -- as above
Error: FOREIGN KEY constraint failed
sqlite> insert into source_stars (source_id, starred) values (null, 1);
sqlite> SELECT * FROM source_stars;
1||1

So the reasons to avoid null source_ids and (especially, e.g. in #6192) journalist_ids are API- rather than database-level. (UUIDs, required to be both non-null and unique, are another matter. :-)

sssoleileraaa commented 2 years ago

@cfm - it's a good point that I missed when I created this issue. Null is a special case in sqlite and will not cause a foreign key contraint error, e.g.

sqlite> select id from journalists;
1
sqlite> update replies set journalist_id=123 where journalist_id=1;   # journalist 123 doesn't exist
Error: FOREIGN KEY constraint failed
sqlite> update replies set journalist_id=null where journalist_id=1;   # journalist <null> doesn't exist but this will work
sqlite>

One might assume both update statements would fail due to the table constraint:FOREIGN KEY(journalist_id) REFERENCES journalists (id), but, yeah, nulls are special. I'll update my comment to avoid confusion. But we still want to enable foreign key constraints, so this is a db-level issue which I will keep open and push for us to do.

Update: I elaborate on the reason the current NULL journalist ID in the db paired with the "deleted" journalist UUID in the API is problematic here: https://github.com/freedomofpress/securedrop/issues/6192#issuecomment-1005202058

zenmonkeykstop commented 2 years ago

Moving this out of the 2.2.0 milestone (and moving #6192 in) - at this stage it's too ambitious to get it into the next release.

freedomofpress / securedrop