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

drop_constraint() does not quote check constraint name #487

Closed
sqlalchemy-bot opened this issue Mar 21, 2018 · 14 comments
Closed

drop_constraint() does not quote check constraint name #487

sqlalchemy-bot opened this issue Mar 21, 2018 · 14 comments

Comments

@sqlalchemy-bot
Copy link

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Migrated issue, originally created by Robert Buchholz (@rbuchholz)

Alembic does not quote a constraint name (at least for MariaDB). If it contains a dot, (in the best case) MariaDB will complain (depending on the number of dots) about missing tables or databases. In the worst case, a constraint on a schema and table not referenced by the table and schema arguments might be dropped.

Actual behavior:

(Pdb) op.drop_constraint('foo.bar',table_name='baz', type_='check')
*** ProgrammingError: (pymysql.err.ProgrammingError) (1103, u"Incorrect table name 'foo'") [SQL: u'ALTER TABLE baz DROP CONSTRAINT foo.bar'] (Background on this error at: http://sqlalche.me/e/f405)
(Pdb) op.drop_constraint('fnord.foo.bar',table_name='baz', type_='check')
*** ProgrammingError: (pymysql.err.ProgrammingError) (1102, u"Incorrect database name 'fnord'") [SQL: u'ALTER TABLE baz DROP CONSTRAINT fnord.foo.bar'] (Background on this error at: http://sqlalche.me/e/f405)

Versions:

  • alembic 0.9.8
  • SQLAlchemy 1.2.5
  • MariaDB 10.2.13
  • Python 2.7.14
@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Changes by Robert Buchholz (@rbuchholz):

  • edited description
@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Robert Buchholz (@rbuchholz) wrote:

FYI: This does not happen on type='foreignkey', type='unique' or op.drop_index.

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Changes by Robert Buchholz (@rbuchholz):

  • changed title from "drop_constraint() does not quote constraint name" to "drop_constraint() does not quote check constraint "
@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Michael Bayer (@zzzeek) wrote:

Dots are controversial here, so here is a better test to illustrate that the basic quoting isn't present:

    def test_drop_check_quoted(self):
        context = op_fixture('mysql')
        op.drop_constraint("MyCheck", "MyTable", "check")
        context.assert_(
            "ALTER TABLE `MyTable` DROP CONSTRAINT `MyCheck`"
        )

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Robert Buchholz (@rbuchholz) wrote:

Thank you for the quick fix, as always. Why would you consider dots controversial -- is it because they have a meaning when not escaped?

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Michael Bayer (@zzzeek) wrote:

dots are often trying to actually produce a "schemaname.tablename" combination or otherwise work within some namespacing provided by the database. the dot is an operator so it's not always clear it's meant as an arbitrary character and not an operator. for a constraint name, it would be very unusual for the dot to be used this way but you never know what some new database does.

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Michael Bayer (@zzzeek) wrote:

Ensure CHECK constraint name is quoted for MySQL

Change-Id: Ib85de299c5f3c2631c64fe30006879ba274fca15
Fixes: #487

533c965

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Changes by Michael Bayer (@zzzeek):

  • changed status to closed
@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Robert Buchholz (@rbuchholz) wrote:

I actually experimented with a new naming convention where constraint name parts are separated by dots, which is how I stumbled upon this issue.
The default convention (in the docs) leads to names like "fk_table_name_property_name" where the underscore has dual meaning (coming from table/column identifiers and separating those identifiers). Do you have a non-dot recommendation to achieve clean separation of identifiers?

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Michael Bayer (@zzzeek) wrote:

I would stick with underscores and not overthink it, it's always nice to not venture into quoting territory and there's not really any other non-alphanumeric characters that won't trigger quoting. looks like the dollar sign isn't quoted either, if you want to try that. the reg is: r'^[A-Z0-9_$]+$'.

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Michael Bayer (@zzzeek) wrote:

I haven't released yet so please work around this by using quoted_name:

http://docs.sqlalchemy.org/en/latest/core/sqlelement.html?highlight=quoted_name#sqlalchemy.sql.elements.quoted_name

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 21, 2018

Michael Bayer (@zzzeek) wrote:

hmmmm, though that might not even work.

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Mar 22, 2018

Robert Buchholz (@rbuchholz) wrote:

I tend to agree with the caution regarding the use of dots in identifiers, thank you for the insight. Maybe $ is a better option, on the other hand it moves the strict quoting requirement from SQL to Shell scripts operating on databases.

I've tried quoted_name for a workaround, but it did not work. For now, I'm using this (a bit whacky) version:

def workaround_alembic_487(name):
    from pkg_resources import parse_version
    # Alembic before 0.9.9 did not quote constraint names
    # https://bitbucket.org/zzzeek/alembic/issues/487/drop_constraint-does-not-quote-check
    if parse_version(alembic.__version__) <= parse_version('0.9.8'):
        return "`%s`" % name
    return name
vvvrrooomm pushed a commit to vvvrrooomm/alembic that referenced this issue Jan 10, 2019
Change-Id: Ib85de299c5f3c2631c64fe30006879ba274fca15
Fixes: sqlalchemy#487
vvvrrooomm pushed a commit to vvvrrooomm/alembic that referenced this issue Jan 10, 2019
Change-Id: Ib85de299c5f3c2631c64fe30006879ba274fca15
Fixes: sqlalchemy#487
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.