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 a0aecf3679da8772a6fd5260a425d87a5e69f87a..b68f4aa29e7e0bf0b36bb4cecd607a7215efa9f2 100644 (file)
--- 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 719178f7ef4a3d8503123a9bc0ab8e22ca07d2db..ea3476295d81a361ee47fb3eb1d75911046ad11c 100644 (file)
--- 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 5d11bf990abfdba7be068a1dc41bd958b97e3f5a..d1c9a22ac28a412b22980e2dd1068b415970e762 100644 (file)
--- 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 27ef9e01b134d588ce087c33c85c1031094e9929..5c48e5bbb5c054745280f3dfac7265e1eb9af4e2 100644 (file)
--- 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=<someengine>")
             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 62568eb4a4fd6e8170b0793768d2ec313786e54c..869fd63e57ff283ac44ffe75b023e06aa203ee79 100644 (file)
--- 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 b782e597c484a4e09e586f83735e189067781f23..789d539a4861ac68484a7e293f99dfd94aa40922 100644 (file)
--- 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(