Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug with bson objects in utils uniq function #1526

Merged
merged 1 commit into from
Dec 4, 2017

Conversation

pbouda
Copy link
Contributor

@pbouda pbouda commented Nov 29, 2017

Description

The uniq function does currently not work when the database is mongodb. In the case of mongodb, the function will receive an array of bson object of bson type ObjectID. The indexOf function in uniq will return a different index for the same mongodb ID, as it is wrapped in the ObjectID. This results in an array that has duplicate mongodb IDs. This commit first transforms any ObjectID in the given array to a string representation. We can then use indexOf to return unique items.

Replaces PR: #1418

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

The uniq function does currently not work when the database is mongodb.
In the case of mongodb, the function will receive an array of bson
object of bson type ObjectID. The indexOf function will return a
different index, even if the mongodb ID is the same, as it is wrapped
in the ObjectID. This commit first transforms any ObjectID in the array
to a string representation. We can then use indexOf to check for
uniqueness.
@kjdelisle
Copy link
Contributor

@slnode test please

@kjdelisle
Copy link
Contributor

@pbouda I'm good with this PR, but only if we switch to the bson implementation of ObjectID in the mongodb connector (I've created a linked PR for this).

@raymondfeng Your thoughts?

@kjdelisle
Copy link
Contributor

@slnode test please

@pbouda
Copy link
Contributor Author

pbouda commented Dec 4, 2017

@kjdelisle yes, makes sense to use bson also in the mongodb connector.

@kjdelisle
Copy link
Contributor

@slnode test please

@kjdelisle
Copy link
Contributor

@pbouda The failures don't seem related to any of the changes made; once the CI run is done, I'll check any remaining failures and if they're all bogus, then I'll land this.

@kjdelisle kjdelisle merged commit 6bd9fca into loopbackio:master Dec 4, 2017
@kjdelisle
Copy link
Contributor

🎉 @pbouda Thank you for your contribution! 🎉

kjdelisle pushed a commit that referenced this pull request Dec 14, 2017
 * Allow new transaction method in postgresql (#1493) (zbarbuto)
 * Fix bug in utils uniq function (#1526) (Peter Bouda)
 * Fix query for related models (#1522) (Joost de Bruijn)
 * chore:update license (#1521) (Diana Lau)
 * Allow customizing embedded relation property (#1513) (zbarbuto)
 * 📖 Typo on README.md (#1517) (JP Ventura)
 * CODEOWNERS: move @lehni to Alumni section (Miroslav Bajtoš)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants