mergesort / Boutique

✨ A magical persistence library (and so much more) for state-driven iOS and Mac apps ✨
https://build.ms/boutique/docs
MIT License
899 stars 43 forks source link

Fix for unexpected removeItems chaining behavior #60

Closed mergesort closed 5 months ago

mergesort commented 5 months ago

Boutique was exhibiting incorrect behavior when chaining the remove() function with an insert() after, due to an underlying implementation bug. The code below demonstrates how the bug manifested.

// We start with self.items = [1, 2, 3, 4, 5, 6, 7, 8]

// An API call is made and we receive 1, 2, 3, 8, 9, and 10 to be inserted into to self.items. We pass that `updatedItems` array into an `update` function that removes any items that need to be removed, and then inserts the newly updated items.

func update(_ updatedItems: [Int]) async throws {
    let items = self.items.filter({ updatedItems.contains($0) })

    try await self.$items
        .remove(items)
        .insert(updatedItems)
        .run()
}

// `self.items` now should be [1, 2, 3, 4, 5, 6, 7, 8] 
// `self.items` is actually [10] 

Unfortunately I made an assumption that when the size of the items being removed and inserted was the same, Boutique would use ItemRemovalStrategy.removeAll rather than ItemRemovalStrategy.removeItems(items). In practice this made sense in a very early version of Boutique, but does not any more and can lead to unexpected errors. The result is that every time a remove was chained to an insert, removeAll would be called, leaving a Store with just it's last element.

zhigang1992 commented 2 months ago

Thanks for the fix. 👍

In the case of removeAll()

    try await self.$items
        .removeAll()
        .insert(updatedItems)
        .run()

The current implementation will not remove 4,5,6 right. Because the ItemRemovalStrategy was only applied to the list of updatedItems?

Which is a bit counter intuitive, and might cause stale data to lingering around in the store.

mergesort commented 2 months ago

Hey @zhigang1992, I believe this issue should actually be fixed on the chained-remove-fix branch, the specific issue you mentioned in this commit. I had not noticed this behavior earlier but discovered it last week, along with a few other improvements to performance and API behavior.

I plan on merging this into main over the next couple of days, it's been useful to use Plinky as a testbed with a lot of Boutique code but I think the code is stable enough for me to ship to production. If this doesn't address the issue do let me know, I'd like to make sure I cover all of the use cases people have for Boutique.