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 SQLAlchemy enum generation #363

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

sheinbergon
Copy link
Collaborator

Fixes #361

@sheinbergon sheinbergon requested a review from agronholm January 24, 2025 07:51
@coveralls
Copy link

Coverage Status

coverage: 96.957% (-0.07%) from 97.025%
when pulling 84b9ecb on sheinbergon:master
into 82251f3 on agronholm:master.

@agronholm
Copy link
Owner

This needs a regression test and a changelog entry.

@sheinbergon
Copy link
Collaborator Author

sheinbergon commented Jan 24, 2025

This needs a regression test and a changelog entry.

OK, I'll add the changes to the Changes.rst

As for a regression test - there's already a test that generates an enum (an intiial trial attempt caused it to fail).
However, like all of your tests, it doesn't really generate code from a live db, but rather from an SQLalchemy definition set,

so it was passing all along, while differing from real world behavior which caused a differen enum statement to be generated. How would you like to tackle that?

I can see if maybe a specific postgresql.ENUM dialect would trigger the error

@agronholm
Copy link
Owner

This needs a regression test and a changelog entry.

OK, I'll add the changes to the Changes.rst

As for a regression test - there's already a test that generates an enum (an intiial trial attempt caused it to fail). However, like all of your tests, it doesn't really generate code from a live db, but rather from an SQLalchemy definition set,

so it was passing all along, while differing from real world behavior which caused a differen enum statement to be generated. How would you like to tackle that?

I can see if maybe a specific postgresql.ENUM dialect would trigger the error

Yes, that would probably do the trick. IIRC I've used this technique elsewhere in the test suite when the generic types weren't reproducing the issues I wanted to fix.

@agronholm agronholm changed the title Fix SQLAlcehmy enum generation Fix SQLAlchemy enum generation Jan 28, 2025
@sheinbergon
Copy link
Collaborator Author

This needs a regression test and a changelog entry.

OK, I'll add the changes to the Changes.rst
As for a regression test - there's already a test that generates an enum (an intiial trial attempt caused it to fail). However, like all of your tests, it doesn't really generate code from a live db, but rather from an SQLalchemy definition set,
so it was passing all along, while differing from real world behavior which caused a differen enum statement to be generated. How would you like to tackle that?
I can see if maybe a specific postgresql.ENUM dialect would trigger the error

Yes, that would probably do the trick. IIRC I've used this technique elsewhere in the test suite when the generic types weren't reproducing the issues I wanted to fix.

@agronholm so, I took a shot at it.

This is not reproducible with the current set of unit tests (not for the tables, nor for the declarative generator).
Even if I define PostgreSQL ENUM dialect type. When the Table is rendered and metadata is being loaded with the column definitions, it always downgrades the dialect specific ENUM type to a plain SQLAlchemy Enum type, thus skipping this entire error.

I can confirm my fix solves the issues for both declarative and tables (as they both share the same function).

The only way I see to do a regression test for something like is by doing real integration / e2e tests against a dockerized PostgreSQL DB. But that's going to be a whole lot of work for a one line fix ( perhaps worth considering for the future)

So, adding a test brings 0 value ATM on top of the current "fancy_types" one. If you know of way to have the dialect specific ENUM type survive metadata reflection, please let me know.

If not, I suggest we merge this fix without adding a test.

Added changes to change log, as you requested.

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.

ENUM name is not correct for first entry
3 participants