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

Include which validator failed in errors.mapped() #1124

Open
amagid opened this issue Dec 21, 2021 · 1 comment · May be fixed by #1130
Open

Include which validator failed in errors.mapped() #1124

amagid opened this issue Dec 21, 2021 · 1 comment · May be fixed by #1130

Comments

@amagid
Copy link

amagid commented Dec 21, 2021

Is your feature request related to a problem? Please describe.

This is not a functionality issue - it's an ease of use enhancement for validation messages. Let me know if there is already a way to get the information I'm looking for.

Basically, I can't see what validator failed in the output from errors.mapped(). I can see that the validation failed, but all I get is "InvalidValue". I want to have expressive validation messages for every validator. I understand that express-validator does not want to be opinionated about what error messages are returned - that makes perfect sense. My thought here is that if errors.mapped() returned the validator instance that failed, I'd be able to write a fairly simple generic middleware that would take that information and create a message in my message style. But as far as I'm aware, there is no way to see which validation condition failed when looking at the output of errors.mapped().

I could add withMessage calls to my validators, but then I'm duplicating code in my validators which isn't great. For example, I'd need to write something like:

body('balance').exists().withMessage('balance is required').isNumeric().withMessage('balance must be numeric').isLength({ min: 2, max: 10 }).withMessage('balance must be between 2 and 10 characters')

While that's doable for a single api parameter, writing and maintaining that for thousands of parameters across hundreds of api endpoints will almost certainly be a huge pain and end up out of sync (the validators will probably get updated and the messages will be forgotten and end up outdated).

Describe the solution you'd like

Note: code snippets and line numbers below were taken from my downloaded copy of the module from my node modules so they are the transpiled JS files

Add a non-enumerable property called "condition" to the error output containing the negated, validator, and options properties from the contextItem that was being processed when the validation failed.

For example, for StandardValidation (src/context-items/standard-validation.js) line 14 where context.addError is called, we could add an additional parameter on context.addError to contain the validation condition that failed like so:

if (this.negated ? result : !result) {
    context.addError(this.message, value, meta, this); // new 4th parameter `this`
}

Then, in context.addError (src/context.js) lines 64 and 72 where the errors are pushed to the this._errors array, we could add a nonenumerable property first to those array elements like so:

    // Added new 4th parameter for contextItem
    addError(message, valueOrNestedErrors, meta, condition) {
        const msg = message || this.message || 'Invalid value';
        if (meta) {
            let errorToPush = {
                value: valueOrNestedErrors,
                msg: typeof msg === 'function' ? msg(valueOrNestedErrors, meta) : msg,
                param: meta.path,
                location: meta.location,
            };
            // Add nonenumerable property containing contextItem before pushing to array
            Object.defineProperty(errorToPush, 'condition', { value: condition });
            this._errors.push(errorToPush);
        }
        else {
            let errorToPush = {
                msg,
                param: '_error',
                nestedErrors: valueOrNestedErrors,
            };
            // Add nonenumerable property containing contextItem before pushing to array
            Object.defineProperty(errorToPush, 'condition', { value: condition });
            this._errors.push(errorToPush);
        }
    }

Of course, we'd need to get all of the other contextItem classes updated as well.

I've tested this modification and it results in errors.mapped returning the condition element in the result as a nonenumerable property. The advantage of the nonenumerable property is it will not be automatically serialized by express so this will not change the return values of existing APIs that use express-validator (except in the extremely specific and extremely unlikely case where someone else already created a parameter called condition as a patch on the error output from express-validator)

The benefit here is that then I can then wrap the validation process in a middleware in my APIs that looks at the error and constructs a message based on the condition which includes the validation function (and by extension, its name), any options passed to that validation function, and whether or not that validation function was negated. It's fairly trivial with that information to construct a good message automatically.

Also - one quick side note: I noticed that exists() is implemented as a CustomValidator rather than a StandardValidator - does anyone know why that is?
I'm also curious if someone who knows this codebase better can tell me what other files would need to be updated to consistently show this type of error information.

@amagid
Copy link
Author

amagid commented Dec 21, 2021

I'm happy to submit a PR for this, looking for some feedback from core maintainers before I write anything.

@amagid amagid linked a pull request Feb 13, 2022 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant