Prevent sorting by identical dates from duplicating documents

[microadam]

With documents like:

.insert({ _id: 'a', date: new Date(2017, 01, 01) })
.insert({ _id: 'b', date: new Date(2017, 01, 01) })
.insert({ _id: 'c', date: new Date(2018, 01, 01) })

and a find like:

.find({}, { sort: { date: 1 } })

Documents are returned like:

{ _id: 'a', date: new Date(2017, 01, 01) }
{ _id: 'b', date: new Date(2017, 01, 01) }
{ _id: 'a', date: new Date(2017, 01, 01) }
{ _id: 'b', date: new Date(2017, 01, 01) }
{ _id: 'c', date: new Date(2018, 01, 01) }

This fixes the problem.

The issue is due to _.uniq not being able to unique date objects. This converts the date objects to strings so they can be uniqued and then sorted appropriately.

[kofrasa]

Thank you for identifying and submitting a fix for this bug.

I do believe however that it may be a good idea to fix the underlying root cause which would also address undiscovered bugs affected by _.uniq not working as expected.

We could introduce a more appropriate uniq function based _.isEqual which is able to compare objects deeply and works for Date.

[microadam]

Hi @kofrasa.

Looking into your suggestion, its not possible to pass a custom comparator function to _.uniq (only a custom itaratee) and due to the nature of mingo being node and browser compatible, bringing in another module which does support a custom comparator isn't really that feasible. Would you be accepting of literally copying and pasting the code from another module which does what we need (https://www.npmjs.com/package/uniq looks like it would do the trick) and using that logic? Otherwise we could write our own (by why re-invent the wheel), or just use my fix as is, because I can't see a use case where someone would want to sort a collection by an object or an array, the only data types that would make any sense are numbers, strings and dates. _.uniq can handle numbers and strings fine on its own and with my modification, dates would be handled as well.

Would be interested to hear your thoughts on the above!

Thanks

[kofrasa]

Hi @microadam

My tests given below show that both _.uniq and uniq do not work correctly for collection of objects. This means that the assumption that _.uniq will be operating only on primitive values is false in the context of Mingo (unit tests assume that implicitly). I concur with avoiding new dependencies, but that also includes not copying code directly from other projects unless absolutely necessary.

Given that _.isEqual already does the right thing, a desirable unique function should not be too much work to implement. That way existing and future code benefit. Similarly, I added an internal groupBy function to work around _.groupBy not meeting the project needs.

For user-defined types as sort keys, that will mean sorting them in natural order post object "stringification".

Would like your feedback.

Testing _.uniq and uniq (I used "Try it out" links on npm page)

function Person(name, age) {
    this.name = name;
    this.age = age;
}

var a = new Person("john", 12);
var b = new Person("john", 12);
var c = {name: "john", age: 12};
var d = {name: "john", age: 12};

// we expect 1 in both cases
console.log(uniq([a,b]).length); // 2 
console.log(uniq([c,d]).length); // 2

[kofrasa]

I noticed that switching to using the internal groupBy function fixes this issue because the function uses _.isEqual for checking equality thus retaining only the unique objects. Will push an update shortly

[microadam]

Hi @kofrasa, sounds great to me! I was just about to pick this up again. Thanks for coming up with a better fix, I look forward to seeing it!

[microadam]

@kofrasa I have update this PR, this seems to fix my issue, is this the sort of thing you were thinking of?

[kofrasa]

Yes something similar to what you have. I added a unit test based on your example and took the opportunity to also change the implementation of groupBy to use hash codes. See release 0.6.5.

Thanks for the update.

[microadam]

Looks like it works to me! Thank you very much :) will close this PR!