From 9b759c95ac9155e23f1b52efd6d8a46d417f0997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Mon, 12 Jan 2015 14:52:54 +0100 Subject: [PATCH 1/2] Include property value in the error message When building a list of errors for `ValidationError.message`, include the values of invalid properties too. In order to keep the message reasonably short, the values are truncated at approx 32 characters. --- lib/validations.js | 81 ++++++++++++++++++++++++++++++--------- test/manipulation.test.js | 4 +- test/relations.test.js | 20 ++++------ test/validations.test.js | 71 ++++++++++++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 33 deletions(-) diff --git a/lib/validations.js b/lib/validations.js index ee371457..fafcf5c2 100644 --- a/lib/validations.js +++ b/lib/validations.js @@ -26,7 +26,7 @@ function Validatable() { } /** - * Validate presence of one or more specified properties. + * Validate presence of one or more specified properties. * Requires a model to include a property to be considered valid; fails when validated field is blank. * * For example, validate presence of title @@ -49,7 +49,7 @@ function Validatable() { Validatable.validatesPresenceOf = getConfigurator('presence'); /** - * Validate absence of one or more specified properties. + * Validate absence of one or more specified properties. * A model should not include a property to be considered valid; fails when validated field not blank. * * For example, validate absence of reserved @@ -85,7 +85,7 @@ Validatable.validatesAbsenceOf = getConfigurator('absence'); * User.validatesLengthOf('state', {is: 2, message: {is: 'is not valid state name'}}); * ``` * @param {String} propertyName Property name to validate. - * @options {Object} Options + * @options {Object} Options * @property {Number} is Value that property must equal to validate. * @property {Number} min Value that property must be less than to be valid. * @property {Number} max Value that property must be less than to be valid. @@ -103,7 +103,7 @@ Validatable.validatesLengthOf = getConfigurator('length'); * ``` * * @param {String} propertyName Property name to validate. - * @options {Object} Options + * @options {Object} Options * @property {Boolean} int If true, then property must be an integer to be valid. * @property {Object} message Optional object with string properties for 'int' for integer validation. Default error messages: * - number: is not a number @@ -123,7 +123,7 @@ Validatable.validatesNumericalityOf = getConfigurator('numericality'); * ``` * * @param {String} propertyName Property name to validate. - * @options {Object} Options + * @options {Object} Options * @property {Array} in Array Property must match one of the values in the array to be valid. * @property {String} message Optional error message if property is not valid. Default error message: "is not included in the list". */ @@ -135,7 +135,7 @@ Validatable.validatesInclusionOf = getConfigurator('inclusion'); * Example: `Company.validatesExclusionOf('domain', {in: ['www', 'admin']});` * * @param {String} propertyName Property name to validate. - * @options {Object} Options + * @options {Object} Options * @property {Array} in Array Property must match one of the values in the array to be valid. * @property {String} message Optional error message if property is not valid. Default error message: "is reserved". */ @@ -146,9 +146,9 @@ Validatable.validatesExclusionOf = getConfigurator('exclusion'); * * Require a model to include a property that matches the given format. Example: * `User.validatesFormat('name', {with: /\w+/});` - * + * * @param {String} propertyName Property name to validate. - * @options {Object} Options + * @options {Object} Options * @property {RegExp} with Regular expression to validate format. * @property {String} message Optional error message if property is not valid. Default error message: " is invalid". */ @@ -223,7 +223,7 @@ Validatable.validateAsync = getConfigurator('custom', {async: true}); * ``` * @param {String} propertyName Property name to validate. - * @options {Object} Options + * @options {Object} Options * @property {RegExp} with Regular expression to validate format. * @property {Array.} scopedTo List of properties defining the scope. * @property {String} message Optional error message if property is not valid. Default error message: "is not unique". @@ -437,7 +437,7 @@ Validatable.prototype.isValid = function (callback, data) { asyncFail = false; var attrs = Object.keys(validations || {}); - + attrs.forEach(function(attr) { var attrValidations = validations[attr] || []; attrValidations.forEach(function(v) { @@ -454,7 +454,7 @@ Validatable.prototype.isValid = function (callback, data) { } }); }); - + if (!async) { validationsDone.call(inst, function () { if (valid) cleanErrors(inst); @@ -498,12 +498,12 @@ function cleanErrors(inst) { function validationFailed(inst, attr, conf, cb) { var opts = conf.options || {}; - + if (typeof attr !== 'string') return false; // here we should check skip validation conditions (if, unless) // that can be specified in conf - if (skipValidation(inst, conf, 'if') + if (skipValidation(inst, conf, 'if') || skipValidation(inst, conf, 'unless')) { if (cb) cb(false); return false; @@ -715,11 +715,11 @@ function ErrorCodes(messages) { * for (var key in changes) { * model[key] = changes[key]; * } - * + * * if (model.isValid()) { * return callback(null, { success: true }); * } - * + * * // This line shows how to create a ValidationError * err = new ValidationError(model); * callback(err); @@ -735,7 +735,7 @@ function ValidationError(obj) { this.message = util.format( 'The %s instance is not valid. Details: %s.', context ? '`' + context + '`' : 'model', - formatErrors(obj.errors) || '(unknown)' + formatErrors(obj.errors, obj.toJSON()) || '(unknown)' ); this.statusCode = 422; @@ -761,7 +761,9 @@ util.inherits(ValidationError, Error); var errorHasStackProperty = !!(new Error).stack; -function formatErrors(errors) { +ValidationError.maxPropertyStringLength = 32; + +function formatErrors(errors, propertyValues) { var DELIM = '; '; errors = errors || {}; return Object.getOwnPropertyNames(errors) @@ -770,9 +772,52 @@ function formatErrors(errors) { }) .map(function(propertyName) { var messages = errors[propertyName]; + var propertyValue = propertyValues[propertyName]; return messages.map(function(msg) { - return '`' + propertyName + '` ' + msg; + return formatPropertyError(propertyName, propertyValue, msg); }).join(DELIM); }) .join(DELIM); } + +function formatPropertyError(propertyName, propertyValue, errorMessage) { + var formattedValue; + var valueType = typeof propertyValue; + if (valueType === 'string') { + formattedValue = JSON.stringify(truncatePropertyString(propertyValue)); + } else if (propertyValue instanceof Date) { + formattedValue = propertyValue.toISOString(); + } else if (valueType === 'object') { + // objects and arrays + formattedValue = util.inspect(propertyValue, { + showHidden: false, + color: false, + // show top-level object properties only + depth: Array.isArray(propertyValue) ? 1 : 0 + }); + formattedValue = truncatePropertyString(formattedValue); + } else { + formattedValue = truncatePropertyString('' + propertyValue); + } + return '`' + propertyName + '` ' + errorMessage + + ' (value: ' + formattedValue + ')'; +} + +function truncatePropertyString(value) { + var len = ValidationError.maxPropertyStringLength; + if (value.length <= len) return value; + + // preserve few last characters like `}` or `]`, but no more than 3 + // this way the last `} ]` in the array of objects is included in the message + var tail; + var m = value.match(/([ \t})\]]+)$/); + if (m) { + tail = m[1].slice(-3); + len -= tail.length; + } else { + tail = value.slice(-3); + len -= 3; + } + + return value.slice(0, len-4) + '...' + tail; +} diff --git a/test/manipulation.test.js b/test/manipulation.test.js index 0f93ae6c..3f3dbed1 100644 --- a/test/manipulation.test.js +++ b/test/manipulation.test.js @@ -66,8 +66,8 @@ describe('manipulation', function () { it('should not allow user-defined value for the id of object - create', function (done) { Person.create({id: 123456}, function (err, p) { err.should.be.instanceof(ValidationError); - err.message.should.equal('The `Person` instance is not valid. Details: `id` can\'t be set.'); err.statusCode.should.equal(422); + err.details.messages.id.should.eql(['can\'t be set']); p.should.be.instanceof(Person); p.id.should.equal(123456); p.isNewRecord().should.be.true; @@ -80,8 +80,8 @@ describe('manipulation', function () { p.isNewRecord().should.be.true; p.save(function(err, inst) { err.should.be.instanceof(ValidationError); - err.message.should.equal('The `Person` instance is not valid. Details: `id` can\'t be set.'); err.statusCode.should.equal(422); + err.details.messages.id.should.eql(['can\'t be set']); inst.id.should.equal(123456); inst.isNewRecord().should.be.true; done(); diff --git a/test/relations.test.js b/test/relations.test.js index e9313acc..f6cd3680 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -2111,9 +2111,7 @@ describe('relations', function () { p.passportItem.create({}, function(err, passport) { should.exist(err); err.name.should.equal('ValidationError'); - var msg = 'The `Passport` instance is not valid.'; - msg += ' Details: `name` can\'t be blank.'; - err.message.should.equal(msg); + err.details.messages.name.should.eql(['can\'t be blank']); done(); }); }); @@ -2125,9 +2123,8 @@ describe('relations', function () { p.save(function(err) { should.exist(err); err.name.should.equal('ValidationError'); - var msg = 'The `Person` instance is not valid.'; - msg += ' Details: `passportItem` is invalid: `name` can\'t be blank.'; - err.message.should.equal(msg); + err.details.messages.passportItem + .should.eql(['is invalid: `name` can\'t be blank']); done(); }); }); @@ -2568,9 +2565,9 @@ describe('relations', function () { addresses.push({ id: 'work', street: '' }); Person.create({ name: 'Wilma', addresses: addresses }, function(err, p) { err.name.should.equal('ValidationError'); - var expected = 'The `Person` instance is not valid. '; - expected += 'Details: `addresses` contains invalid item: `work` (`street` can\'t be blank).'; - err.message.should.equal(expected); + err.details.messages.addresses.should.eql([ + 'contains invalid item: `work` (`street` can\'t be blank)' + ]); done(); }); }); @@ -2769,7 +2766,7 @@ describe('relations', function () { err.name.should.equal('ValidationError'); err.details.codes.street.should.eql(['presence']); var expected = 'The `Address` instance is not valid. '; - expected += 'Details: `street` can\'t be blank.'; + expected += 'Details: `street` can\'t be blank (value: undefined).'; err.message.should.equal(expected); done(); }); @@ -3225,9 +3222,6 @@ describe('relations', function () { should.exist(err); err.name.should.equal('ValidationError'); err.details.codes.jobs.should.eql(['uniqueness']); - var expected = 'The `Category` instance is not valid. '; - expected += 'Details: `jobs` contains duplicate `Job` instance.'; - err.message.should.equal(expected); done(); }); }); diff --git a/test/validations.test.js b/test/validations.test.js index 11833e10..3df0dd48 100644 --- a/test/validations.test.js +++ b/test/validations.test.js @@ -211,6 +211,16 @@ describe('validations', function () { }); }); + it('should include property value in err.message', function(done) { + delete User.validations; + User.validatesPresenceOf('name'); + User.create(function (e, u) { + should.exist(e); + e.message.should.match(/`name` can't be blank \(value: undefined\)/); + done(); + }); + }); + it('should include model name in err.message', function(done) { delete User.validations; User.validatesPresenceOf('name'); @@ -432,4 +442,65 @@ describe('validations', function () { })).should.be.false; }); }); + + describe('invalid value formatting', function() { + var origMaxLen; + beforeEach(function saveAndSetMaxLen() { + origMaxLen = ValidationError.maxPropertyStringLength; + }); + + afterEach(function restoreMaxLen() { + ValidationError.maxPropertyStringLength = origMaxLen; + }); + + it('should truncate long strings', function() { + ValidationError.maxPropertyStringLength = 9; + var err = givenValidationError('prop', '1234567890abc', 'is invalid'); + getErrorDetails(err) + .should.equal('`prop` is invalid (value: "12...abc").'); + }); + + it('should truncate long objects', function() { + ValidationError.maxPropertyStringLength = 12; + var err = givenValidationError('prop', { foo: 'bar' }, 'is invalid'); + getErrorDetails(err) + .should.equal('`prop` is invalid (value: { foo:... }).'); + }); + + it('should truncate long arrays', function() { + ValidationError.maxPropertyStringLength = 12; + var err = givenValidationError('prop', [{ a: 1, b: 2}], 'is invalid'); + getErrorDetails(err) + .should.equal('`prop` is invalid (value: [ { a...} ]).'); + }); + + it('should print only top-level object properties', function() { + var err = givenValidationError('prop', { a: { b: 'c' }}, 'is invalid'); + getErrorDetails(err) + .should.equal('`prop` is invalid (value: { a: [Object] }).'); + }); + + it('should print only top-level props of objects in array', function() { + var err = givenValidationError('prop', [{ a: { b: 'c' }}], 'is invalid'); + getErrorDetails(err) + .should.equal('`prop` is invalid (value: [ { a: [Object] } ]).'); + }); + + function givenValidationError(propertyName, propertyValue, errorMessage) { + var jsonVal = {}; + jsonVal[propertyName] = propertyValue; + var errorVal = {}; + errorVal[propertyName] = [errorMessage]; + + var obj = { + errors: errorVal, + toJSON: function() { return jsonVal; } + }; + return new ValidationError(obj); + } + + function getErrorDetails(err) { + return err.message.replace(/^.*Details: /, ''); + } + }); }); From e4fc38788f3eb314601328021ec1e40803f83e28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Mon, 12 Jan 2015 16:50:51 +0100 Subject: [PATCH 2/2] Fix Model.prototype.inspect Return the raw object data when running on Node v0.11.14+ That way all `inspect` options are always preserved. When running on older version: - Honour the depth argument passed to the custom `inspect` function. - Disable color output, becase there is now way how to detect whether colors were enabled or disabled by the top-level caller. --- lib/model.js | 52 +++++++++++++++++++++++++++------------- test/validations.test.js | 28 ++++++++++++++-------- 2 files changed, 54 insertions(+), 26 deletions(-) diff --git a/lib/model.js b/lib/model.js index eee95e16..916e09d5 100644 --- a/lib/model.js +++ b/lib/model.js @@ -53,14 +53,14 @@ function ModelBaseClass(data, options) { ModelBaseClass.prototype._initProperties = function (data, options) { var self = this; var ctor = this.constructor; - + if(data instanceof ctor) { // Convert the data to be plain object to avoid pollutions data = data.toObject(false); } var properties = _extend({}, ctor.definition.properties); data = data || {}; - + if (typeof ctor.applyProperties === 'function') { ctor.applyProperties(data); } @@ -72,7 +72,7 @@ ModelBaseClass.prototype._initProperties = function (data, options) { if(strict === undefined) { strict = ctor.definition.settings.strict; } - + if (ctor.hideInternalProperties) { // Object.defineProperty() is expensive. We only try to make the internal // properties hidden (non-enumerable) if the model class has the @@ -122,7 +122,7 @@ ModelBaseClass.prototype._initProperties = function (data, options) { this.__strict = strict; this.__persisted = false; } - + if (options.persisted !== undefined) { this.__persisted = options.persisted === true; } @@ -132,13 +132,13 @@ ModelBaseClass.prototype._initProperties = function (data, options) { } var keys = Object.keys(data); - + if (Array.isArray(options.fields)) { keys = keys.filter(function(k) { return (options.fields.indexOf(k) != -1); }); } - + var size = keys.length; var p, propVal; for (var k = 0; k < size; k++) { @@ -156,7 +156,7 @@ ModelBaseClass.prototype._initProperties = function (data, options) { } } else if (ctor.relations[p]) { var relationType = ctor.relations[p].type; - + if (!properties[p]) { var modelTo = ctor.relations[p].modelTo || ModelBaseClass; var multiple = ctor.relations[p].multiple; @@ -165,7 +165,7 @@ ModelBaseClass.prototype._initProperties = function (data, options) { properties[p] = { name: typeName, type: propType }; this.setStrict(false); } - + // Relation if (relationType === 'belongsTo' && propVal != null) { // If the related model is populated @@ -182,15 +182,15 @@ ModelBaseClass.prototype._initProperties = function (data, options) { } } } - + keys = Object.keys(properties); - + if (Array.isArray(options.fields)) { keys = keys.filter(function(k) { return (options.fields.indexOf(k) != -1); }); } - + size = keys.length; for (k = 0; k < size; k++) { @@ -216,11 +216,11 @@ ModelBaseClass.prototype._initProperties = function (data, options) { self.__data[p] = def; } } - + // Handle complex types (JSON/Object) var type = properties[p].type; if (!BASE_TYPES[type.name]) { - + if (typeof self.__data[p] !== 'object' && self.__data[p]) { try { self.__data[p] = JSON.parse(self.__data[p] + ''); @@ -228,7 +228,7 @@ ModelBaseClass.prototype._initProperties = function (data, options) { self.__data[p] = String(self.__data[p]); } } - + if (type.prototype instanceof ModelBaseClass) { if (!(self.__data[p] instanceof type) && typeof self.__data[p] === 'object' @@ -438,8 +438,28 @@ ModelBaseClass.prototype.reset = function () { } }; -ModelBaseClass.prototype.inspect = function () { - return util.inspect(this.__data, false, 4, true); +// Node v0.11+ allows custom inspect functions to return an object +// instead of string. That way options like `showHidden` and `colors` +// can be preserved. +var versionParts = process.versions.node + .split(/\./g).map(function(v) { return +v; }); + +var INSPECT_SUPPORTS_OBJECT_RETVAL = + versionParts[0] > 0 || + versionParts[1] > 11 || + (versionParts[0] === 11 && versionParts[1] >= 14); + +ModelBaseClass.prototype.inspect = function (depth) { + if (INSPECT_SUPPORTS_OBJECT_RETVAL) + return this.__data; + + // Workaround for older versions + // See also https://github.com/joyent/node/commit/66280de133 + return util.inspect(this.__data, { + showHidden: false, + depth: depth, + colors: false + }); }; ModelBaseClass.mixin = function (anotherClass, options) { diff --git a/test/validations.test.js b/test/validations.test.js index 3df0dd48..85c469b0 100644 --- a/test/validations.test.js +++ b/test/validations.test.js @@ -230,7 +230,7 @@ describe('validations', function () { done(); }); }); - + it('should return validation metadata', function() { var expected = {name:[{validation: 'presence', options: {}}]}; delete User.validations; @@ -245,11 +245,11 @@ describe('validations', function () { it('should validate presence', function () { User.validatesPresenceOf('name', 'email'); - + var validations = User.validations; validations.name.should.eql([{validation: 'presence', options: {}}]); validations.email.should.eql([{validation: 'presence', options: {}}]); - + var u = new User; u.isValid().should.not.be.true; u.name = 1; @@ -273,7 +273,7 @@ describe('validations', function () { }); }); - + describe('absence', function () { it('should validate absence', function () { @@ -355,7 +355,7 @@ describe('validations', function () { done(err); }); }); - + it('should skip blank values', function (done) { User.validatesUniquenessOf('email'); var u = new User({email: ' '}); @@ -370,9 +370,9 @@ describe('validations', function () { }); })).should.be.false; }); - + it('should work with if/unless', function (done) { - User.validatesUniquenessOf('email', { + User.validatesUniquenessOf('email', { if: function() { return true; }, unless: function() { return false; } }); @@ -414,7 +414,7 @@ describe('validations', function () { Boolean(u.isValid()).should.be.false; u.errors.codes.should.eql({ email: ['invalid-email'] }); }); - + it('should validate and return detailed error messages', function() { User.validate('global', function (err) { if (this.email === 'hello' || this.email === 'hey') { @@ -427,11 +427,11 @@ describe('validations', function () { u.errors.should.eql({ email: ['Cannot be `hello`'] }); u.errors.codes.should.eql({ email: ['invalid-email'] }); }); - + it('should validate using custom async validation', function(done) { User.validateAsync('email', function (err, next) { process.nextTick(next); - }, { + }, { if: function() { return true; }, unless: function() { return false; } }); @@ -486,6 +486,14 @@ describe('validations', function () { .should.equal('`prop` is invalid (value: [ { a: [Object] } ]).'); }); + it('should exclude colors from Model values', function() { + var obj = new User(); + obj.email = 'test@example.com'; + var err = givenValidationError('user', obj, 'is invalid'); + getErrorDetails(err).should.equal( + '`user` is invalid (value: { email: \'test@example.com\' }).'); + }); + function givenValidationError(propertyName, propertyValue, errorMessage) { var jsonVal = {}; jsonVal[propertyName] = propertyValue;