Validation order: elements go first and collection itself goes second

[abatishchev]

Hi, I have a web api accepting a collection of elements both having validator:

public IHttpActionResult PostData(PaymentCollection payments)
{
}

[Validator(typeof(PaymentCollectionValidator))]
public class PaymentCollection : Collection<Payment>
{
}

[Validator(typeof(PaymentValidator))]
public class Payment
{
}

I want to check the collection for length and its elements for validity.

But whrn I make a request, elements go first and collection itself goes second in the validation order even collection is invalid (has too many elements, for instance) so validation is guaranteed to fail.

How can I change the order?

[JeremySkinner]

That's not possible if you're using the web api integration. You'd need to invoke the validator manually and use SetValidator instead of using the Validator attributes.

[abatishchev]

Can I anyhow extend the existing model validation provider?

[JeremySkinner]

Sure - you'd probably want to look at WebApi's DefaultBodyModelValidator as a starting point; this is where the recusrive validation of properties actually occurs.

[abatishchev]

Would you agree to add a public boolean property ValidateModelFirst (false by default) to FluentValidationBodyModelValidator to control whether or not follow the rule Validate the children first - depth-first traversal? e.g.:

if (ValidateModelFirst)
{
    isValid = ShallowValidate(metadata, validationContext, container);
    isValid = ValidateChildren(metadata, validationContext, model) && isValid;
}
else
{
    isValid = ValidateChildren(metadata, validationContext, model);
    isValid = ShallowValidate(metadata, validationContext, container) && isValid;
}

[JeremySkinner]

