securedrop
securedrop copied to clipboard
Enable foreign key support
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.
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.
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.
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.
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_id
s and (especially, e.g. in #6192) journalist_id
s are API- rather than database-level. (UUIDs, required to be both non-null and unique, are another matter. :-)
@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
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.