Reindex after replacing a model

[Kekos]

When replacing an existing model with a new model on set() which both had the same value for an indexed attribute, the new model was removed from the index.

[dhritzkiv]

I'll review this more in depth tomorrow. Do you think you could add some tests for this? I.e. add tests that fail before this code change?

[dhritzkiv]

I've added tests (that would fail before this fix) and also tweaked the code slightly so that it indexes all models in toAdd, not just the first. Let me know if these changes are satisfactory.

I'll wait for a +1 from another member of the team before merging and releasing.

[wraithgar]

As I was reading through this trying to decide if the added for loop was in the right place, I couldn't figure out if perhaps we have another bug? I can't seem to find where we pass new models from set into this._index.

Perhaps that for loop should move outside the if (remove) clause?

[dhritzkiv]

After spending quite a bit of time reviewing this and trying to find the right place for the code, I think this is the best/simplest way to solve this bug.

I tried to fix the removal of indexing of remaining models within the _deIndex() method, but I didn't have any success. I feel that would be the ideal place to prevent deletion of indexes for existing models.

---

To answer your question: passing new models into _index() happens in the _addReference() method.

To answer your second question: I believe the deletion of secondary indexes and its fix is only applicable in the if (remove) clause. I couldn't write a test to prove this wrong.

[wraithgar]

ah thanks, missed the _addReference() part.

That's a +1 from me then!

[dhritzkiv]

Cool. I'll merge and release a bit later today.

[dhritzkiv]

Released as v2.0.1