Input requested: Business rule subsystem

Input requested: Business rule subsystem

Old forum URL: forums.lhotka.net/forums/t/8481.aspx


RockfordLhotka posted on Friday, February 05, 2010

I'm in the middle of reimplementing the business/validation rules subsystem for CSLA 4, and would value the community's thoughts on the following:

The CSLA business/validation rules subsystem has operated at a method level through its existence. You use AddRule() to link a delegate pointer to a method to a property of your business object.

That has advantages - it is low overhead and it is easy to implement a rule - it is just a method after all.

public static bool MyRule(object target, RuleArgs e)
{
  // return true if success or
  // set e.Description and return false if failure
}

The primary alternative model is to elevate a rule to an object level. So a rule is defined as a method in a class, where that class typically implements an IBusinessRule interface or something like that.

public class MyRule : IBusinessRule
{
  public void Rule(RuleContext e)
  {
    // use e.Results.AddResult() to indicate failure
  }
}

This requires an instance of MyRule be created, so there's a little overhead, and a little more code since the rule isn't just a method - it is also a class.

But the advantage is that there can be a BusinessRule base class that you subclass to create rules. That base class can expose protected members much like ObjectFactory does.

public class MyRule : BusinessRule
{
  public override void Rule(RuleContext e)
  {
    var x = ReadProperty(e.Target, Customer.IdProperty);
    LoadProperty(e.Target, Customer.DataProperty, x + 1);
    // use e.Results.AddResult() to indicate failure
  }
}

This would impact AddRule() of course - because your code would be more like this:

BusinessRules.AddRule(new MyRule(), IdProperty);

I'm strongly leaning toward doing this however, because it radically simplifies the process of passing parameters. Since the rule is your code you can have a meaningful ctor.

public class MaxLength : BusinessRule
{
  public int Max { get; private set; }

  public MaxLength(int max)
  {
    Max = max;
  }

  public override void Rule(RuleContext e)
  {
    if (e.PropertyValues[0].ToString().Length > Max)
      e.Results.AddResult("Value exceeds max length", RuleSeverity.Error);
  }
}

Then the add is like this:

BusinessRules.AddRule(new MaxLength(50), NameProperty);

which is much, much simpler than the current model where you have to create a custom RuleArgs subclass and the business object developer has to magically know to use your custom args type, etc.

One final point - no matter what, the CSLA 4 model will be a breaking change - no existing rule methods will carry forward without at least a little change. So arguing against this idea because it is a breaking change isn't really valid - I'm more looking for thoughts on whether the idea has broad merit over sticking with the current delegate/method-based model.

Thoughts?

JoeFallon1 replied on Friday, February 05, 2010

I like it.

The RuleArgs classes were simple enough but if we can just get rid of them then even better. The idea of a Base Class for rules is interesting too.

Here is one vote in favor.

Joe

 

ajj3085 replied on Friday, February 05, 2010

A few thoughts... just throwing this out there.

Would each instance have its own instance of the rule objects?  Would it be somehow possible to "custom build" rule instances? 

Do you think this would lead to a rule object which does too many things?  Or would you consider it valid for there to essentially be multiple rules.  Say a ContactNameRule, which 1) requires a value, 2) says the value can't be more than 15 characters and 3) forces the value to all upper case.

Will we have lots of problems because people make rule objects which have their own state, and thus one instance of a BO interfers with another?  Along those lines, what about thread saftey?  Especially if the rule instances are shared (one per type) and two BOs on two threads execute the same rule at the same time.

RockfordLhotka replied on Friday, February 05, 2010

Good thoughts Andy.

Each type will have its own instance of the rule object. So you can have a rule type and apply it to different business object types - a different rule instance per business object type.

I'm not sure what you mean by "custom build"?

You could create a rule object that does many things. It is a specific goal to be able to create a "rule" that is actually invoking numerous rules - such as (for example) an external rules engine or workflow. Or all the ValidationRule attributes for a property. That's a goal regardless of whether a rule is an object or a method.

It will be absolutely possible for people to create bad rule objects :)   Rule objects must be designed to be threadsafe - so the Rule() method must be atomic and stateless. I can't help people do that right, but I can't provide ObjectFactory-like support without putting the rule methods in an object instance.

ajj3085 replied on Monday, February 08, 2010

RockfordLhotka
I'm not sure what you mean by "custom build"?

I guess i was thinking of two different things; one would be to generate code and compile a rule class, and then use it.  I assume that answer would be yes.

The other part was that since its an object, you could pass arguments to it, thus giving it some state. Which could be good, or bad.  I can see someone passing a reference to the BO itself, which I imagine would not be pretty.

RockfordLhotka
It will be absolutely possible for people to create bad rule objects :)   Rule objects must be designed to be threadsafe - so the Rule() method must be atomic and stateless. I can't help people do that right, but I can't provide ObjectFactory-like support without putting the rule methods in an object instance.

So I take that to mean that when people create a rule type, there should be no fields whatsoever, static or instance.  It also sounds like the instance RunRule should just be calling static methods to do its work.

That seems like a lot of restrictions to make it work right, and doesn't feel very OO.  I do like the idea, just wonder if this is going to be one of those things that generate a lot of support issues.

bniemyjski replied on Friday, February 05, 2010

Hello,

I'd personally prefer a class that implements IBusinessRule. It's easier to read and enforces code reuse.

Thanks
-Blake Niemyjski

McManus replied on Friday, February 05, 2010

Hi,

I like the idea of a Rule base class, so one vote in favor from me!

Cheers,
Herman

rasupit replied on Friday, February 05, 2010

Rocky, 
My vote for instance rule.  The real question to me is what available on RuleContext.  Looks like you can access property values and add validation result. 

It'll be great if we RuleContext also keep previous value, broken rule result for other property, and also maybe ability to mark a property to also be validated.

Ricky

rxelizondo replied on Friday, February 05, 2010

This is good, but I think you need to up the ante a little bit here.

Unless I am missing something obvious, I see no reason why no to be able to include the PropertyInfo parameter in the rule class too.

So can you please consider something like the following?


---------------------------------------------------------------------------------
// Base class for all CSLA rules.
abstract class BusinessRule<T>
{
// To be used by the CSLA to determined
// what property this rule applies to
internal PropertyInfo<T> ThePropInfo
{
get;
private set;
}

protected BusinessRule(PropertyInfo<T> prop)
{
ThePropInfo = prop;
}

abstract public void Rule(RuleContext e);
}


---------------------------------------------------------------------------------
// Implementation of BusinessRule<T>.
// Note: This is a rule for **string** property
// so generic T is specified as string.
class MaxLength : BusinessRule<string>
{
public int Max
{
get;
private set;
}

// This class is meant to be used by a string
// property and we are able to enforce that
// right on the constructor.
public MaxLength(int max, PropertyInfo<string> theProp)
: base(theProp)
{
Max = max;
}

public override void Rule(RuleContext e)
{
// My rules was design to work with string
// and since I only allow PropertyInfo<string>
// types I can rest assured that I wont get any
// surprised dealing with properties that don’t
// return a string.
}
}


---------------------------------------------------------------------------------
// Usage.
BusinessRules.AddRule(new MaxLength(50, NameProperty));

RockfordLhotka replied on Saturday, February 06, 2010

Binding the entire concept to IPropertyInfo is maybe not a bad idea - though it would pose challenges for property-independent rules. That may be solvable another way though.

The rule itself wouldn't work as you suggest. A rule is independent of any specific object type - or at least can be. Think about these things as being not unlike ValidationAttribute subclasses in the Microsoft DataAnnotations model - but more flexible in their usage perhaps - in that these can be for business processing and not just validation.

In my current (very fluid) thinking there are just a small number of types:

  1. BusinessRule (abstract implementation of IBusinessRule)
    1. DefaultDescription
    2. DefaultSeverity
    3. DefaultStopProcessing
    4. UserState - object
    5. Rule(RuleContext context)
  2. RuleContext (passed to rule on invocation)
    1. InputPropertyValues - Dictionary<IPropertyInfo, object>
    2. PrimaryProperty - IPropertyInfo
    3. Result - List<RuleResult>
    4. RuleDefinition - RuleMethod
    5. Target - object
    6. IsAsync
    7. AddResult(RuleResult result)
    8. Complete()
  3. RuleMethod (links rule to object property)
    1. InputProperties - List<IPropertyInfo>
    2. IsBusy
    3. PrimaryProperty - IPropertyInfo
    4. Priority
    5. Rule - IBusinessRule
    6. Invoke()
    7. BeginInvoke()
  4. RuleResult (contains results that feed into BrokenRules)
    1. Description
    2. OutputPropertyValues - Dictionary<IPropertyInfo, object>
    3. Properties - List<IPropertyInfo>
    4. Severity
    5. StopProcessing
    6. Success - bool
  5. BusinessRuleManager (essentially the existing manager)
    1. AddRule()
    2. AddDependentProperty()
    3. CheckRules()
    4. etc...

BusinessRuleManager will maintain lists of RuleMethod objects - the associations between rule objects and business object properties.

Each association will have its own instance of an IBusinessRule/BusinessRule.

Each invocation of a Rule() method will get a new instance of RuleContext, which will contain copies of the required business object property values. This ensures that the rule can always safely use those values, even in an async context.

The RuleContext does include the Target property - a real reference to the business object. Using that in an async context will be certain doom, but I'm not sure I should coddle people. If they do stupid things like using Target while async, that's their problem. Or should I ensure that is null if the rule is invoked async? Maybe I should do a little coddling?

The RuleResult is pretty rough right now - I'm still working out how to redo BrokenRulesCollection to be somewhat more efficient in terms of its key values The challenge here, is that you now get to make up your own results - plural. So I might just always blank the broken rules for a property and then add in any results values - dropping the unique rulename key requirement entirely.

I also don't yet know how or if the rule:// URI concept will carry through. I'm not so sure it even matters now that DataAnnotations exist and do basically the same job but better.

RockfordLhotka replied on Saturday, February 06, 2010

For example, here's a MaxLength rule using the proposed model:

    public class MaxLength : BusinessRule
    {
      public int Max { get; private set; }

      public MaxLength(int max)
      {
        Max = max;
      }

      protected override void Rule(RuleContext context)
      {
        var value = context.InputPropertyValues[context.PrimaryProperty].ToString();
        if (value.Length > Max)
          context.AddResult(
            string.Format("{0} value too long", context.PrimaryProperty.FriendlyName));
      }
    }

No reflection needed because the property value(s) are provided to the rule. No magic needed to get the friendly name, because it is in the IPropertyInfo object. No custom RuleArgs subclass needed, because the Max value is provided to the rule when the rule is instantiated.

 

sergeyb replied on Saturday, February 06, 2010

The only thing I would say is that it seems we are loosing a bit of compile time checking going from existing AddRule and "staiuc bool SomeRule(T tartet, RuleArgs e)" to the new model.

Thanks
Sergey

RockfordLhotka replied on Saturday, February 06, 2010

True - at the moment the types are not generic, so the Target property is of type object.

I tried making the types generic, but that seemed to be complicating things substantially. The reason is that in the end the only place you care about Target being a specific type is in the rule itself. That implies that IBusinessRule and BusinessRule need to be generic, and that they'd have a constraint on the type of T. Which means the rule types would always be hard-coded to a specific business object type.

That is a high price to pay, I think, for avoiding what comes down to one cast.

JonnyBee replied on Sunday, February 07, 2010

I like the new Rule class - so add my vote for that.

Have you considered changes  to (removing) FriendlyProperyName in PropertyInfo? Seems like it is only going to work as expected in Client scenarios and not in serverside applications. If you have a multi-culture application and have FriendlyName read from a resource file the serverside part would only use the "culture" from the first caller that initialized the static PropertyInfo objects.

RockfordLhotka replied on Sunday, February 07, 2010

I hadn't thought about removing FriendlyName.

DataAnnotations has an attribute for a display/friendly name. Ideally that attribute would be used as the source value for a friendly name - and it supports localization.

So perhaps the answer is to provide a function (maybe in the BusinessRule base class or in PropertyInfo<T>) that gets the display name value from DataAnnotations.

ajj3085 replied on Monday, February 08, 2010

RockfordLhotka
So perhaps the answer is to provide a function (maybe in the BusinessRule base class or in PropertyInfo<T>) that gets the display name value from DataAnnotations.

So moving forward it would be a requirement to use those attributes?

RockfordLhotka replied on Monday, February 08, 2010

Requiring the DataAnnotations display name attribute is one option - which does solve the localization issue.

Another option is to leave the friendly name in PropretyInfo<T>, but to have PropertyInto<T> maintain a reference to the underlying System.Reflection.PropertyInfo, making it easy for your rule to directly reflect against the property to get the attribute if you wanted to go that direction.

But I'm rather leaning toward using only the attribute. I suspect the DataAnnotations model will rapidly become a standard, as it is used already by ADNS, MVC 2 and the SL DataForm. I'd be surprised if WPF doesn't get a DataForm, which would use it too.

In other words, every major interface technology being actively worked on and promoted now has DataAnnotations support - so why wouldn't you use them?

Jav replied on Tuesday, February 09, 2010

This all sounds interesting.  Most of the time we are talking about rules that check the validity of a single object.  I would like to have the ability to easily check the validity of objects in a collection where the collection's validity is determined based on the inter relationship of the state of object(s) in the collection.  For example, if the user answers Yes to a field in one object, the collection becomes invalid until a certain answer is provided to another object in the same collection.

Jav 

am_fusion replied on Sunday, February 07, 2010

how useful is the Target property given that it's usage in an async context will cause problems.... in fact, what would the solution for an async 'mutex' rule look like?

RockfordLhotka replied on Sunday, February 07, 2010

I am somewhat concerned about including the Target property for that reason.

When you say "async mutex" do you mean a solution to make a business object threadsafe? That's essentially impossible. I suppose it could be somewhat possible if you never use data binding or allow any code to hook events from a business object - but that's pretty unrealistic.

am_fusion replied on Monday, February 08, 2010

Normal 0 false false false MicrosoftInternetExplorer4

Mutex = rule that changes the BO.

 

I am thinking:

 

class SomeEntity : BusinessBase

{

   public DateTime SomeDateProperty

   private rangesStruct allowableRangesForSomeValues

   public int SomeValue1Property

   public int SomeValueNProperty

 

 

   public override void AddBusinessRules()

   {

            ValidationRules.AddRule(getAllowableRanges, SomeDateProprty)

       

        validationRules.AddDependentProperty(allowableRangesForSomeValues, SomeValue1Property)

        validationRules.AddDependentProperty(allowableRangesForSomeValues, SomeValueNProperty)

        validationRules.AddRule(checkRange, SomeValue1Property)

            validationRules.AddRule(checkRange, SomeValueNProperty)

   }

 

   private static void getAllowableRanges(object target, RuleArgs args)

   {

      

       rangesStruct newData;

       //async call to get allowableRangesForSomeValues

       //in callback

       {

            newData = e.results;

            mre.set();

       }

       mre.waitOne     

       SetProperty(target, allowableRangesForSomeValues, newData)

 

   }

}

 

I would love to be able to do some kind of Async SetProperty inside of the callback...  I am not so sure I have a full grasp of the

complexity there... but I was thinking something like this:

 

//in call back  base=rule object

using(base.AsyncPropSet(target))

{

    SetProperty(target, allowableRangesForSomeValues, newData)

}

 

where AsyncPropertySet somehow tells the target.SeProperty to use read\write locks or something similar?

I am not so sure I grok the databinding problem, but wouldn't this approach ensure that no downstream property setting\reading logic

gets fired unexpectedly?  The BO itself would not be threadsafe, but the properties would?  Or am I really just missing something.

 

I suppose the argument can be the design itself is poor... better to have some factory of allowablerangesforsomevalues that must have fetched it's data before the rule has a chance to fire?

Andreas replied on Tuesday, February 09, 2010

Hi Rocky,

I like your idea and I vote for the IBusinessRule concept.

I also want to throw the idea in to the discussion to think about "cross business type rules". In complex scenarios when you think about an entire tree of business object relations you sooner or later you need some kind of cross business rules that link depend properties for example between parent and child objects or between objects types at the same child level. Of cause the single business object should not know about its context or that is part of a whole. This should be implemented and managed inside the business rule? Maybe it’s only an extension of the current BusinessRuleManager which already supports dependent rules, but only inside the same object type.

