Skip to content

Compare Enum types #11

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jan 25, 2018
Merged

Compare Enum types #11

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1f1212c
Add comparison of SQLAlchemy Enums
Jul 15, 2017
7c5505c
Add tests for enum comparisons
Jul 15, 2017
8c59af8
Add ignores for the differing enum types
charness Jan 15, 2018
0f8e69c
Address version and dialect differences
charness Jan 15, 2018
b16a525
Expect comments in errors when supported
charness Jan 20, 2018
411c8f8
Adapt tests' native enums for SQLAlchemy 1.0
charness Jan 22, 2018
b67fa24
Exempt get_check_constraints protection from coverage
charness Jan 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 68 additions & 1 deletion sqlalchemydiff/comparer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,12 @@ def compare(left_uri, right_uri, ignores=None, ignores_sep=None):
tables_info.common, left_inspector, right_inspector, ignore_manager
)

info['enums'] = _get_enums_info(
left_inspector,
right_inspector,
ignore_manager.get('*', 'enum'),
)

errors = _compile_errors(info)
result = _make_result(info, errors)

Expand Down Expand Up @@ -161,6 +167,7 @@ def _get_info_dict(left_uri, right_uri, tables_info):
'common': tables_info.common,
},
'tables_data': {},
'enums': {},
}

return info
Expand Down Expand Up @@ -214,6 +221,13 @@ def _get_table_data(
ignore_manager.get(table_name, 'col')
)

table_data['constraints'] = _get_constraints_info(
left_inspector,
right_inspector,
table_name,
ignore_manager.get(table_name, 'cons')
)

return table_data


Expand Down Expand Up @@ -335,6 +349,53 @@ def _get_columns(inspector, table_name):
return inspector.get_columns(table_name)


def _get_constraints_info(left_inspector, right_inspector,
table_name, ignores):
left_constraints_list = _get_constraints_data(left_inspector, table_name)
right_constraints_list = _get_constraints_data(right_inspector, table_name)

left_constraints_list = _discard_ignores_by_name(left_constraints_list,
ignores)
right_constraints_list = _discard_ignores_by_name(right_constraints_list,
ignores)

# process into dict
left_constraints = dict((elem['name'], elem)
for elem in left_constraints_list)
right_constraints = dict((elem['name'], elem)
for elem in right_constraints_list)

return _diff_dicts(left_constraints, right_constraints)


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

except NotImplementedError:
return []


def _get_enums_info(left_inspector, right_inspector, ignores):
left_enums_list = _get_enums_data(left_inspector)
right_enums_list = _get_enums_data(right_inspector)

left_enums_list = _discard_ignores_by_name(left_enums_list, ignores)
right_enums_list = _discard_ignores_by_name(right_enums_list, ignores)

# process into dict
left_enums = dict((elem['name'], elem) for elem in left_enums_list)
right_enums = dict((elem['name'], elem) for elem in right_enums_list)

return _diff_dicts(left_enums, right_enums)


def _get_enums_data(inspector):
try:
return inspector.get_enums()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

except AttributeError:
return []


def _discard_ignores_by_name(items, ignores):
return [item for item in items if item['name'] not in ignores]

Expand Down Expand Up @@ -364,6 +425,7 @@ def _compile_errors(info):
errors_template = {
'tables': {},
'tables_data': {},
'enums': {},
}
errors = deepcopy(errors_template)

Expand All @@ -375,7 +437,8 @@ def _compile_errors(info):
errors['tables']['right_only'] = info['tables']['right_only']

# then check if there is a discrepancy in the data for each table
keys = ['foreign_keys', 'primary_keys', 'indexes', 'columns']
keys = ['foreign_keys', 'primary_keys', 'indexes', 'columns',
'constraints']
subkeys = ['left_only', 'right_only', 'diff']

for table_name in info['tables_data']:
Expand All @@ -386,6 +449,10 @@ def _compile_errors(info):
table_d.setdefault(key, {})[subkey] = info[
'tables_data'][table_name][key][subkey]

for subkey in subkeys:
if info['enums'][subkey]:
errors['enums'][subkey] = info['enums'][subkey]

if errors != errors_template:
errors['uris'] = info['uris']
return errors
Expand Down
2 changes: 1 addition & 1 deletion sqlalchemydiff/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def prepare_schema_from_models(uri, sqlalchemy_base):

class IgnoreManager:

allowed_identifiers = ['pk', 'fk', 'idx', 'col']
allowed_identifiers = ['pk', 'fk', 'idx', 'col', 'cons', 'enum']

def __init__(self, ignores, separator=None):
self.separator = separator or '.'
Expand Down
11 changes: 10 additions & 1 deletion test/endtoend/models_left.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
# -*- coding: utf-8 -*-
from sqlalchemy import Column, ForeignKey, Integer, String, Unicode
import enum

from sqlalchemy import Column, Enum, ForeignKey, Integer, String, Unicode
from sqlalchemy.ext.declarative import declarative_base


Base = declarative_base()


class Polarity(enum.Enum):
NEGATIVE = 'NEGATIVE'
POSITIVE = 'POSITIVE'


class Employee(Base):
__tablename__ = "employees"

Expand All @@ -14,6 +21,8 @@ class Employee(Base):
age = Column(Integer, nullable=False, default=21)
ssn = Column(Unicode(30), nullable=False)
number_of_pets = Column(Integer, default=1, nullable=False)
polarity = Column(Enum(Polarity, native_enum=True))
spin = Column(Enum('spin_down', 'spin_up', native_enum=False))

company_id = Column(
Integer,
Expand Down
11 changes: 10 additions & 1 deletion test/endtoend/models_right.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
# -*- coding: utf-8 -*-
from sqlalchemy import Column, ForeignKey, Integer, String, Unicode
import enum

from sqlalchemy import Column, Enum, ForeignKey, Integer, String, Unicode
from sqlalchemy.ext.declarative import declarative_base


Base = declarative_base()


class Polarity(enum.Enum):
NEG = 'NEG'
POS = 'POS'


class Employee(Base):
__tablename__ = "employees"

Expand All @@ -14,6 +21,8 @@ class Employee(Base):
age = Column(Integer, nullable=False, default=21)
ssn = Column(Unicode(30), nullable=False)
number_of_pets = Column(Integer, default=1, nullable=False)
polarity = Column(Enum(Polarity, native_enum=True))
spin = Column(Enum('down', 'up', native_enum=False))

company_id = Column(
Integer,
Expand Down
39 changes: 39 additions & 0 deletions test/endtoend/test_example.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,39 @@ def test_errors_dict_catches_all_differences(uri_left, uri_right):
}
},
'employees': {
'columns': {
'diff': [
{
'key': 'polarity',
'left': {
'default': None,
'name': 'polarity',
'nullable': True,
'type': "ENUM('NEGATIVE','POSITIVE')"},
'right': {
'default': None,
'name': 'polarity',
'nullable': True,
'type': "ENUM('NEG','POS')"
}
},
{
'key': 'spin',
'left': {
'default': None,
'name': 'spin',
'nullable': True,
'type': 'VARCHAR(9)'
},
'right': {
'default': None,
'name': 'spin',
'nullable': True,
'type': 'VARCHAR(4)'
}
}
]
},
'foreign_keys': {
'left_only': [
{
Expand Down Expand Up @@ -215,6 +248,8 @@ def test_errors_dict_catches_all_differences(uri_left, uri_right):
}
}
},
'enums': {
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'uris': {
'left': uri_left,
'right': uri_right,
Expand Down Expand Up @@ -299,6 +334,8 @@ def test_ignores(uri_left, uri_right):
'phone_numbers',
'companies.col.name',
'companies.idx.name',
'employees.col.polarity',
'employees.col.spin',
'employees.fk.fk_employees_companies',
'employees.fk.fk_emp_comp',
'employees.idx.ix_employees_name',
Expand Down Expand Up @@ -330,6 +367,8 @@ def test_ignores_alternative_sep(uri_left, uri_right):
'phone_numbers',
'companies#col#name',
'companies#idx#name',
'employees#col#polarity',
'employees#col#spin',
'employees#fk#fk_employees_companies',
'employees#fk#fk_emp_comp',
'employees#idx#ix_employees_name',
Expand Down
Loading