custom model validation function runs twice as a result of model-instance.update()

[shaunerikson]

using v2.0.0-rc8

describe('on model update', function() {

    describe('on a previously created model', function() {

        var validatorFunc, Model;

        beforeEach(function(done) {
            validatorFunc = sinon.spy();
            Model = sequelize.define('Model', {
                name: Sequelize.STRING
            }, {
                validate: { isModelGood: validatorFunc  }
            });
            Model.sync({ force: true })
                .then(function() { done(); });
        });

        it('should run once but actually runs twice (BUG?)', function(done) {
            Model.create({ name: 'bob' })
                .then(function(createdModel) {
                    validatorFunc.reset();
                    return createdModel.update({ name: 'new name' });
                })
                .then(function() {
                    validatorFunc.should.have.been.calledOnce;
                    done();
                })
                .catch(done);
        });
    });
});

[mickhansen]

We refactored save to run validations since if hooks changed any fields, this is obviously not the case here so must be a minor bug.

Validations should really be idempotent and side-effect free though so not a major bug.

[shaunerikson]

I was debating on whether to report it or not, in the end I did because the model validations run twice but the column validations don't, and I thought that might be unintentional.

I did another test, and it looks like if a column validator exists, the validation hooks run twice, but the actual validator function only runs once.

describe.only('when updating a single model', function() {

    it('should call hooks in correct order', function(done) {
        var Model = sequelize.define('Model', { 
            //name: Sequelize.STRING
            name: {
                type: Sequelize.STRING,
                validate: { isNameGood: function(value) { trace += 'V|'; } }
            }
        }, {
            hooks: { 
                beforeValidate: function(instance, options) { trace += "BV|"; return Promise.resolve(); },
                afterValidate: function(instance, options) { trace += "AV|"; return Promise.resolve(); },
                beforeUpdate: function(instance, options) { trace += "BU|"; return Promise.resolve(); },
                afterUpdate: function(instance, options) { trace += "AU|"; return Promise.resolve(); }
            }
        });
        var trace = '';
        Model.sync({ force: true })
            .then(function() { return Model.create({ name: 'bob' }); })
            .then(function(createdModel) { 
                trace = '';
                return createdModel.update({ name: 'robert' }); 
            })
            .then(function() {
                // Model validators get run twice due to update() method!!!
                expect(trace).to.equal('BV|V|AV|BU|BV|V|AV|AU|');
                // Actual trace: 'BV|V|AV|BU|BV|AV|AU|'
                done();
            })
            .catch(done);
    });
});```

[epicbagel]

Also just observed this behaviour. I was looking to change the value of a property via a hook in "beforeValidate". In this instance, setting a defaultValue for one of the properties. This value needs to be dynamically generated at creation time as it depends on the value of other properties.

It was firing the beforeValidate message twice, which is expensive if the beforeValidate contains DB queries. This can be fixed if I push the code to the beforeUpdate and beforeCreate hooks, but this doesn't feel correct as I'm causing changes after the validation has taken place.

A solution would be to have a preValidate hook, where you can make any such changes before the validation is performed.

[akotranza]

@mickhansen I don't see this as minor. Elsewhere you (or other major contributors) have suggested using beforeValidate hooks to perform actions that have side-effects, such as hashing passwords. This is a major bug that should have been fixed already, but 18 months later is still active.

[mickhansen]

@akotranza Maintainers just love to hear "should have".

[janmeier]

See https://github.com/sequelize/sequelize/commit/4f3abbb9e6498b95f3caa899ee31990a12545635 for the original reason that validate is being called twice. Values that have changed in hooks need to be validated as well.

But I guess we could set hook: false in the second call

[akotranza]

Thanks Jan for the non snarky reply On Jul 3, 2016 10:41 AM, "Jan Aagaard Meier" notifications@github.com wrote:

See 4f3abbb https://github.com/sequelize/sequelize/commit/4f3abbb9e6498b95f3caa899ee31990a12545635 for the original reason that validate is being called twice. Values that have changed in hooks need to be validated as well.

But I guess we could set hook: false in the second call

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sequelize/sequelize/issues/3243#issuecomment-230156752, or mute the thread https://github.com/notifications/unsubscribe/ACJd7FS-YDQ2acAI3KoauktTbMsFBfS6ks5qR8oygaJpZM4Dnqhz .

[ghost]

Same issue when trying to update a password, the password get's hashed twice in the afterValidate hook

[ghost]

how to fix issue model called twice in update custom directives? I can't solve it anyone can help me with this issue?

[chornos13]

for someone who still struggle finding this answer, this is the solution.

module.exports = (sequelize, DataTypes) => {
    var User = sequelize.define('User', {
        ....
    }, { 
          validate: {
            isValid() {
                if(!this.isValidated) {
                    this.isValidated = true
                } else {
                    return
                }

                //DO VALIDATION/HASH PASSWORD HERE
            },
        },
         }....

[utf4]

@chornos13 Your solution will work if you have to perform only one action (create or update). It will fail for sequencial actions like the following:

const record = await Model.create({key: value1}); // Validations will be executed fine
await record.update({key: value2}); // Validations will not be executed here since 
// isValidated is already true

Also in the following case:

const record = await Model.findByPk(id);
await record.update({key: value2}); // Validations will be executed fine
await record.update({key: value3}); // Validations will not be executed 
// since `record.isValidated` is already set to true but I am expecting 
// the validations to be executed here again.

[utf4]

I am validating some data integrity in custom validate functions by making database queries. Calling validations twice is making update very slow because queries are called twice to db. Quick workaround is, I have to move those validations with throw inside beforeSave function.

[utf4]

@janmeier This if https://github.com/sequelize/sequelize/blob/master/lib/model.js#L3703 is going to be true always. Can we at least set it to check hookChanged.length and should not run the validation once again when nothing got changed inside beforeUpdate hook?

[utf4]

A very quick workaround is to set validate to false inside beforeUpdate hook for the specific model or for the sequelize globally.

const beforeUpdate = function (_record, _options) {
   _options.validate = false;
};

In case you are changing anything inside beforeUpdate / beforeSave hooks in any model, you need to attach this hook to specific models you want to avoid double validation:

// Model is your specefic model you are trying to disable
Model.addHook('beforeUpdate', beforeUpdate);

In case you are not changing anything in beforeUpdate / beforeSave hook of any model and not planning to do so, you can set it to sequelize globally:

// sequelize is instance of Sequelize
sequelize.addHook('beforeUpdate', beforeUpdate);

[sezanzeb]

for someone who still struggle finding this answer, this is the solution.

module.exports = (sequelize, DataTypes) => {
  var User = sequelize.define('User', {
      ....
  }, { 
          validate: {
          isValid() {
              if(!this.isValidated) {
                  this.isValidated = true
              } else {
                  return
              }

              //DO VALIDATION/HASH PASSWORD HERE
          },
      },
         }....

to avoid the issue @furqanaziz pointed out, you maybe could make a hash of the updates and store that instead of true. If a different update happens in the chain the hashes would mismatch and the validation executed again.