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

Fix alembic #57

Closed
wants to merge 12 commits into from
Closed

Fix alembic #57

wants to merge 12 commits into from

Conversation

xerona
Copy link

@xerona xerona commented Aug 23, 2021

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

@xerona
Copy link
Author

xerona commented Aug 23, 2021

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
Copy link
Owner

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
Copy link
Author

xerona commented Aug 24, 2021

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.

Cameron Hurst and others added 8 commits August 24, 2021 07:54
Reworked location of some function calls to support multple purposes
Added migration calls to be manually added into alembic migrations
Session related features are not under SessionManager
There is a BaseVersioningManager for use with Alembic
VersioningManager and FlaskVersioningManager remain api compatible
@xerona
Copy link
Author

xerona commented Aug 24, 2021

This is all set again.

@xerona
Copy link
Author

xerona commented Aug 30, 2021

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

@kvesteri
Copy link
Owner

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:

  • Avoid lambda functions and use list comprehensions instead. Related to this avoid one letter variable names.
  • Avoid abbreviations such as sch and use full words instead (in this case schema)
  • For queries use multiline strings instead of concatenating lots of strings line by line

@xerona
Copy link
Author

xerona commented Sep 1, 2021

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.

@xerona xerona closed this Sep 13, 2021
@wrabit
Copy link

wrabit commented Aug 15, 2023

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?

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

Successfully merging this pull request may close these issues.

3 participants