Add alembic tests

[heartsucker]

Fixes #203
Fixes #55
Fixes #170
Fixes #207

Requires #240 to be merged first otherwise this requires modification to get the migration up to date again. (edit: not true, these could be merged in any order)

Bug (fixed, see comment below)

This Works

make test TESTS=tests/test_alembic.py

This Doesn't

make test

____________________________________________________________ test_alembic_head_matches_db_models _____________________________________________________________

tmpdir = local('/tmp/pytest-of-heartsucker/pytest-3/test_alembic_head_matches_db_m0')

    def test_alembic_head_matches_db_models(tmpdir):
        '''
        This test is to make sure that our database models in `db.py` are always in sync with the schema
        generated by `alembic upgrade head`.
        '''
        models_homedir = str(tmpdir.mkdir('models'))
        subprocess.check_call(['sqlite3', os.path.join(models_homedir, 'svs.sqlite'), '.databases'])
        engine = make_engine(models_homedir)
        Base.metadata.create_all(bind=engine, checkfirst=False)
        session = sessionmaker(engine)()
        models_schema = get_schema(session)
    
        alembic_homedir = str(tmpdir.mkdir('alembic'))
        subprocess.check_call(['sqlite3', os.path.join(alembic_homedir, 'svs.sqlite'), '.databases'])
        session = sessionmaker(make_engine(alembic_homedir))()
        alembic_config = conftest._alembic_config(alembic_homedir)
        upgrade(alembic_config, 'head')
        alembic_schema = get_schema(session)
    
        # The initial migration creates the table 'alembic_version', but this is
        # not present in the schema created by `Base.metadata.create_all()`.
        alembic_schema = {k: v for k, v in alembic_schema.items()
                          if k[2] != 'alembic_version'}
    
>       assert_schemas_equal(alembic_schema, models_schema)

tests/test_alembic.py:118: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

left = {('index', 'sqlite_autoindex_replies_1', 'replies'): None, ('index', 'sqlite_autoindex_sources_1', 'sources'): None, (...ex', 'sqlite_autoindex_submissions_1', 'submissions'): None, ('index', 'sqlite_autoindex_users_1', 'users'): None, ...}
right = {('index', 'sqlite_autoindex_replies_1', 'replies'): None, ('index', 'sqlite_autoindex_sources_1', 'sources'): None, (...ex', 'sqlite_autoindex_submissions_1', 'submissions'): None, ('index', 'sqlite_autoindex_users_1', 'users'): None, ...}

    def assert_schemas_equal(left, right):
        for (k, v) in left.items():
            if k not in right:
                raise AssertionError('Left contained {} but right did not'.format(k))
            if not ddl_equal(v, right[k]):
                raise AssertionError(
                    'Schema for {} did not match:\nLeft:\n{}\nRight:\n{}'
>                   .format(k, v, right[k]))
E               AssertionError: Schema for ('table', 'sources', 'sources') did not match:
E               Left:
E               CREATE TABLE sources (
E               	id INTEGER NOT NULL, 
E               	uuid VARCHAR(36) NOT NULL, 
E               	journalist_designation VARCHAR(255) NOT NULL, 
E               	document_count INTEGER DEFAULT '0' NOT NULL, 
E               	is_flagged BOOLEAN DEFAULT 'false', 
E               	public_key TEXT, 
E               	fingerprint VARCHAR(64), 
E               	interaction_count INTEGER DEFAULT '0' NOT NULL, 
E               	is_starred BOOLEAN DEFAULT 'false', 
E               	last_updated DATETIME, 
E               	CONSTRAINT pk_sources PRIMARY KEY (id), 
E               	CONSTRAINT uq_sources_uuid UNIQUE (uuid), 
E               	CONSTRAINT ck_sources_is_flagged CHECK (is_flagged IN (0, 1)), 
E               	CONSTRAINT ck_sources_is_starred CHECK (is_starred IN (0, 1))
E               )
E               Right:
E               CREATE TABLE sources (
E               	id INTEGER NOT NULL, 
E               	uuid VARCHAR(36) NOT NULL, 
E               	journalist_designation VARCHAR(255) NOT NULL, 
E               	document_count INTEGER DEFAULT '0' NOT NULL, 
E               	is_flagged BOOLEAN DEFAULT 'false', 
E               	public_key TEXT, 
E               	fingerprint VARCHAR(64), 
E               	interaction_count INTEGER DEFAULT '0' NOT NULL, 
E               	is_starred BOOLEAN DEFAULT 'false', 
E               	last_updated DATETIME, 
E               	CONSTRAINT pk_sources PRIMARY KEY (id), 
E               	CONSTRAINT uq_sources_uuid UNIQUE (uuid), 
E               	CHECK (is_flagged IN (0, 1)), 
E               	CHECK (is_starred IN (0, 1))
E               )

tests/test_alembic.py:60: AssertionError

Comments

The error seems that something in the test suite is interfering with how sqlalchemy generates the create_all() DDL, and I cannot for the life of me figure it out.

[heartsucker]

Ok, so I couldn't find an elegant solution to the above problem, but it has something to do with calling this:

from securedrop_client.db import Base
engine = somehow_make_engine_or_whatever()
Base.metadata.create_all(bind=engine)

If this gets called before the alembic tests anywhere in the code, the generated SQL ends up being the above in the error, and the test fails. I can't figure out how or why this is happening. The solution was to split the alembic tests into a separate test fun using pytest's CLI options.

[redshiftzero]

thanks @heartsucker, i'm pretty cool with this being merged (just a nit inline), but i do want to understand the one question I have inline re: alembic check constraints

[redshiftzero]

so with regard to the issue you describe in this PR, I took a look at this today and I don't understand the following: why do check constraints appear at all in the generated alembic schema? alembic claims that they can't autogenerate check constraints (they've got an open issue for it: sqlalchemy/alembic#508). do you understand why that is happening?

[heartsucker]

I remember looking in to this some time in the past. If I remember right, the check constraints for SQLite booleans (INT CHECK foo IN (0, 1) type things) do work because they kinda have to otherwise the booleans just don't work at all. They are a special case that alembic does detect, but in general, check constraints that are explicitly defined are not detected.

[sssoleileraaa]

Hey @heartsucker - Your PR looks good and the docs update is great 🎉 🎉 - since @redshiftzero already gave this a thorough review, I'm mostly interested in investigating the test_alembic_head_matches_db_models test failure.

I was able to reproduce by removing the -k-test_alembic.py workaround and running make test:

E               AssertionError: Schema for ('table', 'replies', 'replies') did not match:                                                                             
E               Left:                                                                                                                                                 
E               CREATE TABLE replies (                                                                                                                                
E                       id INTEGER NOT NULL,                                                                                                                          
E                       uuid VARCHAR(36) NOT NULL,                                                                                                                    
E                       source_id INTEGER,                                                                                                                            
E                       journalist_id INTEGER,                                                                                                                        
E                       filename VARCHAR(255) NOT NULL,                                                                                                               
E                       size INTEGER,                                                                                                                                 
E                       is_downloaded BOOLEAN,                                                                                                                        
E                       CONSTRAINT pk_replies PRIMARY KEY (id),                                                                                                       
E                       CONSTRAINT fk_replies_journalist_id_users FOREIGN KEY(journalist_id) REFERENCES users (id),                                                   
E                       CONSTRAINT fk_replies_source_id_sources FOREIGN KEY(source_id) REFERENCES sources (id),                                                       
E                       CONSTRAINT uq_replies_uuid UNIQUE (uuid),                                                                                                     
E                       CONSTRAINT ck_replies_is_downloaded CHECK (is_downloaded IN (0, 1))                                                                           
E               )                                                                                                                                                     
E               Right:                                                                                                                                                
E               CREATE TABLE replies (                                                                                                                                
E                       id INTEGER NOT NULL,                                                                                                                          
E                       uuid VARCHAR(36) NOT NULL,                                                                                                                    
E                       source_id INTEGER,                                                                                                                            
E                       journalist_id INTEGER,                                                                                                                        
E                       filename VARCHAR(255) NOT NULL,                                                                                                               
E                       size INTEGER,                                                                                                                                 
E                       is_downloaded BOOLEAN,                                                                                                                        
E                       CONSTRAINT pk_replies PRIMARY KEY (id),                                                                                                       
E                       CONSTRAINT uq_replies_uuid UNIQUE (uuid), 
E                       CONSTRAINT fk_replies_source_id_sources FOREIGN KEY(source_id) REFERENCES sources (id), 
E                       CONSTRAINT fk_replies_journalist_id_users FOREIGN KEY(journalist_id) REFERENCES users (id), 
E                       CHECK (is_downloaded IN (0, 1))
E               )

tests/test_alembic.py:60: AssertionError                                                                                                                              

Alembic CHECK contraints

I briefly looked into alembic CHECK constraints wondering if it wasn't fully implemented feature. I read that SQLAlchemy doesn’t "reflect CHECK constraints on any backend." And that SQLite CHECK constraints require manual migration. Before going down that rabbit hole, I looked to see if anything else looked 🐟 y

Running tests in random order

make test (without -k-test_alembic.py) passes when you remove pytest-random-order=global. pytest-random-order=global is there to randomize the order in which tests are run in order to prevent tests from passing just because they run after other tests. The test that is having an issue is test_alembic_head_matches_db_models and the problem goes away when you run this test first, which explains why this PR's workaround works.

To double check the test that is now passing, I manually compared the table schemas and saw that they matched:

> sqlite3 /tmp/pytest-of-creviera/pytest-67/test_alembic_head_matches_db_m0/models/svs.sqlite

sqlite> .schema replies
CREATE TABLE replies (
        id INTEGER NOT NULL,
        uuid VARCHAR(36) NOT NULL,
        source_id INTEGER,
        journalist_id INTEGER,
        filename VARCHAR(255) NOT NULL,
        size INTEGER,
        is_downloaded BOOLEAN,
        CONSTRAINT pk_replies PRIMARY KEY (id),
        CONSTRAINT uq_replies_uuid UNIQUE (uuid),
        CONSTRAINT fk_replies_source_id_sources FOREIGN KEY(source_id) REFERENCES sources (id),
        CONSTRAINT fk_replies_journalist_id_users FOREIGN KEY(journalist_id) REFERENCES users (id),
        CONSTRAINT ck_replies_is_downloaded CHECK (is_downloaded IN (0, 1))
);

> sqlite3 /tmp/pytest-of-creviera/pytest-67/test_alembic_head_matches_db_m0/alembic/svs.sqlite          
sqlite> .schema replies                                                                                                                                               
CREATE TABLE replies (                                                                                                                                                
        id INTEGER NOT NULL,                                                                                                                                          
        uuid VARCHAR(36) NOT NULL,
        source_id INTEGER,
        journalist_id INTEGER,
        filename VARCHAR(255) NOT NULL,
        size INTEGER,
        is_downloaded BOOLEAN,
        CONSTRAINT pk_replies PRIMARY KEY (id),
        CONSTRAINT fk_replies_journalist_id_users FOREIGN KEY(journalist_id) REFERENCES users (id),
        CONSTRAINT fk_replies_source_id_sources FOREIGN KEY(source_id) REFERENCES sources (id),
        CONSTRAINT uq_replies_uuid UNIQUE (uuid),
        CONSTRAINT ck_replies_is_downloaded CHECK (is_downloaded IN (0, 1))
);

Thoughts

Interesting how the CHECK constraint has been moved over after the change above. It makes it seem that this is indeed something alembic is capable of doing and that tests are just messing with the state somehow. Still strange behavior though. Why would only one constraint not be copied over?

It'll take some time to track down which tests are stepping on each other's toes or if there's something wrong with the way pytest is setup. Probably best way to start is by studying the configuration changes made in this PR and look for any shared code like where db files are saved, etc. It would also be helpful to look into how tests are run when they're in separate files.

[heartsucker]

I read that SQLAlchemy doesn’t "reflect CHECK constraints on any backend."

This doesn't really affect us because these schemas are read back out from the SQLite DB. Also the boolean columns are a special case as an extra check constraint is generated by Alembic since SQLite doesn't have a boolean type, just int.

[redshiftzero]

so I added two commits here which revert the separate run of the alembic tests (and CI is passing, but let me know if you disagree with any of the below and we can always drop my commits). here was my investigation process on this:

1. from the test failure we can see that check constraints are being generated by alembic, they are named: CONSTRAINT ck_sources_is_flagged
2. next looking at the schema generated with create_all() we can see that the naming conventions are getting picked up from uq_sources_uuid and pk_sources, but for some reason not for check constraints
3. my thought next was perhaps there is something wrong with named check constraints and sqlalchemy, digging around in their bugtracker revealed this issue: Naming conventions example should use columns_0_name sqlalchemy/sqlalchemy#3345
4. looking at the check constraint names by alembic, it looks like they are not using the convention ck_%(table_name)s_%(constraint_name)s and instead are using ck_%(table_name)s_%(column_0_name)s, so I modified our naming convention to match as suggested in the above sqlalchemy issue, which resolves the test failure

[redshiftzero]

anyway i'm approving this but will let y'all @heartsucker and @creviera comment and merge when you are happy