kiva / protocol-common

Shared code across the various protocol microservices
Apache License 2.0
10 stars 3 forks source link

PRO-2948: Add support for an imperative style to validations #32

Closed ghost closed 3 years ago

ghost commented 3 years ago
🔥 🐞 🙋 🚫 🚀

(Click here if you don't understand these emojis)

What issue is this targeting?

Changes proposed in this pull request

🚀 Deployment changes 🚀
(list added or removed env, db changes etc)

Other Notes

Signed-off-by: Jeff Kennedy jeffk@kiva.org

ghost commented 3 years ago

Sorry in advance for the wall of text!

The main difference between them is that Min is >= where as greaterThan is > (and similarly for Max vs lessThan). But yeah, I didn't add these to solve a particular need. I mostly added these as props to help dig into corner cases, root out possible design constraints, and write helpful tests. I'm sort of punting on what the final set of validations should actually be.

That said, there's a more fundamental difference. You can't just use the class-validator function on a function parameter (at least, not in decorator form), you can only use them on class properties. If you want to use a class-validator function on a function parameter, you need to wrap it in something and turn it into a parameter decorator that registers the function so that it can be called at runtime by a method decorator. So, I figured, this is an independent set of validations (which may occasionally, incidentally, delegate to functionality provided by class-validator).

Then I thought, why be limited to just using decorators? Not everyone is sold on aspect-oriented programming, and maybe you want to use it without the metadata pollution, or want to be able to run the validations at some other point in lifecycle of your function's execution. Sure, you could call the class-validator functions directly if you want, but then you're just using a different library (and one which may have a different set of validations). If you want to use this library, then there should be perfect symmetry between the decorator validations and the imperative validations, because one is not more important than the other.

I'm thinking of this validation library as something complete in its own right and not simply a wrapper around class-validation (even if that's what it really is right now). So if I'm using this library and I use the @LessThan decorator in one place, I expect there to be a lessThan() function. If I have to go to some other library to get access to that function, then that's a problem, not just because it's awkward, but because I can't trust that the underlying implementation will have the same performance.

I do see that it feels a bit strange and maybe confusing to have two parallel implementations of the same validations, though, and I'm totally open to other approaches. But to be honest, It's a bit awkward either way. For example, if it's simply a wrapper around class-validation, then what I called isInteger should be isInt (because that's what they call it). But if we do that, then a class may have an isInt property validation and a function with an isInt parameter validation, which is obviously a name collision. You can of course fix this by aliasing the import, but then you have a different name for isInt anyway and you're really pushing the task of dealing with this awkwardness onto the user.

Anyway, that's my thought process! Sorry again for plastering too many words here.