search

brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.04k stars 355 forks source link

Can I Create a PR with a Function that "Fixes" List in Case of Duplicates? #333

Closed pacarvalho closed 3 months ago

pacarvalho commented 5 years ago

I have noticed that under some peculiar situations it is possible to end up with a list that contains duplicate positions in acts_at_list. The duplicate positions cause unexpected behaviors to acts_as_list and sometimes leads to errors.

Would it be OK if I created a PR with a function that fixes lists that contain duplicate positions?

It could be done by arbitrarily choosing an order for the items with duplicated positions (there is no way of knowing which should come first with respect to one another) and moving other items out of the way.

Related to #76 #317

brendon commented 5 years ago

Hi @pacarvalho, yes that's a known fault and usually occurs due to simultaneous requests that manipulate the list. I'd be very keen for you to come up with a way to self-heal lists like these that isn't too too expensive to run. If you have a look through some of the issues on this repo you'll see there's been attempts to implement various locking mechanisms. The latest involved an advisory lock but it turned out it needed more restructuring to the gem than the author was prepared to do at the time, but you're more than welcome to continue his work :)

DanielHeath commented 5 years ago

I've got an efficient solution for this for postgres (using window functions) and mysql (using connection variables), but sqlite lacks a nice way to do this.

brendon commented 5 years ago

Hi @DanielHeath, this looks promising. I see it in your PR :) The only thing I can think of is that it'd be nice to tidy up these code forks for specific database backends into seperate files so they can be maintained more easily?

DanielHeath commented 5 years ago

Happy to split it up, but would appreciate some guidance about what shape it should take - I tend to favor "too much code" rather than "where's the code", when making that sort of judgement call so I'm less familiar with good patterns for this kind of split.

brendon commented 5 years ago

Hehe, yea, I'm not 100% sure I like the current project structure with all those definers. I like the way ancestry structures their code. But that's a project for another day.

I'm thinking perhaps when loading methods into the model, we could do a check for the current database adapter (on the model - as some projects will have different databases on different models), we then include a file based on whether it's sqlite, mysql or postgres that have identical methods with different implementations?

brendon commented 3 months ago

Closing this for now. Let me know if you want to look at it again. Check out https://github.com/brendon/positioning as an alternative to acts_as_list that uses an advisory lock to prevent corruption.