Pre save runs before pre validate in 4.0

[optikfluffel]

Hi, I found the Model.create method a few days ago and switched to it from doing

x = new Foo({ bar: 'baz' });
x.save(function (error) {...});

but now my

fooSchema.pre('save', function(next) {...});

isn't called anymore. Is that normal behaviour or a bug? I can't really tell, but I didn't find anything in the docs either.

Thanks in advance, optikfluffel

[vkarpov15]

Hmm that should not be happening. Can you provide me a more thorough example? Pre-save hooks should be called on create()...

[optikfluffel]

Sure.

// model
fooSchema = new mongoose.Schema({
  title: {
    type: String,
    index: true
  },
  content: {
    type: String,
    required: true
  }
  createdAt: {
    type: Date,
    required: true,
    default: new Date()
  },
  updatedAt: {
    type: Date,
    required: true
  }
});

// pre-save hook
fooSchema.pre('save', function(next) {
  console.log('in pre-save hook');
  this.updatedAt = new Date();
  return next();
});

// that worked
x = new Foo({
  title: 'baz',
  content: 'le content'
});
x.save(function (error) {
  if (error) { throw error };
  console.log('Yay, saved!');
});

// this gives me a validation error because updatedAt isn't defined
// and nothing inside the pre-save hook is called
x = new Foo({
  title: 'baz',
  content: 'le content'
});
yield Foo.create(x); // it's inside a koa middleware

[vkarpov15]

Hmm can you clarify your versions of Node, Mongoose, and Koa? We have test coverage for pre hooks with create() and the following standalone file works as expected:

var mongoose = require('mongoose'),
var co = require('co');

var connect = function() {
    var options = {
        server: {
            socketOptions: {
                keepAlive: 1
            }
        }
    }
    mongoose.connect("mongodb://localhost/test_mongoose", options);
}
connect();

var Schema = mongoose.Schema;
var schema = new Schema({
  t: String
});
schema.pre('save', function(next) {
  console.log('Saving!');
  next();
});
var Model = mongoose.model('test-gh-2462',schema);
var instance = new Model({ t: 'abcd' });

co(function*() {
  yield Model.create(instance);
  console.log('done');
});

One more detail - why are you using create() here in the first place? create() is mostly useful when you have a list of JSON docs that you want to convert into docs and save them. yield x.save() would be more correct in this case IMO.

[rbaprado]

I am having a similar issue. I was using the unstable mongoose version and my pre-save middleware was working. Today I rolled back to the production version (same application code) and no pre-save middleware is being called anymore (for at least two different schemas). For the record, I am using io.js.

[rbaprado]

Sample code:

/**
 * Token Model
 * @module
 */

var mongoose = require('mongoose');
var uuid = require('node-uuid');
var ms = require('ms');

var accountVerificationLifetime = ms('1d');
var passwordResetLifetime = ms('1d');
var apiSessionLifetime = ms('1d');

var tokenSchema = mongoose.Schema({
  /** @type {String} Token */
  token: {
    type: String,
    required: true,
    index: {
      unique: true
    }
  },
  /** @type {ObjectId} Related User */
  userId: {
    type: mongoose.Schema.Types.ObjectId,
    ref: 'User'
  },
  /** @type {String} Token Type */
  type: {
    type: String,
    required: true,
    enum: [
      'accountVerification',
      'apiSession',
      'passwordReset'
    ]
  },
  /** @type {Date} Expires At */
  expires: {
    type: Date,
    required: true,
    expires: 0
  }
});

tokenSchema.pre('save', function(next) {
  console.log('presave');
  if (typeof(this.token) == 'undefined') this.token = uuid.v4();
  switch (this.type) {
    case 'accountVerification':
      {
        this.expires = new Date(Date.now() + accountVerificationLifetime);
        break;
      }
    case 'passwordReset':
      {
        this.expires = new Date(Date.now() + passwordResetLifetime);
        break;
      }
    case 'apiSession':
      {
        this.expires = new Date(Date.now() + apiSessionLifetime);
        break;
      }
    default:
      {
        this.expires = 0;
      }
  }
  next();
});

module.exports = mongoose.model('Token', tokenSchema);

[vkarpov15]

Hmm here's the output I get on io.js 1.1.0:

$ ~/Workspace/iojs-v1.1.0-linux-x64/bin/iojs gh-2462.js 
[Error: Module did not self-register.]
js-bson: Failed to load c++ bson extension, using pure JS version

##############################################################
#
#   !!! MONGOOSE WARNING !!!
#
#   This is an UNSTABLE release of Mongoose.
#   Unstable releases are available for preview/testing only.
#   DO NOT run this in production.
#
##############################################################

presave
Error: ValidationError: Path `type` is required.

From running the following script:

var mongoose = require('mongoose'),
    Schema = mongoose.Schema;

mongoose.set('debug', true);
mongoose.connect('mongodb://localhost/gh2462');

var mongoose = require('mongoose');
var uuid = require('node-uuid');
var ms = require('ms');

var accountVerificationLifetime = ms('1d');
var passwordResetLifetime = ms('1d');
var apiSessionLifetime = ms('1d');

var tokenSchema = mongoose.Schema({
  /** @type {String} Token */
  token: {
    type: String,
    required: true,
    index: {
      unique: true
    }
  },
  /** @type {ObjectId} Related User */
  userId: {
    type: mongoose.Schema.Types.ObjectId,
    ref: 'User'
  },
  /** @type {String} Token Type */
  type: {
    type: String,
    required: true,
    enum: [
      'accountVerification',
      'apiSession',
      'passwordReset'
    ]
  },
  /** @type {Date} Expires At */
  expires: {
    type: Date,
    required: true,
    expires: 0
  }
});

tokenSchema.pre('save', function(next) {
  console.log('presave');
  if (typeof(this.token) == 'undefined') this.token = uuid.v4();
  switch (this.type) {
    case 'accountVerification':
      {
        this.expires = new Date(Date.now() + accountVerificationLifetime);
        break;
      }
    case 'passwordReset':
      {
        this.expires = new Date(Date.now() + passwordResetLifetime);
        break;
      }
    case 'apiSession':
      {
        this.expires = new Date(Date.now() + apiSessionLifetime);
        break;
      }
    default:
      {
        this.expires = 0;
      }
  }
  next();
});

var Token = mongoose.model('Token', tokenSchema, 'tokens');

var t = new Token();

t.save(function(err) {
  console.log('Error: ' + err);
  process.exit(0);
});

Can you run the above script and verify that you get the desired output?

[rbaprado]

What I did here that worked around the issue was changing from 'save' middleware to 'validate'. There seems to be same different behavior regarding those middlewares between the stable and unstable mongoose versions.

Em qua, 4 de mar de 2015 às 17:08, Valeri Karpov notifications@github.com escreveu:

Hmm here's the output I get on io.js 1.1.0:

$ ~/Workspace/iojs-v1.1.0-linux-x64/bin/iojs gh-2462.js [Error: Module did not self-register.] js-bson: Failed to load c++ bson extension, using pure JS version

############################################################## #

!!! MONGOOSE WARNING !!!

#

This is an UNSTABLE release of Mongoose.

Unstable releases are available for preview/testing only.

DO NOT run this in production.

# ##############################################################

presave Error: ValidationError: Path type is required.

From running the following script:

var mongoose = require('mongoose'), Schema = mongoose.Schema;

mongoose.set('debug', true); mongoose.connect('mongodb://localhost/gh2462'); var mongoose = require('mongoose');var uuid = require('node-uuid');var ms = require('ms'); var accountVerificationLifetime = ms('1d');var passwordResetLifetime = ms('1d');var apiSessionLifetime = ms('1d'); var tokenSchema = mongoose.Schema({ /* @type {String} Token / token: { type: String, required: true, index: { unique: true } }, /* @type {ObjectId} Related User / userId: { type: mongoose.Schema.Types.ObjectId, ref: 'User' }, /* @type {String} Token Type / type: { type: String, required: true, enum: [ 'accountVerification', 'apiSession', 'passwordReset' ] }, /* @type {Date} Expires At / expires: { type: Date, required: true, expires: 0 } });

tokenSchema.pre('save', function(next) { console.log('presave'); if (typeof(this.token) == 'undefined') this.token = uuid.v4(); switch (this.type) { case 'accountVerification': { this.expires = new Date(Date.now() + accountVerificationLifetime); break; } case 'passwordReset': { this.expires = new Date(Date.now() + passwordResetLifetime); break; } case 'apiSession': { this.expires = new Date(Date.now() + apiSessionLifetime); break; } default: { this.expires = 0; } } next(); }); var Token = mongoose.model('Token', tokenSchema, 'tokens'); var t = new Token();

t.save(function(err) { console.log('Error: ' + err); process.exit(0); });

Can you run the above script and verify that you get the desired output?

Reply to this email directly or view it on GitHub https://github.com/LearnBoost/mongoose/issues/2462#issuecomment-77235833 .

[vkarpov15]

Can you explain in more detail what you're seeing and how to reproduce it?

[rbaprado]

The code I am having problem with is the one from my first message, not changes at all. My idea was to fill out some forms prior saving the document, hence my initial preference for pre 'save', it was working fine with the unstable mongoose version. Since I was preparing for production release, I reverted to the stable mongoose and faced the problem.

Just for the sake of it, I tried changing to 'validata' middleware and it fixed my problem, I did not change anything else since.

Em qua, 4 de mar de 2015 às 18:05, Valeri Karpov notifications@github.com escreveu:

Can you explain in more detail what you're seeing and how to reproduce it?

Reply to this email directly or view it on GitHub https://github.com/LearnBoost/mongoose/issues/2462#issuecomment-77247045 .

[vkarpov15]

I ran your code from your first message on 4.0, and it looks like it runs the pre('save') middleware just fine as far as I can tell. Can you take a look at the example that I yesterday and see if it 1) works as expected on your machine, and 2) if its a fair representation of what you're trying to do?

[rbaprado]

I was initially running 3.9.something and it was working, so 4.0 will probably be fine two. 3.8.24 triggers the issue (io.js 1.4.3, windows)

Em qui, 5 de mar de 2015 às 13:50, Valeri Karpov notifications@github.com escreveu:

I ran your code from your first message on 4.0, and it looks like it runs

the pre('save') middleware just fine as far as I can tell. Can you take a look at the example that I yesterday and see if it 1) works as expected on your machine, and 2) if its a fair representation of what you're trying to do?

Reply to this email directly or view it on GitHub https://github.com/LearnBoost/mongoose/issues/2462#issuecomment-77401153 .

[vkarpov15]

Yeah it looks like in 3.8.x pre-save hooks run after validation, but in 4.0 pre-save hooks run before validation. That actually sounds like it might be an issue.

[rbaprado]

Thinking about that, I guess running "validate" prior "save" makes sense, since a common task for middleware is to fill-out some fields automatically, which seems to be more akin to a "validate" step...

What do you think?

Em sex, 6 de mar de 2015 às 13:32, Valeri Karpov notifications@github.com escreveu:

Yeah it looks like in 3.8.x pre-save hooks run after validation, but in 4.0 pre-save hooks run before validation. That actually sounds like it might be an issue.

Reply to this email directly or view it on GitHub https://github.com/LearnBoost/mongoose/issues/2462#issuecomment-77588805 .

[buunguyen]

@vkarpov15 I see this happening in 4.0.1, validate runs before pre-save. I read this thread but am not clear whether you fixed it or considered it a correct behavior. Please confirm. Thanks!

[vkarpov15]

Rght now in 3.8.x and 4.x validate() is run as a pre save hook, and so pre validate, validate, and post validate all run before any pre save hooks you specify. The flow looks like this:

pre validate -> validate -> post validate -> pre save -> save -> post save

This is currently considered the "correct" behavior, but I'm open to arguments for other behaviors for a future release. The point of OP's issue was that, in 4.0.0-rc3, pre save hooks ran before pre validate hooks, which was an unintentional backwards breaking change and not a good choice IMO.

[buunguyen]

I personally think it makes more sense that pre-save runs before validation because that's where many people probably start with when writing plugins to modify models, e.g. set value for a createdAt field. If createdAt is required, it will fail due to the validation. I guess many wouldn't immediately think such logic has to be written within a hook named "validate".

That said, backward compatibility is probably more important, given the simple workaround of just moving code like above to pre validate.

[vkarpov15]

The counter argument would be that if there's a field you're setting in pre save, it doesn't really need to be required because pre save will always set it. Current assumption is that any changes made in pre-save stage are "trusted." However, there is a validateBeforeSave schema option that you can use to turn off the pre save validation logic if you want to handle validation before saving in a different way.

[buunguyen]

That makes sense. In fact, that's what I ended up doing instead of using pre validate. Still, I like the idea of looking at my schema and immediately know what's required and what's not without having to be aware of the rule enforced by a plugin.

[vkarpov15]

That's a fair point, one that's worth considering. The argument for why we have the current approach is to decouple "validate" and "save" as much as possible - "validate" should not fail when run as validate() but succeed when run as part of save(). If we ran pre save hooks before validation, you could easily run into a case where validate() fails but save() succeeds.

[buunguyen]

Good point. I wasn't aware of the validate() method and I never have to use it. For all my use cases working with Mongoose so far, schema validation is the final safety check before data is actually persisted to MongoDB and I never explicit invoke it. I don't really know when it makes sense to explicitly call validate(), but I agree with you that if people do that, they'd expect the separation as you said.

[vkarpov15]

Much of the point of validate() is to enable users to have more control over validation. For instance, you can use validate() to do schema validation in the browser, or to validate a subdoc before $push-ing it onto an array with .update(). Plenty of valid use cases. For instance, you can address your concern by turning the validateBeforeSave option off and then adding a pre save hook that calls validate as the last pre save hook. Right now mongoose attaches validate as the first pre-save hook.

[buunguyen]

Great information, thanks! I'll try the approach you mention to bring back require into the schema. That's good enough for me.

That said, would you consider adding an option to control when validation happens? Maybe 'validation' option with one of 'disabled' | null, 'beforeSave', 'beforePreSave'... (Default is 'beforePreSave', as current.) The validateBeforeSave option can probably be deprecated.

That should satisfy everyone.

[vkarpov15]

Seems like something that would be more easily achieved via code than configuration - you can pretty easily just disable the default validation behavior and then add pre hooks to handle validation yourself.

[buunguyen]

Certainly. I just thought since there's an option to control validation, we might as well make it a bit more fine-grained. But yes, I agree that it's no big deal to handle this in code.