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

assure usability on multi DB development stack #69

Closed
Miguelreis opened this issue Mar 22, 2024 · 7 comments
Closed

assure usability on multi DB development stack #69

Miguelreis opened this issue Mar 22, 2024 · 7 comments

Comments

@Miguelreis
Copy link

Miguelreis commented Mar 22, 2024

Hi, thanks for making your project opensource.

Problem

To my knowledge alembic-postgresql-enum assumes that migrations are being generated by postgres, and independently of the source database, it issues postgres-specific queries to check for existing enums.

This is relevant specially for the common SQLite in development, and postgres in staging/production workflow, where currently this library is not able to fill the same needs as it does currently for a standard development in postgres from local/development to production.

currently, using this library it is not possible to generate simple migrations on a SQLite, that is compatible both for SQLite and Postgres. Its also not possible to turn off the hooks to dispatch_for("schema").

e.g. what happens when trying to generate new migrations having previously imported alembic-postgresql-enum - from alembic_postgresql_enum/compare_dispatch.py. obviously this query is only valid for postgres.

sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) near "(": syntax error
[SQL: 
        SELECT
            pg_catalog.format_type(t.oid, NULL),
            ARRAY(SELECT enumlabel
                  FROM pg_catalog.pg_enum
                  WHERE enumtypid = t.oid
                  ORDER BY enumsortorder)
        FROM pg_catalog.pg_type t
        LEFT JOIN pg_catalog.pg_namespace n ON n.oid = t.typnamespace
        WHERE
            t.typtype = 'e'
            AND n.nspname = ?
    ]

Proposed solution

As I understand it (after a quick look),

@alembic.autogenerate.comparators.dispatch_for("schema")
def compare_enums(..)

is the entrypoint for the other hooks to alembic to check enums and modify enums.

as a simple solution for this library to not be code breaking after import, is to add a check for the dialect, i.e. defined_enums.py.

@alembic.autogenerate.comparators.dispatch_for("schema")
def compare_enums(..):

    if not autogen_context.dialect.name == "postgresql":
        print("Not using PostgreSQL, skipping enum comparison")
        return
    (...)

it would also be relevant that migrations created using this library would be applied only if upgrading a postgres database (my code above doesn't solve this issue).

Final Statements

if this is in the interest of the mantainers, i would make myself available to generate a pull request.

thanks for reading, and my apologies for any mistakes or if not relevant for you.

@RustyGuard
Copy link
Member

This proposal is interesting. However I don't understand how this library will benefit projects that use SqlLite locally. No commands will be generated so what is the point of using it?

@Miguelreis
Copy link
Author

Miguelreis commented Mar 22, 2024

You're correct, with this change this library will not generate its custom commands when making migrations based on a sqlite3 database, but that's important. The main point is that it should be possible to have this lib imported and still do migrations on an sqlite3.

The needs and motivated outlined in alternatives.md, also apply for a sqlite3/postgres parallel development. Enum synchronization in a postgres environment still needs to be done.

this library helped me to easily:

  1. identify with enums are out of sync with my staging/production postgres databases
  2. generate a synchronization migration
  3. correct my postgres databases

however, since now I have a working migration I cannot generate new migrations based on sqlite3 (due to the error I mentioned in this issue's original text - the import from alembic_postgresql_enum import TableReference as it triggers the alembic hook bindings starting from compare_enums.

and now there's a few options:

  1. change the development workflow and use postgres in local development as well
    • suboptimal
  2. go back and change the migration to not use this library and make custom commands in the migration file
    • suboptimal as it would be preferred to not have this concern on our team
  3. ensure this lib doesn't break current workflows

from these options, number 3 i think is the preferred solution, so i created this issue.

with this change:

  1. assures compatibility for the original intended use case and current users
  2. broadens the usability of this library in more diverse workflows

@RustyGuard
Copy link
Member

Good, I'll make this change

@Miguelreis
Copy link
Author

Very happy to hear that :)

when you have time, please also look into this quote of mine, as it would make the use of this lib as seamless as it is today but also for the my described use case, with backward compatibility aswell.

it would also be relevant that migrations created using this library would be applied only if upgrading a postgres database (my code above doesn't solve this issue).

if you need anything else i'll be available! Thank you.

@RustyGuard
Copy link
Member

There is new version 1.2.0a1. Please check whether these changes fulfill your request. If everything is alright write back and full release will be dropped

@Miguelreis
Copy link
Author

Miguelreis commented Apr 12, 2024

Hi, Sorry for the delayed response. I can confirm that generating new migrations in 1.2.0a1 no longer breaks the existing workflow, and it shows the corresponding warning. Thanks.

any feedback on this?

it would also be relevant that migrations created using this library would be applied only if upgrading a postgres database (my code above doesn't solve this issue).

edit:
i see you also changed in sync_enum_values. Dunno if its also usefull in other ops. thanks

@RustyGuard
Copy link
Member

edit: i see you also changed in sync_enum_values. Dunno if its also usefull in other ops. thanks

Sqlalchemy operations such as sa.Enum().create() will not do anything in dialects that do not have native enums

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

2 participants