Pogchamp-company / alembic-postgresql-enum

Alembic autogenerate support for creation, alteration and deletion of enums
MIT License
184 stars 10 forks source link

assure usability on multi DB development stack #69

Closed Miguelreis closed 6 months ago

Miguelreis commented 7 months ago

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

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

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

Good, I'll make this change

Miguelreis commented 7 months ago

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

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 commented 6 months ago

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 commented 6 months ago

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