1602 / jugglingdb

Multi-database ORM for nodejs: redis, mongodb, mysql, sqlite3, postgresql, arango, in-memory...
http://1602.github.io/jugglingdb/
2.04k stars 241 forks source link

Streamline validations #362

Open yanickrochon opened 10 years ago

yanickrochon commented 10 years ago

Rationale

The first thing I noticed when reading the documentation (I'm actually evaluating this project as a suitable ORM for our own project) was the hard-coded base validators and it's lack of flexibility. Reading the issues, I see many tickets on the subject.

Having a non-negligible Zend Framework background, the way validators are handled has inspired many other projects, as it allows effortless flexibility and extensibility. An other such project is Backbone.Validation.

Suggestion

So, instead of

// Setup validations
User.validatesPresenceOf('name', 'email')
User.validatesLengthOf('password', {min: 5, message: {min: 'Password is too short'}});
User.validatesInclusionOf('gender', {in: ['male', 'female']});
User.validatesExclusionOf('domain', {in: ['www', 'billing', 'admin']});
User.validatesNumericalityOf('age', {int: true});
User.validatesUniquenessOf('email', {message: 'email is not unique'});

I would have something like

User.validation({
    name: {
        required: true    // true or string, ex: "Name is not specified!"
    },
    email: {
        required: "Email is not specified!"    // or true
      , unique: "Email is not unique!"
    },
    password: {
        minLength: { min: 5, message: "Password must contain between 5 and 16 caracters" }
      , maxLength: { max: 16, message: "Password must contain between 5 and 16 caracters" }
    },
    gender: {
        in: { values: ["male", "femaile"], ignoreCase: true /*, message: .... */ }
    },
    domaine: {
        notIn: ['www', 'billing', 'admin']
    },
    age: {
        integer: true
      , min: { value: 18, message: "You must be of legal age!" }
    }
});

Then, adding new validators could be done like

var Schema = require('jugglingdb').Schema;

// signature : add(name, callback, async)
//   - name String         the validator's name
//   - callback Function   the validation function :
//                           - model Object      the model instance
//                           - fieldName String  the model field to validate 
//                           - options Object    the options configuration
//                           - callback Function the async callback (optional)
//                           returns true for success
//                           returns false or a string (error message) on failure
//   - async Boolean       if the validation function is async or not (optional) (default false)
Schema.Validation.add("foo", function (model, fieldName, options) { 
    /* ... */
    return true;
});

// async validation must pass a fourth parameter 'cb'
Schema.Validation.add("bar", function (model, fieldName, options, cb) { 
    setTimeout(function() {
        cb(true);
    }, 1000);
}, true);

// to use generators, simply provide the async flag and omit the fourth parameter 'cb'
Schema.Validation.add("buz", function * (model, fieldName, options) { 
    var asyncResult = yield someAsyncCall(model[fieldName], options);
    /* ... */
    return true;
}, true);

// using each custom validators (the third parameter 'options' will be "true")
User.validation({
    name: {
        foo: true,
        bar: true,
        buz: true
    }
};

Internationalisation

All error messages should support a sprintf-like syntax. With Node, this is supported via the util.format function. The default fallback message on failed validation (if the validation function returns false) could be "'%s' validation failed!", which will be formatted with the validated fieldName.

To enable translation of these messages, simply provide a function to

Schema.Validation.translator(function(message) {
    return message;  // translated
});

Obviously, since all (failed) validation messages are formatted inside the validator functions, before being returned, the translations should also be performed before formatting the message. Therefore, and since we already have a reference to Schema.Validation, I would propose

var format = require('util').format;
Schema.Validation.add("foo", function (model, fieldName, options) { 
    return format(Schema.Validation.translate("Foo failed for '%s'!"), fieldName);
});

Integration and Backward compatibility

To support existing projects, all current implementations could remain unchanged and simply proxying to the new validation system, to be deprecated at the next minor release (version 0.3). Thus, the built-in current validators could still be shipped with the ORM, along with new ones provided by the community.

validatesPresenceOf becomes required

Options
{
    message: <the error message to return> (defaults to "Field %s is required.")
}

The model field must be specified, with any given value.

validatesLengthOf becomes minLength and maxLength

Options
{
    min: <the minimum range value for minLength> (defaults to 3)
  , max: <the maximum range value for maxLength> (defaults to 255)
  , message: <the error message to return> (defaults to "Field '%s' must contain more|less than %d character(s)")
}

For strings, check it's length property. Ignored for any other type.

validatesInclusionOf becomes in

Options
{
    values: <array of values>
  , strictCompare: <boolean for strict === comparison> (defaults true)
  , ignoreCase: <boolean to ignore case on string values> (defaults false)
  , message: <the error message to return> (defaults to "Invalid field value '%s'")
}

The values array items' type should be compatible with the model's field type.

validatesExclusionOf becomes notIn

Options
{
    values: <array of values>
  , strictCompare: <boolean for strict === comparison>
  , ignoreCase: <boolean to ignore case on string values> (defaults false)
  , message: <the error message to return> (defaults to "Invalid field value '%s'")
}

The values array items' type should be compatible with the model's field type.

validatesNumericalityOf becomes number, integer, decimal, or finite

Options
{
    message: <the error message to return> (defaults to "Invalid numeric value '%s'")
}

For number, the value must be any numeric value. For integer, the value must be an integer value. For decimal, the value must not be an integer value. For finite, the value must be finite.

validatesUniquenessOf becomes unique

Options
{
    message: <the error message to return> (defaults to "%s must be unique.")
}

This validator checks in all the set for another model with the same field value. This validator is asynchronous.

Other proposed validations

From the sails.js framework.

1602 commented 10 years ago

Good proposal! Just a question: would you like to join our tiny team and implement it yourself or just put it to out loooong list of upcoming stuff. If you like to go for it I would be happy to provide you more information about process. Thank you, @yanickrochon!

yanickrochon commented 10 years ago

Sure, I'd be happy to put my shoulder to the wheel! I'll fork jugglingdb and create a third-party project jugglingdb-validation for that purpose. jugglingdb could simply require the latter in it's dependency.

What do you think?

anatoliychakkaev commented 10 years ago

Excellent! We're happy to have you on board.

I think this is the best solution to build as an add-on or dependency and jugglingdb core should only provide API for that. Current implementation of validations was inspired by ActiveRecord (from ruby), and we should keep it for backward compatibility, actually I don't see any problems in providing both syntax at least for existing validations.

I added you to a team, so you have admin/push/pull access to https://github.com/jugglingdb/validations

Let me know if you have any other questions or I can help you somehow.

On Fri, Jan 24, 2014 at 1:38 PM, Yanick Rochon notifications@github.comwrote:

Sure, I'd be happy to put my shoulder to the wheel! I'll fork jugglingdband create a third-party project jugglingdb-validation for that purpose. jugglingdb could simply require the latter in it's dependency.

What do you think?

— Reply to this email directly or view it on GitHubhttps://github.com/1602/jugglingdb/issues/362#issuecomment-33222923 .

yanickrochon commented 10 years ago

Thanks! That's a good starting point. I will start working on this today. And I agree that, at least for the time being, all current tests will continue to pass, perhaps until the next major release.

Glad to be on board!

yanickrochon commented 10 years ago

An update to the state of this project...

The repository has been created here (thanks Anatoliy Chakkaev). All "standard" validators has been implemented, and should be compatible with the "legacy" JugglingDB validation functions.

The validators and validations are thoroughly tested, however there should be some more adjustments during the integration phase with the core project.

Documentations will come as soon as the integration is done and a PR is submitted.

pocesar commented 10 years ago

@yanickrochon I'm finishing my promise version of jugglingdb, and all validations are async by default. I'm just waiting for your green light to port it over to my version

yanickrochon commented 10 years ago

@pocesar Is this promise version official?

pocesar commented 10 years ago

not yet, but intend to be, it's a dual API, work with old callbacks, but most of sync functions were turned into promise ones

ktmud commented 10 years ago

Why not let async validation callback accept error message as arguments? Instead of call cb(true) when validate succeed, why not just a done() for success and done(err) for error?

yanickrochon commented 10 years ago

Because errors are set directly unto the model. The callback receives an boolean indicating if the model has errors or not. That's all. If the boolean was not passed, then we'd need to manually check if this.errors contains a non-null value. The argument is just for consistency.