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

Sorting during population of a path in a subdocument attaches the populated document to the wrong subdocument #2202

Closed
paton opened this Issue Jul 20, 2014 · 23 comments

Comments

Projects
None yet
@paton
Contributor

paton commented Jul 20, 2014

Sorting of populated documents within subdocuments results in the populated document being attached to the wrong subdocument.

Mongoose: 3.8.13
Mongodb: 2.6.3

Check out the differing quantity fields when sorting the populated product field ascending vs descending...

// Ascending sorting of populated product by "name"
{
  suborders: [
    { _id: '...14cb', product: { name: 'Apple' }, quantity: 1 }  // Correct quantity
    { _id: '...14ca', product: { name: 'Banana' }, quantity: 2 }  // Correct quantity
  ]
}

// Descending sorting of populated product by "name"
{
  suborders: [
    { _id: '...14cb', product: { name: 'Banana' }, quantity: 1 }  // INCORRECT quantity
    { _id: '...14ca', product: { name: 'Apple' }, quantity: 2 }  // INCORRECT quantity
  ]
}

Ascending sort: 1 apple, 2 bananas
Descending sort: 2 apples, 1 banana

Apple should always be attached to the subdocument with _id ending in '14cb', but in the descending sort it's attached to the suborder with _id ending in '14ca'.

Failing test code below...

// SETUP SCHEMA 

var ProductSchema = new mongoose.Schema({ name: String });
mongoose.model('Product', ProductSchema);

var SuborderSchema = new mongoose.Schema({
  product: {
    type: mongoose.Schema.Types.ObjectId,
    ref: 'Product'
  },
  quantity: Number
});

var OrderSchema = new mongoose.Schema({ 
  suborders: [SuborderSchema] 
});
mongoose.model('Order', OrderSchema);


//// FAILING TEST

var Order = mongoose.model('Order');
var Product = mongoose.model('Product');

// Create 2 products
Product.create([
  { name: 'Apple' }, 
  { name: 'Banana' }
], function(err, apple, banana) {

  // Create order with suborders
  Order.create({
    suborders: [
      {
        product: apple._id,
        quantity: 1
      }, {
        product: banana._id,
        quantity: 2
      }
    ]
  }, function(err, order) {

    // Populate with ascending sort 
    Order.findById(order._id)
      .populate({
        path: 'suborders.product',
        options: { sort: 'name' } // ASCENDING SORT
      }).exec(function(err, order) {
        console.log('Ascending sort');
        console.log(order.suborders);
      }
    );

    // Populate with descending sort
    Order.findById(order._id)
      .populate({
        path: 'suborders.product',
        options: { sort: '-name' } // DESCENDING SORT
      }).exec(function(err, order) {
        console.log('Descending sort');
        console.log(order.suborders);
      }
    );
  });
});

@vkarpov15 vkarpov15 added the bug? label Jul 21, 2014

@hex7c0

This comment has been minimized.

Show comment
Hide comment
@hex7c0

hex7c0 Jan 18, 2015

same problem with mongoose 3.9.6 and descending sort

hex7c0 commented Jan 18, 2015

same problem with mongoose 3.9.6 and descending sort

@GlennGeenen

This comment has been minimized.

Show comment
Hide comment
@GlennGeenen

GlennGeenen Mar 18, 2015

Same on 3.8.25 with node v0.10.37 and MongoDB 3.0.1

Test:

var mongoose = require('mongoose');
var assert = require('assert');

mongoose.set('debug', true);
mongoose.connect('mongodb://localhost/test2202');

var ProductSchema = new mongoose.Schema({
  name: String
});
mongoose.model('Product', ProductSchema);

var SuborderSchema = new mongoose.Schema({
  product: {
    type: mongoose.Schema.Types.ObjectId,
    ref: 'Product'
  },
  quantity: Number
});

var OrderSchema = new mongoose.Schema({
  suborders: [SuborderSchema]
});
mongoose.model('Order', OrderSchema);

var Order = mongoose.model('Order');
var Product = mongoose.model('Product');

