Table.tometadata() foreign key copying behavior with different schemas
I think the behavior of Table.tometadata()'s copying of foreign keys is unexpected when it comes to copying foreign keys that reference tables in a schema different than the schema of the table being copied. This occurred to me in the discussion at https://groups.google.com/forum/#!topic/sqlalchemy-alembic/FA8wpmPJc7U
For instance, when we have two tables in different schemas
m = Metadata()
t = Table('t', m,
Column('x', Integer, ForeignKey('a.x')),
schema="shared")
t1 = Table('t1', m,
Column('t_x', Integer, ForeignKey('shared.t.x')),
schema="schema1")
m2 = Metadata()
t1_copy = t1.tometadata(m2)
t1_copy_schema2 = t1.tometadata(m2, schema="schema2")
After this, t1_copy.t_x
now references a column schema1.t.x
, while t1_copy_schema2.t_x
references a schema2.t.x
, both of which are wrong.
I think the correct behavior would be to only modify the schema of the foreign key's reference only if that schema is the same as the table's original schema. To be more precise:
1) If referenced_schema == previous_schema, update as is the current behavior 2) If referenced_schema != previous_schema, then need to decide how/if to update the referenced column's schema. I would leave the schema unchanged, or pass a dictionary that maps previous schemas to the new ones.
In code, something like this:
def tometadata(self, metadata, schema=RETAIN_SCHEMA, fk_schema_replace={}):
# ....
#section on constraints copy
previous_schema = self.schema
#by default, modify FK's schema if the schema is the same as the table's
if previous_schema not in fk_schema_replace:
fk_schema_replace.update({previous_schema: schema})
for c in self.constraints:
constraint_schema = schema
if isinstance(c, ForeignKeyConstraint):
fk_schema = c.elements[0](0).column.table.schema
constraint_schema = fk_schema_replace.get(fk_schema)
#if a replacement is found it is used, otherwise schema is unchanged
table.append_constraint(c.copy(schema=constraint_schema, target_table=table))
With this change, I can do
t1_copy = t1.tometadata(m2)
t1_copy_schema2 = t1.tometadata(m2, schema="schema2", fk_schema_replace={'shared':'schema2'})
Then t1_copy.t_x
will reference shared.t.x
, while t1_copy_schema2.t_x
will reference schema2.t.x
Thoughts?
Comments (5)
-
reporter -
repo owner yeah I had thought it already worked that way in that it wouldn't change the schema of a referenced table with an already different schema, however I think what i was likely remembering was logic that's in Postgresql's reflection system specifically, to deal with the fact that PG won't actually tell us the schema of an FK if that schema is in the current search path.
the second thought is that the change here, while I think that's how it should be, is not backwards compatible. So I'd want to check two things - one is that it has always worked this way, e.g. the current system isn't there for any particular reason, and two is that there's no tests for this behavior right now, which would kind of imply we've just never considered/supported this use. At some point we received a significant patch that added the "schema" argument to MetaData in the first place, and the patch went through a lot of review, so I'm surprised we never touched this aspect of it at all.
-
repo owner - changed milestone to 0.9.2
-
repo owner - changed status to resolved
both of these changes have been made in 2d4b457924fa722ef2cf. this includes the FK schema change based on the referenced schema being the same as the parent schema, as well as a new optional callable
referred_schema_fn
which can be provided to supply any kind of logic to how the referenced schemas should be assigned. -
repo owner - removed milestone
Removing milestone: 0.9.2 (automated comment)
- Log in to comment
Ticket contains some mistakes, but I can't find a way to edit it:
The original code should be
Now
t1_copy.t_x
now references a columnschema1.t.x
, whilet1_copy_schema2.t_x
references aschema2.t.x
, both of which are wrong.The proposed changed changes these results to
shared.t.x
andschema2.t.x
respectively Replying to dtheodor: