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.
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.
![Smile [:)]](/emoticons/emotion-1.gif)
They are a bit of a punished good deed :) The RegEx rule
got a lot of attention for a while – it turns out that there is no
universally accepted expression for email validation, and I should have never put
any expressions in by default. That was perhaps the biggest mistake. But if you
look back through the forum history you’ll find a lot of comments and
wishes for differences in the way the rules work.
One trick with description that would have to be sorted out is
localization. The existing rule descriptions are localized into numerous
languages, and so the default behavior would have to be to use the existing
descriptions from the resource file.
Also, they are dynamic strings, and so any description text
would be constrained by the format used currently so the string built
correctly. Or they’d have to use entirely static description text
provided by you as input.
Rocky
I was tring the exactly the same thing
ValidationRules.AddRule(CommonRules.IntegerMinValue, new CommonRules.IntegerMinValueRuleArgs(ContactIdProperty, 1) { Description = "Choose a Contact Type." });
In that instance the Validation Rule is correct, since I checked in Debug.
But when I make a .Save() in the UI, the ValidationRule is of type "mobile...." and has the default Description ... This is not very intuitive.
This is because, for performance reasons, there's only one RuleArgs object created per rule, and it is reused constantly - obviously thereby wiping out previous values.
Remember that AddRule() is called once per type - so that RuleArgs object you are creating is static and global in scope.
To address this, CSLA 4 is going to be quite different. AddRule() will take values that are used as defaults, and each rule invocation will get a RuleContext object that is unique to that invocation.
There's a whole other thread about the design of the CSLA 4 business rules subsystem, and I won't rehash that here other than to say that the behavior you are observing has been there since 2004, and is about to undergo some big changes.
Copyright (c) Marimer LLC