Call beforeUpdate, beforeDestroy, beforeValidate callbacks with criteria

[mmiller42]

Pass criteria to the beforeUpdate, beforeDestroy, and beforeValidate callbacks, before the callback param.

In beforeValidate callbacks, criteria is null for create operations.

Serves as a solution to issue #1004.

NOTE: For backwards compatibility, it now checks if the beforeValidate or beforeCreate callback(s) only accept 3 arguments or beforeDestroy accepts only 2 and if so, passes the callback as the last argument and omits criteria.

Example usage and explanation:

// User.js
module.exports = {
  attributes: {
    email: { type: 'email', required: true, unique: true },
    password: { type: 'string', required: true }
  },

  /**
   * Hashes the password if it is provided
   * @param {Object} values The data submitted via the API
   * @param {Function} next The callback to invoke when done, with optional error
   */
  beforeCreate: function (values, next) {
    if (values.password === undefined) return next();
    hashPassword(values.password, function (err, hash) {
      if (err) return next(err);
      values.password = hash;
      next();
    });
  },

  /**
   * Hashes the password if it is provided, but only if the provided
   * password is not the user's existing password hash (safeguards
   * against the record.save() bug)
   * @param {Object} values The values to update
   * @param {Object} criteria The criteria to match the record(s) to update
   * @param {Function} next The callback to invoke when done, with optional error
   */
  beforeUpdate: function (values, criteria, next) {
    if (values.password === undefined) return next();

    // Look up user and only change password if values.password isn't the hash
    User.findOne(criteria).exec(function (err, user) {
      if (err) return next(err);
      if (!user) return next(new Error('User to update does not seem to exist!'));

      if (values.password === user.password) return next();

      hashPassword(values.password, function (err, hash) {
        if (err) return next(err);
        values.password = hash;
        next();
      });
    });
  },

  /**
   * Validates the password against constraints, but only if the user does
   * not exist, or the provided password is not the user's existing password
   * hash (safeguards against the record.save() bug)
   * @param {Object} values The values to update or insert
   * @param {Object|null} criteria The criteria to match the record(s) to
   *   update, or null if creating a new record
   * @param {Function} next The callback to invoke when done, with optional error
   */
  beforeValidate: function (values, criteria, next) {
    if (values.password === undefined) return next();
    if (!criteria) { // create
      return validatePassword(values.password, next);
    } else { // update
      // Look up user to see if values.password is actually just the existing hash
      User.findOne(criteria).exec(function (err, user) {
        if (err) return next(err);
        if (!user) return next(new Error('User to update does not seem to exist!'));

        if (user.password === values.password) return next();

        return validatePassword(values.password, next);
    }
  }
};

/**
 * Returns a hash with embedded salt given a string
 * @param {string} password The string to hash
 * @param {Function} next The callback to invoke with null or hash error and the hash
 */
function hashPassword (password, next) {
  require('bcrypt').hash(password, 10, next);
}

/**
 * Validates a password against the constraints
 * @param {string} password The password to validate
 * @param {Function} next The callback to invoke with null or the validation error
 */
function validatePassword (password, next) {
  if (password.length < 5 || password.length > 20)
    return next(new Error('Password should be 5-20 characters.'));
  next(null);
}

[devinivy]

Not forgotten! At a glance this looks pretty good. Will need more review, but as far as I'm concerned this is crucial to the next Waterline release.

[randallmeeker]

LGTM +1
this should have been rolled into the .12rc ??? Is this going to have to wait for .12 to be released now? and then rolled into .13? I'm also using this fork now.

[devinivy]

@particlebanana is this good for the next release?

[LeonardoGentile]

Any plan to integrate this any time soon?

[cazulu]

Same question here, any integration plans for this? I'm already using the fork in one of my projects. Thanks a lot, @mmiller42 !

[jstnjns]

Would love to see this get merged, a great addition!

[ErhanAbi]

+1

[particlebanana]

This is accepted into the 0.11 release. So it will be merged as soon as I can verify it all.

[robertrossmann]

Really looking forward to having this merged. Being able to look up the current state of the database entry (for doing comparisons during lifecycle validations) is very important in my everyday workflow.

[mikermcneil]

@mmiller42 Nice work. This is a great solution, and I appreciate you taking the time to implement backwards compatibility.

I'm a little concerned about using values, criteria. On one hand, I think it would be great to add a quick solution for folks who want to use this, but on the other hand, I think we might be better served by a breaking change that sticks to the conventions we're used to elsewhere (i.e. criteria, values) or better yet, introduces a combined options dictionary:

beforeValidate: function (options, cb) {
 // options.criteria;
 // options.values;
}

This last approach is the cleanest IMO, since it would allow us to pass custom metadata sent using the new .meta() method to lifecycle callbacks.

There will be other breaking changes in the next release of core waterline adapters (related to case insensitivity), so it seems like as good a time as any to go ahead and make that kind of a change.

@mmiller42 would you be willing / have time to adapt this to use the approach of an options dictionary with values and criteria?

Thanks!

[mmiller42]

I started down this path. It didn't end too well. Because it's backwards-incompatible, involves quite the change -- rewriting the method signatures of ~200 different unit tests.

[randallmeeker]

closed w/o comment?

[mmiller42]

I'm guessing because I didn't convert the params to the options dictionary like as was suggested. It's a very intensive change that affects hundreds of unit tests. If anyone wants to complete this, I'm happy to grant access to my fork -- I don't have the time. It also makes this PR a huge thing for the Waterline team to review.

That said, while maybe this isn't ready to be merged, should it be closed? Honestly it seems like a really necessary feature for Waterline.

[particlebanana]

Hmm I didn't close this, I deleted the outdated branch that it was a PR against though.

[particlebanana]

If you want to re-open against master that would be groovy. We can link back here.

[RWOverdijk]

@mmiller42 Could you create a new PR? I think that knowing what is going to get updated is very useful (I'm finding myself in need of this functionality right now).

[mmiller42]

Created #1328