- A warning is emitted in the case of multiple relationships thatticket_3230

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.
fixes #3230

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={