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

alembic attempts to alter_columns in Table def to nullable=True after creating Composite primary keys in create_table #199

Closed
sqlalchemy-bot opened this Issue Apr 15, 2014 · 10 comments

Comments

Projects
None yet
1 participant
@sqlalchemy-bot

sqlalchemy-bot commented Apr 15, 2014

Migrated issue, originally created by Mike Waites (@mikeywaites)

Hey,

Im not sure if this is covered anywhere in the docs and perhaps i just missed it, However I just encountered an issue that felt as though it was perhaps worth reporting.

Using flask-sqlalchemy and flask-migrate along with the latest versions of alembic and sqlalchemy I created a "initial" migration for my current schema. On the the table picked up was a m2m rel defined using the basic Table() pattern.

# -*- coding: utf-8 -*-

"""
person_to_role = Table('person_to_role', db.Model.metadata,
    Column('person_id', Integer, ForeignKey('person.id')),
    Column('role_id', Integer, ForeignKey('role.id')),
    PrimaryKeyConstraint('person_id', 'role_id')
)

This generated the alembic migration, which at first seemed correct.

    op.create_table(u'person_to_role',
    sa.Column(u'person_id', sa.Integer(), nullable=True),
    sa.Column(u'role_id', sa.Integer(), nullable=True),
    sa.ForeignKeyConstraint(['person_id'], [u'person.id'], ),
    sa.ForeignKeyConstraint(['role_id'], [u'role.id'], ),
    sa.PrimaryKeyConstraint(u'person_id', u'role_id')
    )

I decided to run another migration expecting a blank migration to be created. alembic actually generated an upgrade migration for the m2m model as follows.

    op.alter_column(u'person_to_role', u'person_id',
               existing_type=sa.INTEGER(),
               nullable=True)
    op.alter_column(u'person_to_role', u'role_id',
               existing_type=sa.INTEGER(),
               nullable=True)

As we can see, alembic is not trying to modify the columns so null would be a valid value which would obviously break at the db level anyway as both rows compose the PK and are created in the first migration like:

Table "public.person_to_role"
  Column   |  Type   | Modifiers
-----------+---------+-----------
 person_id | integer | not null
 role_id   | integer | not null
Indexes:
    "person_to_role_pkey" PRIMARY KEY, btree (person_id, role_id)
Foreign-key constraints:
    "person_to_role_person_id_fkey" FOREIGN KEY (person_id) REFERENCES person(id)
    "person_to_role_role_id_fkey" FOREIGN KEY (role_id) REFERENCES role(id)

We can however see in the first migration that nullable=True was defined, just ignored due to the PrimaryKeyContraint. setting nullable=False solves the problem for me, however i kinda felt this wasn`t the expected behaviour. Should you explicitly set nullable=False in a Table def like this or is this actually a bug?

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 15, 2014

Michael Bayer (@zzzeek) wrote:

it's a bug on the SQLAlchemy side, added https://bitbucket.org/zzzeek/sqlalchemy/issue/3023/inconsistent-behavior-of-nullable-with.

in your table metadata, use the primary_key=True flag for now to prevent this from happening.

on the Alembic side we can try to work around this also with this patch:

--- a/alembic/autogenerate/compare.py
+++ b/alembic/autogenerate/compare.py
@@ -152,11 +152,13 @@ def _compare_columns(schema, tname, object_filters, conn_table, metadata_table,
             metadata_col,
             col_diff, autogen_context
         )
-        _compare_nullable(schema, tname, colname,
-            conn_col,
-            metadata_col.nullable,
-            col_diff, autogen_context
-        )
+        # work around SQLAlchemy issue #3023
+        if not metadata_col.primary_key:
+            _compare_nullable(schema, tname, colname,
+                conn_col,
+                metadata_col.nullable,
+                col_diff, autogen_context
+            )
         _compare_server_default(schema, tname, colname,
             conn_col,
             metadata_col,

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 15, 2014

Changes by Michael Bayer (@zzzeek):

  • removed labels: low priority
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 15, 2014

Changes by Michael Bayer (@zzzeek):

  • set milestone to "tier 1"
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 15, 2014

Changes by Michael Bayer (@zzzeek):

  • changed milestone from "tier 1" to "fasttrack"
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 15, 2014

Mike Waites (@mikeywaites) wrote:

Great thanks Mike!!!

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented May 3, 2014

Michael Bayer (@zzzeek) wrote:

I'm still getting my milestones straight, was using "critical" as a guide to what's needed in the next release, so this one got missed. Sorry!

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented May 3, 2014

Changes by Michael Bayer (@zzzeek):

  • added labels: high priority
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented May 4, 2014

Mike Waites (@mikeywaites) wrote:

No problems Mike!

I implemented your work around for now which has held up fine!

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 2, 2014

Michael Bayer (@zzzeek) wrote:

  • Added a workaround for SQLAlchemy issue #3023 (fixed in 0.9.5) where
    a column that's part of an explicit PrimaryKeyConstraint would not
    have its "nullable" flag set to False, thus producing a false
    autogenerate. Also added a related correction to MySQL which will
    correct for MySQL's implicit server default of '0' when a NULL integer
    column is turned into a primary key column. fixes #199

92184a5

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 2, 2014

Changes by Michael Bayer (@zzzeek):

  • changed status to closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment