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

Easier identification/exposition of When conditions #2114

Closed
Zxenop opened this issue Jun 15, 2023 · 4 comments
Closed

Easier identification/exposition of When conditions #2114

Zxenop opened this issue Jun 15, 2023 · 4 comments
Milestone

Comments

@Zxenop
Copy link

Zxenop commented Jun 15, 2023

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

To give some background, we are working on an exporter à la Typegen to export FV schemas to Yup schemas, works a treat on basics schemas but I'm facing issues with when conditional rules are used.

The two problems I have are:

  • When uses Func<T, bool> for the predicate, which makes it basically impossible to translate to javascript
  • IRuleComponent only exposes HasCondition which doesn't allow identification of the condition

The func issue I can work around by making a custom abstract validation that's exposes a When with an Expression and caching the expression. That custom When is called over the one with the Func parameter so it's transparent for the rest of the team

For the second issue I've had to resort to reflection shenanigans by getting the internal Rules on the validator and invoking OnItemAdded on it, in my custom When, and caching the added rules with the Expression, lots of fun but not very sustainable

Describe the solution you'd like

Not entirely sure to be honest

  • Replacing the type for conditions from Func to Expression<Func and exposing the condition in IRuleComponent, would be the 'logical' solution but I realize this would be huge undertaking mostly because the condition on the RuleComponents is not the Func that was passed originally
  • Alternatively exposing a public version of OnItemAdded (OnRuleAdded perhaps ?) on AbstractValidator would help a ton, don't know what use it would be beside helping in my particular case though
  • Alternatively alternatively, changing only the type of the predicate parameter on When to Expression compiling it to the use the existing internal logic but passing the original Expression down to the RuleComponents and exposing it

Describe alternatives you've considered

No response

Additional Context

No response

@JeremySkinner
Copy link
Member

JeremySkinner commented Jun 15, 2023

Hi, thanks for the suggestion.

The first thing I'd say is FluentValidation is specifically not intended to be a tool for generating client-side validation rules and this is something I'd consider to be very much out of scope. The type of constraints needed between server-side and client-side are generally different enough that it's better to maintain clientside rules separately from serverside as trying to use a single source for both will result in compromise and limitations on what you can use on both ends, so my strong recommendation would not be to go down this route (although I can't stop you from doing so if you really want to). Although clientside validation and serverside validation are similar, they do actually have separate goals & constraints and I think it's a mistake to try and generate the two from a single source unless it's for the simplest of use cases.

With that out of the way I'll try and address the specific suggestions:

Replacing the type for conditions from Func to Expression<Func and exposing the condition in IRuleComponent

Changing the condition to an expression is something we absolutely don't want to do. Expression compilation is expensive and should be avoided unless absolutely necessary, and even with caching it's not ideal. I've done a lot of work on FluentValidation's performance over the years and handling of expressions are 100% the biggest issue - this would become significantly worse if conditions were changed to be expressions too, so I'm afraid this isn't a change that I'm willing to make.

Alternatively exposing a public version of OnItemAdded (OnRuleAdded perhaps ?) on AbstractValidator

I'm much more open to this, although I'm very wary of opening up internal methods of the library as I've been bitten before by having too much public, which then drastically increases the maintenance burden. I'll have a think about this one though.

However I would still strongly encourage you not to go down this route, it leads to more headaches than it solves in the long run.

@Zxenop
Copy link
Author

Zxenop commented Jun 15, 2023

Hi @JeremySkinner, thanks for the agreeably quick response.

I get not wanting to introduce Expressions, I figured performance was going to be an issue, it's not for us in this context but hey I had to try =P

Regarding the scope the FV, that's very fair too of course, although in our particular case the exporting of FV schemas to Yup schemas stemmed from a willingness to avoid duplicated code and to introduce more client side validation at basically reduced cost, simply put the exporter can't export everything, by being a bit careful it will only export what's common between sever and client validation (meaning no validation against the DbContext) and Yup schemas are easily customizable/overridable

How about adding something like this on AbstractValidator ? :

	public IDisposable OnRuleAdded(Action<IValidationRule<T>> onItemAdded) {
		return Rules.OnItemAdded((_) => onItemAdded(_));
	}

No need to make public anything that isn't already, won't solve all of my problems but it should take care of the When(condition, action) case

@Zxenop Zxenop closed this as completed Jun 15, 2023
@Zxenop
Copy link
Author

Zxenop commented Jun 15, 2023

Butter fingers on my part, reopening the ticket

@JeremySkinner
Copy link
Member

11.8 is released with this change

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants