Array validator receives an object on $pull

[sebastian-nowak]

I'm not sure if it's a bug or just a very surprising behavior, but here we go:

new mongoose.Schema({
  arrayOfDocuments: {
    type: [{
      ...
    }],
    validate: {
      validator: v => _.uniqBy(_.map(v, '_id'), id => id.toString()).length === v.length,
      message: 'Ids have to be unique.'
     }
  }
})

Now let's run an update query on this model:

// validators are set to run on update
await SomeModel.findOneAndUpdate(
  {_id: 'some object id'},
  {$pull: {arrayOfDocuments: {_id: 'another object id'}}}
);

The above validator will be run with {_id: 'another object id'} as an argument - which isn't an array. I don't think this is valid, it's trying to validate a subset of document properties by passing it to an array validator.

[vkarpov15]

Thanks for reporting, will fix asap

[gautier-lefebvre]

Could you also release a fix for 4.x? It worked in 4.11 but not in 4.12+.

I'll have more time to dig into this later this evening, but at first glance:

The value being passed in to the validator during the query is the value being $pulled from the array. This value is taken from the Query update doc.

What value should be passed in to the validator in it's place?

1. That same value, but cast as an array with one element?
   I'm not sure what difference this would make, but it's certainly doable.
2. The remaining array of elements after the $pull?
   This doesn't seem feasible, as the doc isn't necessarily in memory when querying with findOneAndUpdate

Thoughts?

[sebastian-nowak]

Thanks for looking into it @lineus.

I don't think this validator should be called at all. It's a validator for the whole array - and you don't have the whole array here, just some of its elements. The only validators that should be called here are the validators for individual array elements.

Also take a look here: https://mongoosejs.com/docs/validation.html

Also, $push, $addToSet, $pull, and $pullAll validation does not run any validation on the array itself, only individual elements of the array.

Cheers!

Aah, I missed that in the docs. This script shows that currently all four array update operators call validators on the whole array:

6889_2.js

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

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

const schema = new Schema({
  arr: {
    type: [{ x: String }],
    validate: {
      validator: () => { return ++called; }
    }
  }
});

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

const test = new Test({ arr: [{x:'one'},{x:'two'}]});
assert.strictEqual(called, 0);

async function run() {
  await conn.dropDatabase();
  await test.save();
  assert.strictEqual(called, 1);
  let cond = { _id: test._id };
  let update = { $push: { arr: { x: 'three' } } };
  let opts = { new: true, runValidators: true };
  let a,b,c;

  try {
    let u = await Test.findOneAndUpdate(cond, update, opts);
    assert.strictEqual(called, 1, '$push called validator on whole array');
    assert.strictEqual(u.arr.length, 3);
  }catch(e){
    console.log(e.message);
  }

  try {
    update = { $addToSet: { arr: { x: 'four' } } };
    let u2 = await Test.findOneAndUpdate(cond, update, opts);
    a = u2.arr[0];
    b = u2.arr[1];
    c = u2.arr[2];
    assert.strictEqual(called, 2, '$addToSet called validator on whole array');
    assert.strictEqual(u2.arr.length, 4);
  }catch(e){
    console.log(e.message);
  }

  try {
    update = { $pullAll: { arr: [a, b] } };
    let u3 = await Test.findOneAndUpdate(cond, update, opts);
    assert.strictEqual(called, 3, '$pullAll called validator on whole array');
    assert.strictEqual(u3.arr.length, 2);
  }catch(e){
    console.log(e.message);
  }

  try {
    update = { $pull: { arr: c } };
    let u4 = await Test.findOneAndUpdate(cond, update, opts);
    assert.strictEqual(called, 4, '$pull called validator on whole array');
    assert.strictEqual(u4.arr.length, 1);
  }catch(e){
    console.log(e.message);
  }

  return conn.close();
}

run().catch(e => console.error(e.message));

Output:

issues: ./6889_2.js
$push called validator on whole array
$addToSet called validator on whole array
$pullAll called validator on whole array
$pull called validator on whole array
issues:

I'm a slightly hesitant to propose a change like this in a patch release without @vkarpov15's input, even though the docs are fairly explicit in the matter. I do agree though, based on the docs you pointed out @sebastian-nowak, that the whole-array validator shouldn't run at all in the case of $pull, $addToSet, $pullAll, and $push.

@vkarpov15 Should I prevent the whole array validator from running at all when only using a combination of these operators, and if so, should I target the 5.3 branch when I create my PR?