Compare Enum types #11

charness · 2017-07-17T23:30:04Z

Add checks for changes to SQLAlchemy Enums, implemented as native enum types and/or as check constraints.

In our use case, we failed to catch missing migrations for enum columns.

I've verified these changes locally against PostgreSQL, as well, but haven't yet found a clean way to abstract the differences in expected_errors for MySQL vs PostgreSQL.

charness · 2017-07-17T23:30:33Z

Makefile

@@ -3,7 +3,7 @@
 test: flake8 pylint pytest

 pylint:
-	pylint sqlalchemydiff -E
+	pylint sqlalchemydiff -E --disable=E1102


without:

************* Module sqlalchemydiff.comparer
E:137,22: inspector.get_table_names is not callable (not-callable)
E:200, 8: ignore_manager.get is not callable (not-callable)
E:207, 8: ignore_manager.get is not callable (not-callable)
E:214, 8: ignore_manager.get is not callable (not-callable)
E:221, 8: ignore_manager.get is not callable (not-callable)
E:228, 8: ignore_manager.get is not callable (not-callable)
E:289,11: inspector.get_foreign_keys is not callable (not-callable)
E:309,11: inspector.get_primary_keys is not callable (not-callable)
E:327,11: inspector.get_indexes is not callable (not-callable)
E:349,11: inspector.get_columns is not callable (not-callable)
E:373,15: inspector.get_check_constraints is not callable (not-callable)
E:394,15: inspector.get_enums is not callable (not-callable)
E:420,11: type_.compile is not callable (not-callable)
E:448,30: errors['tables_data'].setdefault is not callable (not-callable)
E:449,20: table_d.setdefault is not callable (not-callable)
************* Module sqlalchemydiff.util
E: 71,12: stream.write is not callable (not-callable)
E: 98,14: uri.rsplit is not callable (not-callable)
E:106, 4: sqlalchemy_base.metadata.create_all is not callable (not-callable)
E:124,27: data.strip is not callable (not-callable)
E:138,15: data.count is not callable (not-callable)
E:145,15: data.split is not callable (not-callable)
E:152,16: item.strip is not callable (not-callable)
E:152,41: data.split is not callable (not-callable)

mattbennett · 2017-11-15T15:49:33Z

@charness this looks awesome! Sorry to leave this PR hanging for such a long time.

I've just updated the travis configuration so the tests run on their new infrastructure. Do you want to re-up this branch so we can get it merged?

Compare enum types and/or check constraints where supported by the database.

Besides the test changes themselves, - Update mysql-connector-python to 2.1.6 that's currently available from Oracle. - Suppress "not-callable" errors from pylint. - Fix flake8 indentation errors.

charness · 2017-11-16T00:49:07Z

Thanks for the update. It was an easy rebase.

timbu

@charness This looks great! Sorry it's taken me so long to get to it.

There's just a couple of suggestions and we have a failing test.

timbu · 2018-01-11T15:05:45Z

sqlalchemydiff/comparer.py

+
+def _get_constraints_data(inspector, table_name):
+    try:
+        return inspector.get_check_constraints(table_name)


This is new in sqlalchemy 1.1.0 - could you add the sqlalchemy>=1.1.0 requirement to setup.py?

Maybe, instead, catch AttributeError alongside NotImplementedError and return the same empty list? (A comment mentioning 1.1.0, similar to the one you request below for get_enums, would make sense.)

I think better to leave the sqlalchemy version unconstrained if doing so requires so little.

ah yes - good idea! - let's do the error catching instead.

timbu · 2018-01-11T17:58:39Z

test/endtoend/test_example.py

@@ -215,6 +248,8 @@ def test_errors_dict_catches_all_differences(uri_left, uri_right):
                }
            }
        },
+        'enums': {
+        },


I'm seeing an error when running this test:

test/endtoend/test_example.py:261: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ test/endtoend/test_example.py:297: in compare_error_dicts assert_items_equal(walk_dict(err1, path), walk_dict(err2, path)) ../../.pyenv/versions/3.4.4/lib/python3.4/unittest/case.py:1153: in assertCountEqual self.fail(msg) _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <unittest.case.TestCase testMethod=runTest> msg = "Element counts were not equal:\nFirst has 1, Second has 0: {'key': 'name', 'right': {'type': 'VARCHAR(200)', 'name':...llable': True}, 'left': {'type': 'VARCHAR(200)', 'name': 'name', 'default': None, 'comment': None, 'nullable': False}}" def fail(self, msg=None): """Fail immediately, with the given message.""" > raise self.failureException(msg) E AssertionError: Element counts were not equal: E First has 1, Second has 0: {'key': 'name', 'right': {'type': 'VARCHAR(200)', 'name': 'name', 'default': None, 'nullable': True}, 'left': {'type': 'VARCHAR(200)', 'name': 'name', 'default': None, 'nullable': False}} E First has 0, Second has 1: {'key': 'name', 'right': {'type': 'VARCHAR(200)', 'name': 'name', 'default': None, 'comment': None, 'nullable': True}, 'left': {'type': 'VARCHAR(200)', 'name': 'name', 'default': None, 'comment': None, 'nullable': False}}

It looks like we are now getting an extra 'comment': None field - is this expected?

(TIL) Comment support is new in SQLAlchemy 1.2.0: http://docs.sqlalchemy.org/en/latest/changelog/migration_12.html#change-1546

The change adds a dialect.supports_comments.

I think the most straightforward way to go here is to key off that attribute (e.g., getattr(dialect, 'supports_comments', False)) and add 'comment': None to the expected errors when supported.

Perhaps, though, test_errors_dict_catches_all_differences should compare diff results more selectively? Check that a given key's value differs as expected between left and right, and that the rest match. That would ignore the matching, but unexpected, "comment".

That selectivity should also help make the tests work for a dialect other than mysql (e.g., postgresql), though left_only and right_only would still fail without further work.

Your thoughts?

Agree it would be better to be more selective in the assertions rather than comparing the whole result. But if that's too big a change, your first suggestion will do the job well enough for now.

For now, I've implemented the simpler change. I didn't want the more-directed tests to hold up this PR.

timbu · 2018-01-11T18:02:10Z

sqlalchemydiff/comparer.py

+
+def _get_enums_data(inspector):
+    try:
+        return inspector.get_enums()


perhaps we could add a comment here mentioning that this is specific to the postgres inspector?

Explain the differences in comments and tolerate via try...except.

SQLAlchemy 1.2.0 added support for SQL comments. Since the test example models include no comments, these come back as None in the inspection output. When the dialect indicates it `supports_comments` (a new attribute in 1.2.0), add `"comment": None` to each column in the expected_errors.

SQLAlchemy supports PEP 435 Enum classes as of 1.1. In order to exercise get_check_constraints-related code aimed at < 1.1.0, adapt the Polarity (native enum) columns to the 1.0 Enum API when using 1.0.

Testing with sqlalchemy 1.2 and mysql raises neither the AttributeError nor the NotImplementedError in _get_constraints_data. Disable coverage checking for the except clause so tests can pass.

mattbennett · 2018-01-25T11:18:52Z

Thanks @charness for the contribution! I'll release this in the next cut

charness commented Jul 17, 2017

View reviewed changes

Dave Charness added 2 commits November 15, 2017 16:29

Add comparison of SQLAlchemy Enums

1f1212c

Compare enum types and/or check constraints where supported by the database.

Add tests for enum comparisons

7c5505c

Besides the test changes themselves, - Update mysql-connector-python to 2.1.6 that's currently available from Oracle. - Suppress "not-callable" errors from pylint. - Fix flake8 indentation errors.

charness force-pushed the master branch from b80ddb6 to 7c5505c Compare November 16, 2017 00:44

mattbennett requested a review from timbu November 17, 2017 16:48

timbu suggested changes Jan 11, 2018

View reviewed changes

charness added 5 commits January 22, 2018 10:37

Add ignores for the differing enum types

8c59af8

Address version and dialect differences

0f8e69c

Explain the differences in comments and tolerate via try...except.

Adapt tests' native enums for SQLAlchemy 1.0

411c8f8

SQLAlchemy supports PEP 435 Enum classes as of 1.1. In order to exercise get_check_constraints-related code aimed at < 1.1.0, adapt the Polarity (native enum) columns to the 1.0 Enum API when using 1.0.

Exempt get_check_constraints protection from coverage

b67fa24

Testing with sqlalchemy 1.2 and mysql raises neither the AttributeError nor the NotImplementedError in _get_constraints_data. Disable coverage checking for the except clause so tests can pass.

timbu approved these changes Jan 24, 2018

View reviewed changes

mattbennett merged commit f54fe4c into gianchub:master Jan 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Compare Enum types #11

Compare Enum types #11

Uh oh!

charness commented Jul 17, 2017

Uh oh!

charness Jul 17, 2017

Uh oh!

mattbennett commented Nov 15, 2017

Uh oh!

charness commented Nov 16, 2017

Uh oh!

timbu left a comment

Uh oh!

timbu Jan 11, 2018

Uh oh!

charness Jan 14, 2018

Uh oh!

timbu Jan 15, 2018

Uh oh!

timbu Jan 11, 2018

Uh oh!

charness Jan 14, 2018

Uh oh!

timbu Jan 15, 2018

Uh oh!

charness Jan 22, 2018

Uh oh!

timbu Jan 11, 2018

Uh oh!

mattbennett commented Jan 25, 2018

Uh oh!

Uh oh!

Compare Enum types #11

Compare Enum types #11

Uh oh!

Conversation

charness commented Jul 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattbennett commented Nov 15, 2017

Uh oh!

charness commented Nov 16, 2017

Uh oh!

timbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattbennett commented Jan 25, 2018

Uh oh!

Uh oh!