Dear Frans,

Before I ask your opinion on this matter, I would first like to make myself clear of what I am doing. When I hook my validator classes using DI and handle ValidateFieldValue(), I'll always return true (next to SetEntityFieldError(...)), even when the value is invalid! This is because I would like to see invalid values in the UI, along with an error icon (that is set using SetEntityFieldError(...)); thus the user sees an error icon making him aware that the data is invalid. If I would return true for ValidateFieldValue(), the value is not updated, and the user will see the original value, which I don't want (I don't think restoring an invalid value is good user experience).

The problem with this approach is that the error info doesn't seem to persist to the UI. After expecting you code (using reflector), I've noticed that you reset the error info if the return value of ValidateFieldValue() equals true. For me, this is true for every Validator! Am I misusing the Validator?

Regards,

Michel van den Berg

PS: I wanted to ask you this at codecamp today, but I thought it would be better addressed here, as others might have the same problem.

Could you please post your ValidateFieldValue code?

IMHO, you should return false and set an error if the value is incorrect. This way the user is aware of its invalid value and s/he is able to go back and fix it:

public override bool ValidateFieldValue(IEntityCore involvedEntity, int fieldIndex, object value)
{

    // value to return
    bool fieldIsValid = true;

    if (value != null)
    {
        #region Inspect fieldIndex to determinate if they are in a correct format
        switch ((CustomerFieldIndex)fieldIndex)
        {
            #region Phone
            case CustomerFieldIndex.Phone:

                /// check if phone comes in correct format ofr US-Customers
                if (((CustomerEntity) involvedEntity).Country == "USA" &&
                    !RegexHelper.IsValidUSPhoneNumber((string)value))
                {
                    // set error info, so we could check that outside
                    involvedEntity.SetEntityFieldError(CustomerFieldIndex.Phone.ToString(), PHONE_INCORRECT_FORMAT_ERROR_MESSAGE, false);
                    fieldIsValid = false;
                }
                else
                {
                    // everything seems to be OK with this field so we clean the error info.
                    involvedEntity.SetEntityFieldError(CustomerFieldIndex.Phone.ToString(), "", false);
                }
                break;

                ...
            #endregion

Have you checked the LLBLGen Validation Example (http://llblgen.com/pages/examples.aspx) ? Please look at SelfServicing + WinForms Example (especially frmCustomers and frmOrders).

My code would be the same as yours, however I always return true. This is duo to the fact that when I return false, the value is not set and the UI reflects this! In combination with the error info, the UI now shows an error icon for a restored, valid value! My only options are:

1. return false, without setting error info
2. return true, setting error info

Furthermore, LLBLGen's validation framework implements some sort of hard-constraint for validation, as you can never make an entity invalid. I think what I'd like, is the concept of a more soft-constraint validation, whereas my entities can be invalid, however not saveable if this is the case.

Do note that LLBLGen does have support for this more soft-constaint validation, achievable by always returning true and setting error info (by checking this error info in save validation, you can restrain it from saving). However, as described in the previous post, this is only half true, as you're enforcing the hard-constraint again by clearing the error info.

For my validation code I don't mind clearing the error info myself! Probally by setting the error info using a similair construct as: (isValid ? "" : "an error"). In my opinion, this would allow hard and soft-constraints within LLBLGen. To not break the code, you could opt to add a setting to enable the clearing of the error info (for example, couldn't the SetValue() code check Validator.ClearErrorInfo() ?).

I do know I might be introducing UI code in the BL with the use of IDataErrorInfo, but I find the use of the validator for this really pragmatic!

I do know I might be introducing UI code in the BL with the use of IDataErrorInfo

True.

This has been discussed here: http://www.llblgen.com/TinyForum/Messages.aspx?ThreadID=12654 http://www.llblgen.com/TinyForum/Messages.aspx?ThreadID=13712 http://www.llblgen.com/TinyForum/Messages.aspx?ThreadID=12938

This has been discussed here:

But with no "good" solutions.

arschr wrote:

This has been discussed here:

But with no "good" solutions.

There are no 'good' solutions. IDataErrorInfo is a stupid interface, we can't change that. You have total freedom over what it will provide, though we can't change the nature of the interface.

Another thing, which isn't solvable is that with databinding, a value is pushed from the control into a property, though the bound object can only accept or deny that value. If the value is invalid, there's no such thing as 'accept it but it's invalid'. This is because the bound object isn't part of the control / ui! it's a separate object. So having invalid values inside entities isn't possible, UNLESS you do validation AFTERWARDS. (so accept a value always and then when saving do validation).

But then a problem arises:... when will you do validation? What will you do when some code reads such an 'accepted but invalid' value from said entity?

When a value is accepted by a bound property, the value is considered 'valid' for databinding, that's how databinding works. If some people want to work differently, no problem, but we can't change how databinding works... Furthermore, if a value is accepted inside an entity, the value is also considered 'valid' for the entity. This isn't something silly, it's essential: if values are accepted by entities which are still illegal/invalid, when is the entity considered 'valid' in that situation? What if some code READS teh values from that entity again? etc.

To me the entity should/could maintain a third set of field values (it already maintains the value from the database and the current value), ui value.

This would originally be the same as original value, data binding would be to this set of values. Upon successful validation, the ui value would be moved into the current value.

You may say this is not the data layer's responsibility. However many use the entities in the ui layer.

Otis wrote:

There are no 'good' solutions. IDataErrorInfo is a stupid interface, we can't change that. You have total freedom over what it will provide, though we can't change the nature of the interface.

I do agree that IDataErrorInfo is a stupid interface, and that you can't change it. However, I must say that we don't have total freedom of what to do with it, as you are clearing the error info for us. I would recommend to remove the clearing from SetValue(), however there might be customers whom rely on you clearing the error info. Their validation code would look like:

if (!isValidLastName){
involvedEntity.SetEntityFieldError(CustomerFields.LastName.Name, "Name is not valid"), False);
}

They return isValid and the validation framework "restores" or "updates" the value occordingly. Ofcourse, I do not have any objection in doing this: I think the validation framework design is pretty solid here.

However, if we would like to always return true (because we didn't implement ValidateFieldValue() or because we would like entities to allow invalid values (I do reckon this is not as you've designed it, but still)), we don't have the option to set the error info, as you always clear it!

If you removed the clearing of the error info, the code I would write is:

involvedEntity.SetEntityFieldError(CustomerFields.LastName.Name, (isValidLastName ? "" : "Name is not valid"), False);

I could opt to return true or false, depending if I want to allow invalid values (and perhaps misusing the validator classes). By removing the clearing of the error info, the error info preserves, even if I opt to return true.

As the consequences behind returning true or false is different if you'd remove the clearing of the error info, some customers whom rely on you clearing the error info might get unexpected behaviour (duh). Therefore, I opt for a setting, which has a clearing error info behaviour by default. Perhaps you could add this setting to the validator classes, and do a check on it in SetValue(). I don't see any reasons why you wouldn't do this, besides the reason that it isn't designed to be used this way. But you've once decided to implement IDataErrorInfo, and therefore allowing us to set it within Validator classes. Perhaps not the best design choice, but it is a good pragmatic choice IMO (it just damn easy the hook it into the UI). I just don't like you "dictating" the use of IDataErrorInfo.

I like the way it's now though you have your own reasons . Here is a workaround if you want to take it:

// __LLBLGENPRO_USER_CODE_REGION_START CustomEntityCode 

// overriding the default method ... place this at your Entity USER_CODE_REGION or in a partial class
public override void SetEntityFieldError(string fieldName, string errorMessage, bool append)
{
    // reject the LLBLGenPro EntityFieldError cleaning
    if (!string.IsNullOrEmpty(errorMessage))
    {
        base.SetEntityFieldError(fieldName, errorMessage, append);
    }
}

// this is for my own cleaning...
public void CleanEntityFieldError(string fieldName)
{
    base.SetEntityFieldError(fieldName, string.Empty, false);

}
// __LLBLGENPRO_USER_CODE_REGION_END

the clearing of the errorinfo is there fore convenience: so you don't have to keep track of which errors to clear: that's done for you when a valid value is set.

if you want to change this behavior, and also move towards a situation where the entity can contain invalid values (i seriously don't recommend that, think about FK values which are wrong etc.) I think the simple workaround posted by David will do. A setting for this would switch off logic which would be useful for almost everyone.

I think I'll be implementing the workaround posted by David then. Thanks for the discussion!