So, I think I have discovered a bug in CSLA 3.8.2 (and it looks like the issue still exists in 3.8.3)
Basically, if we have a long running asyc rule and the paramaterless CheckRules() is executed (I.E. from DataPoral.Create<t>), the long running async rule will be executed multiple times (see bugfix comment below).
Also, I believe the meaning of 'validation complete' is different depending on who is invoking it (see 'regarding the OnValidationComplete method\event' comments below)
For now I am implementing these changes in our version of CSLA. Is this a good fix? Will this be addressed in a new release of 3.8.x?
/// <summary>
/// Invokes all rule methods for all properties
/// in the object.
/// </summary>
public void CheckRules()
{
if (_suppressRuleChecking)
return;
ValidationRulesManager rules = RulesToCheck;
if (rules != null)
{
foreach (KeyValuePair<string, RulesList> de in rules.RulesDictionary)
CheckRules(de.Value.GetList(true));
}
}
/// <summary>
/// Given a list
/// containing IRuleMethod objects, this
/// method executes all those rule methods.
/// </summary>
private void CheckRules(List<IRuleMethod> list)
{
bool previousRuleBroken = false;
bool shortCircuited = false;
//bugfix: added asyncRulesToInvoke so that we don't invoke all ValidatingRules.
// we don't want to invoke all of them because when we are called
// from CheckRules (checking rules for all properties), some async validation
// rules may still be in ValidatingRules by the time we are checking the next
// property's rules.
//
// So, when we invoke the rules below, if we invoke all the rules in
// ValidatingRules, we will invoke the privious properties' rules for each
// rule set in the RulesDictionary, until they complete and are remove from the
// ValidatingRules).
var asyncRulesToInvoke = new ObservableCollection<IAsyncRuleMethod>();
// Lock the rules here to ensure that all rules are run before allowing
// async rules to notify that they have completed.
for (int index = 0; index < list.Count; index++)
{
IRuleMethod rule = list[index];
// see if short-circuiting should kick in
if (!shortCircuited && (previousRuleBroken && rule.Priority > _processThroughPriority))
shortCircuited = true;
if (shortCircuited)
{
// we're short-circuited, so just remove
// all remaining broken rule entries
lock (SyncRoot)
BrokenRulesList.Remove(rule);
}
else
{
// we're not short-circuited, so check rule
bool ruleResult;
IAsyncRuleMethod asyncRule = rule as IAsyncRuleMethod;
if (asyncRule != null)
{
lock (SyncRoot)
ValidatingRules.Add(asyncRule);
asyncRulesToInvoke.Add(asyncRule);
lock (SyncRoot)
BrokenRulesList.Remove(rule);
}
else
{
try
{
ruleResult = rule.Invoke(_target);
}
catch (Exception ex)
{
//// force a broken rule
//ruleResult = false;
//rule.RuleArgs.Severity = RuleSeverity.Error;
//rule.RuleArgs.Description =
// string.Format(Properties.Resources.ValidationRuleException & "{{2}}", rule.RuleArgs.PropertyName, rule.RuleName, ex.Message);
// throw a more detailed exception
throw new ValidationException(
string.Format(Properties.Resources.ValidationRulesException, rule.RuleArgs.PropertyName, rule.RuleName), ex);
}
lock (SyncRoot)
{
if (ruleResult)
{
// the rule is not broken
BrokenRulesList.Remove(rule);
}
else
{
// the rule is broken
BrokenRulesList.Add(rule);
if (rule.RuleArgs.Severity == RuleSeverity.Error)
previousRuleBroken = true;
}
}
if (rule.RuleArgs.StopProcessing)
{
shortCircuited = true;
// reset the value for next time
rule.RuleArgs.StopProcessing = false;
}
}
}
}
//using asyncRulesToInvoke instead:
//IAsyncRuleMethod[] asyncRules = null;
//lock (SyncRoot)
// asyncRules = ValidatingRules.ToArray();
//...any reason to ToArray() asyncRulesToInvoke?
// They must all be added to the ValidatingRules list before you can invoke them.
// Otherwise you have a race condition, where if a rule completes before the next one is invoked
// then you may have the ValidationComplete event fire multiple times invalidly.
foreach(IAsyncRuleMethod rule in asyncRulesToInvoke)
{
try
{
rule.Invoke(_target, asyncRule_Complete);
}
catch (Exception ex)
{
// throw a more detailed exception
throw new ValidationException(
string.Format(Properties.Resources.ValidationRulesException, rule.RuleArgs.PropertyName, rule.RuleName), ex);
}
}
//regarding the OnValidationComplete method\event. It appears to have two meanings.
//when the CheckRules(List<IRuleMethod> list) method calls target.OnValidationComplete
//it means 'the rule set passed in is done being validated.'
//when target.OnValidationCopmlete is called from
//target.ValidatingRules_CollectionChanged it means 'all async
//rules have completed'.
//so we should have the following scenerio with the current code
//TEST: When we have no asyncRules (or an extremly fast one?) and
// CheckRules() is executed, we should get OnValidationComplete invoked
//for each property that has a rule attached (invoked from the code below)
//TEST: When we have a reasonably long running async rule and CheckRules() is fired
//we should get only one OnValidationComplete invoked (from the last
//ValidatingRules_CollectionChanged)
//...is this desirable? In my mind we should have it one way or the other but not both.
//this used to check the length of the ValidatingRules Array (I.E. 'all invoked async Rules')
//now it is using asyncRulesToInvoke (I.E. 'all invoked async rules in the current rule list'),
//I believe both to be incorrect based on the above code comment.
if(asyncRulesToInvoke.Count == 0)
{
var target = _target as Csla.Core.BusinessBase;
if(target != null)
target.OnValidationComplete();
}
}
This will not be addressed in a future version of 3.8.x. It is a known side-effect of async rules that they can be executed multiple times with overlapping results. Generally my recommendation is to use the UI to block the user from interacting with the object in ways that would allow such overlapping behaviors.
At this point version 3.8.3 is on track to be finalized this week, and other than minor bug fixes the 3.8 branch will not be changing substantially in the future. All development focus is now on CSLA 4.
So the good news is that your changes to the framework code won't be affected by numerous future releases, because hopefully there won't be any 
cool, no sweat. I will make the changes outlined above to our version of 3.8.2 and re-implement in 3.8.3 when we upgrade. Do you see any problems with my implementation? (I.E. only invoke the async rules for the current 'rule set' being checked instead of invoking all the ValidatingRules over and over again for each 'rule set' being passed in by the parameterless CheckRules()?)
To Be Clear: I don't think that the UI developer has any control over the problem. This problem occurs when ever the parameterless CheckRules is executed. This happens when DataPortal BeginCreate is invoked so the UI developer has little control over preventing the async rules from firing multiple times.
Also, What do you think of the 'OnValidateComplete' question? (when exectuded from the BusinessBase.ValidatingRules_CollectionChanged callback, it means 'all async rules completed'. When executed from the CheckRules(List<IRuleMethod> list) overload it means 'all rules for the 'list' being validated have comleted).
I haven't checked 4.0 to see if it has a similar problem (I will do so 'soon'). Given that the entire subsystem has been re-vamped, chances are this goes away in 4.0.
Is the problem that you've attached the rule to multiple properties, so it runs for each property? That's actually by design - it is entirely legal to attach a rule to multiple properties, and in that case the expectation is that it will run once for each property.
No, this is not the issue (I actually thought the same thing when the developer came to me with this problem).
The problem is when CheckRules() is fired we roll through each property that has a list of rules attached and invoke those rules by executing the CheckRules(List<IRuleMethod> list) overload:
public void CheckRules()
{
if (_suppressRuleChecking)
return;
ValidationRulesManager rules = RulesToCheck;
if (rules != null)
{
foreach (KeyValuePair<string, RulesList> de in rules.RulesDictionary)
CheckRules(de.Value.GetList(true));
}
}
Inside of the CheckRules(List<IRuleMethod> list) overload we add to the validating rules for each rule that implements IAsyncRuleMethod:
IAsyncRuleMethod asyncRule = rule as IAsyncRuleMethod;
for (int index = 0; index < list.Count; index++)
{
//irrelevant code not shown
if (asyncRule != null)
{
lock (SyncRoot)
ValidatingRules.Add(asyncRule);
lock (SyncRoot)
BrokenRulesList.Remove(rule);
}
//irrelevant code not shown
}
after we have rolled through all the rules we invoke all the ValidatingRules:
IAsyncRuleMethod[] asyncRules = null;
lock (SyncRoot)
asyncRules = ValidatingRules.ToArray();
// They must all be added to the ValidatingRules list before you can invoke them.
// Otherwise you have a race condition, where if a rule completes before the next one is invoked
// then you may have the ValidationComplete event fire multiple times invalidly.
foreach(IAsyncRuleMethod rule in asyncRules)
{
try
{
rule.Invoke(_target, asyncRule_Complete);
}
catch (Exception ex)
{
// throw a more detailed exception
throw new ValidationException(
string.Format(Properties.Resources.ValidationRulesException, rule.RuleArgs.PropertyName, rule.RuleName), ex);
}
}
this is where the problem is. When we are only validating rules through the CheckRules(string propertyName) overload, we are ok since the only async rules in ValidatingRules are the rules we have found in the for loop above.
However, when we are executing this through the CheckRules() overload, we may still have ValidatingRules from the previous execution of CheckRules(de.value.GetList(true)) (EDIT: because the async rule is a 'long running' rule) in the foreach (KeyValuePair<string, RulesList> de in rules.RulesDictionary) loop. These rules were already invoked (from that previous execution) and are now invoked again (unless they have completed and are removed from the ValidatingRules list).
Am I making sense?
Hmm, this sounds more like a bug and less like any sort of expected behavior. I'll add it to the bug list.
Regarding the ValidationComplete event - the intent is that this event is raised at the point where validation for a property ends, and there are no outstanding async rules running. A consumer should be able to handle this event, and know that when it is raised it is because the rules are done running. It isn't necessarily linked to a specific CheckRules() invocation - but rather to the fact that we've hit a point where we know there were rules running, and now there aren't.
The behavior of ValidationComplete changes a little in CSLA 4, but this is the philosophy on which the event is implemented. The primary goal is to allow a UI (or viewmodel) author to handle the event so they can do processing knowing that 1 or more rules have run, and that there are no rules currently running when the event is being handled.
Cool, I will implement this fix in our 3.8.2.x.
Regarding ValidationCompete:
This makes sense. The only time we don't know when validation rules are complete are when at least one Async rule is running.
So given the philosophy:
allow an observer to handle the event such that they can do processing knowing that 1 or more rules have run, and that there are no rules currently running when the event is being handled.
and given:
when using CheckRules() we may end up with multiple 'batches' of rules processing, each taking upon itself to decide that all async rules have completed.
would it be safe to say I can modify the 3.8.2.x base such that CheckRules() can ask the CheckRules(List<IRuleMethod> list) not to bother with the async completed check:
private void CheckRules(List<IRuleMethod> list, bool checkAsyncCompleted)
so that CheckRules() can do it on his own?
Edit:
Oh yeah, and regarding:
"At this point version 3.8.3 is on track to be finalized this week, and other than minor bug fixes the 3.8 branch will not be changing substantially in the future. All development focus is now on CSLA 4.
So the good news is that your changes to the framework code won't be affected by numerous future releases, because hopefully there won't be any :-)"
It does kind of feel good to have the ability of being able to modify the code base without fear of being impacted by future releases. However, I will trade that in a heartbeat for the new rules subsystem. 
You want to raise ValidationComplete while there is an outstanding async rule still running? I guess that is up to you, but it is not the intended purpose of the event. Obviously validation is not complete, because there's still a rule running.
Sorry, the point being that while CheckRules() (paramaterless overload) is running (I.E. calling CheckRules(List<IRuleMethod> list) multiple times) I consider 'rules to still be running'
Since the CheckRules(List<IRuleMethod> list) is raising validation complete and I see CheckRules() as not completing ('rules still running') until it exits; I take issue with CheckRules(List<IRuleMethod> list)'s behavior.
My thought is to allow CheckRules() (paramaterless overload) to ask CheckRules(List<IRuleMethod> list) overload to not check for async complete so that CheckRules() (paramaterless overload) can instead.
If I remember correctly there is something I would need to do with the CheckRules(string propertyName) overload as well. I would have to make it such that when the 'propertyName rule set' and the 'dependent properties rule set' are processed we would only get one ValidationCompleted Event.
I suppose if ValidationCompleted were to have the propertyName along with it, I could have CheckRules() raise that with an empty propertyName and CheckRules(List<IRuleMethod> list) raise it with the 'property being validated' that may satisfy my requirement.
EDIT: following Microsofts lead with INotifyPropertyChanged. For instance ValidationComplete with:
EDIT: (my apologize if this is rambling, I wanted to change my thoughts on this before I go to bed and it is kind of late)
hmmm... I see where we don't meet eye to eye. I view rules to 'still be running' while CheckRules() is running. You do not. I could easily accept the alternative view but I guess I have a few concerns regarding the current behavior:
the first is:
if our public facing CheckRules don't translate one to one with ValidationComplete (or at least there is no way of knowing when '*all* rules have been run') we are forcing our UI to react on every single event instead of just one (after all, isn't it a more common scenrio to want all the rules run before acting?)...
when our CheckRules() (paramaterless overload) is being executed we could be firing ValidationComplete a bunch of times if we have a lot of properties with rules (and some properties have dependent properties).
the second is :
I get a 'bad feeling' about the fact that it is in-determinant how many validation completes I will get per execution of CherckRules... but that is just a 'bad feeling', I have no basis I can articulate at this time for it...
I also have no way of knowing which properties are going to fire first... so if I wanted to front load my async validations (assuming I have some properties that don't have async validation) such that I get as few ValidationCompleted events as possible, I can't do that either.
If the purpose of the ValidationCompleted event is truly to know that 1 or more *async* rules have run and that there are no *async* rules currently running, then we should only have raised the ValidationCompleted from CheckRules(List<IRuleMethod> list) if we actually did invoke rules in this 'go round' of the call to the method.
CSLA 4 raises ValidationComplete one time per CheckRules() call, when all rules complete (sync and async) - assuming non-overlapping CheckRules() calls. If you have overlapping CheckRules() calls and async rules you could get less than one ValidationComplete per CheckRules() call.
I do not plan to change the 3.8 behavior. There are apps that rely on the existing behavior (I know, because just a few months ago I altered the behavior to meet their need to get ValidationComplete for sync as well as async rule completion). If ValidationComplete is raised too often, it at least follows the specific definition I provided earlier in the thread - which is that a batch of rules is complete, and there are no currently executing rules (sync or async) when the event is raised.
groovy. Thanks for bearing with me through that 
I will think about this a little more and figure out what we want our behavior to be in our 3.8.3.x version. I suspect we will move in the direction 4.0 is moving (one checkRules, one validationComplete).
Copyright (c) Marimer LLC