CSLA .NET

I had a quick scan around and couldn't find anywhere else to report bugs so hopefully doing it won't offend anyone (and hopefully it is a bug and I haven't missed something obvious).

I am trying to provide feedback to users about why rules are broken (why it's wrong, why they should fix it, what they should do to fix it, ...) and believe I have discovered a bug in the common rules implementations. 

The code for a rule such as CommonRules.StringRequired is:

public static bool StringRequired(object target, RuleArgs e)
    {
      string value = (string)Utilities.CallByName(
        target, e.PropertyName, CallType.Get);
      if (string.IsNullOrEmpty(value))
      {
        e.Description = string.Format(Resources.StringRequiredRule, RuleArgs.GetPropertyName(e));
        return false;
      }
      return true;
    }

The highlighted code provides very helpful default behaviour such as "PropertyXYZ required". 

Problem is that I wanted to pass in my own custom e.Description:

            Dim args As New Csla.Validation.RuleArgs("PropertyXYZ")
            args.Description = "Property XYZ required for ..."
            ValidationRules.AddRule(AddressOf Csla.Validation.CommonRules.StringRequired, args)

It's overwritten by the 'helpful' default behaviour.

I believe the following change in the all of the common rules should help. 

    public static bool StringRequired(object target, RuleArgs e)
    {
      string value = (string)Utilities.CallByName(
        target, e.PropertyName, CallType.Get);
      if (string.IsNullOrEmpty(value))
      {
        // Provide a default Description if none was provided in the RuleArgs e
        if (e.Description == string.Empty)
        {
            e.Description = string.Format(Resources.StringRequiredRule, RuleArgs.GetPropertyName(e));
        }
        return false;
      }
      return true;
    }

 

I understand your point, but I don't think it was intended to work this way.

If you look at the description of RuleArgs.Description, it says "Set by the rule handler method to describe the broken rule."

So, I think the intent is that the Description is provided during the processing of the rule handler and not before.

If you want it to work the way you are describing, then you can implement your own rule handler (which I see you have more or less done above)

 

Thanks.  Looks like you may be correct.  That smells a little to me though.  With a class called RuleArgs you would expect it to be passing arguments to the rule.  This is what all its other other properties are used for. 

I still believe the original intent may have been to allow people to pass in their description.You wouldn't expect to pass in a max length of 50 and then have the rule handler overwrite it with a default value of 10!

Having said that, my implementation doesn't quite work properly.  The ErrorProvider fails to flash with the BrokenRule description. 

I've fixed it by replacing all the "e.Description == string.Empty" with "string.IsNullOrEmpty(e.Description). 

The class was called "RuleArgs" - for passing arguments to rules?  That's what I am doing. 

I view the current behavior as intentional. You may disagree with my common rule implementations, and that's OK, because you can replace them with your own library of rules. That's really the whole point of the rule system - to allow you to do what you need.

The common rule implementations provided by CSLA could never satisfy everyone, so I made them relatively simple, and powerful enough that they satisfy a lot of people. For everyone else, it is pretty darn easy to create your own library of rules that do whatever you require.

---

Rocky

Well the rebuttals don't come much more authoratative than that.  Perhaps I should have titled my post "Suggested enhancement to CommonRules..." rather than "Bug in...".  They don't always get any replies though. :)  

I'm wondering from your reply if you are viewing the CommonRules as a good deed that has not gone unpunished?

It still seems to me that with a minor change, the common rules implementation would satisfy even more cases.  In many cases Description, just like the other properties of RuleArgs, is specific to the business context within which the rule is used.  I don't view my requirement as esoteric in any way and can't see why it wouldn't be of potential use to any consumer of CommonRules.  To re-implement a rule that already works is extra code, even if I am just copy and pasting it.

I am quite happy to go on using my own slightly different implementation of CommonRules but thought the community might benefit from the changes. 

Your stance is fair enough though.  It was a little inflamatory of me to call it a bug and I understand that you have to draw the line somewhere with the CommonRules implementation. 

Martin.

The behavior of using RuleArgs to pass data OUT of the rule method is consistent with the naming of classes in the .net framework too.  Consider CancelEventArgs.    The event handler ("rule method") passes data back to the object which raised the event via the CancelEventArgs.Cancel property.  So this behavior does has precident.

Just wanted to throw that out.. maybe it clears some of the "smell" away. 

---

Rocky