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

MySQL dialect types generating spurious revisions in 1.4 #661

Closed
peterschutt opened this issue Feb 21, 2020 · 10 comments
Closed

MySQL dialect types generating spurious revisions in 1.4 #661

peterschutt opened this issue Feb 21, 2020 · 10 comments
Labels

Comments

@peterschutt
Copy link

Auto-generating a revision in 1.3.3 does not pick up any changes, however after upgrading to 1.4, every MySQL dialect type column generates a revision such as this one:

    op.alter_column('comp_details', 'market_type_id',
               existing_type=mysql.INTEGER(display_width=10, unsigned=True),
               type_=mysql.INTEGER(unsigned=True),
               existing_nullable=True)

Here are the column definitions for that one:

    market_type_id = Column(INTEGER(unsigned=True), nullable=True)

I only use the mysql specific integer types, so am unsure if this manifests with other mysql dialect types but would be happy to dig deeper if it would be helpful.

@zzzeek
Copy link
Member

zzzeek commented Feb 21, 2020

hey there -

this is sort of the kind of bug I figured we'd have with the new system. the logic is supposed to not generate4 a migration if one side has an option that the other doesnt, because they can be implicit on one side or the other. guess that's not working here but we'll add this case and it should hopefully fix this whole class of issues.

@zzzeek
Copy link
Member

zzzeek commented Feb 21, 2020

cc @pbecotte , I'll take a look today

@pbecotte
Copy link
Collaborator

Yeah, I don't think there's any tests using unsigned specifically, so dunno without looking why. I will also have time today.

@pbecotte
Copy link
Collaborator

So- this is definitely a bug. See my PR for more details. However, it left me even more confused trying to duplicate it. I set up test cases-

        (
            mysql.INTEGER(unsigned=True, display_width=10),
            mysql.INTEGER(unsigned=True, display_width=10),
            False,
        ),
        (mysql.INTEGER(unsigned=True), mysql.INTEGER(unsigned=True), False),
        (
            mysql.INTEGER(unsigned=True, display_width=10),
            mysql.INTEGER(unsigned=True),
            False,
        ),
        (
            mysql.INTEGER(unsigned=True),
            mysql.INTEGER(unsigned=True, display_width=10),
            False,
        ),

This did show the bug. My problem is that it showed the bug on case 1 and 3 while 2 and 4 passed. This is exactly the opposite of what I expected with the bug report. Turns out no matter what I do the inspected columns were never coming back with a display_width setting even while setting it directly...while this bug report suggests that it was getting that setting even if it wasn't set.

What is your setup? sqlalchemy, mysql, and driver versions, I mean? Its possible that the testing isn't working the way I think it does :/

@peterschutt
Copy link
Author

Hi @pbecotte - I generated the revision with pymysql 0.9.3, sqla 1.3.13 and MySQL 5.7 although we actually run the Percona XtraDB Cluster version of MySQL (https://www.percona.com/doc/percona-xtradb-cluster/5.7/overview.html). I'm really sorry I didn't mention those configuration details in the issue report, especially if the percona back end is the root cause of the issue.

@peterschutt
Copy link
Author

mysql> show variables like "version%";
+-------------------------+-------------------------------------------------------------------------------------------------+
| Variable_name           | Value                                                                                           |
+-------------------------+-------------------------------------------------------------------------------------------------+
| version                 | 5.7.26-29-57-log                                                                                |
| version_comment         | Percona XtraDB Cluster (GPL), Release rel29, Revision 03540a3, WSREP version 31.37, wsrep_31.37 |
| version_compile_machine | x86_64                                                                                          |
| version_compile_os      | debian-linux-gnu                                                                                |
| version_suffix          | -57-log                                                                                         |
+-------------------------+-------------------------------------------------------------------------------------------------+
5 rows in set (0.01 sec)

@pbecotte
Copy link
Collaborator

No it's a definite bug, just want to make sure I can get the behavior to match so I am sure it is all the way fixed! Thanks for the quick update

@pbecotte
Copy link
Collaborator

Okay, display_width was deprecated in 5.7 and I ran the tests using 8. In 5.7 I was able to duplicate the bug exactly as I expected to, so will be able to work on that fix.

@zzzeek
Copy link
Member

zzzeek commented Feb 24, 2020

yeah i can reproduce easily. also i notice the testing fixtures are broken "AttributeError: module 'alembic.util' has no attribute 'raise_from_cause'" so let me fix that in master

@sqla-tester
Copy link
Collaborator

Paul Becotte has proposed a fix for this issue in the master branch:

Include post-parenthesis tokens when tokenizing type string https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/1739

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants