detach can corrupt the identifierCache

[scytacki]

Bug report

• [x] I've checked documentation and searched for existing issues and discussions
• [x] I've made sure my project is based on the latest MST version
• [x] Fork this code sandbox or another minimal reproduction.

Sandbox link or minimal reproduction code

test("items detached from arrays don't corrupt identifierCache", () => {
    const Item = types.model("Item", { id: types.identifier })
    const ItemArray = types.model("ItemArray",
      { items: types.array(Item)})
    .actions(self => ({
        removeSecondItemWithDetach() {
            detach(self.items[1])
        }
    }));

    const smallArray = ItemArray.create({ items: [
        { id: "A" }, { id: "B" }, { id: "C" }, { id: "D" }, { id: "E"}
    ]})
    expect(resolveIdentifier(Item, smallArray, "E")).toBeDefined()
    smallArray.removeSecondItemWithDetach()
    expect(smallArray.items.length).toBe(4)
    expect(resolveIdentifier(Item, smallArray, "B")).toBeUndefined()
    expect(resolveIdentifier(Item, smallArray, "E")).toBeDefined()

    const largeArray = ItemArray.create({ items: [
        { id: "A" }, { id: "B" }, { id: "C" }, { id: "D" }, { id: "E" },
        { id: "F" }, { id: "G" }, { id: "H" }, { id: "I" }, { id: "J" },
        { id: "K" }
    ]})
    expect(resolveIdentifier(Item, largeArray, "K")).toBeDefined()
    largeArray.removeSecondItemWithDetach()
    expect(largeArray.items.length).toBe(10)
    expect(resolveIdentifier(Item, largeArray, "B")).toBeUndefined()
    expect(resolveIdentifier(Item, largeArray, "J")).toBeDefined()
    // ***** The following expectation fails ******
    expect(resolveIdentifier(Item, largeArray, "K")).toBeDefined()
})

Describe the expected behavior When an item is removed from an array of identified items the remaining items in the array should still be accessible via resolveIdentifier. They should also be available as references.

Describe the observed behavior When the array size is greater than 10, and the second item (index 1) is detached from the array, then all items in the array which have indexes starting with 1 get removed from the identifierCache. This means that resolveIdentifier and reference lookups fail for all of the items that had indexes starting with 1.

Reason for the failure The IdentifierCache#splitCache method is called by detach. splitCache uses a basePath = node.path and then looks or any nodes in the cache that match it with nodes[i].path.indexOf(basePath) === 0. Any matching nodes are removed from the identifierCache. In the test above, this basePath will be /items/1. That basePath will match the "B" item because it has the identical path, but it will also match the "K" item which has a path of /items/10.

Additional possible failures It seems like this code would cause other issues with other structures not just arrays. For example if the node being detached had a path of /item then any nodes with paths of /items/* would also be removed from the identifierCache.

[coolsoftwaretyler]

Hey @scytacki - thanks for the bug report and reproduction/test case/PR. This is awesome!

I'm going to add some labels to this issue and assign it to you since you've already put together the PR. I will review the PR sometime in the coming days.

I'm not exactly sure what our release process is these days, but plan to meet with Jamon at the end of the week to iron that out.

[coolsoftwaretyler]

This will go out with 5.2.0 tomorrow, and already is out with 5.2.0-alpha.2. I'm going to close the issue ahead of time here. Thanks!