Andreas

RockfordLhotka replied on Tuesday, February 09, 2010

Generally speaking a child shouldn't know about its parent. But a parent can absolutely know about its children - they are part of the parent through the composition relationship.

If a child has to know about its parent, that should be abstracted through an interface if at all possible.

Though in CSLA 4 the Parent properties are all public now, so it is terribly easy for a child to cheat an interact with its parent... But that doesn't mean people have to do it wrong - as long as it is done via interface it is generally acceptible.

The point I'm making is that you can create a rule that operates against an entire object graph - that's fine. It would ideally be attached to the root node of the graph, and if it can't be, then the rule should use an interface to interact with any objects higher in the graph than the object to which the rule is attached.

None of that requires any real work on the part of CSLA - other than that we all must acknowledge that object-spanning rules must always be synchronous - trying to lock one object is nearly impossible, and trying to lock an entire object graph...ouch!

Andreas replied on Tuesday, February 09, 2010

That's sound all valid and reasonable but I think it’s not easy to manage and from my point of view definitely not easy to implement. In our object model we make extensive use of lazy loading child objects. The instantiation of child objects depends on user interactions. So the root or parent in the tree doesn’t know if the child object is already instantiated and can be validated against its own properties. Maybe it can check this withFieldManager.FieldExists(). But it currently can do this check only with its own and direct child properties. On the other hand the child object shouldn’t know about its parent. So what if you want to reuse this child object type in other complete different object hierarchies. You better don’t want to dependent on parent object types or interfaces only because you want check if the value of an integer property is in the range of certain values of some other object property in unknown leaf somewhere in the object tree. The only thing you probably know is that it may already exits somewhere in the tree. But as soon as it exists you want to use it in your validation logic. In these scenarios I was thinking about some kind of “BusinessRule Property Registry” that is part of the csla’s BusinessRule subsystem where properties can registered to become validated when the owning business objects is instantiated. Again the current semantic of "AddDependentProperty" smells to be close to this but it should work on cross business object types or better it should work business object type agnostic.

Andreas

am_fusion replied on Wednesday, February 10, 2010

I still can't get over the thought that there should be a way to allow async access to properties (for a single object in the graph... not the whole object graph) but I have a clearer head now (I was all hopped up on NyQuil for my previous posts... now I am just hopped up on DayQuil) and I think I better understand the challenge.

That being said, I would vote for either having the Target property (for the RuleContext) be null or throw a "no async access to target" exception when in an async context.  Throwing an exception would better inform the developer why target is not accessible.

Keeping the target property will allow for business rules that modify the object (which I believe is a valid use case)... the danger of course is if that rule somehow fires in an async manner.  Out of curiosity, how would we specify that a rule should run async?

RockfordLhotka replied on Wednesday, February 10, 2010

am_fusion

Out of curiosity, how would we specify that a rule should run async?

That remains an open question Smile I'm toying with a couple ideas.

One is to have BusinessRules.CheckRules() and BusinessRules.BeginCheckRules(callback) so it is an explicit choice to run rules on a background thread. But I'm not sure this is workable, since there are implementation differences in sync vs async rules. An async rule can always (?) be run synchronously, but a sync rule must be run synchronously. I've started down this road, but I'm increasingly convinced it is wrong.

Another is to have a property in IBusinessRule such as ExecutionMode, which could be ExecutionModes.Any, ExecutionModes.SyncOnly, ExecutionModes.AsyncOnly. Then BusinessRules.CheckRules() would execute each rule based on that property value. Obviously I haven't thought this through entirely, but I rather suspect this is the correct solution.

RockfordLhotka replied on Wednesday, February 10, 2010

am_fusion

That being said, I would vote for either having the Target property (for the RuleContext) be null or throw a "no async access to target" exception when in an async context.  Throwing an exception would better inform the developer why target is not accessible.

Yes, I think this is a good solution - having Target throw an exception if accessed while async.

Copyright (c) Marimer LLC