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

beforeCreate and Model Properties #69

Closed
markandrus opened this issue Mar 8, 2013 · 9 comments
Closed

beforeCreate and Model Properties #69

markandrus opened this issue Mar 8, 2013 · 9 comments

Comments

@markandrus
Copy link
Contributor

Hi,

Thanks a lot for this project. I was wondering if I'm doing something wrong here—I expect to be able to modify the properties of a model in beforeCreate, but Postgres shows the following in the database:

 id |        email        | password | salt | created 
----+---------------------+----------+------+---------
  1 | test@email.com      | visible? |      | 
var orm    = require('orm'),
    bcrypt = require('bcrypt');

orm.connect('postgres://localhost/database', function (err, db) {
  var Person = db.define('Person', {
    email: String,
    password: String,
    salt: String,
    created: Date
  }, {
    validations: {
      email: orm.validators.unique()
    },
    hooks: {
      beforeCreate: function () {
        if (!this.salt) {
          console.log('Generating salt...');
          this.salt = bcrypt.genSaltSync(10);
          this.created = new Date();
        }
        if (this.password) {
          console.log('Hashing password...');
          this.password = bcrypt.hashSync(this.password, this.salt);
        }
      }
    }
  });
  // Person.sync(function (err) { console.log(err); });
  Person.create([{
    email: 'test@email.com',
    password: 'visible?'
  }], function (err, items) {
    console.log(err);
    // console.log(items);
  });
});

Testing with beforeSave yields different results for me, too: the password gets hashed, but salt and created go unset in the database.

@markandrus
Copy link
Contributor Author

After looking at /lib/Instance.js, I guess the functionality I'm looking for requires threading data as an argument through the hook.

@dresende
Copy link
Owner

dresende commented Mar 9, 2013

Actually, the blame is beforeCreate should be right after/before beforeSave (a few lines above). In that zone, you can use the this context to change the data. I'm going to make this change.

dresende added a commit that referenced this issue Mar 9, 2013
@dresende
Copy link
Owner

dresende commented Mar 9, 2013

If you want to test it you have to check the latest git version, it will take some days before pushing a new version. Or you can use the hook beforeSave which should have the same behavior for new instances (but you'll also have it trigger for not new instances).

@markandrus
Copy link
Contributor Author

Thanks! I've tested and it works when updating properties enumerated in the call to create, but I can't add properties—for example, to get the above to populate the salt column, I have to change the create call to include that key:

Person.create([{
  email: 'test@email.com',
  password: 'visible?',
  salt: ''
}], /* ... */

dresende added a commit that referenced this issue Mar 11, 2013
Getters/setters for properties would not be correctly defined
@dresende
Copy link
Owner

Please try it now.

@dxg
Copy link
Collaborator

dxg commented Mar 13, 2013

This works however rangeLength validator is checking for undefined, whereas attributes not listed in Person.create[{...}] are now initialised to null and the validation blows up.

Is it possible for an attribute to ever be undefined? Should the validation only be checking for null, or both null & undefined?

@dresende
Copy link
Owner

You're right. To be consistent, it should initialize to undefined.

dresende added a commit that referenced this issue Mar 13, 2013
@dresende
Copy link
Owner

I'm going to assume it's fixed. If you have any other problems please reopen or create a new issue :)

@markandrus
Copy link
Contributor Author

Just tested--it works! Thanks

dresende added a commit that referenced this issue Mar 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants