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

implement autogenerate detection for foreign key constraints #178

Closed
sqlalchemy-bot opened this Issue Mar 6, 2014 · 23 comments

Comments

Projects
None yet
1 participant
@sqlalchemy-bot

sqlalchemy-bot commented Mar 6, 2014

Migrated issue, originally created by Charles Leifer (@coleifer)

Hi, thanks for your excellent work on this project (and SQA).

I'm using Flask-migrate and ran into a small issue adding a column that had a foreign key constraint. The column was added, but the constraint was not created. I wasn't sure if this was operator error on my part, but thought I'd share it anyway:

I made some model changes in my project and noticed that a foreign key constraint wasn't getting picked up when I ran migrate. Basically I had a blog Entry model and added a User model to represent the author. I also added a foreign key from the entry to the user.

Here are portions of the models:

class Entry(db.Model):
    id = db.Column(db.Integer, primary_key=True)
    title = db.Column(db.String(100))
    author_id = db.Column(db.Integer, db.ForeignKey('user.id'))  # New column

class User(db.Model):  # New model
    id = db.Column(db.Integer, primary_key=True)

This is the output of db migrate:

INFO  [alembic.migration] Context impl SQLiteImpl.
INFO  [alembic.migration] Will assume non-transactional DDL.
INFO  [alembic.autogenerate.compare] Detected added table 'user'
INFO  [alembic.autogenerate.compare] Detected added column 'entry.author_id'

Here is the migration:

    ### commands auto generated by Alembic - please adjust! ###
    op.create_table('user',
    sa.Column('id', sa.Integer(), nullable=False),
    ....
    sa.PrimaryKeyConstraint('id'),
    sa.UniqueConstraint('email'),
    sa.UniqueConstraint('slug')
    )
    op.add_column(u'entry', sa.Column('author_id', sa.Integer(), nullable=True))
    ### end Alembic commands ###

Notice that the foreign key constraint is missing.

I upgraded the db and, while the author_id column is there, the foreign key constraint is not:

CREATE TABLE entry (
	id INTEGER NOT NULL, 
	title VARCHAR(100), 
        author_id INTEGER, 
	PRIMARY KEY (id), 
	UNIQUE (slug)
);

Versions:

  • Flask-Migrate==1.2.0
  • Flask-SQLAlchemy==1.0
  • SQLAlchemy==0.9.1
  • alembic==0.6.2
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 6, 2014

Miguel Grinberg (@miguelgrinberg) wrote:

I can add that I reproduced this problem, even with Alembic 0.6.4. I also found that the foreign key constraint is correctly detected when the whole Entry table is part of the migration.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 6, 2014

Charles Leifer (@coleifer) wrote:

I also tried adding the user model first, then adding the FK in a second migration with the same result.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 6, 2014

Michael Bayer (@zzzeek) wrote:

hi charles !

