Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.84k stars 3.82k forks source link

Model.populate does not correctly populate Array paths, Changes to single object #6985

Closed Hyperblaster closed 5 years ago

Hyperblaster commented 5 years ago

Critical Bug (for us at least)

We've been using version mongoose 5.0.16 with no issues, We just upgraded to 5.2.13 and all hell has broken loose, not sure what the change is (i'm still looking through the diffs)

If we have a document like this and we call Model.populate on it


//Example document
var myDocument = {
    "title":"something",
    "articles":[
        "5b6a2ab047b5883e44b27814",
        "5b6a2ab047b5883e44b27813",
        "5b6a2ab047b5883e44b27813",
        "5b6a2ab047b5883e44b27813",
        "5b6a2ab047b5883e44b27813",
        ]
}

//Populate the articles
Node.populate(myDocument, {
    select:'title',
    path:'articles',
    options:{
        lean:true
    }
}, function(err, populated) {

});

//myDocument now looks like this (it's converted an array into an object)
var myDocument = {
    "title":"something",
    "articles":{
        "_id":"5b6a2ab047b5883e44b27814",
        "title":"title",
    }
}

Expected behaviour is this

var myDocument = {
    "title":"something",
    "articles":[
         {
            "_id":"5b6a2ab047b5883e44b27814",
            "title":"title",
        },
        {
            "_id":"5b6a2ab047b5883e44b27814",
            "title":"title",
        },
        ...
    ]
}
lineus commented 5 years ago

@Hyperblaster the following script works as expected for me. Can you update it to reproduce the issue?

6985.js

#!/usr/bin/env node
'use strict';

const assert = require('assert');
const mongoose = require('mongoose');
mongoose.connect('mongodb://localhost:27017/test', { useNewUrlParser: true });
const conn = mongoose.connection;
const Schema = mongoose.Schema;

const articleSchema = new Schema({
  title: String,
  content: String
});

const schema = new Schema({
  name: String,
  articles: [{
    type: Schema.Types.ObjectId,
    ref: 'article'
  }]
});

const Article = mongoose.model('article', articleSchema);
const Test = mongoose.model('test', schema);

const articles = [];

for (let i = 0; i < 10; i++) {
  let t = `title${i}`;
  let c = `this is the content for article title ${i}.`;
  articles.push(new Article({ title: t, content: c }));
}

const test = new Test({ name: String, articles: articles.map(o => o._id)});

async function run() {
  assert.strictEqual(mongoose.version, '5.2.13');
  await conn.dropDatabase();
  await test.save();
  await Article.create(articles);

  let doc = await Test.findOne({});
  let popObj = {
    path: 'articles',
    select: 'title',
    options: {
      lean: true
    }
  };
  let pDoc = await Test.populate(doc, popObj);
  assert.strictEqual(pDoc.articles.length, 10);
  pDoc.articles.forEach(a => assert.strictEqual(a.content, undefined));
  console.log('All Assertions Pass.');
  return conn.close();
}

run();

Output:

issues: ./6985.js
All Assertions Pass.
issues:
tiago-pereira commented 5 years ago

I'm having the same issue, but it only happens when running in the AWS. When running locally it works as expected.

vkarpov15 commented 5 years ago

@Hyperblaster @tiago-pereira can you please post your schemas?

tiago-pereira commented 5 years ago
const getContentSchema = targetRefPath => {
  const contentSchema = new Schema(
    {
      _contentId: {
        type: Schema.Types.ObjectId,
      },
      target: {
        type: Schema.Types.ObjectId,
        refPath: `${targetRefPath}.model`
      },
      targets: [{
        type: Schema.Types.Object,
        refPath: `${targetRefPath}.model`
      }],
      type: {
        type: String,
        required: true,
        enum: ['Exercise', 'Lesson'],
      },
    },
    {
      timestamps: true,
      _id:false,
      id:false,
      toJSON: {
        virtuals: true,
        transform: (obj, ret) => {
          delete ret._id;
        },
      },
    },
  );

  return contentSchema;
};
const programSchema = new Schema(
  {
    title: {
      type: String,
      required: true,
    },
    description: {
      type: String,
    },
    content: [getContentSchema('content')],
  },
  {
    usePushEach: true,
    timestamps: true,
    toJSON: {
      virtuals: true,
      transform: (obj, ret) => {
        delete ret._id;
      },
    },
  },
);
const exerciseSchema = new Schema(
  {
    subjects: [{
      type: Schema.Types.ObjectId,
      ref: 'Subject',
    }],
    title: {
      type:String,
      required:true
    },
    exercise_type: {
      type: String,
      enum: ['single_answer', 'multiple_answer'],
      default: 'single_answer'
    },
    alternatives: [{text: String, correct: Boolean}],
    resolution: String
  },
);