I'm not at all keen on doing that to be honest - the only reason the
fluentvalidationbodymodelvalidator exists is to work around an issue in the
defaultbodymodelvalidator...id prefer to keep its code as close to the
original as possible (it's essentially a copy and paste straight from the webapi source). Eventually, I'd like to get rid of that class completely.

If you need this behaviour, I'd suggest having your own Bodymodelvalidator
implementation in your project...so you can then swap out the default (or
the FV one) with your version, if you want to make this modification.

On Monday, 21 December 2015, Alexander Batishchev notifications@github.com
wrote:

Would you agree to add a public boolean property ValidateChildrenFirst to
FluentValidationBodyModelValidator to control whether or not follow the
rule "Validate the children first - depth-first traversal
https://github.com/JeremySkinner/FluentValidation/blob/master/src/FluentValidation.WebApi/FluentValidationBodyModelValidator.cs#L109"?
e.g.:

if (ValidateChildrenFirst)
{
ValidateChildren();
isValid = ShallowValidate(metadata, validationContext, container) && isValid;
}else
{
isValid = ShallowValidate(metadata, validationContext, container) && isValid;
ValidateChildren();
}

—
Reply to this email directly or view it on GitHub
#178 (comment)
.

[abatishchev]

I see, fair enough. To minimize the pain of copy-pasting the code, could you please change ValidateNodeAndChildren to protected virtual and other private methods to protected? Please don't make us suffer :)

[JeremySkinner]

Honestly I'd rather not for the same reasons...they're private in the
original webapi version and it's literally a copy and paste...I don't want
people relying on the FV version of the class as it's a fork that is going
to go away...I'm sorry if it sounds like I'm being awkward but by opening
it up, I'm then locked into supporting it going forward, which is something
I absolutely don't want to do as its internal webapi code, owned by MS.

Ideally, MS should make that method virtual in the original class (which
would mean that the code wouldn't have had to have been copied in the first
place), but I doubt they'll change it now that aspnet 5 supersedes webapi.

On Monday, 21 December 2015, Alexander Batishchev notifications@github.com
wrote:

I see, fair enough. To minimize the pain of copy-pasting the code, could
you please change ValidateNodeAndChildren to protected virtual and other
private methods to protected? Please don't make us suffer :)

—
Reply to this email directly or view it on GitHub
#178 (comment)
.

[abatishchev]

Can you please elaborate on your reasons do not open it up?
Since Web Api source code is licensed under the Apache 2.0 license and is released as a separate nuget package, from my point of view, it should be fine. Web Api doesn't go away right away after ASP.NET 6.0 is released, and will be widely used for year after that.

[JeremySkinner]

Sure, the license isn't the issue- it's the maintenance and
support overhead. I understand that from your point of view it's fine,
but by opening it up, it's then something I have to support going forward,
and commit to having it in the codebase for the foreseeable future as
others could be relying on its presence.

Unfortunately I just don't have the capacity to do this...just supporting
FV by itself is already a massive time investment for me, and I have to
be very careful about what becomes part of the public api.

Once something is publicly accessible, then I have to commit to supporting
it, because inevitably someone else's code can now be relying on this. It
essentially becomes a contract, in the same way as an interface. Removing
it, or changing it later now becomes a breaking change for all consumers.

This particular class is only in the codebase as a workaround for an issue
with webapi...the aim is that it'll be removed eventually, so I don't want
it to become something that becomes consumed directly or is relied upon. As
this class is essentially a fork, the fewer changes that are made to it the
easier it is for me to maintain. When MS update their version of the class,
the merge process is much easier if I don't have to make lots of small
amendments. If MS completely refactor the class and suddenly change its
structure, I'd have no choice but to make a breaking change to the public
api, and breaking changes Need to be kept to a minimum unless there's a
very good reason.

I know that's not what you want to hear, but this is the reality of
supporting a library that a lot of people rely on, especially when it's a
1-person job

On Monday, 21 December 2015, Alexander Batishchev notifications@github.com
wrote:

Can you please elaborate on your reasons do not open it up?
Since Web Api source code is licensed under the Apache 2.0 license
https://aspnetwebstack.codeplex.com/license and is released as a
separate nuget package, from my point of view, it should be fine. Web Api
doesn't go away right away after ASP.NET 6.0 is released, and will be
widely used for year after that.

—
Reply to this email directly or view it on GitHub
#178 (comment)
.

[abatishchev]

Sure, I see your point. I can easily believe that maintaining a heavily used open source library is a huge deal and chore.
Can I propose two things:

• myself as a co-maintainer of the web api integration package. I'm mainly working with web api projects, for years now and for years to go, I hope.
• since I'm internal to MS, I can contact web api official maintainers and try to convenience them on particular changes. So let's sync on this, what kind of changes you'd like to have in the built-in classes so both your and my goals were achievable.

[JeremySkinner]

That would be very helpful, and very much appereciated if you're up for
being involved-thank you.

That'd be especially welcome as I don't personally work with webapi much at
all, and my experience with it is very limited.

I'll document the changes that'd need to happen to support this properly.
Do you mind if I wait until tomorrow morning? I don't have my dev laptop
with me at the moment.

Thanks again

On Monday, 21 December 2015, Alexander Batishchev notifications@github.com
wrote:

Sure, I see your point. I can believe that maintaining a heavily used
open-source library is a huge deal and chore.
Can I propose two things:

• myself as a co-maintainer of the web api integration package. I'm
  mainly working with web api projects, for years now and for years to go, I
  hope.
• since I'm internal to MS, I can contact web api official maintainers
  and try to convenience them on particular changes. So let's sync on this,
  what kind of changes you'd like to have in the built-in classes so both
  your and my goals were achievable.

—
Reply to this email directly or view it on GitHub
#178 (comment)
.

[abatishchev]

Awesome, sounds good!
No rush at all, anyway it's the holiday season, so no changes would be reviewed till mid-January, I'm afraid.

[JeremySkinner]

So there's a few things that I'd like to see changed in WebApi to better support FV, but for now just focusing on the BodyModelValidator:

If the binder added errors into ModelState before validation occurs, and then FV generates an error message for the same property, the message generated by FV is ignored and never appears. This is because of the following check in DefaultBodyModelValidator.ShallowValidate:

if (!validationContext.ModelState.IsValidField(key))
{
  return false;
}

The FluentValidationBodyModelValidator changes this check to:

var isFluentModelValidator = validator is FluentValidationModelValidator;
...
if ((!isFluentModelValidator && !validationContext.ModelState.IsValidField(key)) ||
    (isFluentModelValidator && validationContext.ModelState.ContainsKey(key))) {
   return false;
}

...so it explicitly checks if we're invoking FV, and if so it allows the second error to be added, with a separate key:

validationContext.ModelState.AddModelError(isFluentModelValidator ? CreatePropertyModelName(key, error.MemberName) : key, error.Message);

Unfortunately, the ShallowValidate method is private static, which is why FluentValidationBodyModelValidator includes so much copy-pasted code.

The simplest change would be for DefaultBodyModelValidator.ShallowValidate to be virtual, but this would still require some duplicate code. However, this method is nested within several other private static method calls, so could possibly have a large refactoring impact for the webapi team.

[SeanKilleen]

Maintenance / cleanup: Closing this as there hasn't been any activity for a few months.