kvesteri / postgresql-audit

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

Fix alembic #57

Closed xerona closed 2 years ago

xerona commented 3 years ago

This is just https://github.com/wakemaster39/postgresql-audit rebased with the tests fixed and linter appeased. Hopefully it helps https://github.com/kvesteri/postgresql-audit/issues/47 along.

xerona commented 3 years ago

I used black to address a lot of the lint issues. I just noticed I shamelessly replaced your quotes. I can revisit that laziness if it's annoying.

kvesteri commented 3 years ago

Yeah please don't change single quotes to double quotes. Also please use single quotes wherever possible. Only use double quotes if the string itself contains a single quote for example "O'Malleys"

xerona commented 3 years ago

Yeah please don't change single quotes to double quotes. Also please use single quotes wherever possible. Only use double quotes if the string itself contains a single quote for example "O'Malleys"

I'll clean that up soon.

xerona commented 3 years ago

This is all set again.

xerona commented 3 years ago

@kvesteri Is there anything else I can do for this pull request?

kvesteri commented 3 years ago

There is lots of strange things in this PR that seem unrelated to just supporting alembic. For example introducing a separate SessionManager seems off. Why do SA transaction objects need to be tracked?

Stylewise the following things should be changed:

xerona commented 3 years ago

Credit for all of this work belongs to @wakemaster39. I only hoped to get the tests passing. Implementation for me looks something like this

In alembic/env.py I set process_revision_directives.

from postgresql_audit.alembic import writer

context.configure(
    url=url,
    include_object=include_object,
    target_metadata=target_metadata,
    process_revision_directives=writer.process_revision_directives
)

In versioned models, I updated __table_args__

class Account(db.Model):
    __versioned__ = {}
    __table_args__ = {"info": {"versioned": __versioned__}}

For the first alembic migration I generated, I still had to add a few things.

def upgrade():
    op.execute(sa.text("CREATE EXTENSION IF NOT EXISTS btree_gist;"))

    op.create_table(
        "transaction",
        sa.Column("id", sa.BigInteger(), nullable=False),
        sa.Column("issued_at", sa.DateTime(), nullable=True),
        sa.Column("native_transaction_id", sa.BigInteger(), nullable=True),
        sa.Column("client_addr", postgresql.INET(), nullable=True),
        sa.Column("actor_id", sa.Integer(), nullable=True),
        sa.PrimaryKeyConstraint("id", name=op.f("pk_transaction")),
        # Only the following line needed added to what was generated
        postgresql.ExcludeConstraint((sa.column('native_transaction_id'), '='), (sa.text("tsrange(issued_at - INTERVAL '1 hour', issued_at)"), '&&'), using='gist', name='transaction_unique_native_tx_id'),
    )

Finally, I could not find a pleasing way to format this string. Using triple quotes to form a multiline string truncates trailing whitespace and the generated sql fails.

@kvesteri, if there is anything else you'd like done here, I'll address it as soon as I have time.

wrabit commented 1 year ago

Are there any plans to re-visit this feature? Or at least some insight on how to get this integrated without a drop and create_all?