And the find:

const programInstance = await Program.findById(params.program)
      .populate('content.targets');

The idea here is to have a Program with a list of Content. The Content, if type 'Exercise', has a list of ExerciseIds in targets.

The log without the populate shows targets: [Array], with populate, targets: [Object]

Hyperblaster commented 5 years ago

Sorry! I left out this piece of the puzzle, the part i am trying to populate is actually schemaless and a subfield of a Schema.Types.Mixed,

I'm not sure if that helps narrow down the issue, But yeah it's always worked for me until upgrading the mongoose package last week, then it broke everything and i had to revert

var MySchema = new Schema({
    title: {
        type: String,
        trim: true,
    },
    data: {
        type: Schema.Types.Mixed,
    },
});

//Example document
var myDocument = {
    "title":"something",
    "data":{
        articles:[
            "5b6a2ab047b5883e44b27814",
            "5b6a2ab047b5883e44b27813",
            "5b6a2ab047b5883e44b27813",
            "5b6a2ab047b5883e44b27813",
            "5b6a2ab047b5883e44b27813",
        ]
    }
}

//Populate the articles
Node.populate(myDocument, {
    select:'title',
    path:'data.articles',
    options:{
        lean:true
    }
}, function(err, populated) {

    //myDocument now looks like this (it's converted an array into an object)
    //populated looks like this (typeof populated.articles == object)
    {
        "title":"something",
        "articles":{
            "_id":"5b6a2ab047b5883e44b27814",
            "title":"title",
        }
    }

    //It should look like this (Array)
    {
        "title":"something",
        "articles":[{
            "_id":"5b6a2ab047b5883e44b27814",
            "title":"title",
        }, {
            "_id":"5b6a2ab047b5883e44b27813",
            "title":"title",
        }]
    }

});
lineus commented 5 years ago

@Hyperblaster Thanks for clarifying. I am able to reproduce this with the following example:

Note: I added the model property to the population object

6985.js

#!/usr/bin/env node --no-deprecation
'use strict';

const assert = require('assert');
const mongoose = require('mongoose');
mongoose.connect('mongodb://127.0.0.1:50794/test'/*, { useNewUrlParser: true }*/);
const conn = mongoose.connection;
const Schema = mongoose.Schema;

const articleSchema = new Schema({
    title: String,
    content: String
});

const schema = new Schema({
    title: {
        type: String,
        trim: true,
    },
    data: {
        type: Schema.Types.Mixed,
    },
});

const Article = mongoose.model('article', articleSchema);
const Test = mongoose.model('test', schema);

const articles = [];

for (let i = 0; i < 10; i++) {
    let t = `title${i}`;
    let c = `this is the content for article title ${i}.`;
    articles.push(new Article({ title: t, content: c }));
}

const test = new Test({ title: 'hello', data: { articles: articles.map(o => o._id) } });

async function run() {
    assert.strictEqual(mongoose.version, process.argv[2]);
    await conn.dropDatabase();
    await test.save();
    await Article.create(articles);

    let doc = await Test.findOne({});

    let popObj = {
        path: 'data.articles',
        select: 'title',
        model: 'article',
        options: {
            lean: true
        }
    };

    let pDoc = await Test.populate(doc, popObj);
    assert.strictEqual(pDoc.data.articles.length, 10, 'length not equal to 10.');
    pDoc.data.articles.forEach((a,i) => assert.strictEqual(a.title, `title${i}`));
    pDoc.data.articles.forEach(a => assert.strictEqual(a.content, undefined));
    console.log('All Assertions Pass.');
    return conn.close();
}

run().catch(console.error);

Output on 5.0.16:

issues: ./6985.js '5.0.16'
All Assertions Pass.

Output on 5.2.14:

issues: ./6985.js '5.2.14'
{ AssertionError [ERR_ASSERTION]: length not equal to 10.
    at run (/Users/lineus/dev/node/mongoose/issues/6985.js:56:12)
    at process._tickCallback (internal/process/next_tick.js:68:7)
  generatedMessage: false,
  name: 'AssertionError [ERR_ASSERTION]',
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: 10,
  operator: 'strictEqual' }
^C
issues:
Hyperblaster commented 5 years ago

Awesome! Please let me know if i can be of more help

On 13 Sep 2018, at 5:53 am, Kev notifications@github.com wrote:

@Hyperblaster Thanks for clarifying. I am able to reproduce this with the following example:

