Updated ModelValidationResult and ModelValidationError

[thecodejunkie]

Instead of errors being represented by a simple
IList<ModelValidationError> structure, it is not stored in an
IDictionary<string, IList<ModelValidationError>> structure. The key is the
name of the property that the error represents.

The error message formatter has also been updated from Func<string, string to string

Last, the ModelValidationError can now implicitly be cast to a string

[GiscardBiamby]

What if the validation is across multiple properties? Like "if (country == usa && age >22) then Model is valid"? I'm building a fairly large app with hundreds of individual models that have rules like this.

[jchannon]

Looks good. I assume the Func is for performance? I don't really like it would, prefer just a string but I think I get why its there

[thecodejunkie]

@jchannon I assume you mean for the ModelValidationError? The Func has always been there, but ut used to be Func<string, string> but now it's Func<IEnumerable<string>, string>

We could add a string overload perhaps?

[thecodejunkie]

@GiscardBiamby could you please elaborate you scenario? I am not sure if you are worried that the changes (in this pull-request) is going to break your existing code or if you would like additional changes to make your use case easier to manage? Interested in hearing about this! :P

[jchannon]

@thecodejunkie just not sure why its a func. An error should have a string key for the model property name and then a IEnumerable for error messages associated to it. Why do you need something called GetMessage to get the error message. Should just be able to do something like

 <for each="var k in Model.Errors.Keys">
    <h2>${k}</h2>
    <ul>
      <for each="var e in Model.Errors[k]">
        <li>${e}</li>
      </for>
    </ul>
  </for>

[thecodejunkie]

@jchannon it was originally created that way because the data annotations provide an error message formatter function, so the approach was mirrored in the Nancy validation abstraction and it worked well for fluent validation.

However, the more I think about it, we should change it to a string and have the various wrappers conform to this. So what it means for (example) data annotations is that we invoke the func and take the result and stick in our type.

[jchannon]

Sounds like a plan

[CumpsD]

On ModelValidationError you have this:

        public static implicit operator string(ModelValidationError error)
        {
            return error.ErrorMessage;
        }

Any reason we don't have an override for ToString() that does the same thing?

The reason I'm asking is because this:

<p>@e</p>

Is giving me the typename, and not the error message, I still have to do

<p>@e.ErrorMessage</p>

[CumpsD]

Could you please ellaborate on the intended usage for IDictionary<string, IList<ModelValidationError>>

As I see it now, we have a:
Dictionary<ErrorProperty, List<MemberName(s), ErrorMessage>>

What is supposed to go in the key, and what is to go in the MemberName?

I see there is a helper method to Add an error taking key+error message, which creates a ModelValidationError using the ctor taking 1 membername, but there is no helper method to add using multiple memberNames

I assume the idea was to have the key of the dictionary be the single property in error? But what if an error applies to 2 properties, for example the error "Wrong combination of email and password", involves 2 properties. It doesn't seem logical to add 2 entries in the Dictionary, because iterating them will spew out 2 identical errors.

I thought to just add an error to the dictionary with key "InvalidPassword" and have the ModelValidationError then take 2 memberNames. This is how I am doing it now, but it feels very convulted

So, I'd love to see some reasoning behind intended usage please :)

[CumpsD]

I'm not even sure if this overload is still being used somewhere now?

public ModelValidationError(IEnumerable<string> memberNames, string errorMessage)

[thecodejunkie]

@CumpsD the idea is to group the errors per field that they belonged to. Earlier they were just a random set of rules that had fields assigned to them. Do you could have

key: firstname -> value: rules that are broken for firstname
key: lastname -> value: rules that are broken for lastname

This change was made to facilitate the desire we have to soon be able to provide an adapter layer for client-side validation frameworks. The adapters will use the rules provided by Nancy and map them to the corresponding stuff in client-side code (what ever js framework you want to use)

We're definitely not against the idea of making further modifications to the structure. Happy to discuss what this would look like and how it would function from a rule perspective.

[thecodejunkie]

@CumpsD oh and we can definitely overload ToString to do the same as the implicit cast. In fact I would say that's a miss on my part other than a deliberate choice. I try and make both do the same things

[thecodejunkie]

@CumpsD #1426

[CumpsD]

@thecodejunkie Yeah, I thought that was the idea, key: firstname, value: rules that are broken for firstname

How would you deal with a rule that checks firstname and lastname together, and would only throw one error? Add it twice? Then you can't just iterate over the keys anymore to print out an error list, or you end up with duplicate

eg:

Username: Invalid combination username and password
Password: Invalid combination username and password

It feels like it is sending a confusing message, instead of SDHP. ModelValidationError has an array of memberNames, but the key of the validation results dictionary is always about 1 member.

[GiscardBiamby]

@CumpsD, I asked that same question in the first comment of this thread. I haven't looked at it since, but I've been thinking that maybe I'd handle it by using a special value as the dictionary key to indicate that the errors are on the model itself rather than on a single property. Either the name would be the name of the type, or "MODEL" , or something like that.

[CumpsD]

Yes, for now I am doing something similar: https://github.com/CumpsD/CC.TheBench/blob/master/src/CC.TheBench.Frontend.Web/Utilities/Extensions/NancyExtensions/AddValidationErrorExtension.cs

But it's not very clean imho.

I guess there are 2 use cases:

• Have a list of all properties in error (with message)
• Have a unique list of error messages

The first to be used for client side highlighting of input fields in state of error (where you need all of them), and the second used for ValidationSummary-style overviews

[CumpsD]

Don't bother looking at that file anymore, just refactored it :) https://github.com/CumpsD/CC.TheBench/commit/6227e612cdb7bb847f00d2ec20dbe43b3f6ea366

The approach I'm using now is to keep it as is in Nancy and have a helper method to give me unique error messages to use in a ValidationSummary, while the errors will have multiple entries for all UI fields involved in the combined error

[thecodejunkie]

Like I said, I recognize the limitation/problem and am open for suggestion on how we re-design the validation result class to support this. Suggestions? Thanks