Fix incorrect results in collection when models have duplicate ids

[ricardograca]

• Related Issues: Duplicate IDs cause incorrect return results #1554, duplicate id's in collection #964

Introduction

Fixes incorrect model attributes when fetching collections that contain models with duplicate primary keys.

If the database contains models with duplicate id values, and these are fetched either with fetchAll() or by eager loading with withRelated, the resulting collection will contain duplicate models with the exact same attributes even though the other attributes besides id may be different.

See issue #1554 for more info.

Motivation

These duplicate values are obviously incorrect, but even worse is that the collection length property is correct since it doesn't account for the duplicates, so there is incongruence between the length and the actual number of models in the collection.

Proposed solution

This just ensures that the default documented behavior of Collection#set is enforced when dealing with duplicates, that is, any duplicates are merged, resulting in the correct number of models in the collection matching the collection length.

However it is now possible to override this behavior by using both the merge: false, remove: false options which will make sure that any duplicates are not merged and existing models that are not present in the new set are not removed, resulting in the coexistence of models with the same id but potentially different attributes.

Fixes #1554.

This is a breaking change, since previously setting merge: false, remove: false would just cause any duplicates to be ignored.

Current PR Issues

The proposed solution means that you can't both remove non existing models in the new set and maintain duplicates. This kind of makes sense since you're saying that you don't want to remove any models from the collection but you also don't want to merge duplicates, although it may not be very obvious.

This also doesn't change the default of not allowing duplicates by default, but changing that would be an even greater breaking change, since it's very possible that users are using that functionality to merge duplicates into already existing collections. At least one test in the test suite (provides "attach" for creating or attaching records) is expecting a merge to happen.

As it is, users have to know they want to deal with duplicate ids to be able to take advantage of this bug fix since duplicates will only be present when passing merge: false, remove: false options.

Alternatives considered

A better alternative would be to allow duplicates by default in Collection#fetch(), Model#fetchAll(), and when eager loading collections, but that would break the current value of Collection#length and would require substantially more work and would also break backwards compatibility. Maybe a future PR or some other refactoring will address this, although the plan is to remove the concept of collections for simple arrays which would not have any of these problems, since data is just returned as it is in the database.