From acdeaf43f79e7f881e6a105bc3c5149266a3c580 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sat, 25 Feb 2012 18:04:40 -0500 Subject: [PATCH] almost through all the fine tuning --- lib/sqlalchemy/orm/relationships.py | 44 ++-- test/orm/test_rel_fn.py | 46 ++++- test/orm/test_relationships.py | 301 +++++++++++++--------------- 3 files changed, 213 insertions(+), 178 deletions(-) diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py index 76e219efee..adf3b0cde9 100644 --- a/lib/sqlalchemy/orm/relationships.py +++ b/lib/sqlalchemy/orm/relationships.py @@ -58,7 +58,7 @@ class JoinCondition(object): self_referential=False, prop=None, support_sync=True, - can_be_synced_fn=lambda c: True + can_be_synced_fn=lambda *c: True ): self.parent_selectable = parent_selectable self.parent_local_selectable = parent_local_selectable @@ -531,7 +531,8 @@ class JoinCondition(object): % (self.prop, )) def _check_foreign_cols(self, join_condition, primary): - """Check the foreign key columns collected and emit error messages.""" + """Check the foreign key columns collected and emit error + messages.""" can_sync = False @@ -554,17 +555,19 @@ class JoinCondition(object): # (not just ==), perhaps they need to turn on "viewonly=True". if self.support_sync and has_foreign and not can_sync: err = "Could not locate any simple equality expressions "\ - "involving foreign key columns for %s join condition "\ + "involving locally mapped foreign key columns for "\ + "%s join condition "\ "'%s' on relationship %s." % ( primary and 'primary' or 'secondary', join_condition, self.prop ) - err += " Ensure that referencing columns are associated with a "\ - "ForeignKey or ForeignKeyConstraint, or are annotated "\ - "in the join condition with the foreign() annotation. "\ - "To allow comparison operators other than '==', "\ - "the relationship can be marked as viewonly=True." + err += \ + " Ensure that referencing columns are associated "\ + "with a ForeignKey or ForeignKeyConstraint, or are "\ + "annotated in the join condition with the foreign() "\ + "annotation. To allow comparison operators other than "\ + "'==', the relationship can be marked as viewonly=True." raise sa_exc.ArgumentError(err) else: @@ -574,9 +577,11 @@ class JoinCondition(object): join_condition, self.prop ) - err += " Ensure that referencing columns are associated with a "\ - "ForeignKey or ForeignKeyConstraint, or are annotated "\ - "in the join condition with the foreign() annotation." + err += \ + ' Ensure that referencing columns are associated '\ + 'with a ForeignKey or ForeignKeyConstraint, or are '\ + 'annotated in the join condition with the foreign() '\ + 'annotation.' raise sa_exc.ArgumentError(err) def _determine_direction(self): @@ -617,11 +622,15 @@ class JoinCondition(object): self.direction = MANYTOONE else: raise sa_exc.ArgumentError( - "Can't determine relationship" - " direction for relationship '%s' - foreign " - "key columns are present in both the parent " - "and the child's mapped tables. Specify " - "'foreign_keys' argument." % self.prop) + "Can't determine relationship" + " direction for relationship '%s' - foreign " + "key columns within the join condition are present " + "in both the parent and the child's mapped tables. " + "Ensure that only those columns referring " + "to a parent column are marked as foreign, " + "either via the foreign() annotation or " + "via the foreign_keys argument." + % self.prop) elif onetomany_fk: self.direction = ONETOMANY elif manytoone_fk: @@ -647,7 +656,8 @@ class JoinCondition(object): "remote" not in right._annotations and \ self.can_be_synced_fn(right): lrp.add((right, left)) - if binary.operator is operators.eq: + if binary.operator is operators.eq and \ + self.can_be_synced_fn(left, right): if "foreign" in right._annotations: collection.append((left, right)) elif "foreign" in left._annotations: diff --git a/test/orm/test_rel_fn.py b/test/orm/test_rel_fn.py index 9fe6ce1706..e35dd925a6 100644 --- a/test/orm/test_rel_fn.py +++ b/test/orm/test_rel_fn.py @@ -86,6 +86,40 @@ class _JoinFixtures(object): Column('base_id', Integer, ForeignKey('base.id')) ) + cls.three_tab_a = Table('three_tab_a', m, + Column('id', Integer, primary_key=True), + ) + cls.three_tab_b = Table('three_tab_b', m, + Column('id', Integer, primary_key=True), + Column('aid', Integer, ForeignKey('three_tab_a.id')) + ) + cls.three_tab_c = Table('three_tab_c', m, + Column('id', Integer, primary_key=True), + Column('aid', Integer, ForeignKey('three_tab_a.id')), + Column('bid', Integer, ForeignKey('three_tab_b.id')) + ) + + def _join_fixture_overlapping_three_tables(self, **kw): + def _can_sync(*cols): + for c in cols: + if self.three_tab_c.c.contains_column(c): + return False + else: + return True + return relationships.JoinCondition( + self.three_tab_a, + self.three_tab_b, + self.three_tab_a, + self.three_tab_b, + support_sync=False, + can_be_synced_fn=_can_sync, + primaryjoin=and_( + self.three_tab_a.c.id==self.three_tab_b.c.aid, + self.three_tab_c.c.bid==self.three_tab_b.c.id, + self.three_tab_c.c.aid==self.three_tab_a.c.id + ) + ) + def _join_fixture_m2m(self, **kw): return relationships.JoinCondition( self.m2mleft, @@ -338,7 +372,7 @@ class _JoinFixtures(object): assert_raises_message( sa.exc.ArgumentError, "Could not locate any simple equality expressions " - "involving foreign key columns for %s join " + "involving locally mapped foreign key columns for %s join " "condition '%s' on relationship %s. " "Ensure that referencing columns are associated with a " "ForeignKey or ForeignKeyConstraint, or are annotated in " @@ -626,6 +660,16 @@ class ColumnCollectionsTest(_JoinFixtures, fixtures.TestBase, AssertsCompiledSQL set([self.selfref.c.id]) ) + def test_determine_local_remote_cols_three_tab_viewonly(self): + joincond = self._join_fixture_overlapping_three_tables() + eq_( + joincond.local_remote_pairs, + [(self.three_tab_a.c.id, self.three_tab_b.c.aid)] + ) + eq_( + joincond.remote_columns, + set([self.three_tab_b.c.id, self.three_tab_b.c.aid]) + ) class DirectionTest(_JoinFixtures, fixtures.TestBase, AssertsCompiledSQL): def test_determine_direction_compound_2(self): diff --git a/test/orm/test_relationships.py b/test/orm/test_relationships.py index 859aa5b4e0..5c273a6431 100644 --- a/test/orm/test_relationships.py +++ b/test/orm/test_relationships.py @@ -15,6 +15,103 @@ from test.lib import fixtures from test.orm import _fixtures from sqlalchemy import exc +class _RelationshipErrors(object): + def _assert_raises_no_relevant_fks(self, fn, expr, relname, + primary, *arg, **kw): + assert_raises_message( + sa.exc.ArgumentError, + "Could not locate any relevant foreign key columns " + "for %s join condition '%s' on relationship %s. " + "Ensure that referencing columns are associated with " + "a ForeignKey or ForeignKeyConstraint, or are annotated " + r"in the join condition with the foreign\(\) annotation." + % ( + primary, expr, relname + ), + fn, *arg, **kw + ) + + def _assert_raises_no_equality(self, fn, expr, relname, + primary, *arg, **kw): + assert_raises_message( + sa.exc.ArgumentError, + "Could not locate any simple equality expressions " + "involving locally mapped foreign key columns for %s join " + "condition '%s' on relationship %s. " + "Ensure that referencing columns are associated with a " + "ForeignKey or ForeignKeyConstraint, or are annotated in " + r"the join condition with the foreign\(\) annotation. " + "To allow comparison operators other than '==', " + "the relationship can be marked as viewonly=True." % ( + primary, expr, relname + ), + fn, *arg, **kw + ) + + def _assert_raises_ambig_join(self, fn, relname, secondary_arg, + *arg, **kw): + if secondary_arg is not None: + assert_raises_message( + exc.ArgumentError, + "Could not determine join condition between " + "parent/child tables on relationship %s - " + "there are multiple foreign key paths linking the " + "tables via secondary table '%s'. " + "Specify the 'foreign_keys' argument, providing a list " + "of those columns which should be counted as " + "containing a foreign key reference from the " + "secondary table to each of the parent and child tables." + % (relname, secondary_arg), + fn, *arg, **kw) + else: + assert_raises_message( + exc.ArgumentError, + "Could not determine join condition between " + "parent/child tables on relationship %s - " + "there are no foreign keys linking these tables. " + % (relname,), + fn, *arg, **kw) + + def _assert_raises_no_join(self, fn, relname, secondary_arg, + *arg, **kw): + if secondary_arg is not None: + assert_raises_message( + exc.NoForeignKeysError, + "Could not determine join condition between " + "parent/child tables on relationship %s - " + "there are no foreign keys linking these tables " + "via secondary table '%s'. " + "Ensure that referencing columns are associated with a ForeignKey " + "or ForeignKeyConstraint, or specify 'primaryjoin' and " + "'secondaryjoin' expressions" + % (relname, secondary_arg), + fn, *arg, **kw) + else: + assert_raises_message( + exc.NoForeignKeysError, + "Could not determine join condition between " + "parent/child tables on relationship %s - " + "there are no foreign keys linking these tables. " + "Ensure that referencing columns are associated with a ForeignKey " + "or ForeignKeyConstraint, or specify a 'primaryjoin' " + "expression." + % (relname,), + fn, *arg, **kw) + + def _assert_raises_ambiguous_direction(self, fn, relname, *arg, **kw): + assert_raises_message( + sa.exc.ArgumentError, + "Can't determine relationship" + " direction for relationship '%s' - foreign " + "key columns within the join condition are present " + "in both the parent and the child's mapped tables. " + "Ensure that only those columns referring to a parent column " + r"are marked as foreign, either via the foreign\(\) annotation or " + "via the foreign_keys argument." + % relname, + fn, *arg, **kw + ) + class DependencyTwoParentTest(fixtures.MappedTest): """Test flush() when a mapper is dependent on multiple relationships""" @@ -2043,7 +2140,7 @@ class InvalidRemoteSideTest(fixtures.MappedTest): "mean to set remote_side on the many-to-one side ?", configure_mappers) -class AmbiguousFKResolutionTest(fixtures.MappedTest): +class AmbiguousFKResolutionTest(_RelationshipErrors, fixtures.MappedTest): @classmethod def define_tables(cls, metadata): Table("a", metadata, @@ -2146,14 +2243,9 @@ class AmbiguousFKResolutionTest(fixtures.MappedTest): 'bs':relationship(B, secondary=a_to_b) }) mapper(B, b) - assert_raises_message( - sa.exc.NoForeignKeysError, - "Could not determine join condition between " - "parent/child tables on relationship A.bs - " - "there are no foreign keys linking these " - "tables via secondary table 'atob'. " - "Specify 'primaryjoin' and 'secondaryjoin' expressions.", - sa.orm.configure_mappers + self._assert_raises_no_join( + sa.orm.configure_mappers, + "A.bs", a_to_b, ) def test_ambiguous_fks_m2m(self): @@ -2186,7 +2278,8 @@ class AmbiguousFKResolutionTest(fixtures.MappedTest): mapper(B, b) sa.orm.configure_mappers() -class InvalidRelationshipEscalationTest(fixtures.MappedTest): + +class InvalidRelationshipEscalationTest(_RelationshipErrors, fixtures.MappedTest): @classmethod def define_tables(cls, metadata): @@ -2211,87 +2304,6 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest): class Bar(cls.Basic): pass - def _assert_raises_no_relevant_fks(self, fn, expr, relname, - primary, *arg, **kw): - assert_raises_message( - sa.exc.ArgumentError, - "Could not locate any relevant foreign key columns " - "for %s join condition '%s' on relationship %s. " - "Ensure that referencing columns are associated with " - "a ForeignKey or ForeignKeyConstraint, or are annotated " - r"in the join condition with the foreign\(\) annotation." - % ( - primary, expr, relname - ), - fn, *arg, **kw - ) - - def _assert_raises_no_equality(self, fn, expr, relname, - primary, *arg, **kw): - assert_raises_message( - sa.exc.ArgumentError, - "Could not locate any simple equality expressions " - "involving foreign key columns for %s join " - "condition '%s' on relationship %s. " - "Ensure that referencing columns are associated with a " - "ForeignKey or ForeignKeyConstraint, or are annotated in " - r"the join condition with the foreign\(\) annotation. " - "To allow comparison operators other than '==', " - "the relationship can be marked as viewonly=True." % ( - primary, expr, relname - ), - fn, *arg, **kw - ) - - def _assert_raises_ambig_join(self, fn, relname, secondary_arg, - *arg, **kw): - if secondary_arg is not None: - assert_raises_message( - exc.ArgumentError, - "Could not determine join condition between " - "parent/child tables on relationship %s - " - "there are multiple foreign key paths linking the " - "tables via secondary table '%s'. " - "Specify the 'foreign_keys' argument, providing a list " - "of those columns which should be counted as " - "containing a foreign key reference from the " - "secondary table to each of the parent and child tables." - % (relname, secondary_arg), - fn, *arg, **kw) - else: - assert_raises_message( - exc.ArgumentError, - "Could not determine join condition between " - "parent/child tables on relationship %s - " - "there are no foreign keys linking these tables. " - % (relname,), - fn, *arg, **kw) - - def _assert_raises_no_join(self, fn, relname, secondary_arg, - *arg, **kw): - if secondary_arg is not None: - assert_raises_message( - exc.NoForeignKeysError, - "Could not determine join condition between " - "parent/child tables on relationship %s - " - "there are no foreign keys linking these tables " - "via secondary table '%s'. " - "Ensure that referencing columns are associated with a ForeignKey " - "or ForeignKeyConstraint, or specify 'primaryjoin' and " - "'secondaryjoin' expressions" - % (relname, secondary_arg), - fn, *arg, **kw) - else: - assert_raises_message( - exc.NoForeignKeysError, - "Could not determine join condition between " - "parent/child tables on relationship %s - " - "there are no foreign keys linking these tables. " - "Ensure that referencing columns are associated with a ForeignKey " - "or ForeignKeyConstraint, or specify a 'primaryjoin' " - "expression." - % (relname,), - fn, *arg, **kw) def test_no_join(self): bars, Foo, Bar, foos = (self.tables.bars, @@ -2333,11 +2345,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest): primaryjoin=foos.c.id>bars.c.fid)}) mapper(Bar, bars) - assert_raises_message( - sa.exc.ArgumentError, - "Could not determine relationship direction " - "for primaryjoin condition", - configure_mappers) + self._assert_raises_no_relevant_fks( + configure_mappers, + "foos.id > bars.fid", "Foo.bars", "primary" + ) def test_no_equated_fks(self): bars, Foo, Bar, foos = (self.tables.bars, @@ -2350,15 +2361,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest): primaryjoin=foos.c.id>bars.c.fid, foreign_keys=bars.c.fid)}) mapper(Bar, bars) - - assert_raises_message( - sa.exc.ArgumentError, - "Could not locate any foreign-key-equated, " - "locally mapped column pairs for primaryjoin " - "condition 'foos.id > bars.fid' on relationship " - "Foo.bars. For more relaxed rules on join " - "conditions, the relationship may be marked as viewonly=True.", - sa.orm.configure_mappers) + self._assert_raises_no_equality( + sa.orm.configure_mappers, + "foos.id > bars.fid", "Foo.bars", "primary" + ) def test_no_equated_wo_fks_works_on_relaxed(self): foos_with_fks, Foo, Bar, bars_with_fks, foos = (self.tables.foos_with_fks, @@ -2382,19 +2388,12 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest): )}) mapper(Bar, bars_with_fks) - assert_raises_message( - sa.exc.ArgumentError, - "Could not locate any foreign-key-equated, locally mapped " - "column pairs for primaryjoin condition " - "'bars_with_fks.fid = foos_with_fks.id AND " - "foos_with_fks.id = foos.id' on relationship Foo.bars. " - "Ensure that the referencing Column objects have a " - "ForeignKey present, or are otherwise part of a " - "ForeignKeyConstraint on their parent Table, or specify " - "the foreign_keys parameter to this relationship. For " - "more relaxed rules on join conditions, the relationship " - "may be marked as viewonly=True.", - sa.orm.configure_mappers) + self._assert_raises_no_equality( + sa.orm.configure_mappers, + "bars_with_fks.fid = foos_with_fks.id " + "AND foos_with_fks.id = foos.id", + "Foo.bars", "primary" + ) def test_ambiguous_fks(self): bars, Foo, Bar, foos = (self.tables.bars, @@ -2408,20 +2407,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest): foreign_keys=[foos.c.id, bars.c.fid])}) mapper(Bar, bars) - assert_raises_message(sa.exc.ArgumentError, - "Could not determine relationship " - "direction for primaryjoin condition " - "'foos.id = bars.fid', on relationship " - "Foo.bars, using manual 'foreign_keys' " - "setting. Do the columns in " - "'foreign_keys' represent all, and only, " - "the 'foreign' columns in this join " - r"condition\? Does the mapped Table " - "already have adequate ForeignKey and/or " - "ForeignKeyConstraint objects " - r"established \(in which case " - r"'foreign_keys' is usually unnecessary\)\?" - , sa.orm.configure_mappers) + self._assert_raises_ambiguous_direction( + sa.orm.configure_mappers, + "Foo.bars" + ) def test_ambiguous_remoteside_o2m(self): bars, Foo, Bar, foos = (self.tables.bars, @@ -2510,10 +2499,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest): viewonly=True)}) mapper(Bar, bars) - assert_raises_message(sa.exc.ArgumentError, - 'Could not determine relationship ' - 'direction for primaryjoin condition', - sa.orm.configure_mappers) + self._assert_raises_no_relevant_fks( + sa.orm.configure_mappers, + "foos.id > bars.fid", "Foo.bars", "primary" + ) sa.orm.clear_mappers() mapper(Foo, foos_with_fks, properties={ @@ -2537,15 +2526,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest): viewonly=True)}) mapper(Bar, bars) - assert_raises_message(sa.exc.ArgumentError, - "Could not determine relationship " - "direction for primaryjoin condition " - "'foos.id > foos.fid', on relationship " - "Foo.foos. Ensure that the referencing " - "Column objects have a ForeignKey " - "present, or are otherwise part of a " - "ForeignKeyConstraint on their parent " - "Table.", sa.orm.configure_mappers) + self._assert_raises_no_relevant_fks( + sa.orm.configure_mappers, + "foos.id > foos.fid", "Foo.foos", "primary" + ) sa.orm.clear_mappers() mapper(Foo, foos_with_fks, properties={ @@ -2580,11 +2564,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest): primaryjoin=foos.c.id==bars.c.fid)}) mapper(Bar, bars) - assert_raises_message( - sa.exc.ArgumentError, - "Could not determine relationship direction for primaryjoin " - "condition", - configure_mappers) + self._assert_raises_no_relevant_fks( + configure_mappers, + "foos.id = bars.fid", "Foo.bars", "primary" + ) sa.orm.clear_mappers() mapper(Foo, foos_with_fks, properties={ @@ -2600,11 +2583,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest): 'foos':relationship(Foo, primaryjoin=foos.c.id==foos.c.fid)}) - assert_raises_message( - sa.exc.ArgumentError, - "Could not determine relationship direction for primaryjoin " - "condition", - configure_mappers) + self._assert_raises_no_relevant_fks( + configure_mappers, + "foos.id = foos.fid", "Foo.foos", "primary" + ) def test_equated_self_ref_wrong_fks(self): @@ -2617,11 +2599,10 @@ class InvalidRelationshipEscalationTest(fixtures.MappedTest): primaryjoin=foos.c.id==foos.c.fid, foreign_keys=[bars.c.id])}) - assert_raises_message( - sa.exc.ArgumentError, - "Could not determine relationship direction for primaryjoin " - "condition", - configure_mappers) + self._assert_raises_no_relevant_fks( + configure_mappers, + "foos.id = foos.fid", "Foo.foos", "primary" + ) class InvalidRelationshipEscalationTestM2M(fixtures.MappedTest): -- 2.47.3