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

need operation to drop foreign key #44

Closed
sqlalchemy-bot opened this Issue Apr 27, 2012 · 17 comments

Comments

Projects
None yet
1 participant
@sqlalchemy-bot

sqlalchemy-bot commented Apr 27, 2012

Migrated issue, originally created by aodag (@aodag)

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 27, 2012

Michael Bayer (@zzzeek) wrote:

dropping a foreign key is via the op.drop_constraint() operation. it requires the name of the constraint be given.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 27, 2012

Changes by Michael Bayer (@zzzeek):

  • added labels: wontfix
  • changed status to closed
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 30, 2012

aodag (@aodag) wrote:

On mysql, op.drop_constraint() causes error.

For example, op.drop_constraint('fk_id', 't1') generate sql, "ALTER TABLE t1 DROP fk_id'.
But that statement means drop column named fk_id.

drop_constraint uses schema.Constraint, but mysql dialects requires subclasses of Constraint (e.g ForeignKeyConstraint, PrimaryKeyConstraint..)

Maybe drop_constraint must detect kind of constraints.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 30, 2012

Changes by aodag (@aodag):

  • removed labels: wontfix
  • changed status to reopened
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 30, 2012

Changes by aodag (@aodag):

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

This comment has been minimized.

sqlalchemy-bot commented Apr 30, 2012

Michael Bayer (@zzzeek) wrote:

can you show me the correct syntax for MySQL ?

yes, the answer would be an extra parameter to drop_constraint() to suit MySQL's incompatibility here.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 30, 2012

Michael Bayer (@zzzeek) wrote:

needs params for CHECK and UNIQUE as well, mysql breaking all the rules here. drop_constraint() alone should fail when run with MySQL if this param isn't present.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 30, 2012

aodag (@aodag) wrote:

For drop foreign key, correct statement is" ALTER TABLE tbl_name DROP FOREIGN KEY fk_symbol".
And for drop unique key, just execute "ALTER TABLE tbl_name DRIP KEY key_name".

These statements seems supported by sqlalchemy.dialects.mysql.

reference:
http://dev.mysql.com/doc/refman/5.5/en/alter-table.html

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 30, 2012

Michael Bayer (@zzzeek) wrote:

here's what I need you to test:

  1. alembic patch:

    #!diff

    diff -r 17b7299 alembic/operations.py
    --- a/alembic/operations.py Mon Apr 30 15:17:18 2012 -0400
    +++ b/alembic/operations.py Mon Apr 30 16:53:24 2012 -0400
    @@ -529,10 +529,29 @@
    # 0.7.6 and further raises on Index with no columns
    self.impl.drop_index(self._index(name, tablename, ['x']))

    • def drop_constraint(self, name, tablename):
    •    """Drop a constraint of the given name"""
      
    • def drop_constraint(self, name, tablename, type=None):
    •    """Drop a constraint of the given name
      
    •    :param name: name of the constraint.
      
    •    :param tablename: tablename:
      
    •    :param type: optional, required on MySQL.  can be 
      
    •    'foreignkey', 'unique', or 'check'
      
    •    """
         t = self._table(tablename)
      
    •    const = schema.Constraint(name=name)
      
    •    types = {
      
    •        'foreignkey':schema.ForeignKeyConstraint,
      
    •        'unique':schema.UniqueConstraint,
      
    •        'check':schema.CheckContraint,
      
    •        None:schema.Constraint
      
    •    }
      
    •    try:
      
    •        const = types[type]
      
    •    except KeyError:
      
    •        raise TypeError("'type' can be one of %s" % 
      
    •                    ", ".join(repr(x) for x in types)) 
      
    •    const = const(name=name)
         t.append_constraint(const)
         self.impl.drop_constraint(const)
      
  2. sqlalchemy patch:

    #!diff
    diff -r 572d4ebbca4b lib/sqlalchemy/dialects/mysql/base.py
    --- a/lib/sqlalchemy/dialects/mysql/base.py Sun Apr 29 18:53:29 2012 -0400
    +++ b/lib/sqlalchemy/dialects/mysql/base.py Mon Apr 30 16:54:11 2012 -0400
    @@ -1521,9 +1521,11 @@
    elif isinstance(constraint, sa_schema.UniqueConstraint):
    qual = "INDEX "
    const = self.preparer.format_constraint(constraint)

    •    elif isinstance(constraint, sa_schema.CheckConstraint):
      
    •        qual = "CHECK "
      
    •        const = self.preparer.format_constraint(constraint)
         else:
      
    •        qual = ""
      
    •        const = self.preparer.format_constraint(constraint)
      
    •        raise NotImplementedError("no generic 'DROP CONSTRAINT' in mysql")
         return "ALTER TABLE %s DROP %s%s" % \
                     (self.preparer.format_table(constraint.table),
                     qual, const)
      
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 30, 2012

Michael Bayer (@zzzeek) wrote:

we would need new tests both for sqlalchemy and alembic here, since this suggests sqlalchemy has a bug too regarding CHECK and generic constraints.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 30, 2012

aodag (@aodag) wrote:

I tryed workaround for foreignkey constraint too.

https://bitbucket.org/aodag/alembic/changeset/17ae9180ee40550f4160ea745e6c895e469c15e0

ForeignKeyConstraint requries arguments of cols and refcols.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 30, 2012

Michael Bayer (@zzzeek) wrote:

modify ForeignKeyConstraint in SQLAlchemy to accept just "name" alone as a kw arg if no column lists are given. all of these constraints need to be able to exist as "name only" objects.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 30, 2012

Michael Bayer (@zzzeek) wrote:

SQLA is on 0.8 right now, I can backport whatever to 0.7.7.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented May 1, 2012

Michael Bayer (@zzzeek) wrote:

another thought - we should fix sqlalchemy but let's put the full series of DROP CONSTRAINT compilations into alembic, just to make things easy for now.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented May 1, 2012

Michael Bayer (@zzzeek) wrote:

mysql has no CHECK constraint - brilliant ! I think i used to know this.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented May 1, 2012

Michael Bayer (@zzzeek) wrote:

fixed in [[https://bitbucket.org/zzzeek/alembic/changeset/494ac8d84a4e827fcf7dc117b84ba76f442d60a2|494ac8d84a4e827fcf7dc117b84ba76f442d60a2]], please verify I got it right, thanks !

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented May 1, 2012

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