Issues

Issue #3060 resolved

delete/insert into update doesn't handle relationship changes

ods
created an issue

New object with the same ID as existing one implicitly merged via cascade rules has old value of relationship property after the commit when the new value is None.

In short:

doc.doc_links = [DocLink(id=doc_link_id, link=None)]
session.commit()
assert doc.doc_links[0].link is None  # fails

Full test is attached (test3 is the test case that fails).

Comments (18)

  1. Mike Bayer repo owner

    okey doke. well first thing to note, that "merge" is less than ideal. the test passes if we apply #2501 and then map:

    class DocLink(Base):
        __tablename__ = 'doc_link'
        id = Column(Integer, primary_key=True, autoincrement=True)
        doc_id = Column(Integer, ForeignKey(Doc.id), nullable=False)
        doc = relationship(Doc)
        link_id = Column(Integer, ForeignKey(Link.id))
        link = relationship(Link)
        __mapper_args__ = {"maintain_unique_attributes": [("id", )]}
    

    the output is then:

    DELETE FROM doc_link WHERE doc_link.id = ?
    (1,)
    INSERT INTO doc_link (id, doc_id, link_id) VALUES (?, ?, ?)
    

    the maintain_unique_attributes feature introduces other caveats as far as the order of things so it's unlikely at this point it can be made a default for primary keys. it would be a good idea to use in cases like these where you expect rows to be switching on the same primary and/or unique keys.

    as far as the delete->insert->update taking into account simultaneous changes in relationships, not really sure on that yet.

  2. Mike Bayer repo owner
    • Fixed a few edge cases which arise in the so-called "row switch" scenario, where an INSERT/DELETE can be turned into an UPDATE. In this situation, a many-to-one relationship set to None, or in some cases a scalar attribute set to None, may not be detected as a net change in value, and therefore the UPDATE would not reset what was on the previous row. This is due to some as-yet unresovled side effects of the way attribute history works in terms of implicitly assuming None isn't really a "change" for a previously un-set attribute. See also ticket3061. fixes #3060

    → <<cset bc08ee902925>>

  3. Mike Bayer repo owner
    • Fixed a few edge cases which arise in the so-called "row switch" scenario, where an INSERT/DELETE can be turned into an UPDATE. In this situation, a many-to-one relationship set to None, or in some cases a scalar attribute set to None, may not be detected as a net change in value, and therefore the UPDATE would not reset what was on the previous row. This is due to some as-yet unresovled side effects of the way attribute history works in terms of implicitly assuming None isn't really a "change" for a previously un-set attribute. See also ticket3061. fixes #3060

    → <<cset 16b7f1816d54>>

  4. Mike Bayer repo owner

    here's the test:

    diff --git a/test/orm/test_unitofworkv2.py b/test/orm/test_unitofworkv2.py
    index a76f928..25fd11d 100644
    --- a/test/orm/test_unitofworkv2.py
    +++ b/test/orm/test_unitofworkv2.py
    @@ -1387,6 +1387,19 @@ class RowswitchM2OTest(fixtures.MappedTest):
             sess.commit()
             assert a1.bs[0].data is None
    
    +    def test_dont_blow_away_partial_relationship(self):
    +        A, B, C = self._fixture()
    +        sess = Session()
    +
    +        c1 = C()
    +        sess.add(c1)
    +        sess.flush()
    +
    +        b1 = B(cid=c1.id)
    +        assert b1.c is None
    +        sess.add(b1)
    +        sess.flush()
    +        assert b1.cid == c1.id
    
    
     class BasicStaleChecksTest(fixtures.MappedTest):
    

    not sure what I'm doing yet. this is a serious regression and i might have to revert this whole thing and re-release. not sure.

  5. Mike Bayer repo owner

    OK that test is not quite it, in 1.0 we are expecting "b1.c" accessor which sets up None to actually generate an event. what's happening in openstack is this:

        def test_joinedload_doesnt_produce_bogus_event(self):
            A, B, C = self._fixture()
            sess = Session()
    
            c1 = C()
            sess.add(c1)
            sess.flush()
    
            b1 = B()
            sess.add(b1)
            sess.commit()
    
            # test that was broken by #3060
            from sqlalchemy.orm import joinedload
            b1 = sess.query(B).options(joinedload("c")).first()
            b1.cid = c1.id
            sess.flush()
    
            assert b1.cid == c1.id
    
  6. Mike Bayer repo owner

    in openstack, the joinedload in question is nova/db/sqlalchemy/api.py -> _floating_ip_get_by_address, there's a joinedload() in there of fixed_ip.instance. The API there is otherwise manipulating FloatingIP/FixedIP using the foreign key values directly.

  7. Log in to comment