CodeSOD: Terned Off Validations

Validating user input is one of those problems that's deceptively simple. The actual validation rules tend to be simple to describe: this field should be at least ten characters long, that field should match this regex, this one should be a number. But applying those rules can get cumbersome, especially when you write it yourself.

It can quickly get as thorny and difficult as date handling.

Well, Carole inherited some home-brew validation code. It's been running in the front-end of a web app for years.

const validateGeneral = async function (importantDataStructure) { log.info(`Validating schema for ${importantDataStructure.name} `) let errors = [] try { importantDataStructure.importantField === 'onlyThisStringIsAllowed' ? null : errors.push(' Invalid importantField') validations.NAME_VALIDATION.test(importantDataStructure.name) ? null : errors.push(' importantDataStructure name has invalid characters') importantDataStructure.requiredField === undefined ? errors.push(' importantDataStructure is missing requiredField') : null if (errors.length > 0) { log.error(`Errors detected for ${importantDataStructure.name} `) throw new Error(errors.toString().trim()) } } catch (err) { console.log(err) }}

The first hint that there's something bad here might just be the function name: validateGeneral. That's not a particularly informative name. The second hint is that the function is marked async, but does nothing asynchronous.

The meat of the function though is the validation rules, all of which are implemented as ternary expressions, but ignoring the expression part. They're using ternaries for flow control: validationRule ? null : handleError is the basic formula. I'm not certain that I've seen this particular abuse of ternaries before. It certainly does an excellent job making the code more complicated and harder to read.

For each failed validation rule, we push the error message into an array. This is already a bad choice, as we're losing any information about which field caused the failure, making it hard to understand the actual validation rules. But then, once we're done, we concatenate all the failed rules together into a single output string, so "making the errors easy for the user to understand" goes right out the window.

Which is just as well, since we never show the errors to the user anyway.

If there are errors, we throw them. And then we immediately catch them, and simply log them to the console. The user can't see them, unless they pull up the developer console.

Carole also did us the favor of digging through the code history, which allows us to understand a little bit more about the weird choices.

The original developer marked the function async, because they wrapped all the ternaries in a Promise.all call- clearly trying to make the obstinately single-threaded JavaScript do some multithreaded validation. It never worked, so they removed the Promise.all call, but not the async.

The function used to throw the errors up the chain (which is its own WTF, since a return value probably makes more sense), but while debugging another developer wrapped it all in the try/catch block. And then they never removed the debugging code, and thus broke the function entirely. It passed code review, testing, and was released to production in that state. If anyone noticed that validations didn't work, nobody's reported the problem yet.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.