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

Add option to show parameter names in Validation Errors #32

Merged

Conversation

duncanhall
Copy link
Contributor

Implementation for #31

Adds a useNamedValidators property to the options object.

If this value is set to true then all ValidationError messages will show the name of the parameter that was being validated.

For backwards compatibility, if no useNamedValidators property is set, then 'value' is used as before (ie, the property defaults to false).

@tlivings
Copy link
Member

This information should already be present in the error itself. For example:

  "name": "ValidationError",
  "details": [
    {
      "message": "lastName is required",
      "path": "lastName",
      "type": "any.required"
    }
  ],
  "_object": {
    "firstName": "John",
    "age": 45,
    "tags": [
      "man",
      "human"
    ]
  }
}

Are you seeing something different?

@duncanhall
Copy link
Contributor Author

Yes, as mentioned it is always value is required rather than lastName is required.

In the PR I submitted you should see I have added tests for the new functionality. If you remove the implementation I created, but leave the tests you will see that they fail (because it expects to see the parameter name in the error message.)

@tlivings
Copy link
Member

The snippet I posted above is from enjoi - without your additions. Let me dig into why something different is being seen here.

@tlivings
Copy link
Member

I see what's going on now. When a validation occurs for a parameter, joi has no context regarding the parameter being validated (as opposed to validating an object's properties in my example above).

So, on the face of it, I see nothing wrong with modifying the error object in this way. I don't think you actually need to make this optional, since this should be expected behavior (since we do know the context).

If you would, please modify the PR to update the error object always, and ensure it modifies error.message, error.details[n].message, and error.details[n].path for consistency.

Sound like a plan?

@tlivings
Copy link
Member

Example:

if (error) {
    error.message = error.message.replace('value', parameter.name);
    error.details.forEach(function (detail) {
        detail.message = detail.message.replace('value', parameter.name);
        detail.path = parameter.name;
    });
    utils.debuglog('%s', error.message);
    callback(error);
    return;
}

@duncanhall
Copy link
Contributor Author

Thanks, this is done as suggested, with some extra tests.

@@ -31,6 +31,7 @@ Options:
- `handlers` - either a directory structure for route handlers or a premade object (see *Handlers Object* below).
- `basedir` - base directory to search for `handlers` path (defaults to `dirname` of caller).
- `schemas` - an array of `{name: string, schema: string|object}` representing additional schemas to add to validation.
- `useNamedValidators` - whether to show parameter names in validation errors (defaults to false).
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this now that it is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, will do.

@tlivings
Copy link
Member

👍

tlivings added a commit that referenced this pull request Mar 3, 2015
…tors

Add option to show parameter names in Validation Errors
@tlivings tlivings merged commit 7e61fc2 into krakenjs:master Mar 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants