Hi,
I have lazy loading private field property registered like this:
public static readonly PropertyInfo<Integration> IntegrationProperty = RegisterProperty<Integration>(p => p.Integration, "Integration", RelationshipTypes.LazyLoad | RelationshipTypes.PrivateField);
Now if you check the implementation of BusinessObject, ReadProperty method in the last csla version(4.2.2) you will see:
protected virtual object ReadProperty(IPropertyInfo propertyInfo)
{
if ((propertyInfo.RelationshipType == RelationshipTypes.LazyLoad) && !FieldManager.FieldExists(propertyInfo))
throw new InvalidOperationException(Resources.PropertyGetNotAllowed);
.........
}
With this implementation my property will not enter in the “if” statement but I think that logically it should and the correct implementation should be:
if ((propertyInfo.RelationshipType & RelationshipTypes.LazyLoad == RelationshipTypes.LazyLoad) && !FieldManager.FieldExists(propertyInfo))
throw new InvalidOperationException(Resources.PropertyGetNotAllowed);
However, let’s suppose that we use the new implementation and we have some business rule applied to our IntegrationProperty. Now if we call manually BussinessRules.CheckRules(); and the property is not loaded yet then we will see that in some point of the execution CSLA calls the above mentioned ReadProperty for our IntegrationProperty. This happened in the RunRules method in the following lines:
foreach (var item in rule.InputProperties)
context.InputPropertyValues.Add(item, target.ReadProperty(item));
The call will throw exception so if we have any BusinessRule for the lazyloaded property CheckRules will fail.
I am wondering if it is correct to check rules applied to lazy loading property which is not loaded yet? Also what do you think about this CheckRules lazy loading properties problem? Am I doing something wrong?
Thanks,
Ivo
Thank you for the bug report on the flag check - I'll add this to the bug list.
In terms of the lazy load business rule issue - what do you think should happen in this case? It isn't clear to me exactly what should happen to make this scenario work in a logical way.
If the value doesn't exist, it obviously can't be read. If the rules engine can't read it we have perhaps three options:
I'm certainly open to discussion on how this could be made to work better in some way - it just isn't clear to me that there's a great answer?
Thanks for reporting the bug.
In my view - when a rule needs to know the inner workings of an object like LazyLoading it must be a private rule for that class.
I'd prefer to create a private "flow rule" that would short circuit the rule engine if the field has no value with a priority below 0 (always execute first)
You could also create a private gateway rule that would check FieldManager if property has value before running inner rule.
Rocky, I absolutely agree that there is a lot to discuss about the behavior in such a situation.
My opinion is that firstly the consumer of the framework should have different options depending on his current use case.
From what I understand about business rules checking when you load an already persisted object (and you have confidence that data source will provide you with valid data) you expect that it is valid. For that reason we don’t call the CheckRules when fetching an object but logically if we call it, it should return true. Now let’s have business object with required (for example) lazy loaded property. Obviously when the object is fetched the property is not loaded and we expect that it is valid but when we call CheckRules explicitly in the current situation we will receive an exception that property can’t be loaded.
I think that in this situation the CheckRules method should validated successfully the object and this could happened in two ways:
· Op.1: When the underling logic calls ReadProperty, instead of throwing an exception we could call property getter which will trigger the lazy loading of the property and then we could check if its value is valid. If this will break some other logic check rules could call some special overload of ReadProperty. In this scenario if we want to skip checking of business rule for property we could use Jonny’s approach.
· Op.2: CheckRules skip lazy loaded properties which haven’t been loaded yet and which are not dependent to properties that have been already modified (I am not sure if Csla has a mechanism to check if particular property value has been modified). Of course if our lazy loading property depends on modified property then we should load it like in op. 1 and check its value against the rules. I think that this approach is correct because if we trust the data in the data source and we haven’t changed our property or any of the properties it depends on, then we should not modify it and it will continue to be valid. Obviously this approach is a lot more complicated than the first one and it should be carefully considered. For example I missed the crating of New object scenario. We should definitely check rules for any lazy loaded property in it because the reason why property is not loaded could be because a value has never been set to it.
Now after all this writings and after I reread them my opinion is that best solution would be a mix of both mentioned above. Imagine that in the PropertyInfo object you have some kind of delegate - Boolean function with name like “NeedBusinessRulesCheckingDelegate which receive as arguments the current object instance and the propertyInfo itself. This delegate will be optional. There are 3 cases about a particular property:
1) If the delegate is not set at all then CSLA assign the default delegate which always returns true. In this case the property will behave like in Op.1
2)
Set a delegate defined in the CSLA core which
has an implementation satisfying op.2
pseudo code example: (bo, pi) => bo.IsNew
|| bo.ModifiedProperties.ContainsAnyOf(pi.GetPropertiesWhichIAmDepenedOn())
3) Provide custom method which could handle some specific scenario for the property
May be this is a little bit over-designed but this was the consequence of my thinking.
I can't wait to here your opinion.
Ivo
Hi,
I disagree and here is my reasoning:
LazyLoading is typically used for asyncronous load of child objects. And there is no way the rule engine can detect if the lazy loading is asyncronous or syncronous.The rule engine itself is syncronous and provides a callback function for context.Complete so that async rules can update the result in the BO. You must also keep in mind that LazyLading use OnPropertyChanged event to notify UI (and others) that values has been updated.
So if this was to work - the rule engine would have to:
The problem here is #2. There is no way of knowing whether the OnPropertyChanged came as a result of a BusinessRule, the field was initialized to null (before lazy load) or actually because it was fetched. The developer would also be responsible for calling OnPropertyChanged even if there was an error in the async data access. All in all - in my opinion this is not realistic.
IMO: Lazy loading - whether async or sync - is the developers responsibility and an inner working of the business property that the rule engine has no knowledge of. And I do NOT want to allow rules to trigger lazy loading - if you face this situation you should rather load the data immediately and NOT use lazy loading. You app would be more "sluggish" if you trigger a lot of LaxyLoading rather than loading the data up front.
CSLA might have a convention that says - skip rules for properties that are lazy loaded and not initialized yed (as PrimaryProperty) - but I can hardly see how it could skip rule processing if a lazy loaded property is an InputProperty to another rule.
But - you - the developer - could create short cut rules so that the rules for a property would not run when a lazy loaded input property has not been initiallized yet.
Imagine using rules like this.
// Gateway rule BusinessRules.AddRule(new FieldExists(IntegrationProperty, new Required(IntegrationProperty))); or
// Short circuit rule BusinessRules.AddRule(new StopIfNotFieldExists(IntegrationProperty){Priority = -1}); BusinessRules.AddRule(new Required(IntegrationProperty)); // add more buiness rules
The gateway rule will only allow the inner rule to execute if Integration has been loaded/initialized.
The short circuit rule wil stop rule processing for that field if Integration has not been loaded/initalized.
My proposed change is to skip lazy loaded field which have no FieldData (FieldExists = false) and so not add an entry in InputPropertyValues. Else the rule may be provided with "fake" data when in reality it has no value.
I'll even include code for the rules here (and I'll consider to add these to RuleTutorial sample)public class StopIfNotFieldExists : BusinessRule { private class Accessor : ObjectFactory { internal new bool FieldExists(object obj, IPropertyInfo property) { return base.FieldExists(obj, property); } } private readonly Accessor _accessor = new Accessor(); public StopIfNotFieldExists(IPropertyInfo primaryProperty) : base(primaryProperty) { } protected override void Execute(RuleContext context) { if (!_accessor.FieldExists(context.Target, PrimaryProperty)) { context.AddSuccessResult(true); } } }
and
public class FieldExists : BusinessRule { private class Accessor : ObjectFactory { internal new bool FieldExists(object obj, IPropertyInfo property) { return base.FieldExists(obj, property); } } private readonly Accessor _accessor = new Accessor(); /// <summary> /// Gets the inner rule. /// </summary> public IBusinessRule InnerRule { get; private set; } /// <summary> /// Initializes a new instance of the <see cref="IsNew"/> class. /// </summary> /// <param name="primaryProperty"> /// The primary property. /// </param> /// <param name="innerRule"> /// The inner rule. /// </param> public FieldExists(IPropertyInfo primaryProperty, IBusinessRule innerRule) : base(primaryProperty) { if (InputProperties == null)InputProperties = new List<IPropertyInfo>();InnerRule = innerRule; RuleUri.AddQueryParameter("rule", System.Uri.EscapeUriString(InnerRule.RuleName)); // merge InnerRule input property list into this rule's list if (InnerRule.InputProperties != null) { InputProperties.AddRange(InnerRule.InputProperties); } // remove any duplicates InputProperties = new List<IPropertyInfo>(InputProperties.Distinct()); AffectedProperties.AddRange(innerRule.AffectedProperties); } /// <summary> /// Executes the rule /// </summary> /// <param name="context"> /// The rule context. /// </param> protected override void Execute(RuleContext context) { if (_accessor.FieldExists(context.Target, PrimaryProperty)) { context.ExecuteRule(InnerRule); } } }You should be very cautions in how you interact whil child collections/objects in the rule implementation.Added to bugtracker: http://lhotka.net/cslabugs/edit_bug.aspx?id=1014
Read more in this thread. http://forums.lhotka.net/forums/p/10433/48859.aspx#48859
Csla trunk is updated and now the rules can be even more simplified and written like this:
public class StopIfNotFieldExists : BusinessRule { public StopIfNotFieldExists(IPropertyInfo primaryProperty) : base(primaryProperty) { if (InputProperties == null) InputProperties = new List<IPropertyInfo>(); InputProperties.Add(primaryProperty); } protected override void Execute(RuleContext context) { if (!context.InputPropertyValues.ContainsKey(PrimaryProperty)) { context.AddSuccessResult(true); } } }
public class FieldExists : BusinessRule { public IBusinessRule InnerRule { get; private set; } public FieldExists(IPropertyInfo primaryProperty, IBusinessRule innerRule) : base(primaryProperty) { if (InputProperties == null) InputProperties = new List<IPropertyInfo>(); InputProperties.Add(primaryProperty); InnerRule = innerRule; RuleUri.AddQueryParameter("rule", System.Uri.EscapeUriString(InnerRule.RuleName)); // merge InnerRule input property list into this rule's list if (InnerRule.InputProperties != null) { InputProperties.AddRange(InnerRule.InputProperties); } // remove any duplicates InputProperties = new List<IPropertyInfo>(InputProperties.Distinct()); AffectedProperties.AddRange(innerRule.AffectedProperties); } protected override void Execute(RuleContext context) { if (context.InputPropertyValues.ContainsKey(PrimaryProperty)) { context.ExecuteRule(InnerRule); } } }
Your rule implementation may also check for the existance of the lazy loded field in
context.InputPropertyValues.
Hi,
Thank you very much for your fast and exhaustive reply.
Your idea for StopIfNotFieldExists rule sounds great. It absolutely replaces my idea for NeedBusinessRulesCheckingDelegate and is far more elegant. I have some observations and thoughts about your proposed solution and RulesEngine on the whole.
I think that you have a bug in the Execute implementation of StopIfNotFieldExists rule:
protected override void Execute(RuleContext context)
{
if (!context.InputPropertyValues.ContainsKey(PrimaryProperty))
{
context.AddSuccessResult(true);
}
}
Shouldn’t it be the exactly opposite:
if (context.InputPropertyValues.ContainsKey(PrimaryProperty))
I prefer to talk with samples so let’s have Invoice business object which has Buyer property declared like this:
public static readonly PropertyInfo<Client> BuyerProperty = RegisterProperty<Client>(c => c.Buyer, RelationshipTypes.Child | RelationshipTypes.LazyLoad);
[Required]
public Client Buyer
{
get
{
if (!FieldManager.FieldExists(BuyerProperty))
{
LoadProperty(BuyerProperty, ...);
OnPropertyChanged(BuyerProperty);
}
return GetProperty(BuyerProperty);
}
set
{
SetProperty(BuyerProperty, value);
}
}
The question is when the Required rule should be applied and when not. I think that when we create new invoice the rule should be applied because each invoice should have a Buyer but if we load an existing invoice and haven’t loaded the Buyer we don’t need to apply the rule. My point here is that if you want to make StopIfNotFieldExists a best practice it should be more complicated and should handle this “New” scenario and also the scenario in which Buyer is dependent on some other property that has been changed.
Moreover, there will be a problem if a Buyer property depends on another property that has been actually changed; in this case, the rules for Buyer property should be executed no matter if the Buyer property is LazyLoad or not in order to preserve correctness of the whole object. So in this case somehow we should load the Buyer property – the question who will do it, since your valid objection of the rule itself to load it?
My final observation is related to FieldExists method of the FieldManager. I am wondering why FieldExists implementation can’t distinguish between loaded property with null value and not loaded yet property? Jonny increase my attention on that with this statement “The problem here is #2. There is no way of knowing whether the OnPropertyChanged came as a result of a BusinessRule, the field was initialized to null (before lazy load) or actually because it was fetched”. May be this point is for other thread but I think that it will be great if we could distinguish between this two states because logically they are different and practically this mixture could lead to performance issues when a value of a lazy loaded property for a business object is null and its loading is computation intensive. In this case every time the property is accessed it will trigger the computation. Am I misunderstanding something here?
Ivo
Yes, that is a bug in the rule - good catch.
No, it is as it should be. If the field is not initalized then it will not be in the InputPropertyValues dictionary and so it should short circuit the rules for this property.
FieldManager will distinguish between a "non" initilized field value and a field with value=null. When you set the field value to null then FieldExists will return true.
Problem #2: when using async loadiing it is quite common to initialize the field to "null" (so that FieldExits returns true) to avoid multiple async fetch.
For state checking look at RuleTutorial Sample and Gateway rules - that wrap any inner sync rule.
Ex: IsNew, IsNotNew, CanWrite etc rules.
I would also say that it is an edge case if you have a required field on a lazy loaded child object/list. You would typically rather have required fields on that object and CSLA will automatically check that all child objects are OK before Save is allowed. In BusinessObject terms we rather tend to simplify the object model and not think of it as an entity model. If invoice has only on possible Buyer then I would flatten the hierarchy. This will also greatly simplify relationship between rules and properties.
Yes, I completely misunderstand what the method AddSuccessResult do.
Thanks, those getaway rules seem to be very helpful.
It is great that you have plans to improve the FieldManager implementation, in order to “distinguish between a "non" initilized field value and a field with value=null”, if I understand you correctly, but do you have any idea approximately when it will happen?
However, I’m not exactly sure if I understand your last point:
“If invoice has only on possible Buyer then I would flatten the hierarchy. This will also greatly simplify relationship between rules and properties.”
Did you suggest that if the Invoice class has only one Buyer, I should put all properties of Buyer object in the Invoice itself? So instead of having hierarchy like
Invoice
- Buyer
- Name
- Address
I should have
Invoice
- BuyerName
- BuyerAddress
If that’s the case, I don’t think I like this approach at least because in this way business objects can hardly be reused and I think that they are coupled too much with the UI.
Thanks again,
Ivo
Hi,
FieldManager as-is will distinguish between a non initialized value and a field set to null value.(No change planned for FieldManager)
The point of Business Objects is NOT reuse - it's Maintainability with lowest level of coupling and complexity.
Read Rocky's post here: http://forums.lhotka.net/forums/p/3465/17230.aspx#17257
BusinessObjects works best when they support the UI to make easy-to-bind objects and properties that encapsulate the behavior for that use case.
For more info read these posts: (and especially Rocky's posts in these threads):
http://www.lhotka.net/cslanet/faq/CslaObjectFaq.ashx
Copyright (c) Marimer LLC