Sorting order is messed up with ordering_list

[Aleksandr Osipov]

Sorting order is messed up if we replace existing list with new one with some items removed. This happens in the function orm.collections.bulk_replace, when removing items missing in new collection from old one in order to fire events.

Calling .remove method of ordering_list cause reordering of items in old collection, but this also affects ordering in new collection.

Changing two loops in orm.collections.bulk_replace seems to solve the problems.

[Mike Bayer]

tests still pass? I have very little interest/resources to work with orderinglist.

[Mike Bayer]

oh wow that change is in collections.py? no way. those events cannot be changed.

[Denis Otkidach]

Although proposed patch does work for this particual test case I don't think it solves the actual problem. I believe the problem is not related to orderinglist. One thing in bulk_replace function looks strange to me: all additions go to the newly formed collection, while removals go to the old one. It looks like inconsistent behavior. Any custom collection doing some salculations based on the state of the whole collection will recieve remove calls for old collection that presents some state that is not related to what we actually have. Here is an example without orderinglist (even without customising collection class) that demonstrates this:

from sqlalchemy import Column, Integer, ForeignKey
from sqlalchemy.orm import relationship
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()

class Link(Base):
    __tablename__ = 'Link'
    id = Column(Integer, primary_key=True)
    doc_id = Column(Integer, ForeignKey('Doc.id'), nullable=False)
    order = Column(Integer, nullable=False, default=0)
    def __repr__(self):
        return 'Link(id={})'.format(self.id)

class Doc(Base):
    __tablename__ = 'Doc'
    id = Column(Integer, primary_key=True, autoincrement=True)
    links = relationship(Link, order_by=[Link.order])

a2 = Link(id=2)
doc = Doc()
doc.links = [Link(id=1), a2]
old_links = doc.links
print old_links # [Link(id=1), Link(id=2)]

doc.links = [Link(id=3), a2]
print old_links # [Link(id=2)]

Here we see old_links now has the state that is neither old, nor new one. It's confusing.

[Mike Bayer]

well it looks like it wants to send the remove events to the list from which the item is being removed, which seems very reasonable. depending on what someone wants to do with the remove event, it can be confusing either way.

I haven't looked at ordering list in a long time, but my initial thought is that it should be rewritten as append/remove event listeners on the attribute itself, instead of instrumenting the collection. the listeners get sent the target state and the attribute name so that should be all that's needed to manage other state within that list.

[Mike Bayer]

here's that, simple:

from sqlalchemy import Column, Integer, ForeignKey
from sqlalchemy.orm import relationship
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy import event
Base = declarative_base()


class Link(Base):
    __tablename__ = 'Link'
    id = Column(Integer, primary_key=True)
    doc_id = Column(Integer, ForeignKey('Doc.id'), nullable=False)
    order = Column(Integer, nullable=False, default=0)


class Doc(Base):
    __tablename__ = 'Doc'
    id = Column(Integer, primary_key=True, autoincrement=True)
    links = relationship(
        Link,
        order_by=[Link.order]
    )


@event.listens_for(Doc.links, "append")
def append_link(target, value, initiator):
    collection = target.links
    if not collection:
        value.order = 0
    else:
        value.order = collection[-1].order + 1


@event.listens_for(Doc.links, "remove")
def remove_link(target, value, initiator):
    i = 0
    for elem in target.links:
        if elem is value:
            continue
        elem.order = i
        i += 1

doc = Doc()
doc.links = [Link(id=1), Link(id=2), Link(id=3)]

assert [0, 1, 2] == [link.order for link in doc.links]

doc.links = [Link(id=4), doc.links[1], doc.links[2]]

assert [0, 1, 2] == [link.order for link in doc.links]

[Mike Bayer]

• Fixed bug in ordering list where the order of items would be thrown off during a collection replace event, if the reorder_on_append flag were set to True. The fix ensures that the ordering list only impacts the list that is explicitly associated with the object. fixes #3191

→ <<cset 14d2bb074ccc>>

[Mike Bayer]

• Fixed bug in ordering list where the order of items would be thrown off during a collection replace event, if the reorder_on_append flag were set to True. The fix ensures that the ordering list only impacts the list that is explicitly associated with the object. fixes #3191

→ <<cset cfaa32b4d7dc>>