Note: I added the model property to the population object

6985.js

!/usr/bin/env node --no-deprecation

'use strict';

const assert = require('assert'); const mongoose = require('mongoose'); mongoose.connect('mongodb://127.0.0.1:50794/test'/, { useNewUrlParser: true }/); const conn = mongoose.connection; const Schema = mongoose.Schema;

const articleSchema = new Schema({ title: String, content: String });

const schema = new Schema({ title: { type: String, trim: true, }, data: { type: Schema.Types.Mixed, }, });

const Article = mongoose.model('article', articleSchema); const Test = mongoose.model('test', schema);

const articles = [];

for (let i = 0; i < 10; i++) { let t = title${i}; let c = this is the content for article title ${i}.; articles.push(new Article({ title: t, content: c })); }

const test = new Test({ title: 'hello', data: { articles: articles.map(o => o._id) } });

async function run() { assert.strictEqual(mongoose.version, process.argv[2]); await conn.dropDatabase(); await test.save(); await Article.create(articles);

let doc = await Test.findOne({});

let popObj = {
    path: 'data.articles',
    select: 'title',
    model: 'article',
    options: {
        lean: true
    }
};

let pDoc = await Test.populate(doc, popObj);
assert.strictEqual(pDoc.data.articles.length, 10, 'length not equal to 10.');
pDoc.data.articles.forEach((a,i) => assert.strictEqual(a.title, `title${i}`));
pDoc.data.articles.forEach(a => assert.strictEqual(a.content, undefined));
console.log('All Assertions Pass.');
return conn.close();

}

run().catch(console.error); Output on 5.0.16:

issues: ./6985.js '5.0.16' All Assertions Pass. Output on 5.2.14:

issues: ./6985.js '5.2.14' { AssertionError [ERR_ASSERTION]: length not equal to 10. at run (/Users/lineus/dev/node/mongoose/issues/6985.js:56:12) at process._tickCallback (internal/process/next_tick.js:68:7) generatedMessage: false, name: 'AssertionError [ERR_ASSERTION]', code: 'ERR_ASSERTION', actual: undefined, expected: 10, operator: 'strictEqual' } ^C issues: — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

vkarpov15 commented 5 years ago

Found the issue. This is due to our recent changes to consolidate the internal justOne option, which determines whether the output should be an array or single doc. In the case of a Mixed type, Mongoose shouldn't infer that justOne = true, we fixed that in c93e256.

In d1dbcda we added an override. You can explicitly overwrite the justOne populate option if Mongoose is inferring it incorrectly from your schema. This will help provide a workaround for any other bugs related to justOne that pop up.

      const popObj = {
        path: 'data.articles',
        select: 'title',
        model: 'gh6985_Article_0',
        justOne: true, // Explicitly say that you want the output as a doc rather than an array
        options: { lean: true }
      };

      res = yield Test.populate(res, popObj);
Hyperblaster commented 5 years ago

Why can it not simply work the way it did before, (understanding that if the source you are populating is an array return an array, if its a single object id then return an object)

It seems a bit strange to now have to specify each time you populate a field whether it’s an array or an object

On 19 Sep 2018, at 12:09 am, Valeri Karpov notifications@github.com wrote:

Found the issue. This is due to our recent changes to consolidate the internal justOne option, which determines whether the output should be an array or single doc. In the case of a Mixed type, Mongoose shouldn't infer that justOne = true, we fixed that in c93e256.

In d1dbcda we added an override. You can explicitly overwrite the justOne populate option if Mongoose is inferring it incorrectly from your schema. This will help provide a workaround for any other bugs related to justOne that pop up.

  const popObj = {
    path: 'data.articles',
    select: 'title',
    model: 'gh6985_Article_0',
    justOne: true, // Explicitly say that you want the output as a doc rather than an array
    options: { lean: true }
  };

  res = yield Test.populate(res, popObj);

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

trueter commented 5 years ago

I agree with @Hyperblaster. For us, this is a major breaking change.

vkarpov15 commented 5 years ago

@trueter @Hyperblaster you don't have to specify justOne, Mongoose should be able to infer it from the schema. The justOne option is a fallback in case Mongoose is giving you an array when you expect an object and vice versa. There are no intentional backwarks breaking changes here.

@trueter if you're having an issue related to this, please open up a separate issue with code samples.

trueter commented 5 years ago

@vkarpov15 Thanks for the update. I noticed one place where issues occured specified key: Array instead of key: [Object] in the schema. I'll give it another shot and will follow up if necessary.

vkarpov15 commented 5 years ago

@trueter thanks. Looking forward to seeing what you come up with :+1: