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

Handle discriminators with insertMany() #7818

Closed
vkarpov15 opened this issue May 18, 2019 · 3 comments
Closed

Handle discriminators with insertMany() #7818

vkarpov15 opened this issue May 18, 2019 · 3 comments

Comments

@vkarpov15
Copy link
Collaborator

Re: #6087 (comment), there may be an issue with BaseModel.insertMany() not respecting discriminator keys.

@vkarpov15 vkarpov15 added this to the 5.5.10 milestone May 18, 2019
@vkarpov15 vkarpov15 added the needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue label May 18, 2019
@Fonger
Copy link
Contributor

Fonger commented May 18, 2019

I can't reproduce this on 5.5.9.
The following code works for me. I also tried with __t without { discriminatorKey: 'kind' },
pre('validation') and Number type with min, max, required validator.

repro script (can't reproduce)

  /**
   * When you use `Model.create()`, mongoose will pull the correct type from
   * the discriminator key for you.
   */
  it('Using discriminators with `Model.create()` or `Model.insertMany()`', function(done) {
    var Schema = mongoose.Schema;
    var shapeSchema = new Schema({
      name: String
    }, { discriminatorKey: 'kind' });
    var squareSchema = new Schema({ side: Number });
    var validateCount = 0;
    squareSchema.pre('validate', function() {
      validateCount++;
    });

    var Shape = db.model('Shape', shapeSchema);

    var Circle = Shape.discriminator('Circle',
      new Schema({ radius: Number }));
    var Square = Shape.discriminator('Square', squareSchema);

    var shapeObjects = [
      { name: 'Test' },
      { kind: 'Circle', radius: 5 },
      { kind: 'Square', side: 10 }
    ];

    Shape.create(shapeObjects).then(function(shapes) { // test model.create
      assert.ok(shapes[0] instanceof Shape);
      assert.ok(shapes[1] instanceof Circle);
      assert.equal(shapes[1].radius, 5);
      assert.ok(shapes[2] instanceof Square);
      assert.equal(shapes[2].side, 10);
      return Shape.insertMany(shapeObjects); // test model.insertMany
    }).then(function(shapes) {
      assert.ok(shapes[0] instanceof Shape);
      assert.ok(shapes[1] instanceof Circle);
      assert.equal(shapes[1].radius, 5);
      assert.ok(shapes[2] instanceof Square);
      assert.equal(shapes[2].side, 10);
      assert.equal(validateCount, 2);
      done();
    }).
    catch(done);
  });

Data in DB

$ npm test -- -g "Using discriminators with"

> mongoose@5.5.9 test /home/fonger/mongoose
> mocha --exit test/*.test.js test/**/*.test.js "-g" "Using discriminators with"


 You're not testing shards! 
 Please set the MONGOOSE_SHARD_TEST_URI env variable. 
 e.g: `mongodb://localhost:27017/database 
 Sharding must already be enabled on your database




  1 passing (130ms)


$ mongo
> use mongoose_test
switched to db mongoose_test
> db.shapes.find({}).pretty()
{ "_id" : ObjectId("5ce000d3a6a1014a1cc9fc46"), "name" : "Test", "__v" : 0 }
{
	"_id" : ObjectId("5ce000d3a6a1014a1cc9fc47"),
	"__t" : "Circle",
	"radius" : 5,
	"__v" : 0
}
{
	"_id" : ObjectId("5ce000d3a6a1014a1cc9fc48"),
	"__t" : "Square",
	"side" : 10,
	"__v" : 0
}
{ "_id" : ObjectId("5ce000d3a6a1014a1cc9fc49"), "name" : "Test", "__v" : 0 }
{
	"_id" : ObjectId("5ce000d3a6a1014a1cc9fc4a"),
	"__t" : "Circle",
	"radius" : 5,
	"__v" : 0
}
{
	"_id" : ObjectId("5ce000d3a6a1014a1cc9fc4b"),
	"__t" : "Square",
	"side" : 10,
	"__v" : 0
}

Unlike Model.create() which new Model based on its discriminator keys, Model.insertMany() directly call new _this(doc). I thought it was the source of bug. However it seems this is handled during the initialization of document internally and I was unable to reproduce this bug with the script above.

Model.create() new model based on discriminator key

mongoose/lib/model.js

Lines 2874 to 2882 in 5011b56

args.forEach(doc => {
toExecute.push(callback => {
const Model = this.discriminators && doc[discriminatorKey] != null ?
this.discriminators[doc[discriminatorKey]] || getDiscriminatorByValue(this, doc[discriminatorKey]) :
this;
if (Model == null) {
throw new Error(`Discriminator "${doc[discriminatorKey]}" not ` +
`found for model "${this.modelName}"`);
}

mongoose/lib/model.js

Lines 2894 to 2897 in 5011b56

if (!(toSave instanceof Model)) {
try {
toSave = new Model(toSave);
} catch (error) {

Model.insertMany() new model based on this

mongoose/lib/model.js

Lines 3061 to 3065 in 5011b56

arr.forEach(function(doc) {
toExecute.push(function(callback) {
if (!(doc instanceof _this)) {
doc = new _this(doc);
}

@ValYouW
Copy link
Contributor

ValYouW commented May 19, 2019

Yes, it seems that there was a change between version 5.3.15 and 5.5.9 that fixed how docs are saved...
I had the following issue on 5.3.15:

var ShapeSchema = new db.Schema({
	type: {type: String},
	name: String,
}, {collection: 'shapes', discriminatorKey: 'type'});

var ShapeModel = db.model('ShapeModel', ShapeSchema);

var SquareSchema = new db.Schema({
	radius: Number
});

var CircleModel = ShapeModel.discriminator('Circle', SquareSchema);


var dalDoc = new ShapeModel({type: 'Circle', radius: 12});
dalDoc.save().then(res => {
	console.log('Inserted --->> ', res.toJSON());
});

//// Output on 5.3.15
Inserted --->>  { _id: 5ce10ca93ca3ff1964d15ce1, type: 'Circle', __v: 0 }

//// Output on 5.5.9
Inserted --->>  { _id: 5ce10cdc6d05443ba4107f43,
  type: 'Circle',
  radius: 12,
  __v: 0 
}

So looks good :)

@vkarpov15
Copy link
Collaborator Author

@ValYouW @Fonger confirmed this was fixed in 5.4.20, looks like this fix was a positive but unexpected side effect of #7586

@vkarpov15 vkarpov15 removed this from the 5.5.10 milestone May 19, 2019
@vkarpov15 vkarpov15 added duplicate and removed needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue duplicate labels May 19, 2019
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

No branches or pull requests

3 participants