fluentvalidation-ts icon indicating copy to clipboard operation
fluentvalidation-ts copied to clipboard

[Enhancement] Allow rules to be specified on transformed values (`.ruleForTransformed`)

Open michael-georgiadis opened this issue 4 years ago • 5 comments

Your library looks awesome, except one little part.

When I change a name of a property I want the Typescript compiler to throw me a warning about where that property was in the project, so I can go and change them.

So, it's more of a suggestion than an issue, to actually have it like this:

RuleFor(x => x.name)

You have already the structure of a type generic so that means that you have thought of that. Anyways, something to think of!

michael-georgiadis avatar Dec 09 '20 10:12 michael-georgiadis

Especially if it allows, for e.g.,

ruleFor(x => x.name.trim())

like the C# inspiration.

avenmore avatar Feb 05 '21 13:02 avenmore

Hi there 😃

First off, apologies it's taken me so long to respond to this - it's been a busy year so far!

The syntax you're suggesting here was what I'd originally intended to go with, but after some thought and experimentation I decided it wasn't the best fit.

What's the problem?

It really all comes down to the shape of the ValidationErrors object - which is intended to mirror the shape of the model being validated, in that for each property on the model there is potentially a corresponding property on the errors object.

Consider the following rule definition (taken from the comment above):

this.ruleFor(person => person.name.trim()).notEmpty();

The question is, if this validation rule fails, what do we put in the ValidationErrors object? In particular, how do we know what the property name is? In the FluentValidation library for C# this example would produce a ValidationFailure where the PropertyName is blank, which highlights the problem - we can't sensibly decide what the property name should be.

FluentValidation for C# does provide a way to override the property name, but this isn't mandatory. For us the property name would need to be mandatory, even when we're just referencing the raw property:

this.ruleFor(person => person.name, 'name').notEmpty();

It would theoretically be possible to parse the source code of the lambda function and determine the property name in cases like this (where we're just doing model => model.someProperty), in which case it wouldn't be necessary to pass in the property name, but there is no way to distinguish between a "raw property" lamda and a "modified property" lambda in the type system (they're both of type (model: TModel) => any).

RE: @Funnyman420

So, in terms of the original suggestion (from @Funnyman420) - I don't believe this is feasible without requiring the property name (as a string) to be specified in addition to the lambda function. As you've pointed out, the property name in the call to .ruleFor is already typed such that it must match a property name on the type you're validating, so:

  • If you change the name of a property on your model, you'll get a compiler error in your validation code in the exact place you need to change
  • If you change the name of a property in your validation code, you'll get a compiler error in your validation code and can then click through to the code for your model (which is referenced as the type parameter of Validator<TModel>) to see what property names are available
    • I'm not sure why you'd ever be doing things this way round (changing the property name in the validation code before updating the model) - please let me know if there's a use-case I'm not thinking of!

The only obvious downside to the current approach is that you can't necessarily use refactoring tools to rename the property on the model and have it update in the validation code - however I would imagine that in the vast majority of cases there is only ever a single Validator defined for a given model, so the scope of the necessary follow-up changes is small.

RE: @avenmore

In terms of the comment from @avenmore - the use-case here seems to be that you want to transform the property in some way before applying validation. This is an interesting idea, but I'd like to understand the motivation a bit before coming to any conclusions.

The first reason I can think of why you might want to do this is because you're going to "normalise" the model at some point - either before or after passing it to your API (or wherever else it's going). Using the example you've given above, your API might have a 50-character limit on the name property, but you know you're going to trim the name before sending it off. So, you wouldn't want a 10-character name followed by 50 spaces to be rejected by your front-end validation. In this case I'd argue that you should be performing validation on the normalised model rather than the raw model:

// ❌ 
const errors = validator.validate(model);
// reject if errors..
api.send(normalise(model));

// ✔️ 
const normalisedModel = normalise(model);
const errors = validator.validate(normalisedModel);
// reject if errors..
api.send(normalisedModel);

The second reason I can think of why you might want to do this is because you have some bespoke validation that isn't directly supported by the built-in validation rules. Suppose your model has a yearOfBirth property modeled with a string input, and you want to check that it's a number which is no less than 1900. With the approach you're suggesting, you might write something like:

const earliestYear = 1900;

this.ruleFor(x => isNaN(Number(x.yearOfBirth)) ? null : Number(x.yearOfBirth))
  .notNull().withMessage('Please enter a number')
  .greaterThanOrEqualTo(earliestYear).withMessage(`Cannot be earlier than ${earliestYear}`);

The way complex validation rules can be handled with the current API is by using .must (and potentially even building some custom rules):

const earliestYear = 1900;

this.ruleFor('yearOfBirth')
  .must(yearOfBirth => !isNaN(Number(yearOfBirth))).withMessage('Please enter a number')
  .must(yearOfBirth => Number(yearOfBirth) >= earliestYear).withMessage(`Cannot be earlier than ${earliestYear}`);

On reflection, this use-case does present a good argument for allowing values to be transformed prior to validation - I think the way you have to do this currently is quite cumbersome. In fact, FluentValidation for C# supports property transformation and uses basically the exact same example in their documentation (see here) as motivation.

AlexJPotter avatar May 10 '21 09:05 AlexJPotter

Proposal

In light of the above, I'd like to propose the introduction of a new .ruleForTransformed method which allows specifying validation rules on a transformed property value.

This will obviously need to be typed appropriately so that the rules offered match the type of the transformed value. Introducing a new method rather than modifying the existing .ruleFor method will help keep the types from becoming messy. The suggested syntax is inspired by what FluentValidation for C# does (see here).

.ruleForTransformed

An example of how this might be used is as follows:

const asNumberOrNull = (value: string): number | null =>
  isNaN(Number(value)) ? null : Number(value);

const earliestYear = 1900;

this.ruleForTransformed('yearOfBirth', asNumberOrNull)
  .notNull().withMessage('Please enter a number')
  .greaterThanOrEqualTo(earliestYear).withMessage(`Cannot be earlier than ${earliestYear}`);

I'll start knocking together an implementation for this, but in the meantime I'd love to hear people's thoughts 😃

AlexJPotter avatar May 10 '21 09:05 AlexJPotter

Hey! I've managed to actually get the property name using lambda.

You can actually do it with a proxy. I have a a repo on my profile that has your code but I didn't fork your repo to change it cause that change knocked the whole repo out 😂. So I wrote it from scratch using the code that I could understand mixing it with mine.

https://www.github.com/Funnyman420/valimo/tree/main/src%2Fabstract.validator.ts

It's in the ruleFor function.

(I've mentioned you in the readme as a main inspiration)

michael-georgiadis avatar May 11 '21 06:05 michael-georgiadis

Firstly, I originally thought the lambda would be good as intellisense could help pick the property, it would be type-checked and refactoring could be automated. As you've mentioned, a property is already typed and refactoring manually in this scope would be easy.

Secondly, my motivation was pure laziness (and lazy thinking). I saw a way that I could use my exiting extension methods (in my C# version) and existing validators to avoid writing a custom validator. A specific example was where I wanted to make sure that there were at least 3 alpha characters and already had a string extension that could return just letters, so just used RuleFor(e => e.cLastName.OnlyLetters()).MinimumLength(3). This quickly ran into another problem when a property was null and the library started throwing awkward exceptions. This was easily fixed by just changing it to RuleFor(e => e.cLastName).Must(n => (n == null) || (n.OnlyLetters().Length >= 3)) and I abandoned trying to modify the input in that unexpected way.

Thirdly, I wasn't aware of the Transform() option before this, but think that it could be a handy tool to make validation more concise, reuasable and readable. If you were to implement your .ruleForTransformed proposal, I would certainly make use of it and like the way you've described it.

avenmore avatar May 11 '21 08:05 avenmore

The .ruleForTransformed proposal has been implemented in v2.3.0 🎉

AlexJPotter avatar Oct 07 '22 12:10 AlexJPotter