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 the wrong foreign key on columns.column_name #466

Closed
x4base opened this issue May 13, 2016 · 7 comments · Fixed by #1172
Closed

Fix the wrong foreign key on columns.column_name #466

x4base opened this issue May 13, 2016 · 7 comments · Fixed by #1172
Labels
!deprecated-label:bug Deprecated label - Use #bug instead

Comments

@x4base
Copy link
Contributor

x4base commented May 13, 2016

In the commit c2bb49f, the foreign key that references datasource.datasource_name was set on columns.column_name when it should be columns.datasource_name. This causes errors when there are any new columns to be created when refreshing the druid datasources.

I am trying to generate a migration script to fix this. However, op.drop_constraint() requires the name of the foreign key you want to drop.
The following code works on SQLite because the foreign keys don't have any names:

naming_convention = {
    "fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s",
}

def upgrade():
    with op.batch_alter_table('columns', schema=None, naming_convention=naming_convention) as batch_op:
        fk_name = 'fk_columns_column_name_datasources'
        batch_op.drop_constraint(fk_name, type_='foreignkey')
        batch_op.create_foreign_key(fk_name, 'datasources',
                                    ['datasource_name'], ['datasource_name'])

But for MySQL, the foreign keys are named like "columns_ibfk_1". Should I presume that on every machines, this foreign key has the same name and just drop the foreign key named columns_ibfk_1?
Do you have any suggestion about how to write a universal migration script that works on every SQL dialects? Or should I iterate through the known default names in different dialects in try-catch blocks?
I just couldn't figure out a way to drop the foreign key without knowing its name.

@sid88in
Copy link
Contributor

sid88in commented May 31, 2016

For me https://github.com/airbnb/caravel/pull/531/files fixed the issue

@x4base
Copy link
Contributor Author

x4base commented May 31, 2016

Yes, because it is specialized for MySQL and will only work on MySQL. You can try it on SQLite or other databases. It won't work because the foreign key is not named that way

@x4base
Copy link
Contributor Author

x4base commented May 31, 2016

That's why I said I want a "universal migration script that works on every SQL dialects"

@waterwoodwind
Copy link

Yes,it can't get the real value from the foreign key on SQLite

@xrmx xrmx added the !deprecated-label:bug Deprecated label - Use #bug instead label Aug 9, 2016
@bkyryliuk
Copy link
Member

class DruidColumn(Model, AuditMixinNullable):

    """ORM model for storing Druid datasource column metadata"""

    __tablename__ = 'columns'
    id = Column(Integer, primary_key=True)
    datasource_name = Column(
        String(255),
        ForeignKey('datasources.datasource_name'))
    # Setting enable_typechecks=False disables polymorphic inheritance.
    datasource = relationship('DruidDatasource', backref='columns',
                              enable_typechecks=False)

Models show correct foreign key now. Closing the issue.

@x4base
Copy link
Contributor Author

x4base commented Sep 12, 2016

Are you sure? It's not about the model, I know the model was fixed. The problem is whether the underlying database is migrated correctly.

@bkyryliuk bkyryliuk reopened this Sep 12, 2016
@bkyryliuk
Copy link
Member

@x4base - you right, looks like migrated DB doesn't have the foreign keys:
screen shot 2016-09-12 at 10 52 12 am
screen shot 2016-09-12 at 10 52 29 am

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
!deprecated-label:bug Deprecated label - Use #bug instead
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants