mysql.TINYINT(display_width=1) vs sa.Boolean()

Issue #46 invalid
dieselmachine
created an issue

I'm generating diffs that are failing to equate these two types (in autogenerate._compare_columns). I understand the two are not strictly equivalent, but I found this note in the sqlalchemy comments (dialects/mysql/base.py):

"Note: following the usual MySQL conventions, TINYINT(1) columns reflected during Table(..., autoload=True) are treated as Boolean columns."

I am not sure whether the issue lies with sqlalchemy or alembic, but I did some grepping through the sqla code attempting to find where the tinyint was being cast to boolean and I was unable to find anything that looked relevant.

I'd like alembic to not detect these as changes during autogenerate, is there something I need to do in order to have this work?

Comments (13)

  1. dieselmachine reporter

    After reading the discussion at https://groups.google.com/group/sqlalchemy/browse_thread/thread/b9c020804456f066 and reviewing the changeset at http://www.sqlalchemy.org/trac/changeset?reponame=&new=95ac46ca88eef268c54357e345a44fc5a930283a%40lib%2Fsqlalchemy%2Fdialects%2Fmysql%2Fbase.py&old=3d0efe038b8543a5ba78871860336346e7b17d96%40lib%2Fsqlalchemy%2Fdialects%2Fmysql%2Fbase.py I can see this behavior was intentional, so I guess it's not really a bug at all.

    You may want to scrub the related comment from the dialects/mysql/base.py file, as the information there is out of date and may lead people to expect unsupported behavior from the Inspector.

    The fix suggested in the linked discussion thread worked for me, and caused the tinyint != boolean ALTER statements to vanish from the migration. Thanks.

  2. Michael Bayer repo owner

    a more API-centric way of doing this is to use the column_reflect() event, new in 0.7. note the API for this event is changing in 0.8. Here's a demonstration:

    from sqlalchemy import *
    e = create_engine("mysql://scott:tiger@localhost/test", echo=True)
    
    m = MetaData()
    t = Table("bool_table", m, 
        Column("mybool", Boolean)
    )
    m.drop_all(e)
    m.create_all(e)
    
    from sqlalchemy.dialects import mysql
    
    m2 = MetaData()
    t2 = Table("bool_table", m2, autoload=True, autoload_with=e)
    assert type(t2.c.mybool.type) is mysql.TINYINT
    
    from sqlalchemy import event
    
    @event.listens_for(Table, "column_reflect")
    # in 0.7, use this:
    def column_reflect(table, column_info):
    # in 0.8
    # def column_reflect(inspector, table, column_info):
        if type(column_info['type']) is mysql.TINYINT:
            column_info['type'] = Boolean()
    
    m3 = MetaData()
    t3 = Table("bool_table", m3, autoload=True, autoload_with=e)
    assert type(t3.c.mybool.type) is Boolean
    

    you might also use this approach to deal with your issue in #47.

  3. Leonid Umanskiy

    Please, can you tell me why this issue is marked as invalid? Original link to the changeset doesn't work. For me, this is clearly a bug of Alembic. The simplest example with boolean causes infinite number of migrations/upgrades to be generated (every migrate will generate an upgrade script that does nothing), and in order fix this bug, I need to implement weird and version-specific patches in my own source code.

  4. Leonid Umanskiy

    Also, the example provided above is not correct. Doing

        if type(column_info['type']) is mysql.TINYINT:
            column_info['type'] = Boolean()
    

    will cause all TINYINT's to be treated as Booleans, which is incorrect. Every Boolean() is TINYINT(display_width=1) but not every TINYINT is Boolean. Take a look at this example:

    class Item(db.Model):
        item_id = db.Column(INTEGER(unsigned=True), primary_key=True)
        unique = db.Column(db.Boolean(), nullable=False, default=False)
        code = db.Column(TINYINT(unsigned=True), nullable=False)
    

    With no column_reflect, this example will cause "TINYINT(display_width=1) to Boolean()" change on "unique" field to be generated every time you run a migrate command. With column_reflect, this example will cause "Boolean() to TINYINT(unsigned=True)" change on "code" field.

  5. Michael Bayer repo owner

    how on earth would any tool distinguish a TINYINT(display_width=1) that is intended to be a TINYINT(display_width=1) from one that was originally intended to be a Boolean ? the bug here was that SQLAlchemy used to make this assumption, and we turned it off. your issue AFAICT seems to be that this assumption should be turned back on. There's no way to have both here. It's customizable.

  6. Leonid Umanskiy

    Mike,

    This fix worked for me:

    def my_compare_type(context, inspected_column,
                        metadata_column, inspected_type, metadata_type):
    
        if type(inspected_type) == TINYINT and type(metadata_type) == Boolean:
            return False  # Types are identical!
    
        return None
    

    This will treat types as identical only if in the model it's specified as Boolean and in in the database it's TINYINT (this will not generate any migrations if inb model it's TINYINT). Why can't we have something like that enabled by the default?

    P.S. Returning False in this function is totally counter-intuitive, but this is another topic.

  7. Michael Bayer repo owner

    not by default since it is not backwards compatible but it can be an option in alembic env, sure, mysql_compare_tinyint_as_boolean sent to Environment.configure(). make that a separate issue.

  8. Leonid Umanskiy

    Mike,

    Why is it not backwards compatible? We can always check for the specific MySQL type. Boolean() and sqlalchemy.dialects.mysql.TINYINT are always identical and they always be identical.

  9. Michael Bayer repo owner

    Why is it not backwards compatible?

    of all the dozens of changes and fixes I've made to autogenerate comparison over many months, not one of them broke something for somebody. test suites rely upon the compare feature alone returning the exact same result every time. If you were in charge of handling the fallout from making this change permanent/fixed, that would be fine, but for me I don't have the time to worry about side effects.

  10. Leonid Umanskiy

    How is it going to break anything? Sorry, I simply don't understand how fixing this bug (bug that makes Alembic unusable if you are using Booleans and need to compare_type) can make any difference. I can not imagine how someone can have a test suite that fails when Boolean becomes identical to MySQL.TINYINT.

    Of course, that is your repo, and you make decisions here. You got my feedback, I am not going to argue anymore.

  11. Michael Bayer repo owner

    I can not imagine how someone can have a test suite that fails when Boolean becomes identical to MySQL.TINYINT.

    same thing I said before all the other changes that in fact did break people's test suites (namely openstack), and for which I got flamed to a crisp on the mailing list for not warning them strongly enough.

    Sorry, I am tasked with an enormous support burden for Alembic / SQLAlchemy etc. and I really don't see how your day is more inconvenienced by this hypothetical feature being an option and not on by default. For me it means I don't even have to try to think of how this will break anything. Alembic is given to you free of charge so I hope you find it useful.

  12. Log in to comment