Fix incorrect results in collection when models have duplicate ids #1846
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- If either or both the merge and remove options are specified then any duplicates are ignored when attempting to add them to a collection, otherwise they will be added.
- This allows it to behave similarly to regular fetch and is currently only used to pass the merge and remove options to the collection.set method.
ricardograca
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduction
Fixes incorrect model attributes when fetching collections that contain models with duplicate primary keys.
If the database contains models with duplicate
idvalues, and these are fetched either withfetchAll()or by eager loading withwithRelated, the resulting collection will contain duplicate models with the exact same attributes even though the other attributes besidesidmay be different.See issue #1554 for more info.
Motivation
These duplicate values are obviously incorrect, but even worse is that the collection
lengthproperty is correct since it doesn't account for the duplicates, so there is incongruence between thelengthand the actual number of models in the collection.Proposed solution
This just ensures that the default documented behavior of
Collection#setis 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: falseoptions 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 sameidbut potentially different attributes.Fixes #1554.
This is a breaking change, since previously setting
merge: false, remove: falsewould 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 passingmerge: false, remove: falseoptions.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 ofCollection#lengthand 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.