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

False change for unique constraint with case-sensitive names in 1.4.0 #654

Closed
ods opened this issue Feb 6, 2020 · 6 comments
Closed

False change for unique constraint with case-sensitive names in 1.4.0 #654

ods opened this issue Feb 6, 2020 · 6 comments

Comments

@ods
Copy link

@ods ods commented Feb 6, 2020

PostgreSQL, a unique constraint with uppercase letters in name. _compare_indexes_and_uniques takes name as _constraint_sig.name (just the name without quotes) for constraint from connection, but as _constraint_sig.md_name_to_sql_name(context) (gives the name in double quotes) for constraint from metadata. That causes false changes detection with script that drops and creates the same constraint.

The following change solves the problem:

--- a/alembic/autogenerate/compare.py
+++ b/alembic/autogenerate/compare.py
@@ -541,7 +541,7 @@ def _compare_indexes_and_uniques(
     conn_uniques_by_name = dict((c.name, c) for c in conn_unique_constraints)
     conn_indexes_by_name = dict((c.name, c) for c in conn_indexes)
     conn_names = dict(
-        (c.name, c)
+        (c.md_name_to_sql_name(autogen_context), c)
         for c in conn_unique_constraints.union(conn_indexes)
         if c.name is not None
     )
@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Feb 6, 2020

Ok there's no test case for that so need to add one.

@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Feb 6, 2020

I can't reproduce [edit: got it, doesnt happen on SQLA master]. Can you provide a reproduction case? I think just a CREATE TABLE that is generating the false positives should be enough clue.

    def test_nothing_changed_uq_w_mixed_case_name(self):
        m1 = MetaData()
        m2 = MetaData()

        Table(
            "nothing_changed",
            m1,
            Column("id", Integer, primary_key=True),
            Column("x", Integer),
            UniqueConstraint("x", name="SomeConstraint"),
            mysql_engine="InnoDB",
        )

        Table(
            "nothing_changed",
            m2,
            Column("id", Integer, primary_key=True),
            Column("x", Integer),
            UniqueConstraint("x", name="SomeConstraint"),
            mysql_engine="InnoDB",
        )
        diffs = self._fixture(m1, m2)
        eq_(diffs, [])

in PG the table is created as:

CREATE TABLE nothing_changed (
	id SERIAL NOT NULL, 
	x INTEGER, 
	PRIMARY KEY (id), 
	CONSTRAINT "SomeConstraint" UNIQUE (x)
)

@sqla-tester

This comment has been minimized.

Copy link
Collaborator

@sqla-tester sqla-tester commented Feb 6, 2020

Mike Bayer has proposed a fix for this issue in the master branch:

Handle mixed case identifiers correctly. https://gerrit.sqlalchemy.org/1709

@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Feb 6, 2020

that's just my test, will run it against multiple PG versions to confirm no issue so far.

@zzzeek zzzeek added needinfo and removed needinfo labels Feb 6, 2020
@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Feb 6, 2020

oh, well duh, the whole issue that was fixed with that logic didn't happen in master :) k we're good thanks

@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Feb 6, 2020

different fix though. names aren't supposed to be quoted here.

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.