Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for alembic migrations #47

Open
wakemaster39 opened this issue Dec 18, 2019 · 7 comments
Open

Add support for alembic migrations #47

wakemaster39 opened this issue Dec 18, 2019 · 7 comments

Comments

@wakemaster39
Copy link

Scrolling through the issue as I was setting this up, I found the following that appear to be relevant. #43 #7 #20 and #39 .

Basically it comes down to the way that the triggers are added into the DB is via the after_create signal in SQLAlchemy. This unfortunately only triggers the first time a table is created and it only triggers when you are using the table metadata.

Working with Alembic, Flask-Migrate or other wrappers around alembic this means that the postgres triggers are never properly added and as such nothing ever gets added to the activity table.

#46 is my first pass at attempting to resolve this problem. Its not perfect but it is a start.

Basically at this point all the SQL calls back been removed to external functions from the base VersionManager. In addition, new functions have been added to the migrations file.

manually adding init_before_create_transaction, init_activity_table_triggers, rollback_create_transaction and register_table allow most things to function.

Basically the first migration will look something like this:

def upgrade():
    # ### commands auto generated by Alembic - please adjust! ###
    op.execute(text("CREATE EXTENSION btree_gist;"))
    init_before_create_transaction(op)

    op.create_table('ipam_vrfs',
    sa.Column('DateCreated', sa.DateTime(), nullable=True),
    sa.Column('LastModified', sa.DateTime(), nullable=True),
    sa.Column('id', sa.Integer(), nullable=False),
    sa.Column('name', sa.String(), nullable=True),
    sa.PrimaryKeyConstraint('id')
    )
    op.create_table('transaction',
    sa.Column('id', sa.BigInteger(), nullable=False),
    sa.Column('native_transaction_id', sa.BigInteger(), nullable=True),
    sa.Column('issued_at', sa.DateTime(), nullable=True),
    sa.Column('client_addr', postgresql.INET(), nullable=True),
    sa.Column('actor_id', sa.Text(), nullable=True),
    postgresql.ExcludeConstraint((sa.column('native_transaction_id'), '='), (text("tsrange(issued_at - INTERVAL '1 hour', issued_at)"), '&&'), using='gist', name='transaction_unique_native_tx_id'),
    sa.PrimaryKeyConstraint('id')
    )
    op.create_table('activity',
    sa.Column('id', sa.BigInteger(), nullable=False),
    sa.Column('schema_name', sa.Text(), nullable=True),
    sa.Column('table_name', sa.Text(), nullable=True),
    sa.Column('relid', sa.Integer(), nullable=True),
    sa.Column('issued_at', sa.DateTime(), nullable=True),
    sa.Column('native_transaction_id', sa.BigInteger(), nullable=True),
    sa.Column('verb', sa.Text(), nullable=True),
    sa.Column('old_data', postgresql.JSONB(astext_type=sa.Text()), server_default='{}', nullable=True),
    sa.Column('changed_data', postgresql.JSONB(astext_type=sa.Text()), server_default='{}', nullable=True),
    sa.Column('transaction_id', sa.BigInteger(), nullable=True),
    sa.ForeignKeyConstraint(['transaction_id'], ['transaction.id'], ),
    sa.PrimaryKeyConstraint('id')
    )
    op.create_index(op.f('ix_activity_native_transaction_id'), 'activity', ['native_transaction_id'], unique=False)

    init_activity_table_triggers(op.get_bind())
    register_table(op, "ipam_vrfs", ["DateCreated", "LastModified"])

    # ### end Alembic commands ###


def downgrade():
    # ### commands auto generated by Alembic - please adjust! ###
    rollback_create_transaction(op)
    op.drop_index(op.f('ix_activity_native_transaction_id'), table_name='activity')
    op.drop_table('activity')
    op.drop_table('transaction')
    op.drop_table('ipam_vrfs')
    # ### end Alembic commands ###

