From: Mike Bayer Date: Wed, 23 Nov 2016 14:14:02 +0000 (-0500) Subject: Add _extend_on deduplicating set for metadata.reflect() X-Git-Tag: rel_1_1_5~31 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=55ad10370f9eb50795e136aac595193168982e92;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Add _extend_on deduplicating set for metadata.reflect() The "extend_existing" option of :class:`.Table` reflection would cause indexes and constraints to be doubled up in the case that the parameter were used with :meth:`.MetaData.reflect` (as the automap extension does) due to tables being reflected both within the foreign key path as well as directly. A new de-duplicating set is passed through within the :meth:`.MetaData.reflect` sequence to prevent double reflection in this way. Change-Id: Ibf6650c1e76a44ccbe15765fd79df2fa53d6bac7 Fixes: #3861 --- diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst index a0aecf3679..b68f4aa29e 100644 --- a/doc/build/changelog/changelog_11.rst +++ b/doc/build/changelog/changelog_11.rst @@ -28,6 +28,18 @@ datatypes now support a setting of zero for "precision"; previously a zero would be ignored. Pull request courtesy Ionuț Ciocîrlan. + .. change:: 3861 + :tags: bug, engine + :tickets: 3861 + + The "extend_existing" option of :class:`.Table` reflection would + cause indexes and constraints to be doubled up in the case that the parameter + were used with :meth:`.MetaData.reflect` (as the automap extension does) + due to tables being reflected both within the foreign key path as well + as directly. A new de-duplicating set is passed through within the + :meth:`.MetaData.reflect` sequence to prevent double reflection in this + way. + .. change:: 3859 :tags: bug, sql :tickets: 3859 diff --git a/lib/sqlalchemy/engine/default.py b/lib/sqlalchemy/engine/default.py index 719178f7ef..ea3476295d 100644 --- a/lib/sqlalchemy/engine/default.py +++ b/lib/sqlalchemy/engine/default.py @@ -366,9 +366,10 @@ class DefaultDialect(interfaces.Dialect): return sqltypes.adapt_type(typeobj, self.colspecs) def reflecttable( - self, connection, table, include_columns, exclude_columns): + self, connection, table, include_columns, exclude_columns, **opts): insp = reflection.Inspector.from_engine(connection) - return insp.reflecttable(table, include_columns, exclude_columns) + return insp.reflecttable( + table, include_columns, exclude_columns, **opts) def get_pk_constraint(self, conn, table_name, schema=None, **kw): """Compatibility method, adapts the result of get_primary_keys() diff --git a/lib/sqlalchemy/engine/reflection.py b/lib/sqlalchemy/engine/reflection.py index 5d11bf990a..d1c9a22ac2 100644 --- a/lib/sqlalchemy/engine/reflection.py +++ b/lib/sqlalchemy/engine/reflection.py @@ -532,7 +532,8 @@ class Inspector(object): return self.dialect.get_check_constraints( self.bind, table_name, schema, info_cache=self.info_cache, **kw) - def reflecttable(self, table, include_columns, exclude_columns=()): + def reflecttable(self, table, include_columns, exclude_columns=(), + _extend_on=None): """Given a Table object, load its internal constructs based on introspection. @@ -553,6 +554,13 @@ class Inspector(object): in the reflection process. If ``None``, all columns are reflected. """ + + if _extend_on is not None: + if table in _extend_on: + return + else: + _extend_on.add(table) + dialect = self.bind.dialect schema = self.bind.schema_for_object(table) @@ -602,7 +610,7 @@ class Inspector(object): self._reflect_fk( table_name, schema, table, cols_by_orig_name, - exclude_columns, reflection_options) + exclude_columns, _extend_on, reflection_options) self._reflect_indexes( table_name, schema, table, cols_by_orig_name, @@ -692,7 +700,7 @@ class Inspector(object): def _reflect_fk( self, table_name, schema, table, cols_by_orig_name, - exclude_columns, reflection_options): + exclude_columns, _extend_on, reflection_options): fkeys = self.get_foreign_keys( table_name, schema, **table.dialect_kwargs) for fkey_d in fkeys: @@ -715,6 +723,7 @@ class Inspector(object): sa_schema.Table(referred_table, table.metadata, autoload=True, schema=referred_schema, autoload_with=self.bind, + _extend_on=_extend_on, **reflection_options ) for column in referred_columns: @@ -724,6 +733,7 @@ class Inspector(object): sa_schema.Table(referred_table, table.metadata, autoload=True, autoload_with=self.bind, schema=sa_schema.BLANK_SCHEMA, + _extend_on=_extend_on, **reflection_options ) for column in referred_columns: diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py index 27ef9e01b1..5c48e5bbb5 100644 --- a/lib/sqlalchemy/sql/schema.py +++ b/lib/sqlalchemy/sql/schema.py @@ -485,6 +485,8 @@ class Table(DialectKWArgs, SchemaItem, TableClause): autoload = kwargs.pop('autoload', autoload_with is not None) # this argument is only used with _init_existing() kwargs.pop('autoload_replace', True) + _extend_on = kwargs.pop("_extend_on", None) + include_columns = kwargs.pop('include_columns', None) self.implicit_returning = kwargs.pop('implicit_returning', True) @@ -504,19 +506,22 @@ class Table(DialectKWArgs, SchemaItem, TableClause): # we do it after the table is in the singleton dictionary to support # circular foreign keys if autoload: - self._autoload(metadata, autoload_with, include_columns) + self._autoload( + metadata, autoload_with, + include_columns, _extend_on=_extend_on) # initialize all the column, etc. objects. done after reflection to # allow user-overrides self._init_items(*args) def _autoload(self, metadata, autoload_with, include_columns, - exclude_columns=()): + exclude_columns=(), _extend_on=None): if autoload_with: autoload_with.run_callable( autoload_with.dialect.reflecttable, - self, include_columns, exclude_columns + self, include_columns, exclude_columns, + _extend_on=_extend_on ) else: bind = _bind_or_error( @@ -528,7 +533,8 @@ class Table(DialectKWArgs, SchemaItem, TableClause): "metadata.bind=") bind.run_callable( bind.dialect.reflecttable, - self, include_columns, exclude_columns + self, include_columns, exclude_columns, + _extend_on=_extend_on ) @property @@ -557,6 +563,8 @@ class Table(DialectKWArgs, SchemaItem, TableClause): autoload = kwargs.pop('autoload', autoload_with is not None) autoload_replace = kwargs.pop('autoload_replace', True) schema = kwargs.pop('schema', None) + _extend_on = kwargs.pop('_extend_on', None) + if schema and schema != self.schema: raise exc.ArgumentError( "Can't change schema of existing table from '%s' to '%s'", @@ -579,12 +587,15 @@ class Table(DialectKWArgs, SchemaItem, TableClause): if autoload: if not autoload_replace: + # don't replace columns already present. + # we'd like to do this for constraints also however we don't + # have simple de-duping for unnamed constraints. exclude_columns = [c.name for c in self.c] else: exclude_columns = () self._autoload( self.metadata, autoload_with, - include_columns, exclude_columns) + include_columns, exclude_columns, _extend_on=_extend_on) self._extra_kwargs(**kwargs) self._init_items(*args) @@ -3758,7 +3769,8 @@ class MetaData(SchemaItem): 'autoload': True, 'autoload_with': conn, 'extend_existing': extend_existing, - 'autoload_replace': autoload_replace + 'autoload_replace': autoload_replace, + '_extend_on': set() } reflect_opts.update(dialect_kwargs) diff --git a/test/engine/test_reflection.py b/test/engine/test_reflection.py index 62568eb4a4..869fd63e57 100644 --- a/test/engine/test_reflection.py +++ b/test/engine/test_reflection.py @@ -1,7 +1,8 @@ import unicodedata import sqlalchemy as sa from sqlalchemy import schema, inspect -from sqlalchemy import MetaData, Integer, String +from sqlalchemy import MetaData, Integer, String, Index, ForeignKey, \ + UniqueConstraint from sqlalchemy.testing import ( ComparesTables, engines, AssertsCompiledSQL, fixtures, skip) @@ -201,6 +202,39 @@ class ReflectionTest(fixtures.TestBase, ComparesTables): assert t4.c.z.type._type_affinity is String assert t4.c.q is old_q + @testing.provide_metadata + def test_extend_existing_reflect_all_dont_dupe_index(self): + m = self.metadata + d = Table( + "d", m, Column('id', Integer, primary_key=True), + Column('foo', String(50)), + Column('bar', String(50)), + UniqueConstraint('bar') + ) + Index("foo_idx", d.c.foo) + Table( + "b", m, Column('id', Integer, primary_key=True), + Column('aid', ForeignKey('d.id')) + ) + m.create_all() + + m2 = MetaData() + m2.reflect(testing.db, extend_existing=True) + + eq_( + len([idx for idx in m2.tables['d'].indexes + if idx.name == 'foo_idx']), + 1 + ) + if testing.requires.\ + unique_constraint_reflection_no_index_overlap.enabled: + eq_( + len([ + const for const in m2.tables['d'].constraints + if isinstance(const, UniqueConstraint)]), + 1 + ) + @testing.emits_warning(r".*omitted columns") @testing.provide_metadata def test_include_columns_indexes(self): diff --git a/test/requirements.py b/test/requirements.py index b782e597c4..789d539a48 100644 --- a/test/requirements.py +++ b/test/requirements.py @@ -319,6 +319,10 @@ class DefaultRequirements(SuiteRequirements): "sqlite" ) + @property + def unique_constraint_reflection_no_index_overlap(self): + return self.unique_constraint_reflection + skip_if("mysql") + @property def check_constraint_reflection(self): return fails_on_everything_except(