Default to per-type rules??

RockfordLhotka posted on Monday, August 07, 2006

I am considering something and would like input.

Verison 2.1 will have per-type rules - added by calling AddSharedRule() (instead of AddRule()) from an AddSharedBusinessRules() method (rather than AddBusinessRules()). These methods have the same signature, they are just invoked and used by the framework a bit differently.

Now what I COULD do, is make AddBusinessRules() be called on a per-type basis. And AddRule() could add per-type rules. Then I could implement AddInstanceBusinessRules() where you'd call AddInstanceRule() to get today's behavior.

Obviously this could be a breaking change.

But I bet that for the vast majority of people it would NOT be a breaking change - they would just see a performance increase without changing their code.

The only people who'd see a breaking difference are those who really are using per-instance rules, where the rules for one instance of an object are different from another.

Though this does fly in the face of my goal of avoiding breaking changes, it also means that the "default" AddBusinessRules() and AddRules() concepts would do the "right" thing by using the per-type functionality. Only if you have the rare scenario where you need per-instance rules would you have to use AddInstanceBusinessRules() and call AddInstanceRule().

(the same concept would apply to authorization rules btw)

Thoughts?

xal replied on Monday, August 07, 2006

A lot of people create rule methods as private to their bo, and in that case they are not shared (or static).
Making that change will break a lot of code. You know I strongly belive in per type rules, but it's good that you ask for oppinions.
It is probably the right way to go in 99% of the cases, and now that we have them, I think instance rules should be avoided in any circumstance, but I'm not sure how the public will take that change, and I'm sure a lot of people will hold off upgrading because of time constraints.

The necessary changes in the BOs are trivial though, so I guess it's just a matter of figuring out how willing you are to break code (even though it's just a little).


Andrés

ajj3085 replied on Monday, August 07, 2006

I think this is one of those cases where risking breaking existing code is the right way to go.  And better to do it now than later, when even more people may be relying on the existing behavior.

RockfordLhotka replied on Monday, August 07, 2006

Hmm, 1 for, 1 against...

The thing is, Andrés point is very valid. It would break some code. And at the same time, a couple quick, project-wide, mass-subtitutions of AddBusinessRules() to AddInstanceBusinessRules() and AddRule() to AddInstanceRule(), would bring any existing code into the new model with very minimal pain.

I remain somewhat torn...

ktweedy replied on Monday, August 07, 2006

I guess I am curious why the rules would change per instance.  Seems if the object is really a different object then an new object type should be derived or a polymorphic object created to support the reason it is different.

 

SonOfPirate replied on Monday, August 07, 2006

Rocky,

 

Perhaps a little clarification what you are trying to accomplish with this change would help garner more opinions.  Are you simply trying to establish validation as a shared (per type) feature or looking to expand the existing framework with something new?

For what is it worth, in my implementation of CSLA, our business rules ARE implemented as static/shared methods. It was our opinion that validation rules are object/type-specific and not instance specific and it would serve no purpose having a ValidationRules collection for each and every instance of an object.  So, to save on some memory and to maintain a logical, behavior-based approach, we have all of these methods marked static/shared.

What I am having a hard time grasping is what would be considered an 'instance' rule?  If the rule was different from one instance to another wouldn't we really be dealing with separate objects?  Especially if we are following a behavior-based model?  Maybe an example of an 'instance' rule would help cuz all I can think of is a phrase like "if I am this, then validate this way; otherwise I must be this other thing, so validate this other way."  Sounds like two different objects to me.

So, aside from a more tanglible example of an 'instance' rule, I would firmly agree that making methods such as the various validation methods (and authorization methods) static/shared is a logically sound thing.  I believe they are already a per-type device.

As for breaking code, I think it is always acceptable to break code if you are enforcing an improvement in design, approach, technology, etc. (as opposed to a feature change or different way of doing something) - fixing bad practices, for example.  I think this is a case of a positive change that breaking code would force users to make the necessary improvements to their code for the better.

Just my two cents.

xal replied on Monday, August 07, 2006

SonOfPirate:

The difference for instance rules is that in your AddBusinessRules you could (i'm not saying this is a good approach, just stating the problem) do something like this:

Protected Sub AddBusinessRules()
    If mSomeValue = 1 Then
       ValidationRules.AddRule (addressof x, ... )
    Else
       ValidationRules.AddRule (addressof y, ... )
    End If
    ValidationRules.AddRule (...)
    ValidationRules.AddRule (...)
End Sub

In those situations you'd need to rework the way your rules work, provided that you want to change to type rules.

Andrés

RockfordLhotka replied on Monday, August 07, 2006

Per-instance rules are useful for two reasons.

First, you might have rules that are implemented as part of your class - as instance methods. This type of rule method avoids reflection, and has access to the full set of instance fields in your object, and so can very efficiently do complex rule processing.

Second, you might have rules that vary depending on the state of your object. You may only want to enforce a certain rule for domestic customers, or something like that. Now in this case I agree that there are other (and probably better) ways to approach the problem - either by making the rule itself context-aware, or by creating seperate objects to handle the different behaviors.

What I am doing in version 2.1 is adding a whole new capability - which is per-type rules. These are rules that are attached to all instances of a type, and the rule methods must be Shared/static - they can't be instance methods. These per-type rules consume far less resources (memory and CPU) and should be the normal approach used for all objects.

In other words, once 2.1 comes out, everyone should switch to per-type rules, unless they have an overwhelming need to use per-instance for specific objects, in which case only those particular objects should continue to use the per-instance scheme.


My question is whether I should change AddRule() to do the new behavior (which would be a breaking change, but would get most people using per-type automatically), or to use the existing behavior (which would make everyone change all their code to use the new per-type concept).

ktweedy replied on Monday, August 07, 2006

Perhaps a 3rd way to handle this is to mark the existing function with a Obsolete warning and implement a new AddRule function in some way so that the old code would keep working and new code could use a new function.

 

david.wendelken replied on Monday, August 07, 2006

RockfordLhotka:

Per-instance rules are useful for two reasons.
My question is whether I should change AddRule() to do the new behavior (which would be a breaking change, but would get most people using per-type automatically), or to use the existing behavior (which would make everyone change all their code to use the new per-type concept).

Given the simplicity of the edit/replace commands, I would go with the change.

 

ajj3085 replied on Monday, August 07, 2006

RockfordLhotka:

In other words, once 2.1 comes out, everyone should switch to per-type rules, unless they have an overwhelming need to use per-instance for specific objects, in which case only those particular objects should continue to use the per-instance scheme.

If this is how you feel about the per-type rules, then breaking the existing code is probably the proper course of action, as it should get people to move to the per-type method. 

Another factor to consider, even if it breaks rules, how many are in Andres spot?  Hopefully not many..but more would need to post.

One question I have thought about though.. with the shared / static rules, do you get a reference to the instance which is being validated?  Or do you just get the value of the changed property?  I haven't had a chance to look at the code yet, and I can't recall the discussion..

Andy

SonOfPirate replied on Monday, August 07, 2006

Yes, the existing approach passes the instance as an argument to the rule method delegate.

If the intent is to replace the existing approach with the per-type approach, then I am in agreement that is should just be changed.  Only takes one attempt to build to identify all the locations affected should it break your code.  Then a quick double-click takes you to the affected line to make what I'd consider a minor change.  Once it's done, it's done.  Well worth the improvement.

ajj3085 replied on Monday, August 07, 2006

SonOfPirate:

Yes, the existing approach passes the instance as an argument to the rule method delegate.

Great, thanks for clearing that up.  I'm again all in favor of the change.

If Rocky decides to go the breaking change route, I'm glad we all have unit tests to ensure the rules are still checked, right??

RockfordLhotka replied on Monday, August 07, 2006

I really appreciate all your input on this topic. I have decided to go ahead with the breaking change, and in fact I've implemented it.

I also ran it through my unit tests. I did have to tweak a couple tests which were actually testing per-instance-specific features, but all the other tests switched to per-type without incident. That speaks well to my expectation that most people, who are using all Shared/static rule methods, will be able to upgrade without changing code, and will just get the improved performance and decreased memory consumption automatically.

skagen00 replied on Wednesday, August 16, 2006

This is great - I saw the discussion unfold and once the initial question was first posed I knew exactly the result I was hoping for. This simply saves me a good amount of effort of going through each class. It sure seems to me like this should be the default behavior.

Great change, thanks.

 

ajj3085 replied on Monday, August 28, 2006

Ok,

I'm testing the beta (well, actually building upon it, since the 2.1 RTM sounds like it will be done before my next version), and I'm trying to make sure I do things the 'right' way.

I'm moving to the static (shared in vb) rules, and all seems well.  My only question is this; do I still add the static rules via overriding of AddBusinessRules?  I see AddInstanceBusiness rules, which makes sense.  It seems though that AddBusinessRules still gets run everytime through the DataPortal, but the framework ensures duplicate rules aren't re-added.  I would have though that AddBusinessRules would only be called once though, likely though the class' static constructor.  Just want to be sure this is how everything should be working.

Thanks
Andy

xal replied on Monday, August 28, 2006

Andy:
Yes, you should use AddBusinessRules().
In the server, the rules will always load, because the dataportal is "destroyed" after it's use, so the static values won't be kept. There is no gain really from that perspective.
The real gain is that if you have an object with many children, the rules for each of those children will only be loaded _once_.
For only one root object, there is no difference in the dataportal. If you were doing that in the static constructor, the constructor would run every time you call the dp... In the client though, there is a difference, because the rules are still only loaded once.

You could change the remoting portal to work as a singleton through it's config file and the rules would be kept, but that might bring some other issues that I'm unaware of...

Andrés

xal replied on Monday, August 07, 2006

I just wanted to note that I am all in favor of making shared rules default. I was just stating the obvious in my first post.

Perhaps a good approach (since we're falling into breaking) would be to have NO method called AddRule and instead have AddInstanceRule and AddSharedRule. That would force everybody to choose either one or the other and would be more clear for anyone trying to get the difference between a method called AddRule and AddXyzRule...

Andy:
Even more, Rocky finally decided to support generic delegates for rules, so you can now have a strongly typed target and ruleargs instead of having to stick with object and ruleargs and then casting or using reflection...


Andrés

Brian Criswell replied on Monday, August 07, 2006

xal:

I just wanted to note that I am all in favor of making shared rules default. I was just stating the obvious in my first post.

Perhaps a good approach (since we're falling into breaking) would be to have NO method called AddRule and instead have AddInstanceRule and AddSharedRule. That would force everybody to choose either one or the other and would be more clear for anyone trying to get the difference between a method called AddRule and AddXyzRule...

Andy:
Even more, Rocky finally decided to support generic delegates for rules, so you can now have a strongly typed target and ruleargs instead of having to stick with object and ruleargs and then casting or using reflection...

Andrés

I agree with Andrés.  I have no problem with breaking changes as long as they break in the compiler and not at run-time.  Even then, I do not mind breaking changes when they represent a real step forward.

marklindell replied on Tuesday, August 08, 2006

My vote is for changing the method name, breaking code and causing the user to change the method.  Although, I must say that's an easy call for me because I  am not using  CLSA 2.0 in production code yet. :-)

Did we not learn anything from VB.NET and how it obsurces the FCL objects? :-)

tymberwyld replied on Monday, August 07, 2006

I know I'm new here, but Shared Rules sounds like a more logical approach. 

Also, maybe I missed this in the book.  Why can't Rules be Attributes?  Don't get me wrong, I'd still want some Rules saved / restored from a database.  But I was thinking that there are some cases when I might want to setup a rule on a Property or even at the Class / Object level.  It would be great if it was just:

// [ValidationRule(Property, ErrorDescription)]
[ValidationRule(new ValueRequiredRule("MyProperty", "You need a value here!"),
ValidationRule(new ValueRequiredRule("MyUserName", "You need a value here!")]
public class MyBO {
   
    public string MyProperty { ... }
    public string MyUserName { ... }

    [ValidationRule(new ValueRequiredRule("A valid User Name is required"))]
    public string MyPassword { ... }
}

Brian Criswell replied on Monday, August 07, 2006

tymberwyld:

I know I'm new here, but Shared Rules sounds like a more logical approach. 

Also, maybe I missed this in the book.  Why can't Rules be Attributes?  Don't get me wrong, I'd still want some Rules saved / restored from a database.  But I was thinking that there are some cases when I might want to setup a rule on a Property or even at the Class / Object level.  It would be great if it was just:

// [ValidationRule(Property, ErrorDescription)]
[ValidationRule(new ValueRequiredRule("MyProperty", "You need a value here!"),
ValidationRule(new ValueRequiredRule("MyUserName", "You need a value here!")]
public class MyBO {
   
    public string MyProperty { ... }
    public string MyUserName { ... }

    [ValidationRule(new ValueRequiredRule("A valid User Name is required"))]
    public string MyPassword { ... }
}

For one thing, attribute strings are not localizable.

RockfordLhotka replied on Monday, August 07, 2006

I know I'm new here, but Shared Rules sounds like a more logical approach. 

Also, maybe I missed this in the book.  Why can't Rules be Attributes?  Don't get me wrong, I'd still want some Rules saved / restored from a database.  But I was thinking that there are some cases when I might want to setup a rule on a Property or even at the Class / Object level.  It would be great if it was just:

tymberwyld replied on Monday, August 07, 2006

No, actually I've seen some examples of using Attributes to localize descriptions.  DevExpress uses this in their XtraEditors library.  They have a DXDescriptionAttribute which is used to discover the description for the properties.  I'm assuming here that they use something similar to the links below to interpret this.

[Category("Behavior"), DXDescription("DevExpress.XtraEditors.BaseListBoxControl,AllowGrayed"), DefaultValue(false)]
public virtual bool AllowGrayed { get; set; }

Also, there have been a few samples into globalizing / localizing properties / attributes:
http://www.codeproject.com/cs/miscctrl/gpg_revisited.asp
http://www.codeproject.com/aspnet/DeclarativeGlobalization.asp

I realize the Reflection would be slow, but if we're talking about a one-time deal with static / shared Rules, the hit would only be incured once.  It's just an idea I'm tossing around, it may or may not pan-out...

paultyng replied on Tuesday, August 15, 2006

1. If you look through reflector, microsoft uses resource keys instead of actual string descriptions in their attributes to make them localizable, look at the System.Web.WebSysDescriptionAttribute as an example.

2. You would only need to use reflection once though, since its type specific.  Considering the other uses of reflection and stack walks in CSLA already, I don't beleive that its that much of an issue.  (You did say arguably, but I think the decision seems kind of arbitrary considering reflection is already used so prevalently)

I went with an Attribute based rules in my CSLA usage.  I have a base class that inherits from CSLA and just overrides the add business rules by going through the attirubtes so it kind of just rides on top of the instance rules for right now.  The biggest benefit I have though is that I can make my web databinding UI use reflection to create client side validators (ie. i have it create regex validators for a regex rule attribute) without an instance.  This is crucial in the web because control creation happens before data binding potentially to fire post back events.

xal replied on Tuesday, August 15, 2006

Here's a good argument on why _not_ to go with rules as attributes (although I agree they sound nice):


Most of us use code generation to get the basics out of the way and then add rules in a partial / base class (among other stuff). If you went with attributes you wouldn't be able to do that.

I believe that there is a way to solve that though but it's so ugly that I'm not even going to bother mentioning it.


Andrés

tpancoast replied on Tuesday, October 03, 2006

It looks like your direction is set, but just out of curiosity, why wouldn't you have AddRule() check the method signature to see if it is static and from the same class as the object and then either create an instance or a type rule accordingly?  You are already checking the method in order to raise an exception if it isn't what's expected.

RockfordLhotka replied on Tuesday, October 03, 2006

---

From: tpancoast [mailto:cslanet@lhotka.net]
Sent: Tuesday, October 03, 2006 2:01 PM
To: rocky@lhotka.net
Subject: Re: [CSLA .NET] Default to per-type rules??

It looks like your direction is set, but just out of curiosity, why wouldn't you have AddRule() check the method signature to see if it is static and from the same class as the object and then either create an instance or a type rule accordingly?  You are already checking the method in order to raise an exception if it isn't what's expected.