summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--doc/build/changelog/changelog_10.rst18
-rw-r--r--doc/build/orm/relationships.rst135
-rw-r--r--lib/sqlalchemy/orm/relationships.py51
-rw-r--r--test/orm/test_assorted_eager.py4
-rw-r--r--test/orm/test_joins.py39
-rw-r--r--test/orm/test_relationships.py80
6 files changed, 302 insertions, 25 deletions
diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst
index 5aed3bddd..4454dd98a 100644
--- a/doc/build/changelog/changelog_10.rst
+++ b/doc/build/changelog/changelog_10.rst
@@ -22,6 +22,24 @@
on compatibility concerns, see :doc:`/changelog/migration_10`.
.. change::
+ :tags: bug, orm
+ :tickets: 3230
+
+ A warning is emitted in the case of multiple relationships that
+ ultimately will populate a foreign key column in conflict with
+ another, where the relationships are attempting to copy values
+ from different source columns. This occurs in the case where
+ composite foreign keys with overlapping columns are mapped to
+ relationships that each refer to a different referenced column.
+ A new documentation section illustrates the example as well as how
+ to overcome the issue by specifying "foreign" columns specifically
+ on a per-relationship basis.
+
+ .. seealso::
+
+ :ref:`relationship_overlapping_foreignkeys`
+
+ .. change::
:tags: feature, sql
:tickets: 3172
diff --git a/doc/build/orm/relationships.rst b/doc/build/orm/relationships.rst
index c65f06cbc..f512251a7 100644
--- a/doc/build/orm/relationships.rst
+++ b/doc/build/orm/relationships.rst
@@ -1079,12 +1079,15 @@ The above relationship will produce a join like::
ON host_entry_1.ip_address = CAST(host_entry.content AS INET)
An alternative syntax to the above is to use the :func:`.foreign` and
-:func:`.remote` :term:`annotations`, inline within the :paramref:`~.relationship.primaryjoin` expression.
+:func:`.remote` :term:`annotations`,
+inline within the :paramref:`~.relationship.primaryjoin` expression.
This syntax represents the annotations that :func:`.relationship` normally
applies by itself to the join condition given the :paramref:`~.relationship.foreign_keys` and
-:paramref:`~.relationship.remote_side` arguments; the functions are provided in the API in the
-rare case that :func:`.relationship` can't determine the exact location
-of these features on its own::
+:paramref:`~.relationship.remote_side` arguments. These functions may
+be more succinct when an explicit join condition is present, and additionally
+serve to mark exactly the column that is "foreign" or "remote" independent
+of whether that column is stated multiple times or within complex
+SQL expressions::
from sqlalchemy.orm import foreign, remote
@@ -1157,6 +1160,130 @@ Will render as::
flag to assist in the creation of :func:`.relationship` constructs using
custom operators.
+.. _relationship_overlapping_foreignkeys:
+
+Overlapping Foreign Keys
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+A rare scenario can arise when composite foreign keys are used, such that
+a single column may be the subject of more than one column
+referred to via foreign key constraint.
+
+Consider an (admittedly complex) mapping such as the ``Magazine`` object,
+referred to both by the ``Writer`` object and the ``Article`` object
+using a composite primary key scheme that includes ``magazine_id``
+for both; then to make ``Article`` refer to ``Writer`` as well,
+``Article.magazine_id`` is involved in two separate relationships;
+``Article.magazine`` and ``Article.writer``::
+
+ class Magazine(Base):
+ __tablename__ = 'magazine'
+
+ id = Column(Integer, primary_key=True)
+
+
+ class Article(Base):
+ __tablename__ = 'article'
+
+ article_id = Column(Integer)
+ magazine_id = Column(ForeignKey('magazine.id'))
+ writer_id = Column()
+
+ magazine = relationship("Magazine")
+ writer = relationship("Writer")
+
+ __table_args__ = (
+ PrimaryKeyConstraint('article_id', 'magazine_id'),
+ ForeignKeyConstraint(
+ ['writer_id', 'magazine_id'],
+ ['writer.id', 'writer.magazine_id']
+ ),
+ )
+
+
+ class Writer(Base):
+ __tablename__ = 'writer'
+
+ id = Column(Integer, primary_key=True)
+ magazine_id = Column(ForeignKey('magazine.id'), primary_key=True)
+ magazine = relationship("Magazine")
+
+When the above mapping is configured, we will see this warning emitted::
+
+ SAWarning: relationship 'Article.writer' will copy column
+ writer.magazine_id to column article.magazine_id,
+ which conflicts with relationship(s): 'Article.magazine'
+ (copies magazine.id to article.magazine_id). Consider applying
+ viewonly=True to read-only relationships, or provide a primaryjoin
+ condition marking writable columns with the foreign() annotation.
+
+What this refers to originates from the fact that ``Article.magazine_id`` is
+the subject of two different foreign key constraints; it refers to
+``Magazine.id`` directly as a source column, but also refers to
+``Writer.magazine_id`` as a source column in the context of the
+composite key to ``Writer``. If we associate an ``Article`` with a
+particular ``Magazine``, but then associate the ``Article`` with a
+``Writer`` that's associated with a *different* ``Magazine``, the ORM
+will overwrite ``Article.magazine_id`` non-deterministically, silently
+changing which magazine we refer towards; it may
+also attempt to place NULL into this columnn if we de-associate a
+``Writer`` from an ``Article``. The warning lets us know this is the case.
+
+To solve this, we need to break out the behavior of ``Article`` to include
+all three of the following features:
+
+1. ``Article`` first and foremost writes to
+ ``Article.magazine_id`` based on data persisted in the ``Article.magazine``
+ relationship only, that is a value copied from ``Magazine.id``.
+
+2. ``Article`` can write to ``Article.writer_id`` on behalf of data
+ persisted in the ``Article.writer`` relationship, but only the
+ ``Writer.id`` column; the ``Writer.magazine_id`` column should not
+ be written into ``Article.magazine_id`` as it ultimately is sourced
+ from ``Magazine.id``.
+
+3. ``Article`` takes ``Article.magazine_id`` into account when loading
+ ``Article.writer``, even though it *doesn't* write to it on behalf
+ of this relationship.
+
+To get just #1 and #2, we could specify only ``Article.writer_id`` as the
+"foreign keys" for ``Article.writer``::
+
+ class Article(Base):
+ # ...
+
+ writer = relationship("Writer", foreign_keys='Article.writer_id')
+
+However, this has the effect of ``Article.writer`` not taking
+``Article.magazine_id`` into account when querying against ``Writer``:
+
+.. sourcecode:: sql
+
+ SELECT article.article_id AS article_article_id,
+ article.magazine_id AS article_magazine_id,
+ article.writer_id AS article_writer_id
+ FROM article
+ JOIN writer ON writer.id = article.writer_id
+
+Therefore, to get at all of #1, #2, and #3, we express the join condition
+as well as which columns to be written by combining
+:paramref:`~.relationship.primaryjoin` fully, along with either the
+:paramref:`~.relationship.foreign_keys` argument, or more succinctly by
+annotating with :func:`~.orm.foreign`::
+
+ class Article(Base):
+ # ...
+
+ writer = relationship(
+ "Writer",
+ primaryjoin="and_(Writer.id == foreign(Article.writer_id), "
+ "Writer.magazine_id == Article.magazine_id)")
+
+.. versionchanged:: 1.0.0 the ORM will attempt to warn when a column is used
+ as the synchronization target from more than one relationship
+ simultaneously.
+
+
Non-relational Comparisons / Materialized Path
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index 56a33742d..4a6159144 100644
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -16,6 +16,7 @@ and `secondaryjoin` aspects of :func:`.relationship`.
from __future__ import absolute_import
from .. import sql, util, exc as sa_exc, schema, log
+import weakref
from .util import CascadeOptions, _orm_annotate, _orm_deannotate
from . import dependency
from . import attributes
@@ -1532,6 +1533,7 @@ class RelationshipProperty(StrategizedProperty):
self._check_cascade_settings(self._cascade)
self._post_init()
self._generate_backref()
+ self._join_condition._warn_for_conflicting_sync_targets()
super(RelationshipProperty, self).do_init()
self._lazy_strategy = self._get_strategy((("lazy", "select"),))
@@ -2519,6 +2521,55 @@ class JoinCondition(object):
self.secondary_synchronize_pairs = \
self._deannotate_pairs(secondary_sync_pairs)
+ _track_sync_targets = weakref.WeakKeyDictionary()
+
+ def _warn_for_conflicting_sync_targets(self):
+ if not self.support_sync:
+ return
+
+ # totally complex code that takes place for virtually all
+ # relationships, detecting an incredibly rare edge case,
+ # and even then, all just to emit a warning.
+ # we would like to detect if we are synchronizing any column
+ # pairs in conflict with another relationship that wishes to sync
+ # an entirely different column to the same target. This is typically
+ # when using complex overlapping composite foreign keys.
+ for from_, to_ in [
+ (from_, to_)
+ for (from_, to_) in self.synchronize_pairs
+ ] + [
+ (from_, to_) for
+ (from_, to_) in self.secondary_synchronize_pairs
+ ]:
+ if to_ not in self._track_sync_targets:
+ self._track_sync_targets[to_] = weakref.WeakKeyDictionary(
+ {self.prop: from_})
+ else:
+ other_props = []
+ prop_to_from = self._track_sync_targets[to_]
+ for pr, fr_ in prop_to_from.items():
+ if pr.mapper in mapperlib._mapper_registry and \
+ fr_ is not from_ and \
+ pr not in self.prop._reverse_property:
+ other_props.append((pr, fr_))
+
+ if other_props:
+ util.warn(
+ "relationship '%s' will copy column %s to column %s, "
+ "which conflicts with relationship(s): %s. "
+ "Consider applying "
+ "viewonly=True to read-only relationships, or provide "
+ "a primaryjoin condition marking writable columns "
+ "with the foreign() annotation." % (
+ self.prop,
+ from_, to_,
+ ", ".join(
+ "'%s' (copies %s to %s)" % (pr, fr_, to_)
+ for (pr, fr_) in other_props)
+ )
+ )
+ self._track_sync_targets[to_][self.prop] = from_
+
@util.memoized_property
def remote_columns(self):
return self._gather_join_annotations("remote")
diff --git a/test/orm/test_assorted_eager.py b/test/orm/test_assorted_eager.py
index 2bee3cbd6..48faa172f 100644
--- a/test/orm/test_assorted_eager.py
+++ b/test/orm/test_assorted_eager.py
@@ -82,8 +82,8 @@ class EagerTest(fixtures.MappedTest):
mapper(Category, categories)
mapper(Option, options, properties=dict(
- owner=relationship(Owner),
- test=relationship(Thing)))
+ owner=relationship(Owner, viewonly=True),
+ test=relationship(Thing, viewonly=True)))
mapper(Thing, tests, properties=dict(
owner=relationship(Owner, backref='tests'),
diff --git a/test/orm/test_joins.py b/test/orm/test_joins.py
index 40bc01b5d..eba47dbec 100644
--- a/test/orm/test_joins.py
+++ b/test/orm/test_joins.py
@@ -361,6 +361,27 @@ class InheritedJoinTest(fixtures.MappedTest, AssertsCompiledSQL):
)
+class JoinOnSynonymTest(_fixtures.FixtureTest, AssertsCompiledSQL):
+ @classmethod
+ def setup_mappers(cls):
+ User = cls.classes.User
+ Address = cls.classes.Address
+ users, addresses = (cls.tables.users, cls.tables.addresses)
+ mapper(User, users, properties={
+ 'addresses': relationship(Address),
+ 'ad_syn': synonym("addresses")
+ })
+ mapper(Address, addresses)
+
+ def test_join_on_synonym(self):
+ User = self.classes.User
+ self.assert_compile(
+ Session().query(User).join(User.ad_syn),
+ "SELECT users.id AS users_id, users.name AS users_name "
+ "FROM users JOIN addresses ON users.id = addresses.user_id"
+ )
+
+
class JoinTest(QueryTest, AssertsCompiledSQL):
__dialect__ = 'default'
@@ -409,24 +430,6 @@ class JoinTest(QueryTest, AssertsCompiledSQL):
sess.query(literal_column('x'), User).join, Address
)
- def test_join_on_synonym(self):
-
- class User(object):
- pass
- class Address(object):
- pass
- users, addresses = (self.tables.users, self.tables.addresses)
- mapper(User, users, properties={
- 'addresses':relationship(Address),
- 'ad_syn':synonym("addresses")
- })
- mapper(Address, addresses)
- self.assert_compile(
- Session().query(User).join(User.ad_syn),
- "SELECT users.id AS users_id, users.name AS users_name "
- "FROM users JOIN addresses ON users.id = addresses.user_id"
- )
-
def test_multi_tuple_form(self):
"""test the 'tuple' form of join, now superseded
by the two-element join() form.
diff --git a/test/orm/test_relationships.py b/test/orm/test_relationships.py
index 4c5a5abee..2a15ce666 100644
--- a/test/orm/test_relationships.py
+++ b/test/orm/test_relationships.py
@@ -672,12 +672,89 @@ class CompositeSelfRefFKTest(fixtures.MappedTest):
self._test()
+ def test_overlapping_warning(self):
+ Employee, Company, employee_t, company_t = (self.classes.Employee,
+ self.classes.Company,
+ self.tables.employee_t,
+ self.tables.company_t)
+
+ mapper(Company, company_t)
+ mapper(Employee, employee_t, properties={
+ 'company': relationship(Company, backref='employees'),
+ 'reports_to': relationship(
+ Employee,
+ primaryjoin=sa.and_(
+ remote(employee_t.c.emp_id) == employee_t.c.reports_to_id,
+ remote(employee_t.c.company_id) == employee_t.c.company_id
+ ),
+ backref=backref('employees')
+ )
+ })
+
+ assert_raises_message(
+ exc.SAWarning,
+ r"relationship .* will copy column .* to column "
+ "employee_t.company_id, which conflicts with relationship\(s\)",
+ configure_mappers
+ )
+
+ def test_annotated_no_overwriting(self):
+ Employee, Company, employee_t, company_t = (self.classes.Employee,
+ self.classes.Company,
+ self.tables.employee_t,
+ self.tables.company_t)
+
+ mapper(Company, company_t)
+ mapper(Employee, employee_t, properties={
+ 'company': relationship(Company, backref='employees'),
+ 'reports_to': relationship(
+ Employee,
+ primaryjoin=sa.and_(
+ remote(employee_t.c.emp_id) ==
+ foreign(employee_t.c.reports_to_id),
+ remote(employee_t.c.company_id) == employee_t.c.company_id
+ ),
+ backref=backref('employees')
+ )
+ })
+
+ self._test_no_warning()
+
+ def _test_no_overwrite(self, sess, expect_failure):
+ # test [ticket:3230]
+
+ Employee, Company = self.classes.Employee, self.classes.Company
+
+ c1 = sess.query(Company).filter_by(name='c1').one()
+ e3 = sess.query(Employee).filter_by(name='emp3').one()
+ e3.reports_to = None
+
+ if expect_failure:
+ # if foreign() isn't applied specifically to
+ # employee_t.c.reports_to_id only, then
+ # employee_t.c.company_id goes foreign as well and then
+ # this happens
+ assert_raises_message(
+ AssertionError,
+ "Dependency rule tried to blank-out primary key column "
+ "'employee_t.company_id'",
+ sess.flush
+ )
+ else:
+ sess.flush()
+ eq_(e3.company, c1)
+
+ @testing.emits_warning("relationship .* will copy column ")
def _test(self):
+ self._test_no_warning(overwrites=True)
+
+ def _test_no_warning(self, overwrites=False):
self._test_relationships()
sess = Session()
self._setup_data(sess)
self._test_lazy_relations(sess)
self._test_join_aliasing(sess)
+ self._test_no_overwrite(sess, expect_failure=overwrites)
def _test_relationships(self):
configure_mappers()
@@ -3044,7 +3121,8 @@ class SecondaryNestedJoinTest(fixtures.MappedTest, AssertsCompiledSQL,
secondaryjoin=d.c.id == b.c.d_id,
#primaryjoin=and_(a.c.b_id == j.c.b_id, a.c.id == j.c.c_a_id),
#secondaryjoin=d.c.id == j.c.b_d_id,
- uselist=False
+ uselist=False,
+ viewonly=True
)
})
mapper(B, b, properties={
View Lorry Controller Status