Product.create([
  {
    name: 'Apple'
  },
  {
    name: 'Banana'
  }
], function (err, apple, banana) {

  Order.create({
    suborders: [
      {
        product: apple._id,
        quantity: 1
      }, {
        product: banana._id,
        quantity: 2
      }
    ]
  }, function (err, order) {

    Order.findById(order._id)
      .populate({
        path: 'suborders.product',
        options: {
          sort: '-name'
        }
      }).exec(function (err, order) {

        assert.equal(order.suborders[0].product.name, 'Banana', 'DESC: 0 should be Banana');
        assert.equal(order.suborders[1].product.name, 'Apple', 'DESC: 1 should be Apple');

        assert.equal(order.suborders[0].quantity, 2, 'DESC: Banana quantity should be 2');
        assert.equal(order.suborders[1].quantity, 1, 'DESC: Apple quantity should be 1');

      });
  });
});

Same on 3.8.25 with node v0.10.37 and MongoDB 3.0.1

Test:

var mongoose = require('mongoose');
var assert = require('assert');

mongoose.set('debug', true);
mongoose.connect('mongodb://localhost/test2202');

var ProductSchema = new mongoose.Schema({
  name: String
});
mongoose.model('Product', ProductSchema);

var SuborderSchema = new mongoose.Schema({
  product: {
    type: mongoose.Schema.Types.ObjectId,
    ref: 'Product'
  },
  quantity: Number
});

var OrderSchema = new mongoose.Schema({
  suborders: [SuborderSchema]
});
mongoose.model('Order', OrderSchema);

var Order = mongoose.model('Order');
var Product = mongoose.model('Product');

Product.create([
  {
    name: 'Apple'
  },
  {
    name: 'Banana'
  }
], function (err, apple, banana) {

  Order.create({
    suborders: [
      {
        product: apple._id,
        quantity: 1
      }, {
        product: banana._id,
        quantity: 2
      }
    ]
  }, function (err, order) {

    Order.findById(order._id)
      .populate({
        path: 'suborders.product',
        options: {
          sort: '-name'
        }
      }).exec(function (err, order) {

        assert.equal(order.suborders[0].product.name, 'Banana', 'DESC: 0 should be Banana');
        assert.equal(order.suborders[1].product.name, 'Apple', 'DESC: 1 should be Apple');

        assert.equal(order.suborders[0].quantity, 2, 'DESC: Banana quantity should be 2');
        assert.equal(order.suborders[1].quantity, 1, 'DESC: Apple quantity should be 1');

      });
  });
});

@vkarpov15 vkarpov15 added this to the 3.8.26 milestone Mar 18, 2015

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Mar 18, 2015

Collaborator

Thanks for the repro @GlennGeenen, I'll look into this

Collaborator

vkarpov15 commented Mar 18, 2015

Thanks for the repro @GlennGeenen, I'll look into this

@vkarpov15 vkarpov15 modified the milestones: 3.8.27, 3.8.26 Mar 31, 2015

@vkarpov15 vkarpov15 modified the milestones: 4.2, 3.8.27 Apr 13, 2015

@vkarpov15 vkarpov15 added confirmed-bug and removed bug? labels Apr 13, 2015

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Apr 13, 2015

Collaborator

Unfortunately this is a more significant issue than I thought - don't have an easy solution. PRs welcome, but I'm gonna have to postpone this one.

Collaborator

vkarpov15 commented Apr 13, 2015

Unfortunately this is a more significant issue than I thought - don't have an easy solution. PRs welcome, but I'm gonna have to postpone this one.

@vkarpov15 vkarpov15 modified the milestones: 4.3, 4.2 Sep 21, 2015

@PhantomRay

This comment has been minimized.

Show comment
Hide comment
@PhantomRay

PhantomRay Oct 26, 2015

I found out this as well using unit testing on my own application. It happens randomly depending on how the records got created in what order. It took me the whole day to figure out it is a bug in mongoose (4.2.2).

I found out this as well using unit testing on my own application. It happens randomly depending on how the records got created in what order. It took me the whole day to figure out it is a bug in mongoose (4.2.2).

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Oct 27, 2015

Collaborator

Yeah it's very tricky unfortunately, because mongoose needs to sort the whole array rather than just the docs that its going to populate. It's really gnarly to get this right with things like limit and skip in populate, which is why we haven't made much progress on this front. Ideas for how this should work in that case are most welcome.

Collaborator

vkarpov15 commented Oct 27, 2015

Yeah it's very tricky unfortunately, because mongoose needs to sort the whole array rather than just the docs that its going to populate. It's really gnarly to get this right with things like limit and skip in populate, which is why we haven't made much progress on this front. Ideas for how this should work in that case are most welcome.

@dmash

This comment has been minimized.

Show comment
Hide comment
@dmash

dmash Dec 3, 2015

I think you are using the options object incorrectly. Try using options: {sort: {name: 1}} or options: {sort: {name: -1} for descending.

dmash commented Dec 3, 2015

I think you are using the options object incorrectly. Try using options: {sort: {name: 1}} or options: {sort: {name: -1} for descending.

@PhantomRay

This comment has been minimized.

Show comment
Hide comment
@PhantomRay

PhantomRay Dec 3, 2015

No. It is a confirmed bug.

On Thu, 3 Dec 2015 at 14:29, domen notifications@github.com wrote:

I think you are using the options object incorrectly. Try using options:
{sort: {name: 1}} or options: {sort: {name: -1} for descending.


Reply to this email directly or view it on GitHub
#2202 (comment)
.

No. It is a confirmed bug.

On Thu, 3 Dec 2015 at 14:29, domen notifications@github.com wrote:

I think you are using the options object incorrectly. Try using options:
{sort: {name: 1}} or options: {sort: {name: -1} for descending.


Reply to this email directly or view it on GitHub
#2202 (comment)
.

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Dec 3, 2015

Collaborator

This is definitely a confirmed bug. A very messy confirmed bug that is admittedly quite difficult to fix. Contributions most welcome.

Collaborator

vkarpov15 commented Dec 3, 2015

This is definitely a confirmed bug. A very messy confirmed bug that is admittedly quite difficult to fix. Contributions most welcome.

@vkarpov15 vkarpov15 modified the milestones: 4.5, 4.4 Jan 6, 2016

@vkarpov15 vkarpov15 modified the milestones: 4.6, 4.5 May 3, 2016

@Ra1da35ma

This comment has been minimized.

Show comment
Hide comment
@Ra1da35ma

Ra1da35ma May 31, 2016

has this been fixed??

has this been fixed??

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Jun 1, 2016

Collaborator

@Ra1da35ma nope, that's why the issue is still open. PRs welcome but not a pressing priority ATM.

Collaborator

vkarpov15 commented Jun 1, 2016

@Ra1da35ma nope, that's why the issue is still open. PRs welcome but not a pressing priority ATM.

@ezecafre89

This comment has been minimized.

Show comment
Hide comment
@ezecafre89

ezecafre89 Nov 1, 2016

Hey guys, i'm facing the same issue, is there any workaround?
I really need to do this on my app and if it is not possible at all, i guess i'm going to have to give up on mongo and start thinking to migrate to some other database, that is a kind of a big deal at this point on my project.

Thanks for any advice!

ezecafre89 commented Nov 1, 2016

Hey guys, i'm facing the same issue, is there any workaround?
I really need to do this on my app and if it is not possible at all, i guess i'm going to have to give up on mongo and start thinking to migrate to some other database, that is a kind of a big deal at this point on my project.

Thanks for any advice!

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Nov 6, 2016

Collaborator

There's a couple workarounds - you can use JS' built-in sort() function to sort the array after it's loaded, or you can bypass populate and execute the query yourself.

Collaborator

vkarpov15 commented Nov 6, 2016

There's a couple workarounds - you can use JS' built-in sort() function to sort the array after it's loaded, or you can bypass populate and execute the query yourself.

@paton

This comment has been minimized.

Show comment
Hide comment
@paton

paton Nov 6, 2016

Contributor

Until this is fixed, I'd recommend returning an error to queries using the descending sort option in populate in cases where it can lead to incorrect data, if that's feasible. Silently failing and returning corrupt data (in this or similar bugs) could cause hard to discover, major issues in production apps.

The original example of apples and bananas was silly.

Ascending sort: 1 apple, 2 bananas
Descending sort: 2 apples, 1 banana

More seriously: in our real-world production application we have an "Organization" schema with user permissions embedded as subdocuments associated with a User _id that is populated for backend user-permission / authentication checks. We also populate various permissions on the User model that are also used in user-permission checks.

If one of our developers were assigned the task of alphabetizing the list of Organization Users in our application UI as an example, it's very possible they would start by adding the sort option to the populate() query. It may appear to sort the list in the UI while not obviously breaking the application, leading the developer to submit a PR. Anyone reviewing such PR would merge it immediately since it's just a simple 1-line sort option that looks completely innocuous. It would likely get merged unless the subtly corrupt data happened to trigger something in a test suite to fail. In this example, things can become problematic if the data that's used to render the UI is also used for permission checks or authentication or handling of other secured data.

It's a very real possibility that returning corrupt data could cause critical user permission and security vulnerabilities with severe implications in a large-scale production environment.

I understand this is an edge case and it's complicated to fix. If this were simply a case of incorrect sorting, it would be a minor issue.

But this operation returns corrupt data by incorrectly swapping values between documents, resulting in inconsistent data returned to the application.

@vkarpov15 Should we prioritize a patch to return an error for this edge-case (and any other edge cases that can result in data corruption) until a permanent fix is proposed?

On a related note, since Mongoose has grown to be become so widely used, likely reaching hundreds of millions of end-users through applications that rely on it, should we consider adding a "known issues" page somewhere to highlight any bugs that might in any way compromise the validity of data written to or returned from the database via mongoose?

Apologies for the long rant :) TLDR - I feel strongly that verified bugs affecting the consistency or validity of data in a widely used ORM / ODM should be prominently noted. This would help developers build more secure + reliable applications, while hopefully getting more people (and companies) to contribute resources toward maintaining and improving mongoose long-term.

Contributor

paton commented Nov 6, 2016

Until this is fixed, I'd recommend returning an error to queries using the descending sort option in populate in cases where it can lead to incorrect data, if that's feasible. Silently failing and returning corrupt data (in this or similar bugs) could cause hard to discover, major issues in production apps.

The original example of apples and bananas was silly.

Ascending sort: 1 apple, 2 bananas
Descending sort: 2 apples, 1 banana

More seriously: in our real-world production application we have an "Organization" schema with user permissions embedded as subdocuments associated with a User _id that is populated for backend user-permission / authentication checks. We also populate various permissions on the User model that are also used in user-permission checks.

If one of our developers were assigned the task of alphabetizing the list of Organization Users in our application UI as an example, it's very possible they would start by adding the sort option to the populate() query. It may appear to sort the list in the UI while not obviously breaking the application, leading the developer to submit a PR. Anyone reviewing such PR would merge it immediately since it's just a simple 1-line sort option that looks completely innocuous. It would likely get merged unless the subtly corrupt data happened to trigger something in a test suite to fail. In this example, things can become problematic if the data that's used to render the UI is also used for permission checks or authentication or handling of other secured data.

It's a very real possibility that returning corrupt data could cause critical user permission and security vulnerabilities with severe implications in a large-scale production environment.

I understand this is an edge case and it's complicated to fix. If this were simply a case of incorrect sorting, it would be a minor issue.

But this operation returns corrupt data by incorrectly swapping values between documents, resulting in inconsistent data returned to the application.

@vkarpov15 Should we prioritize a patch to return an error for this edge-case (and any other edge cases that can result in data corruption) until a permanent fix is proposed?

On a related note, since Mongoose has grown to be become so widely used, likely reaching hundreds of millions of end-users through applications that rely on it, should we consider adding a "known issues" page somewhere to highlight any bugs that might in any way compromise the validity of data written to or returned from the database via mongoose?

Apologies for the long rant :) TLDR - I feel strongly that verified bugs affecting the consistency or validity of data in a widely used ORM / ODM should be prominently noted. This would help developers build more secure + reliable applications, while hopefully getting more people (and companies) to contribute resources toward maintaining and improving mongoose long-term.

vkarpov15 added a commit that referenced this issue Nov 11, 2016

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Nov 11, 2016

Collaborator

Yeah I'm going to put this in the FAQ. This is a longstanding bug in mongoose that's tricky to fix. This is why you should be extra careful in testing your code :)

Collaborator

vkarpov15 commented Nov 11, 2016

Yeah I'm going to put this in the FAQ. This is a longstanding bug in mongoose that's tricky to fix. This is why you should be extra careful in testing your code :)

vkarpov15 added a commit that referenced this issue Jan 26, 2017

@vkarpov15 vkarpov15 added the fixed? label Jan 26, 2017

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Jan 26, 2017

Collaborator

@paton made it return an error. Very tricky to come up with a way to do this since populate is not architected to handle sort, limit, and skip underneath document arrays. Reworking the current architecture will be a substantial project that will take time to scope out.

Collaborator

vkarpov15 commented Jan 26, 2017

@paton made it return an error. Very tricky to come up with a way to do this since populate is not architected to handle sort, limit, and skip underneath document arrays. Reworking the current architecture will be a substantial project that will take time to scope out.

@vkarpov15 vkarpov15 closed this in 1062a17 Jan 29, 2017

@rknell

This comment has been minimized.

Show comment
Hide comment
@rknell

rknell Mar 22, 2017

:( just got hit by error when it was working before.

So should I just sort the records manually after a populate?

rknell commented Mar 22, 2017

:( just got hit by error when it was working before.

So should I just sort the records manually after a populate?

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Mar 28, 2017

Collaborator

Sorry for the inconvenience @rknell . Yeah, your only options right now are to sort the records manually after populate or not use populate.

Collaborator

vkarpov15 commented Mar 28, 2017

Sorry for the inconvenience @rknell . Yeah, your only options right now are to sort the records manually after populate or not use populate.

simison added a commit to Trustroots/trustroots that referenced this issue May 4, 2017

Don't sort populated Mongo documents
Sorting populated documents is not possible at the moment due bug in
Mongoose:

http://mongoosejs.com/docs/faq.html#populate_sort_order
Automattic/mongoose#2202
@mrm8488

This comment has been minimized.

Show comment
Hide comment
@mrm8488

mrm8488 Jan 10, 2018

I think it can be solved using the MongoDB's aggregation framework. With the $lookup operator we can "join" the two collections, then, we can project and sort the fields.

mrm8488 commented Jan 10, 2018

I think it can be solved using the MongoDB's aggregation framework. With the $lookup operator we can "join" the two collections, then, we can project and sort the fields.

@EmixMaxime

This comment has been minimized.

Show comment
Hide comment
@EmixMaxime

EmixMaxime Jul 28, 2018

Could you provide an example @mrm8488? I've the same problem, I had to sort with JS.

I've also a problem with limit, it works only on doc not on arrays :/

Thanks all!

EmixMaxime commented Jul 28, 2018

Could you provide an example @mrm8488? I've the same problem, I had to sort with JS.

I've also a problem with limit, it works only on doc not on arrays :/

Thanks all!

@mrm8488

This comment has been minimized.

Show comment
Hide comment
@mrm8488

mrm8488 Jul 28, 2018

Sure, let me know the schema of your collection and the query you wanna do.

mrm8488 commented Jul 28, 2018

Sure, let me know the schema of your collection and the query you wanna do.

@atulmy

This comment has been minimized.

Show comment
Hide comment
@atulmy

atulmy Jul 29, 2018

@mrm8488

Can you provide an example using $lookup in Mongoose for following case:

User model

User {
  _id,
  name.
  ...
}

Participant Model

Participant {
  userId,
  ...
}

I'm trying:

Participant
  .find()
  .populate({ 
    path: 'userId', 
    options: { sort: { name : 1 } } 
  })

atulmy commented Jul 29, 2018

@mrm8488

Can you provide an example using $lookup in Mongoose for following case:

User model

User {
  _id,
  name.
  ...
}

Participant Model

Participant {
  userId,
  ...
}

I'm trying:

Participant
  .find()
  .populate({ 
    path: 'userId', 
    options: { sort: { name : 1 } } 
  })
@mrm8488

This comment has been minimized.

Show comment
Hide comment
@mrm8488

mrm8488 Jul 31, 2018

@atulmy , here you have the solution: mrm8488/ama#5

mrm8488 commented Jul 31, 2018

@atulmy , here you have the solution: mrm8488/ama#5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment