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

Crash in Alembic comparator for a custom TypeDecorator column when default impl differs from a specific #395

Closed
sqlalchemy-bot opened this Issue Nov 8, 2016 · 7 comments

Comments

Projects
None yet
1 participant
@sqlalchemy-bot

sqlalchemy-bot commented Nov 8, 2016

Migrated issue, originally created by frol (@frol)

Having a custom column type, e.g.:

class PasswordType(types.TypeDecorator, ScalarCoercible):
    def load_dialect_impl(self, dialect):
        if dialect.name == 'sqlite':
            # Use a NUMERIC type for sqlite
            impl = sqlite.NUMERIC(self.length)
        else:
            # Use a VARBINARY for all other dialects.
            impl = types.VARBINARY(self.length)
        return dialect.type_descriptor(impl)

Alembic crashes when tries to compare the existing (the first migration is fine) column with the custom type:

  File ".../lib/python3.5/site-packages/alembic/autogenerate/compare.py", line 77, in _autogen_for_tables
    inspector, metadata, upgrade_ops, autogen_context)
  File ".../lib/python3.5/site-packages/alembic/autogenerate/compare.py", line 177, in _compare_tables
    modify_table_ops, autogen_context, inspector):
  File ".../lib/python3.5/contextlib.py", line 59, in __enter__
    return next(self.gen)
  File ".../lib/python3.5/site-packages/alembic/autogenerate/compare.py", line 256, in _compare_columns
    schema, tname, colname, conn_col, metadata_col
  File ".../lib/python3.5/site-packages/alembic/util/langhelpers.py", line 314, in go
    fn(*arg, **kw)
  File ".../lib/python3.5/site-packages/alembic/autogenerate/compare.py", line 624, in _compare_type
    conn_col, metadata_col)
  File ".../lib/python3.5/site-packages/alembic/runtime/migration.py", line 391, in _compare_type
    metadata_column)
  File ".../lib/python3.5/site-packages/alembic/ddl/impl.py", line 261, in compare_type
    return comparator and comparator(metadata_type, conn_type)
  File ".../lib/python3.5/site-packages/alembic/ddl/impl.py", line 335, in _numeric_compare
    t1.precision is not None and
  File ".../lib/python3.5/site-packages/sqlalchemy/sql/type_api.py", line 913, in __getattr__
    return getattr(self.impl, key)
AttributeError: 'VARBINARY' object has no attribute 'precision'

I have attempted to fix this in PR: https://bitbucket.org/zzzeek/alembic/pull-requests/64/

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 8, 2016

Changes by frol (@frol):

  • added labels: easy
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 8, 2016

Michael Bayer (@zzzeek) wrote:

OK, I'm still not able to reproduce this condition. Note that Alembic is not related to the sqlalchemy-utils project so the ultimate test case cannot rely upon a symbol such as "ScalarCoercible". It does not make sense that "metadata_impl" in this case is a VARBINARY type, that's not how load_dialect_impl() works - load_dialect_impl() provides another PasswordType that has as its impl, the thing you returned from load_dialect_impl():

from sqlalchemy import types


class MyType(types.TypeDecorator):
    impl = types.String

    def load_dialect_impl(self, dialect):
        return types.Integer()

    def copy(self, **kw):
        return MyType()

from sqlalchemy.dialects import sqlite

dialect = sqlite.dialect()

metadata_type = MyType()

metadata_impl = metadata_type.dialect_impl(dialect)

print repr(metadata_impl)
print repr(metadata_impl.impl)

output:

$ python test.py 
MyType()
Integer()

So I'm not seeing how VARBINARY is leaking into there.

Here's my test, I'm trying to get the line in question to trigger and I cannot:

diff --git a/tests/test_autogen_diffs.py b/tests/test_autogen_diffs.py
index 04c9e96..3193ba1 100644
--- a/tests/test_autogen_diffs.py
+++ b/tests/test_autogen_diffs.py
@@ -592,6 +592,28 @@ class CompareTypeSpecificityTest(TestBase):
         return impl.DefaultImpl(
             default.DefaultDialect(), None, False, True, None, {})
 
+    def test_typedec_to_nonstandard(self):
+        from sqlalchemy import types
+        from sqlalchemy.dialects import sqlite
+
+        #class PasswordType(types.TypeDecorator, types.String): # works
+        class PasswordType(types.TypeDecorator, types.VARBINARY): # works
+            impl = types.VARBINARY # works
+
+            def load_dialect_impl(self, dialect):
+                if dialect.name == 'default':
+                    # Use a NUMERIC type for sqlite
+                    impl = sqlite.NUMERIC(self.length)
+                else:
+                    # Use a VARBINARY for all other dialects.
+                    impl = types.VARBINARY(self.length)
+                return dialect.type_descriptor(impl)
+
+        impl = self._fixture()
+        impl.compare_type(
+            Column('x', types.VARBINARY(50)),  # works
+            Column('x', PasswordType(50)))  # works
+
     def test_string(self):
         t1 = String(30)
         t2 = String(40)

to run this test after applying the patch, run py.test tests/test_autogen_diffs.py -k test_typedec_to_nonstandard. So far this is looking like something is off in sqlalchemy-utils ScalarCoercible class. You might want to get Konsta involved here.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 8, 2016

Michael Bayer (@zzzeek) wrote:

also, there's no reason metadata_impl would be needed inside the proposed comparison because the dialect-specific impl is always a simple subclass of the base type, and the built-in comparators here are only comparing very basic attributes. For more elaborate comparison needs the "compare_against_backend" hook would be used.

If there is a bug here, it may very well be in TypeDecorator somehow.

anyway hoping this illustrates why I don't accept one-line PRs...

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 9, 2016

frol (@frol) wrote:

Please, try to use the following in your test:

+        impl.compare_type(
+            Column('x', types.NUMERIC(50)),
+            Column('x', PasswordType(50)))

The change I made is:

-            Column('x', types.VARBINARY(50)), # works
+            Column('x', types.NUMERIC(50)),

I get the crash when I run such test.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 9, 2016

Michael Bayer (@zzzeek) wrote:

Compare to metadata_impl in compare_type() to guard against custom TypeDecorator

Fixed bug where usage of a custom TypeDecorator which returns a
per-dialect type via :meth:.TypeDecorator.load_dialect_impl that differs
significantly from the default "impl" for the type decorator would fail
to compare correctly during autogenerate.

Change-Id: I384df35be9513bf8a2ae55e7daa9a52c23108a49
Fixes: #395

01e9b8d

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 9, 2016

Changes by Michael Bayer (@zzzeek):

  • changed status to closed
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Nov 9, 2016

frol (@frol) wrote:

Thank you!

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