All future migrations will need lines like register_table(op, "ipam_vrfs", ["DateCreated", "LastModified"]) added for all tables that have had either their `excluded_columns modified or a new table for revision tracking is added.

Excluded columns are explicitly added and not read from the models to allow full history tracking of what was ignored.

@wakemaster39
Copy link
Author

PR has been updated to include support for alembic autogenerated migrations. To support this, __versioned__ needed to be moved into the __table_args__['info'] dict under the versioned key. This appeared to be an acceptable move by comments in #7.

To enable alembic support in your env.py file you need to add a couple lines.

import postgresql_audit.alembic

postgresql_audit.alembic.writer.process_revision_directives needs to be passed in to the context.configure call under the process_revision_directives keyword arg.

If you are using flask-migrate, process_revision_directives function can be slightly modified to

    def process_revision_directives(context, revision, directives):
        if getattr(config.cmd_opts, "autogenerate", False):
            script = directives[0]
            if script.upgrade_ops.is_empty():
                directives[:] = []
                logger.info("No changes in schema detected.")
                return
        postgresql_audit.alembic.writer.process_revision_directives(context, revision, directives)

@vvvrrooomm
Copy link

op.execute(text("CREATE EXTENSION btree_gist;"))

this needs superuser privileges. [1] suggests to use the following instead:
op.execute(text("CREATE EXTENSION IF NOT EXISTS btree_gist;"))

[1]https://stackoverflow.com/a/51395480/10617111

@vvvrrooomm
Copy link

vvvrrooomm commented Jun 21, 2020

@wakemaster39 could you please show a complete example, I'm interested to use you PR but in my test the trigger are never added

@jpollard-cs
Copy link

@wakemaster39 @kvesteri what's currently blocking the related PR from getting merged? What can I do to help get this over the line? @wakemaster39 is there any way to exclude your refactoring in postgresql_audit/base.py? It makes it difficult to find the relevant changes.

@jpollard-cs
Copy link

Oh I think I see - that refactoring (43e99f4) is needed to enable the changes that followed?

@jpollard-cs
Copy link

So I spent some time on trying to get the tests fixed and was able to get all but 7 working (at least in python 3.7 with postgresql 12). Notable changes were:

  • usages of VersioningManager() had to be modified to VersioningManager(actor_cls="User", session_manager_factory=FlaskSessionManager) in the test_flask_integration.py test file
  • set_activity_values and is_modified now must be accessed through versioning_manager.session_manager which is a breaking API change so we may want to undo this to preserve backwards compatibility

the two errors I haven't resolved yet are a couple instances of this error

>       assert activity.transaction.actor_id == '1'
E       AttributeError: 'NoneType' object has no attribute 'actor_id'

tests/test_sqlalchemy_integration.py:111: AttributeError

and this one

>       assert session.query(versioning_manager.transaction_cls).count() == 1
E       AssertionError: assert 0 == 1
E        +  where 0 = <bound method Query.count of <sqlalchemy.orm.query.Query object at 0x11197af10>>()
E        +    where <bound method Query.count of <sqlalchemy.orm.query.Query object at 0x11197af10>> = <sqlalchemy.orm.query.Query object at 0x11197af10>.count
E        +      where <sqlalchemy.orm.query.Query object at 0x11197af10> = <bound method Session.query of <sqlalchemy.orm.session.Session object at 0x111968150>>(<class 'postgresql_audit.base.BasicVersioningManager.transaction_model_factory.<locals>.Transaction'>)
E        +        where <bound method Session.query of <sqlalchemy.orm.session.Session object at 0x111968150>> = <sqlalchemy.orm.session.Session object at 0x111968150>.query
E        +        and   <class 'postgresql_audit.base.BasicVersioningManager.transaction_model_factory.<locals>.Transaction'> = <postgresql_audit.base.VersioningManager object at 0x1119b1550>.transaction_cls

tests/test_sqlalchemy_integration.py:219: AssertionError

@xerona xerona mentioned this issue Aug 23, 2021
@adrianschneider94
Copy link

I also wanted to use postgresql-audit with alembic, here is my approach:

https://gist.github.com/adrianschneider94/1557e3c6052c1ecc6401cecb1a79bab4

I utilize alembic-utils' PGFunction for the needed sql-functions and PGTrigger to create the triggers on the column - not the audit_table(...) sql function.
This way, changes in __versioned__ will be reflected and the related triggers will get updated accordingly.

To use this, there have to be two migrations in the beginning:

  1. one that creates the activity and transaction tables
  2. one that creates the sql functions and triggers

To create the first migration:

  • use versioning_manager.init on the Base
  • create an alembic revision

To create the second migration:

  • register the items calculated by versioning_manager.get_alembic_utils_objects() with alembic_utils
  • Autogenerate a revision
  • Add the jsonb diff operator to the migration (see migration.py in the gist).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants