Laxer validation for properties and beforeValidate() as filter

[salieri]

Assuming...

User = function() {

  this.property( 'username', 'string', { required: true } );
  this.property( 'password', 'string', { required: false } );

  this.validatesFormat( 'username', /^[^a-z]$/ );
  this.validatesFormat( 'password', /^[^a-z]$/ );
};

And:

var u = new User();

u.setProperties( { username: 'testuser' } );
u.save();

...Current validation system will fail, because undefined property password will not validate against the regular expression validator I have created. However, typical database design allows default values for fields not mentioned in the insert query, not to mention use of null.

Laxer validation checks if the property's required parameter has been explicitly been set to false. If that is the case, the validation test will not be run against the empty value, because we're signalling that empty/null is safe.

---

Secondly, properties are passed to beforeValidate() callback as its first parameter. This is to allow beforeValidate() function to be used as a filter:

User.prototype.beforeValidate = function( params )
{
  if( params.hasOwnProperty( 'username' ) === true )
  {
    params.username = params.username.toLowerCase();
  }
};

var u = new User();

u.setProperties( { username: 'TESTuser' } );
u.save();

Currently, validation would fail, because User has a validation tester which allows only lowercase letters through. In many cases -- particularly with data received from users -- we could safely enforce data formatting rules so that we won't have to worry about the format of the data we received.

[mde]

I'm not totally opposed to this, but what you're wanting here could easily be achieved simply by using validatesWithFunction. The "required: true" shortcut just adds an explicit 'validatesPresent', (similar to the length shortcut and 'validatesLength') so adding yet a different behavior if it's set explicitly to false seems a little confusing.

this.validatesWithFunction('password', function (val) {
  return ((!val) || /^[^a-z]$/.test(val));
});

And of course if you were doing this a lot you'd make it a reusable function. It's not quite as declarative this way, but it's definitely clearer what exactly is going on.

I do like the idea of passing the params to the beforeValidate.

And of course, it seems like another real problem here is the inability to set default values. This has been requested before, and something I think would be really beneficial to people.

[salieri]

I agree, repurposing required may not be the most intuitive idea. Perhaps I could use another field for it? Or divide it in three -- allowNull, optional, and default?

The reason why I'm in favor of dealing with null as a special case is because I have a large number of validators. You are right, I could switch to functions, but it would mean two things:

1. I would have to either write a function for both "accepts null" and "doesn't accept null" cases, or add that logic into every single validator function. There are cases where we do allow null and cases where we don't, after all.
2. I could not take advantage of the built-in functions. This may be a minor issue, but from the point of view of application maintenance, I'm trying to think for the next guy who comes in -- does he have to learn "the Geddy way" or "the Geddy way plus these fifty things we do differently"?

If this sounds like a doable plan, I'm happy to refactor as needed.

[mde]

Ah, I can see what you mean. Seems like allowing null (empty?) values is a common pattern. AR does indeed have both allow_nil and allow_empty. I would implement an allowNull or allowEmpty (or both) option that can be used (as you implemented it) as a short-circuit for other subsequent validations such as validatesLength or validatesFormat. But you would not be able to use it along with "required: true"/validatesPresent (null/empty is decidedly not 'present'). If you're validating length/format, presence is implied -- unless you short-circuit because of a null/empty value.

Default values are another whole can of worms, but definitely a feature we really want.

Does that give you enough info for a rough implementation?

[salieri]

This is now refactored to use allowEmpty and allowNull. It would be very easy to rig model.validateProperty() to also support default values, I think.

[mde]

Can we get some test and doc updates for this? Then I'll merge it ASAP!

[salieri]

Will do.

Could you take a look at lib/index.js? I'm thinking that the lines 919-930 should be moved to take place before beforeValidate calls and emits. Otherwise those may use data from non-camelized keys. Does it make sense, or I'm worrying over nothing?

[mde]

That's actually a good question. That code is there for conversion from balls of data passed from snake_case APIs. It's not entirely clear whether beforeValidate should get the raw params that were originally passed, or converted ones. Presumably the user knows what params have been passed, so they wouldn't be surprised to find snake_case keys in some cases. OTOH, it might be nice to allow writers of JS code to have the post-conversion camelCase keys to work with. Let's go ahead and move it, like you suggest.

[mde]

Where are we on this? Do you want to make those changes? Or do you want me to merge and make the change?

[salieri]

I'm in a crunch mode at the moment, so can't get this finished for another few weeks. Would prefer to write a few tests before merging.

[mde]

Looking forward to it! Lemme know what I can do to help.