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

Issue #395 resolved
frol
created an issue

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/

Comments (6)

  1. Michael Bayer repo owner

    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.

  2. Michael Bayer repo owner

    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...

  3. frol reporter

    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.

  4. Michael Bayer repo owner

    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

    → <<cset 01e9b8df995a>>

  5. Log in to comment