-
Notifications
You must be signed in to change notification settings - Fork 685
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Make sources.filesystem_id non-nullable
We already set this field all the time and in all places expect it to be a string, not checking if its None first. The database migration will delete any sources that have a NULL filesystem_id, plus anything that references those sources. Discovered by the mypy work I'm doing in #6346. Refs #6226.
- Loading branch information
Showing
3 changed files
with
138 additions
and
1 deletion.
There are no files selected for viewing
41 changes: 41 additions & 0 deletions
41
securedrop/alembic/versions/b7f98cfd6a70_make_filesystem_id_non_nullable.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
"""make_filesystem_id_non_nullable | ||
Revision ID: b7f98cfd6a70 | ||
Revises: d9d36b6f4d1e | ||
Create Date: 2022-03-18 18:10:27.842201 | ||
""" | ||
from alembic import op | ||
import sqlalchemy as sa | ||
|
||
|
||
# revision identifiers, used by Alembic. | ||
revision = 'b7f98cfd6a70' | ||
down_revision = 'd9d36b6f4d1e' | ||
branch_labels = None | ||
depends_on = None | ||
|
||
|
||
def upgrade() -> None: | ||
# Not having a filesystem_id makes the source useless, so if any of those do exist, we'll | ||
# delete them first, as part of this migration. | ||
for table in ("source_stars", "submissions", "replies"): | ||
# FIXME: what about things referencing submissions/replies? | ||
op.execute( | ||
f"DELETE FROM {table} WHERE source_id IN (SELECT id FROM sources WHERE filesystem_id IS NULL)") | ||
op.execute("DELETE FROM sources WHERE filesystem_id IS NULL") | ||
with op.batch_alter_table('sources', schema=None) as batch_op: | ||
batch_op.alter_column( | ||
'filesystem_id', | ||
existing_type=sa.VARCHAR(length=96), | ||
nullable=False | ||
) | ||
|
||
|
||
def downgrade() -> None: | ||
with op.batch_alter_table('sources', schema=None) as batch_op: | ||
batch_op.alter_column( | ||
'filesystem_id', | ||
existing_type=sa.VARCHAR(length=96), | ||
nullable=True | ||
) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
import uuid | ||
|
||
from sqlalchemy import text | ||
|
||
from db import db | ||
from journalist_app import create_app | ||
|
||
|
||
# Random, chosen by fair dice roll | ||
FILESYSTEM_ID = 'FPLIUY3FWFROQ52YHDMXFYKOTIK2FT4GAFN6HTCPPG3TSNAHYOPDDT5C3TR' \ | ||
'JWDV2IL3JDOS4NFAJNEI73KRQ7HQEVNAF35UCCW5M7VI=' | ||
|
||
|
||
class UpgradeTester: | ||
"""Insert a source, verify the filesystem_id makes it through untouched | ||
""" | ||
|
||
def __init__(self, config): | ||
"""This function MUST accept an argument named `config`. | ||
You will likely want to save a reference to the config in your | ||
class, so you can access the database later. | ||
""" | ||
self.config = config | ||
self.app = create_app(config) | ||
|
||
def load_data(self): | ||
"""This function loads data into the database and filesystem. It is | ||
executed before the upgrade. | ||
""" | ||
with self.app.app_context(): | ||
sources = [ | ||
{ | ||
'uuid': str(uuid.uuid4()), | ||
'filesystem_id': FILESYSTEM_ID, | ||
'journalist_designation': 'sunburned arraignment', | ||
'interaction_count': 0, | ||
}, | ||
{ | ||
'uuid': str(uuid.uuid4()), | ||
'filesystem_id': None, | ||
'journalist_designation': 'needy transponder', | ||
'interaction_count': 0, | ||
} | ||
] | ||
sql = """\ | ||
INSERT INTO sources (uuid, filesystem_id, journalist_designation, interaction_count) | ||
VALUES (:uuid, :filesystem_id, :journalist_designation, :interaction_count)""" | ||
for params in sources: | ||
db.engine.execute(text(sql), **params) | ||
# Insert a source_stars row associated each source | ||
for source_id in (1, 2): | ||
db.engine.execute( | ||
text("INSERT INTO source_stars (source_id, starred) VALUES (:source_id, :starred)"), | ||
{"source_id": source_id, "starred": True} | ||
) | ||
|
||
def check_upgrade(self): | ||
"""This function is run after the upgrade and verifies the state | ||
of the database or filesystem. It MUST raise an exception if the | ||
check fails. | ||
""" | ||
with self.app.app_context(): | ||
# Verify remaining single source is the one with a non-NULL filesystem_id | ||
sources = db.engine.execute( | ||
'SELECT filesystem_id FROM sources' | ||
).fetchall() | ||
assert len(sources) == 1 | ||
assert sources[0][0] == FILESYSTEM_ID | ||
# Verify that the source_stars #2 row got deleted | ||
stars = db.engine.execute('SELECT source_id FROM source_stars').fetchall() | ||
assert stars == [(1,)] | ||
|
||
|
||
class DowngradeTester: | ||
"""Downgrading only makes fields nullable again, which is a | ||
non-destructive and safe operation""" | ||
|
||
def __init__(self, config): | ||
"""This function MUST accept an argument named `config`. | ||
You will likely want to save a reference to the config in your | ||
class, so you can access the database later. | ||
""" | ||
self.config = config | ||
|
||
def load_data(self): | ||
"""This function loads data into the database and filesystem. It is | ||
executed before the downgrade. | ||
""" | ||
pass | ||
|
||
def check_downgrade(self): | ||
"""This function is run after the downgrade and verifies the state | ||
of the database or filesystem. It MUST raise an exception if the | ||
check fails. | ||
""" | ||
pass |