1. Michael Bayer
  2. alembic

Pull requests

#4 Merged
Repository
mvschaik
Branch
fix_custom_compare_type
Repository
zzzeek
Branch
master

Fix calling of custom type compare function

Author
  1. Maarten van Schaik
Reviewers
Description

When using a custom comparison function for compare_type, the calling code crashes. This issue was introduced in 949367b.

Steps to reproduce:

def my_compare_type(context, inspected_column, metadata_column, inspected_type, metadata_type):
    pass

context.configure(connection=connection, target_metadata=target_metadata, compare_type=my_compare_type)
...
  File "alembic/env.py", line 74, in run_migrations_online
    context.run_migrations()
  File "<string>", line 7, in run_migrations
  File "/.../python2.7/site-packages/alembic-0.6.1dev-py2.7.egg/alembic/environment.py", line 652, in run_migrations
    self.get_context().run_migrations(**kw)
  File "/.../python2.7/site-packages/alembic-0.6.1dev-py2.7.egg/alembic/migration.py", line 209, in run_migrations
    self):
  File "/.../python2.7/site-packages/alembic-0.6.1dev-py2.7.egg/alembic/command.py", line 83, in retrieve_migrations
    autogen._produce_migration_diffs(context, template_args, imports)
  File "/.../python2.7/site-packages/alembic-0.6.1dev-py2.7.egg/alembic/autogenerate.py", line 143, in _produce_migration_diffs
    autogen_context, object_filters, include_schemas)
  File "/.../python2.7/site-packages/alembic-0.6.1dev-py2.7.egg/alembic/autogenerate.py", line 205, in _produce_net_changes
    inspector, metadata, diffs, autogen_context)
  File "/.../python2.7/site-packages/alembic-0.6.1dev-py2.7.egg/alembic/autogenerate.py", line 249, in _compare_tables
    diffs, autogen_context)
  File "/.../python2.7/site-packages/alembic-0.6.1dev-py2.7.egg/alembic/autogenerate.py", line 299, in _compare_columns
    col_diff, autogen_context
  File "/.../python2.7/site-packages/alembic-0.6.1dev-py2.7.egg/alembic/autogenerate.py", line 349, in _compare_type
    isdiff = autogen_context['context']._compare_type(conn_col, metadata_col)
  File "/.../python2.7/site-packages/alembic-0.6.1dev-py2.7.egg/alembic/migration.py", line 283, in _compare_type
    inspector_column['type'],
  File "/.../python2.7/site-packages/sqlalchemy/sql/operators.py", line 320, in __getitem__
    return self.operate(getitem, index)
  File "/.../python2.7/site-packages/sqlalchemy/sql/expression.py", line 2300, in operate
    return op(self.comparator, *other, **kwargs)
  File "/.../python2.7/site-packages/sqlalchemy/sql/operators.py", line 320, in __getitem__
    return self.operate(getitem, index)
  File "/.../python2.7/site-packages/sqlalchemy/sql/expression.py", line 1986, in operate
    return o[0](self, self.expr, op, *(other + o[1:]), **kwargs)
  File "/.../python2.7/site-packages/sqlalchemy/sql/expression.py", line 2137, in _unsupported_impl
    "this expression" % op.__name__)
NotImplementedError: Operator 'getitem' is not supported on this expression
  • Learn about pull requests

Comments (6)

  1. Michael Bayer repo owner

    I'm fairly shocked there's already no test for this, my fault. a test needs to be written, which I can do at some point or if you want to write one in test_autogenerate that would be great (though there's no existing pattern there for testing comparison code, I'd push to keep it simple), before this is merged.

  2. Maarten van Schaik author

    Probably there's an easier way to test the fact that either all columns are excluded or all columns are excluded than comparing the string from the template, but I'm not that familiar with the innards of alembic yet...

    Maybe there are some easy tweaks you can make to simplify my proposed tests?

  3. Michael Bayer repo owner

    well im starting to use mock.Mock() and similar to just test the functions in isolation more and more. the test_autogenerate tests are horrible, i hate working with them. I see you used mock, great, maybe the part where it's comparing strings we can skip that , instead just compare the _produce_net_changes() outputs ? if you look at test_diffs and others, you can see that. it only needs to compare the parts that are sensitive to what you're testing.

  4. Maarten van Schaik author

    Good idea!

    I still don't really like the way self.context._user_compare_type = my_compare_type is uses the internals of the MigrationContext, but otherwise I would need to duplicate most of the the AutogenTest-setup. If you want I could extract the value of compare_type into a separate classmethod that I can override in the test case, but my feeling is that is it a bit overkill and wouldn't improve the readability of the tests.