kvesteri / postgresql-audit

Audit trigger for PostgreSQL
BSD 2-Clause "Simplified" License
126 stars 28 forks source link

Add support for alembic migrations #47

Open wakemaster39 opened 4 years ago

wakemaster39 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

Oh I think I see - that refactoring (https://github.com/kvesteri/postgresql-audit/pull/46/commits/43e99f45659f726633f8550319f06561e491234a) is needed to enable the changes that followed?

jpollard-cs commented 4 years ago

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:

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
adrianschneider94 commented 4 months ago

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:

To create the second migration: