Skip to content

Sorting order is messed up with ordering_list #3191

Closed
@sqlalchemy-bot

Description

@sqlalchemy-bot
Collaborator

Migrated issue, originally created by Aleksandr Osipov (@oas89)

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.


Attachments: ordering.py | ordering_patch.diff

Activity

sqlalchemy-bot

sqlalchemy-bot commented on Sep 9, 2014

@sqlalchemy-bot
CollaboratorAuthor

Michael Bayer (@zzzeek) wrote:

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

sqlalchemy-bot

sqlalchemy-bot commented on Sep 9, 2014

@sqlalchemy-bot
CollaboratorAuthor

Changes by Michael Bayer (@zzzeek):

  • added labels: ext
sqlalchemy-bot

sqlalchemy-bot commented on Sep 9, 2014

@sqlalchemy-bot
CollaboratorAuthor

Changes by Michael Bayer (@zzzeek):

  • set milestone to "0.9.8"
sqlalchemy-bot

sqlalchemy-bot commented on Sep 9, 2014

@sqlalchemy-bot
CollaboratorAuthor

Michael Bayer (@zzzeek) wrote:

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

sqlalchemy-bot

sqlalchemy-bot commented on Sep 9, 2014

@sqlalchemy-bot
CollaboratorAuthor

Changes by Michael Bayer (@zzzeek):

  • changed milestone from "0.9.8" to "1.0.xx"
sqlalchemy-bot

sqlalchemy-bot commented on Sep 10, 2014

@sqlalchemy-bot
CollaboratorAuthor

Denis Otkidach (@ods) wrote:

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.

sqlalchemy-bot

sqlalchemy-bot commented on Sep 10, 2014

@sqlalchemy-bot
CollaboratorAuthor

Michael Bayer (@zzzeek) wrote:

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.

sqlalchemy-bot

sqlalchemy-bot commented on Sep 10, 2014

@sqlalchemy-bot
CollaboratorAuthor

Michael Bayer (@zzzeek) wrote:

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]

sqlalchemy-bot

sqlalchemy-bot commented on Sep 10, 2014

@sqlalchemy-bot
CollaboratorAuthor

Michael Bayer (@zzzeek) wrote:

  • 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 Sorting order is messed up with ordering_list #3191

14d2bb0

sqlalchemy-bot

sqlalchemy-bot commented on Sep 10, 2014

@sqlalchemy-bot
CollaboratorAuthor

Michael Bayer (@zzzeek) wrote:

  • 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 Sorting order is messed up with ordering_list #3191

cfaa32b

sqlalchemy-bot

sqlalchemy-bot commented on Sep 10, 2014

@sqlalchemy-bot
CollaboratorAuthor

Changes by Michael Bayer (@zzzeek):

  • changed status to closed
sqlalchemy-bot

sqlalchemy-bot commented on Sep 10, 2014

@sqlalchemy-bot
CollaboratorAuthor

Changes by Michael Bayer (@zzzeek):

  • changed milestone from "1.0.xx" to "0.9.8"
added
bugSomething isn't working
sqlalchemy.extextension modules, most of which are ORM related
on Nov 27, 2018

1 remaining item

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingsqlalchemy.extextension modules, most of which are ORM related

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @sqlalchemy-bot

        Issue actions

          Sorting order is messed up with ordering_list · Issue #3191 · sqlalchemy/sqlalchemy