note that this limitation has always been documented as not yet implemented (see "Autogenerate can’t currently, but will eventually detect:" at http://alembic.readthedocs.org/en/latest/tutorial.html#auto-generating-migrations). alembic might never know how to autogenerate everything (e.g. triggers, stored procs) though foreign keys is a big one.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 6, 2014

Changes by Michael Bayer (@zzzeek):

  • removed labels: bug
  • added labels: feature
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 6, 2014

Changes by Michael Bayer (@zzzeek):

  • changed title from "Missing foreign key constraint" to "implement autogenerate detection for foreign key c"
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 6, 2014

Michael Bayer (@zzzeek) wrote:

as far as why the FK is "correctly" detected when part of the Entry table itself, that's because it is detecting "need to add new table 'entry'", then it just copies the whole table definition wholesale. So it's not "detecting" the foreign key in that case either.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 6, 2014

Charles Leifer (@coleifer) wrote:

I promise I googled really hard before posting this! Thanks for the quick response, the explanation and docs make sense.

I modified the code to read like this:

op.add_column(
    u'entry', 
    sa.Column('author_id', sa.Integer(), sa.ForeignKey('user.id'), nullable=True)
)

I assume this will work on postgres or mysql, but since I'm using SQLite the alter table for the FK constraint appears to not be supported.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 6, 2014

Michael Bayer (@zzzeek) wrote:

SQLite supports addition of columns via ALTER (and not much else). Though it's true the FK constraint is added as a second step using ADD CONSTRAINT and SQLite doesn't support that.

SQLite's very poor support for schema migrations is kind of a problem Alembic hasn't tried very hard to solve. SQLAlchemy-Migrate did, though.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 10, 2014

Changes by Michael Bayer (@zzzeek):

  • added labels: autogenerate - detection
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 10, 2014

Changes by Michael Bayer (@zzzeek):

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

This comment has been minimized.

sqlalchemy-bot commented Oct 21, 2014

Michael Bayer (@zzzeek) wrote:

a big chunk of what we need for this is here: https://review.openstack.org/#/c/116238/17/oslo/db/sqlalchemy/test_migrations.py

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 5, 2014

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 5, 2014

Michael Bayer (@zzzeek) wrote:

hi @AKamyshnikova - looks pretty good, so we have to see...if it works ! the test suites for autogenerate are a little bit chaotic at the moment, for this feature they'd currently be part of tests/test_autogenerate.py, though maybe we can just add a new suite test_autogen_fks.py, which would draw from test_autogen_indexes.py for patterns. I can work on that or if you want to add to the PR let me know.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 6, 2014

akamyshnikova (@akamyshnikova) wrote:

Sure, I'll add some tests! I just have doubts how should adoption of this code for alembic look like.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 6, 2014

Michael Bayer (@zzzeek) wrote:

ah right, this needs to be wired into the render system, at least, ill make a note on the PR

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 21, 2014

nickretallack (@nickretallack) wrote:

Wow this is pretty major. I didn't notice that many of my constraints are missing until just now. I should have noticed the migrations looked a little light.

The docs say:

"Some free-standing constraint additions and removals, like CHECK and FOREIGN KEY - these are not fully implemented."

I didn't think of constraints in "add column" as free-standing. To be honest, when I read that line I initially thought "there's no way this can be right. Those sqlalchemy people love their foreign key constraints" :P so I swiftly forgot about it.

I hope this gets fixed soon.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 21, 2014

Michael Bayer (@zzzeek) wrote:

@nickretallack - that's what the "vote" button is for, I'm well aware this is a major issue and the PR is on the way for 0.7.1, thanks

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 30, 2014

Michael Bayer (@zzzeek) wrote:

Update and add some tests for checking fk

fixes issue #178

9b55ad2

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 30, 2014

Changes by Michael Bayer (@zzzeek):

  • changed status to closed
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 30, 2014

Michael Bayer (@zzzeek) wrote:

  • name the include_object hook "foreign_key_constraint"
  • changelog and other doc updates, fixes #178
  • fix drop_constraint() unit tests and add two more for FKs

0a47420

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Dec 22, 2014

rkrzr (@rkrzr) wrote:

Hi, and thanks a lot for this new feature!

It works as expected for foreign keys, but I get a strange result for a 'unique' constraint:
My column definition is

Column('url', String, unique=True, index=True, nullable=False)

But somehow alembic now tries to drop this constraint:

op.drop_constraint(u'table1_url_key', 'table1', schema='schema1', type_='unique')

Not sure if this is a bug, or because I have been monkeying with different Postgres schemas in the same migration.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Dec 22, 2014

Michael Bayer (@zzzeek) wrote:

hi @rkrzr -

there's always a lot of glitches with the unique constraint thing, did this problem appear with a certain release of alembic and do you have any details on your schema? you can create a new issue if you can provide specifics.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 11, 2015

Ben Beanfield (@benbeanfield) wrote:

@rkrzr
Perhaps alembic sees that you have a unique index (create_index(unique=True)) and therefore no longer need a separate constraint?

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