Closed
Description
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
Metadata
Metadata
Assignees
Type
Projects
Relationships
Development
No branches or pull requests
Activity
sqlalchemy-bot commentedon Sep 9, 2014
Michael Bayer (@zzzeek) wrote:
tests still pass? I have very little interest/resources to work with orderinglist.
sqlalchemy-bot commentedon Sep 9, 2014
Changes by Michael Bayer (@zzzeek):
sqlalchemy-bot commentedon Sep 9, 2014
Changes by Michael Bayer (@zzzeek):
sqlalchemy-bot commentedon Sep 9, 2014
Michael Bayer (@zzzeek) wrote:
oh wow that change is in collections.py? no way. those events cannot be changed.
sqlalchemy-bot commentedon Sep 9, 2014
Changes by Michael Bayer (@zzzeek):
sqlalchemy-bot commentedon Sep 10, 2014
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 inbulk_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 recieveremove
calls for old collection that presents some state that is not related to what we actually have. Here is an example withoutorderinglist
(even without customising collection class) that demonstrates this:Here we see
old_links
now has the state that is neither old, nor new one. It's confusing.sqlalchemy-bot commentedon Sep 10, 2014
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 commentedon Sep 10, 2014
Michael Bayer (@zzzeek) wrote:
here's that, simple:
sqlalchemy-bot commentedon Sep 10, 2014
Michael Bayer (@zzzeek) wrote:
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 commentedon Sep 10, 2014
Michael Bayer (@zzzeek) wrote:
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 commentedon Sep 10, 2014
Changes by Michael Bayer (@zzzeek):
sqlalchemy-bot commentedon Sep 10, 2014
Changes by Michael Bayer (@zzzeek):
1 remaining item