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

Streamline validations #362

Open
yanickrochon opened this issue Jan 22, 2014 · 10 comments
Open

Streamline validations #362

yanickrochon opened this issue Jan 22, 2014 · 10 comments

Comments

@yanickrochon
Copy link

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.

  • empty
  • notEmpty
  • undefined
  • string
  • alpha
  • alphanumeric
  • email
  • url
  • urlish
  • ip
  • ipv4
  • ipv6
  • creditcard
  • uuid
  • uuidv3
  • uuidv4
  • falsey
  • truthy
  • null
  • notNull
  • boolean
  • array
  • date
  • hexadecimal
  • hexColor
  • lowercase
  • uppercase
  • after
  • before
  • is
  • regex
  • not
  • notRegex
  • equals
  • contains
  • notContains
  • len
  • max
  • min
@1602
Copy link
Owner

1602 commented Jan 24, 2014

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
Copy link
Author

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
Copy link
Collaborator

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//issues/362#issuecomment-33222923
.

@yanickrochon
Copy link
Author

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
Copy link
Author

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 in validator has been renamed to whitelist
  • the notIn validator has been renamed to blacklist
  • the finite validator has been removed, as deemed unnecessary (anyone finding a use for it should create a ticket in the project's issue tracker)

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
Copy link
Contributor

pocesar commented Feb 20, 2014

@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
Copy link
Author

@pocesar Is this promise version official?

@pocesar
Copy link
Contributor

pocesar commented Feb 20, 2014

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
Copy link
Contributor

ktmud commented Mar 2, 2014

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
Copy link
Author

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.

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

5 participants