[starting 1.4.7] Children objects are duplicated when setting backref

[chaen]

Describe the bug

I have Parent and Child classes, with a one-to-many relationship. The Child class has a backref to the Parent called _parent. These objects often live outside of SQLAlchemy, so I need to maintain this reference myself sometimes.
If I get a family from the DB, and then try to assign myself the backref parameter, the children objects are duplicated

Expected behavior

This behaviour is new in 1.4.7 and still happens against the master branch of the repo.
When running the example bellow with 1.4.7 and 1.4.6, the debug logs are perfectly identical.

To Reproduce

from __future__ import absolute_import
from __future__ import division
from __future__ import print_function

import logging

from sqlalchemy.orm import relationship, backref, sessionmaker, joinedload, mapper
from sqlalchemy import create_engine, Table, Column, MetaData, ForeignKey, \
    Integer, String


logging.basicConfig()
logging.getLogger("sqlalchemy.engine").setLevel(logging.DEBUG)


class Parent(object):
  """ Parent class, managing children """

  def __init__(self):
    self.ParentName = None

  def _notify(self):
    for child in self.__children__:
      # PROBLEM HAPPENS HERE
      # When setting the _parent value once the
      # object was retrieved from the DB, so
      # in the del r[0] call,
      # the children ends up being duplicated
      child._parent = self

  def __contains__(self, child):
    return bool(child in self.__children__)

  def __iadd__(self, child):
    """ Add a Child to the list of children, and
        set the backref
    """
    if child not in self:
      self.__children__.append(child)
      child._parent = self
      self._notify()
    return self

  def __delitem__(self, i):
    self.__children__.__delitem__(i)
    self._notify()

  def __len__(self):
    """ nb of subParents """
    return len(self.__children__)


class Child(object):
  """ Dummy Child class, with backref to the parent """

  def __init__(self, ChildName):
    self.ChildName = ChildName
    # backref
    self._parent = None

########################################################################


metadata = MetaData()

childTable = Table('Child', metadata,
                   Column('ChildName', String(64), nullable=False),
                   Column('ChildID', Integer, primary_key=True),
                   Column('ParentID', Integer,
                          ForeignKey('Parent.ParentID', ondelete='CASCADE'),
                          nullable=False),
                   mysql_engine='InnoDB')


mapper(Child, childTable)


parentTable = Table('Parent', metadata,
                    Column('ParentName', String(255), nullable=False),
                    Column('ParentID', Integer, primary_key=True),
                    mysql_engine='InnoDB')

mapper(Parent, parentTable, properties={'__children__': relationship(Child,
                                                                     backref=backref('_parent',
                                                                                     lazy='immediate'),
                                                                     lazy='immediate',
                                                                     passive_deletes=True,
                                                                     cascade="all, delete-orphan")})


########################################################################
class FamilyDB(object):

  def __init__(self):

    self.dbHost = 'mysql'
    self.dbPort = 3306
    self.dbUser = 'root'
    self.dbPass = 'password'
    self.dbName = 'Test'

    self.engine = create_engine('mysql://root:password@mysql:3306/Test')

    metadata.bind = self.engine

    self.DBSession = sessionmaker(bind=self.engine)
    metadata.create_all(self.engine)

  def putParent(self, parent):
    """ Put a parent and children in the DB """

    session = self.DBSession(expire_on_commit=False)
    try:

      parent = session.merge(parent)
      session.add(parent)
      session.commit()
      session.expunge_all()

      return parent.ParentID

    except Exception:
      session.rollback()
      raise
    finally:
      session.close()

  def getParent(self, parentID):
    """Get a parent from the DB with its children"""

    session = self.DBSession(expire_on_commit=False)

    try:
      parent = session.query(Parent) \
          .options(joinedload('__children__')) \
          .filter(Parent.ParentID == parentID)\
          .one()

      session.expunge_all()
      return parent

    except Exception:
      session.rollback()
      raise
    finally:
      session.close()


def deathScenario():

  db = FamilyDB()

  # Create my test data, a parent with 2 children
  p = Parent()
  p.ParentName = "Father"

  ch1 = Child("Child1")
  ch2 = Child("Child2")

  p += ch1
  p += ch2

  parentID = db.putParent(p)

  # Now retrieve that family from the DB
  p = db.getParent(parentID)

  # I expect here that the parent has 2 children
  assert len(p) == 2

  # Let's kill one child :-)
  del p[0]

  # I expect the parent to have a single child left
  # But here it fails, and has 2.
  # The same one is duplicated
  assert len(p) == 1, p.__children__


if __name__ == '__main__':
  deathScenario()

Error

Traceback (most recent call last):
  File "/tmp/reproducer/reproducer2.py", line 181, in <module>
    deathScenario()
  File "/tmp/reproducer/reproducer2.py", line 177, in deathScenario
    assert len(p) == 1, p.__children__
AssertionError: [<__main__.Child object at 0x7fb7e541e590>, <__main__.Child object at 0x7fb7e541e590>]

Versions.

• OS: CentOS Linux release 7.8.2003
• Python: 2.7.13
• SQLAlchemy: from 1.4.7 onward
• Database: Mysql
• DBAPI: MySQL-python 1.2.5

Have a nice day!

[zzzeek]

ok, you're using lazy="immediate" and we necessarily changed some things about it in #6232. I havent deeply understood waht's happening here yet, but if we take out those "immediateload" settings, then run your script in any SQLAlchemy version, the assertion at the end fails in the same way.

the "immediateload" directive should not change any behaviors, so this suggests this script is somehow relying on the previously broken behavior of "immediateload", since now the script behaves consistently no matter what loader strategy is used.

it looks like in your operation you are essentially doing this:

(pdb) !p.__children__
[<__main__.Child object at 0x7fa2d92a8e50>, <__main__.Child object at 0x7fa2d92a8ee0>]
p.__children__[1]._parent = p
(Pdb) !p.__children__
[<__main__.Child object at 0x7fa2d92a8e50>, <__main__.Child object at 0x7fa2d92a8ee0>, <__main__.Child object at 0x7fa2d92a8ee0>]
(Pdb) 

whether or not we like what that says, that's the normal behavior.

what I'm not getting yet is how the former "immediateload" behavior was changing this, as it seems to be, so i will do that now but at the moment it's looking like this program somehow relies upon a weird unexpected behavior.

[chaen]

Thanks for the impressively fast response :-)

I am not quite sure to understand how what you show is a normal behaviour ? Naively, I would expect that this __children__ list is just an ORM representation of what you would get by doing SELECT * from Child where ParentID = 123. And I would not expect seeing duplicates there. But probably I am just misunderstanding this relationship

[zzzeek]

when you append to a relationship collection that has a backref to a many-to-one, or you assign to a many-to-one that has a backref to a collection, there's an attribute event that will populate in the other direction. What seems to be happening here is that the "immediateload" was previously being fired off on the other side of the Parent.__children__ joinedload, onto the Child._parent, which is not the normal eager loader behavior right now, as the eager loaders want to avoid going into recursive loops. The fix in #6232 was to repair this recursive loop as stated in the "immediateload" loader; this was formerly not a very widely used loader so the apparent recursion issue was not apparent until 1.4 made greater use of this strategy.

So what would happen is that since Child._parent was already loaded, when you set it again, no backref event would grow the collection. With the repaired behavior, Child._parent is not loaded so when you assign to it, the relationship mechanics don't try to guess what the old value was, the new one goes right in, which fires off the event.

the overall problem with the program here is that it is doubling the work that backref already does. if you simply remove the whole _notify() step, which is unnecessary, as well as the redundant child._parent assignments that follow the equivalent collection operation. you get a much shorter script and everything works:

class Parent(object):
    """ Parent class, managing children """

    def __init__(self):
        self.ParentName = None

    def _notify(self):
        pass
        # for child in self.__children__:
            # this should never be done if backrefs are in use
        #     child._parent = self

    def __contains__(self, child):
        return bool(child in self.__children__)

    def __iadd__(self, child):
        """Add a Child to the list of children, and
        set the backref
        """
        if child not in self:
            self.__children__.append(child)
            # child._parent = self  # redundant
            # self._notify()
        return self

    def __delitem__(self, i):
        self.__children__.__delitem__(i)
        # self._notify() # redundant

    def __len__(self):
        """ nb of subParents """
        return len(self.__children__)

[zzzeek]

also if you want to avoid dupes in a collection, you would use collection_class=set.

[chaen]

I see thanks.
Unfortunately, the notify in reality does much more than what it says here, and I need to have this backref mechanism in place already before the objects are known to SQLAlchemy.

So basically, what I need to make sure is that I do not set the _parent attribute if that object is currently managed by SQLAlchemy. Is there a recommended way to do that ? I've realized that funnily enough hasattr(child, '_parent') returns False if that backref is managed by SQLAlchemy, so I could rely on that, but that's sort of a hack...

[zzzeek]

you are....mapping classes after you've used instances of those classes?

[chaen]

Yep :-/ We create the instances, do whatever we have to do with them, and later on persist them with SQLAlchemy in the DB. The reason being that they are created on some machines without access to the DB (nor to SQLAlchemy in fact), used, and then serialized over the network to later be persisted. And they go the other way around too.

[zzzeek]

OK but I would assume the notion is you have code that deals with objects in both states and does not know which state they are in. this is a really dangerous pattern and I think your app should "know" whether or not classes are mapped, and have distinct codepaths for dealing with them.

Also no database access is implied by having SQLAlchemy installed, imported, and the classes mapped. It doesn't make sense that you have your software installed in Python environments where SQLAlchemy is not installed, you should add it to your requirements for those installations and the classes should just be mapped up front so that they behave consistently.

all of that aside, in this case if you are duplicating the behavior of a backref because your current envrionment may or may not have imported SQLAlchemy and mapped the classes or not, you should check if "child._parent is self" before setting it.

[chaen]

I totally agree that it is a bad design to start with. A redesign I have on my plate is long overdue :-/ SQLAlchemy 1.4 seems to have cleaned a lot of things, so the flaws in my piece of code start showing up.

you should check if "child._parent is self" before setting it.

That actually raises a DetachedInstanceError, but that's fine, that's good enough for me to test

[zzzeek]

I totally agree that it is a bad design to start with. A redesign I have on my plate is long overdue :-/ SQLAlchemy 1.4 seems to have cleaned a lot of things, so the flaws in my piece of code start showing up.

you should check if "child._parent is self" before setting it.

That actually raises a DetachedInstanceError, but that's fine, that's good enough for me to test

OK look inside __dict__ instead , child.__dict__.get('_parent', None) is self

[chaen]

Thanks. turns out I also need to check child.__class__.__dict__ in case it comes from the DB (so after the get).

Clearly, I must redesign my code... It actually comes from legacy code, before using SQLAlchemy. At the time of introducing, one requirement we had that we could still have clients without sqlalchemy installed at all. I understand that this mixed environment is clearly not something you recommend ?

[zzzeek]

Thanks. turns out I also need to check child.__class__.__dict__ in case it comes from the DB (so after the get).

Clearly, I must redesign my code... It actually comes from legacy code, before using SQLAlchemy. At the time of introducing, one requirement we had that we could still have clients without sqlalchemy installed at all. I understand that this mixed environment is clearly not something you recommend ?

it just seems unnecessary unless this is an application running on raspberry pi's or cellphones?

[chaen]

actually we may get some ARM and Power9..
In any case, many thanks for the very reactive help !

[chaen]

So I tried to let SQLAlchemy deal with the _parent attribute all by itself, but the behavior is not what I would expect it to be. It seems to invalidate the backref once the session is closed that retrieves the Parent is closed:

from __future__ import absolute_import
from __future__ import division
from __future__ import print_function

from sqlalchemy.orm import relationship, backref, sessionmaker, mapper
from sqlalchemy import create_engine, Table, Column, MetaData, ForeignKey, \
    Integer, String


class Parent(object):
  def __init__(self, ParentName):
    self.ParentName = ParentName


class Child(object):
  def __init__(self, ChildName):
    self.ChildName = ChildName


metadata = MetaData()

childTable = Table('Child', metadata,
                   Column('ChildName', String(64), nullable=False),
                   Column('ChildID', Integer, primary_key=True),
                   Column('ParentID', Integer,
                          ForeignKey('Parent.ParentID', ondelete='CASCADE'),
                          nullable=False),
                   mysql_engine='InnoDB')


mapper(Child, childTable)


parentTable = Table('Parent', metadata,
                    Column('ParentName', String(255), nullable=False),
                    Column('ParentID', Integer, primary_key=True),
                    mysql_engine='InnoDB')

mapper(Parent, parentTable, properties={'__children__': relationship(Child,
                                                                     backref=backref('_parent',
                                                                                     lazy='joined'),
                                                                     lazy='joined',
                                                                     passive_deletes=True,
                                                                     cascade="all, delete-orphan")})


engine = create_engine('mysql://root:password@mysql:3306/Test')

metadata.bind = engine

DBSession = sessionmaker(bind=engine)
metadata.create_all(engine)



#######################################
# Create the test data


parent = Parent("Father")
ch1 = Child("Child1")
parent.__children__.append(ch1)



with DBSession(expire_on_commit=False) as session:
  parent = session.merge(parent)
  session.add(parent)
  session.commit()
  session.expunge_all()

parentID = parent.ParentID


############################### 
# Get the data back

with DBSession(expire_on_commit=False) as session:

  parent2 = session.query(Parent) \
      .filter(Parent.ParentID == parentID)\
      .one()

  session.expunge_all()
  session.close()

print(parent.__children__)
print(parent2.__children__)

print(parent.__children__[0]._parent)

# That last one fails
print(parent2.__children__[0]._parent)

This is the output

[<__main__.Child object at 0x7fc2da3a0a90>]
[<__main__.Child object at 0x7fc2da37f1d0>]
<__main__.Parent object at 0x7fc2db783d50>
Traceback (most recent call last):
  File "/tmp/reproducer/reproducer3.py", line 93, in <module>
    print(parent2.__children__[0]._parent)
  File "/home/dirac/ServerInstallDIR/diracos/usr/lib/python2.7/site-packages/sqlalchemy/orm/attributes.py", line 465, in __get__
    return self.impl.get(state, dict_)
  File "/home/dirac/ServerInstallDIR/diracos/usr/lib/python2.7/site-packages/sqlalchemy/orm/attributes.py", line 909, in get
    value = self.callable_(state, passive)
  File "/home/dirac/ServerInstallDIR/diracos/usr/lib/python2.7/site-packages/sqlalchemy/orm/strategies.py", line 835, in _load_for_state
    % (orm_util.state_str(state), self.key)
sqlalchemy.orm.exc.DetachedInstanceError: Parent instance <Child at 0x7fc2da37f1d0> is not bound to a Session; lazy load operation of attribute '_parent' cannot proceed (Background on this error at: http://sqlalche.me/e/14/bhk3)

I would expect that the _parent attribute is still accessible.

[zzzeek]

_parent isn't accessible because it wasn't loaded. this is all about that same logic that's been tripping up your example from the beginning,where a single kind of loader, in this most recent example joinedload, does not want to render out a JOIN from Parent -> Child ->back to the same -> Parent again, this would be a wasteful query that is joining unnecessarily to the same rows. As the ORM is designed with the assumption that objects are only being used within the context of a Session, the issue is solved instead by the fact that any Parent.__children__[0]._parent call need only look in the identity map for the child->parent, and that's how it works. But it can't do that if the object is detached, which is the use case you're working with.

unfortunately SQLAlchemy is not easily predictable right now in how to eager load A->B->already loaded A without using loader options at query time. There are some combinations of relationship->lazy that will load both sides and some that don't but they are a little bit arbitrary and should be improved. in particular, the "immediateload" option you are using in the first place, as I've mentioned previously, probably should do its thing regardless if you have it on a relationship, and it's possible other people are going to run into the same behavioral change you are and that I have to introduce a third form of "non-recursive" logic just for immediateload.

anyway, using loader options however it will load both sides because that bypasses most of the "dont recursively laod" checks:

  parent2 = session.query(Parent) \
      .filter(Parent.ParentID == parentID)\
      .options(joinedload(Parent.__children__).immediateload(Child._parent))\
      .one()

overall, the main thing about your approach that's making SQLAlchemy not fun to work with is you're traversing object graphs that were loaded from database row, but have been detached from the rows they represent.

[zzzeek]

I've added #6301 to look into the overall issue of making the behavior of mututally-dependent eagerloaders more predictable and useful.

[chaen]

Thanks for the issue, I'm going to follow that one closely. (although bottom line is that I should be redesigning my code :-) )

[zzzeek]

sure, but as i looked at this, the "immedaiteload" thing people are really going to want that for the asyncio case at least. so after all that you had a good idea! thanks for sticking around.

[chaen]

My pleasure ! Let me know if I can be of any help ! I'll be glad to !