Skip to content

ValidationError messages include field value #368

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.

Sign up for GitHub

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

Closed
bitmage wants to merge 1 commit into from
Closed

ValidationError messages include field value #368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions lib/validations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) || '(unknown)'
);

this.statusCode = 422;
Expand All @@ -760,18 +760,29 @@ function ValidationError(obj) {
util.inherits(ValidationError, Error);

var errorHasStackProperty = !!(new Error).stack;
var inspect = require('util').inspect;

function formatProperty(obj, propertyName) {
var value = obj[propertyName];
if ((typeof value) === 'function')
return propertyName
else if ((typeof value) === 'string')
return propertyName + ': ' + inspect(value.substring(0, 32));
else
return propertyName + ': ' + inspect(value);
}

@bitmage bitmage Jan 7, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of this? I noticed we were producing some strange output in the case of nested documents (which have a type of function). I don't know how to output a sensible output in that case... but I think it's an edge case that can be left for another day.

For strings, objects, and undefined/null we now have some pretty clear output.

@bajtos bajtos Jan 8, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handling non-scalar values (objects, arrays, functions) will be always tricky. You can take a look at chai.js which basically prints only top-level keys of the object.

As for function values, JSON.stringify removes them from the output, which solves the problem. On the other hand, it does not handle circular references, which may be a problem.

function formatErrors(errors) {
function formatErrors(obj) {
var DELIM = '; ';
errors = errors || {};
var errors = obj.errors || {};
return Object.getOwnPropertyNames(errors)
.filter(function(propertyName) {
return Array.isArray(errors[propertyName]);
})
.map(function(propertyName) {
var messages = errors[propertyName];
return messages.map(function(msg) {
return '`' + propertyName + '` ' + msg;
return '`' + formatProperty(obj, propertyName) + '` ' + msg;
}).join(DELIM);
})
.join(DELIM);
Expand Down
4 changes: 2 additions & 2 deletions test/manipulation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ 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.message.should.equal('The `Person` instance is not valid. Details: `id: 123456` can\'t be set.');
err.statusCode.should.equal(422);
p.should.be.instanceof(Person);
p.id.should.equal(123456);
Expand All @@ -80,7 +80,7 @@ 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.message.should.equal('The `Person` instance is not valid. Details: `id: 123456` can\'t be set.');
err.statusCode.should.equal(422);
inst.id.should.equal(123456);
inst.isNewRecord().should.be.true;
Expand Down
6 changes: 3 additions & 3 deletions test/relations.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2112,7 +2112,7 @@ describe('relations', function () {
should.exist(err);
err.name.should.equal('ValidationError');
var msg = 'The `Passport` instance is not valid.';
msg += ' Details: `name` can\'t be blank.';
msg += ' Details: `name: undefined` can\'t be blank.';
err.message.should.equal(msg);
done();
});
Expand Down Expand Up @@ -2569,7 +2569,7 @@ describe('relations', function () {
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).';
expected += 'Details: `addresses: [ { id: \u001b[32m\'home\'\u001b[39m, street: \u001b[32m\'Home Street\'\u001b[39m },\n { id: \u001b[32m\'work\'\u001b[39m, street: \u001b[32m\'\'\u001b[39m } ]` contains invalid item: `work` (`street` can\'t be blank).';

@bajtos bajtos Jan 8, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. Error messages must not include ANSI color escape sequences.

@bajtos bajtos Jan 8, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice how an invalid item in an array is handled: the message contains id of the item, and then information about the error in this item only.

This patch should build on top of that and print only the invalid item in the message. Example:

Details: `addresses` contains invalid item: `work` (`street: undefined`) can't be blank

@bajtos bajtos Jan 8, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice how an invalid item in an array is handled: the message contains id of the item, and then information about the error in this item only.

This patch should build on top of that and print only the invalid item in the message. Example:

On the second thought, that would be a much more involved change, let's leave it for another day.

However, you still need to remove colour codes.

@bitmage bitmage Jan 8, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Why are the color codes getting inserted in the first place? I'm using 'inspect' from node core API. I had never noticed it to print colors before. Is one of the Loopback dependencies monkey patching it to add colors?

I decided to use 'inspect' because it limits how deep it will parse the tree. This seemed like desirable behavior which JSON.stringify does not offer.

On Jan 8, 2015, at 1:03 AM, Miroslav Bajtoš notifications@github.com wrote:

In test/relations.test.js:

@@ -2569,7 +2569,7 @@ describe('relations', function () {
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).';

  •    expected += 'Details: `addresses: [ { id: \u001b[32m\'home\'\u001b[39m, street: \u001b[32m\'Home Street\'\u001b[39m },\n  { id: \u001b[32m\'work\'\u001b[39m, street: \u001b[32m\'\'\u001b[39m } ]` contains invalid item: `work` (`street` can\'t be blank).';
    Notice how an invalid item in an array is handled: the message contains id of the item, and then information about the error in this item only.

This patch should build on top of that and print only the invalid item in the message. Example:

On the second thought, that would be a much more involved change, let's leave it for another day.

However, you still need to remove colour codes.

—
Reply to this email directly or view it on GitHub.

@bajtos bajtos Jan 9, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Why are the color codes getting inserted in the first place? I'm using 'inspect' from node core API. I had never noticed it to print colors before. Is one of the Loopback dependencies monkey patching it to add colors?

That's the default behaviour of util.inspect - see http://nodejs.org/api/util.html#util_customizing_util_inspect_colors.

@bajtos bajtos Jan 9, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently util.inspect has a config option to enable/disable colors:

colors - if true, then the output will be styled with ANSI color codes. Defaults to false. Colors are customizable, see below.

err.message.should.equal(expected);
done();
});
Expand Down Expand Up @@ -2769,7 +2769,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: undefined` can\'t be blank.';
err.message.should.equal(expected);
done();
});
Expand Down
2 changes: 1 addition & 1 deletion test/validations.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ describe('validations', function () {
User.validatesPresenceOf('name');
User.create(function (e, u) {
should.exist(e);
e.message.should.match(/`name` can't be blank/);
e.message.should.match(/`name: undefined` can't be blank/);
done();
});
});
Expand Down