Rule with multiple properties only fires when the Primary Property changes

[lwmorris067]

I have a rule that takes multiple properties to calculate something. I thought that CSLA automatically associated all the properties passed in the constructor so that any changes to any of the properties would cause the rule to execute. It fires when the PrimaryProperty changes, but none of the properties added to the InputProperties or AffectedProperties collection.

I'm either remembering incorrectly or that's not the way the v8 pre-release (8.0.0-R24021201) works, or I'm just doing something wrong. I have not utilized the Dependency rule.

"CSLA .NET provides a rule designed specifically to set up dependency relationships between
properties. In many cases, dependencies are automatically detected because properties affected by
a rule become dependent properties. But sometimes you’ll want to explicitly designate a
dependency relationship by using the Dependency rule."

[hurcane]

Extra parameters on the rule's constructor need to be added to the InputProperties collection. The GitHub repo has a RuleTutorial project with some examples that contain multiple properties. A good example is GreaterThanOrEqual.

Here's the constructor from that sample:

public GreaterThanOrEqual(IPropertyInfo primaryProperty, IPropertyInfo compareToProperty)
      : base(primaryProperty)
    {
      CompareTo = compareToProperty;
      InputProperties = new List<IPropertyInfo>() { primaryProperty, compareToProperty };
      this.RuleUri.AddQueryParameter("compareto", compareToProperty.Name);
    }

Note that the primary property and the comparison property are both added to the InputProperties collection. A SetProperty call to either property will cause the rule to be executed.

[lwmorris067]

That is not the behavior I'm seeing. The rule only fires when the primary property changes.

The constructor of my rule looks like:

 public ClaimIsReOpenable(IPropertyInfo subStatusIdProperty, IPropertyInfo approvedToProperty, IPropertyInfo isReOpenableProperty) 
     : base(subStatusIdProperty)
 {
     InputProperties = new List<IPropertyInfo>() { subStatusIdProperty, approvedToProperty, isReOpenableProperty };
     AffectedProperties.Add(isReOpenableProperty);

     this.RuleUri.AddQueryParameter("approvedTo", approvedToProperty.Name);
 }

My Business Object looks like:

[Serializable]
public class TestObject : BusinessBase<TestObject>
{
    public static readonly PropertyInfo<Guid> SubStatusIdProperty = RegisterProperty<Guid>(nameof(SubStatusId));
    public Guid SubStatusId
    {
        get => GetProperty(SubStatusIdProperty);
        set => SetProperty(SubStatusIdProperty, value);
    }

    public static readonly PropertyInfo<DateTime> ApprovedToProperty = RegisterProperty<DateTime>(nameof(ApprovedTo));
    public DateTime ApprovedTo
    {
        get => GetProperty(ApprovedToProperty);
        set => SetProperty(ApprovedToProperty, value);
    }

    public static readonly PropertyInfo<bool> IsReOpenableProperty = RegisterProperty<bool>(nameof(IsReOpenable));
    public bool IsReOpenable
    {
        get => GetProperty(IsReOpenableProperty);
        set => SetProperty(IsReOpenableProperty, value);
    }

    protected override void AddBusinessRules()
    {
        BusinessRules.AddRule(new ClaimIsReOpenable(SubStatusIdProperty, ApprovedToProperty, IsReOpenableProperty));
        base.AddBusinessRules();
    }

    [Create]
    protected void Create()
    {
        BusinessRules.CheckRules();
    }
}

[lwmorris067]

I can make it work if I manually add a Dependency rule:

            protected override void AddBusinessRules()
            {
                BusinessRules.AddRule(new ClaimIsReOpenable(SubStatusIdProperty, ApprovedToProperty, IsReOpenableProperty));
                BusinessRules.AddRule(new Dependency(ApprovedToProperty, SubStatusIdProperty));
                base.AddBusinessRules();
            }

[hurcane]

The Execute method is not fired when ApprovedToProperty changes?

Things I would be validating:

• ApprovedTo value really is changing.
• ApprovedTo is a DateTime, but you're not using the Csla.SmartDate conversion. Might that be an issue?
• Does directly setting IsReopenable cause the rule to fire? Based on the sample, it should because IsReopenable is included in the InputProperties collection.

[lwmorris067]

Here's my unit test:

       [Fact, Category("Unit")]
       public void ReopenableIfClosedLessThan365DaysAgo()
       {
           var portal = Fixture.DataPortalFactory.GetPortal<TestObject>();
           var testItem = portal.Create();
           testItem.ApprovedTo = DateTime.Now;
           Assert.True(testItem.IsReOpenable);
       }

[hurcane]

@mtavares628 If a property changes value and it is used as an input property on other rules, you need to run the rule to guarantee that the rule evaluation is accurate and up-to-date.

I think my proposal would address extra running of rules during CheckRules. Do you have an example where the same rule is being run multiple times due to a property change? Here's the original example you provided:

protected override void AddBusinessRules()
        {
            base.AddBusinessRules();
            BusinessRules.AddRule(new ReferenceDateRule(ReferenceDateProperty));
            BusinessRules.AddRule(new IDRule(IDProperty, ReferenceDateProperty));
        }

With a property change to ReferenceDate, the ReferenceDateRule would run and then IDRule would run because it depends on ReferenceDate. If IDRule is not run, then IDRule could be in the wrong state. I don't see excessive running of rules in this scenario.

With the proposed change to CheckRules, both properties would run once. Previously, the IDRule would be run twice.

[mtavares628]

@hurcane I follow what you are saying now. After running a few tests of my own, I think attacking it from the CheckRules method may be the way to go. However, I'm not sure if CascadeOnDirtyProperties should actually be turned off. I think there still needs to be the determination whether the property is considered a dirty property, and if its value did change as a result of one of the rules run by CheckRules, then all of the rules where it is an an affected/input property should be re-run. They just shouldn't be re-run no matter what which was happening before v8.

[hurcane]

If CascadeOnDirtyProperties is enabled, affected properties will immediately trigger running the rules that reference the affected properties, even if the method's cascade parameter is false. CheckRules should handle that after all properties have had their first run.

After CheckRules has done its job, it would restore the setting so it can be applied on future property change events.

[hurcane]

If done properly, CheckRules should have no recursive nature to it. At the top level, this is the flow I envision:

1. Remember CascadeOnDirtyProperties setting
2. Turn off CascadeOnDirtyProperties
3. Run all rules, gathering distinct result of affected properties, no cascade
4. Run rules for dirty properties (primary and input), gathering distinct result of affected properties, no cascade - this preserves the behavior of one level of cascade by default
5. If CascadeOnDirtyProperties was true, repeat the previous step until no new dirty properties are returned - this preserves the behavior when CascadeOnDirtyProperties was enabled
6. Restore CascadeOnDirtyProperties

There will be some rules that run multiple times, but only if they are triggered by dirty properties.

[mtavares628]

That makes sense to me, thanks for clarifying.

[Chicagoan2016]

I have several csla 4.xx applications. My observation is the same as what @lwmorris067 described.

[mtavares628]

To my understanding the AffectedProperties list is used to fire the rules for the property that you add to the list where that property is the primary property, not fire the rule that you are in when that affected property changes. The same thing is actually true of the InputProperties as well. It would fire off the rules for all of the other input properties where those properties have rules where they are the primary property. With the old logic, depending upon what your other rules would look like, you could end up in a situation where the Affected/InputProperties would trigger the original rule as a dependency from ping-ponging back and forth between the rules. With the change, the InputProperties's rules are only fired if those input property values actually changed. Otherwise, they don't fire.

The only real way to guarantee that your rule triggers from the dependency is to use the Dependency rule like you did.

@rockfordlhotka That was why I was proposing creating a TriggeredProperties list in the rule that would have the rule run whenever one of the properties in that list were changed, and it would encapsulate all of the dependencies within the rule. Here's what I was proposing: #3415 (reply in thread)

[rockfordlhotka]

I do consider this something we need to address. I've opened an issue so it is on the CSLA 8 schedule: #3721

I don't yet know that we've arrived at a consensus, and I hope the conversation continues here in this thread so we can work this out.

[mtavares628]

@rockfordlhotka I'd still like to see some method where the rule itself controls what properties trigger it, so that it is all encapsulated within the rule rather than relying on other outside rules to trigger it based on affected/input properties. To me, it's cleaner this way because you don't have to worry about bouncing back and forth between rules. But maybe I'm missing something where that won't work. The triggered properties idea gives the rules that kind of control, but it would be additive, so the affected/input properties methodology would still work. All of this is still a breaking change from what it was before #3616, though.

[rockfordlhotka]

The potential side-effect is that a rule becomes coupled to a specific business class, and that is something I want to avoid.

The goal is always that a rule stands alone and is attached to a property in a domain class, rather than having rules be aware of any domain class or properties specifically.

Yes, I know that using Target allows us to break this goal - but generally speaking rules should not be tightly coupled to business classes or properties.

The idea that a rule could accept a list of properties that it will cascade down to is reasonable - because that list of properties would be provided via the constructor and thus avoids being tightly coupled.

I agree with you that this should be additive to the existing #3616 behavior.

[mtavares628]

Just so I'm on the same page, my idea on Triggered Properties would still have those properties be passed in on the constructor. They wouldn't just be pulled from the Target or the Domain class. So I don't think they would be tightly coupled. I would just rather be able to have the option to shut off the cascading on InputProperties, and only trigger the rule when the property is the primary, or if is one of the other passed in properties is in that rule's TriggeredProperties list. My initial thought was to also add a flag on the rule to bypass the cascading of the Input Properties, but you could also just not add those properties to the InputProperties list, and just add a GetTriggeredValue function to pull the values of the triggered properties in the Execute method.

So a rule could look something like this:

    public class IDRule : Csla.Rules.BusinessRuleAsync
    {
        IPropertyInfo referenceDateProperty;
        IPropertyInfo nameProperty;
        public IDRule(IPropertyInfo primaryProperty, IPropertyInfo referenceDateProperty, IPropertyInfo nameProperty) : base(primaryProperty)
        {
            referenceDateProperty = referenceDateProperty;
            nameProperty = nameProperty;
            InputProperties = new List<IPropertyInfo> { PrimaryProperty };
            TriggeredProperties = new List<IPropertyInfo> { referenceDateProperty, nameProperty };
        }

        protected override async Task ExecuteAsync(IRuleContext context)
        {
            var id = context.GetInputValue<int>(PrimaryProperty);
            var referenceDate = context.GetTriggeredValue<DateTime>(referenceDateProperty);
            var name = context.GetTriggeredValue<string>(nameProperty);
            // rest of business rule logic here

        }
    }

And the calling code would be similar to what I had here: #3415 just without the check for the boolean flag (InputPropertiesRunAsAffectedProperties)

I am also open to @hurcane's idea of changing up the full CheckRules, but I think it would be a bigger undertaking because you would no longer want to rely on that function calling CheckRulesOnProperty.

[hurcane]

@mtavares628 If I am understanding correctly, you desire to have a scenario where a rule can have multiple input properties, but only be triggered when some of the input properties are changed, and not run at all when other input properties are changed? I am unable to come up with a practical example where I would want to have this behavior. I presume that if an input property is needed to evaluate the rule, then a change to that property could affect the rule, and the rule should be evaluated. But I'm open to counter examples that demonstrate why my presumption is false.

The previous behavior, while not efficient, allowed developers of business rules to have comprehensively evaluated business rules without having to set up explicit dependency rules. In my primary project, this behavior is relied on by literally tens of thousands of rules across ~1500 business classes, many of which have quite complex sets of rules. I shudder to think about having to ask our management team for resources to fix this breaking change.

The changes I proposed to CheckRules would still pass all the rule executions through CheckRulesOnProperty. It would change the cascade parameter to false, so CheckRulesOnProperty will not trigger rules for the input properties and affected properties.

The changes in CheckRules would result in CheckRules and CheckRulesOnProperty both having logic to deal with cascading affected properties and input properties, but I think that is appropriate considering that CheckRules is a "batch" operation, while other callers of CheckRulesOnProperty are calling in response to a single property change.

If somebody can come up with a concrete example of a rule that needs a property but doesn't care if the property changes, I have a potential proposal for that scenario. But I am withholding it for now because

[Chicagoan2016]

I am enjoying this discussion, I need to allocate some time to this over the weekend.
@hurcane Do you think a sample code is in order?
Thanks for the generous help.

[hurcane]

@Chicagoan2016 What kind of sample code are you looking for?

[rockfordlhotka]

I'd like to reset the discussion a bit, as I want to more clearly understand the desired behavior.

Right now in the CSLA 8 preview, AffectedProperties cascade, which is (imo) the desired behavior - and so I'm not 100% sure of the value of TriggeredProperties in addition to AffectedProperties.

For example, this is some output (code is below):

ReferenceDateRule
IDRule
ReferenceDateRule
---
ReferenceDateRule
---
IDRule
ReferenceDateRule

The first block is from Fetch, where CheckRules() triggers each property's rules - and you can see that ReferenceDataRule runs twice - once directly and once due to cascading from IDRule.

The second block is due to a change to the date property - so only the date rule runs.

The third block is due to a change to the ID property - and IDRule indicates that it affects the date property, so the rules cascade.

From this code:

using Csla;
using Csla.Configuration;
using Csla.Core;
using Csla.Rules;
using Microsoft.Extensions.DependencyInjection;

var services = new ServiceCollection();
services.AddCsla();
var provider = services.BuildServiceProvider();

var portal = provider.GetRequiredService<IDataPortal<Root>>();
var obj = portal.Fetch();
Console.WriteLine("---");
obj.ReferenceDate = "tom";
Console.WriteLine("---");
obj.ID = 2;

[Serializable]
public class Root : BusinessBase<Root>
{
    public static readonly PropertyInfo<string> NameProperty = RegisterProperty<string>(nameof(Name));
    public string Name
    {
        get => GetProperty(NameProperty);
        set => SetProperty(NameProperty, value);
    }

    public static readonly PropertyInfo<Csla.SmartDate> ReferenceDateProperty = RegisterProperty<Csla.SmartDate>(nameof(ReferenceDate));
    public string ReferenceDate
    {
        get => GetPropertyConvert<Csla.SmartDate, string>(ReferenceDateProperty);
        set => SetPropertyConvert<Csla.SmartDate, string>(ReferenceDateProperty, value);
    }

    public static readonly PropertyInfo<int> IDProperty = RegisterProperty<int>(nameof(ID));
    public int ID
    {
        get => GetProperty(IDProperty);
        set => SetProperty(IDProperty, value);
    }



    protected override void AddBusinessRules()
    {
        base.AddBusinessRules();
        BusinessRules.AddRule(new ReferenceDateRule(ReferenceDateProperty));
        BusinessRules.AddRule(new IDRule(IDProperty, ReferenceDateProperty));

        //BusinessRules.AddRule(new MyRule { PrimaryProperty = NameProperty });
    }

    [Fetch]
    private void Fetch()
    {
        using (BypassPropertyChecks)
        {
            Name = "Test";
            ReferenceDate = DateTime.Today.ToShortDateString();
            ID = 1;
        }
        BusinessRules.CheckRules();
    }
}

public class ReferenceDateRule : Csla.Rules.BusinessRuleAsync
{
    public ReferenceDateRule(IPropertyInfo primaryProp) : base(primaryProp)
    {
        InputProperties = new List<IPropertyInfo>() { PrimaryProperty };
    }

    protected override async Task ExecuteAsync(IRuleContext context)
    {
        context.AddSuccessResult(false);
        Console.WriteLine(nameof(ReferenceDateRule));
    }
}

public class IDRule : Csla.Rules.BusinessRuleAsync
{
    IPropertyInfo ReferenceDateProperty;
    public IDRule(IPropertyInfo primaryProp, IPropertyInfo referenceDateProp) : base(primaryProp)
    {
        ReferenceDateProperty = referenceDateProp;
        InputProperties = new List<IPropertyInfo>() { PrimaryProperty, ReferenceDateProperty };
        AffectedProperties.Add(referenceDateProp);
    }

    protected override async Task ExecuteAsync(IRuleContext context)
    {
        context.AddSuccessResult(false);
        Console.WriteLine(nameof(IDRule));
    }
}

[rockfordlhotka]

@lwmorris067 now I'm confused - I thought the desire was to enable the previous non-optimized behavior because the optimization breaks your code?

Mike can opt-in to the optimization by adding CascadeIfDirty = true to his rule's constructors.

[lwmorris067]

@rockfordlhotka, I'm sorry if I'm confusing you. Returning to the previous non-optimized behavior is what my code depends on; firing a rule when any of the input properties change.

I'll ask any other questions / suggestions in a different thread. Sorry.

[hurcane]

This is not code, but it has the logic flow. These changes are within CheckRules. CheckRulesForProperty would revert to its previous code, and this change would not otherwise modify CheckRulesForProperty.

The motivation for this design is that CheckRules is a batch operation. It's going to attempt to run all rules for every property. Dependencies are only affected if the rules cause values to change.

Logic flow

1. Remember CascadeOnDirtyProperties setting (local variable) and set CascadeOnDirtyProperties = false.
   • This is important because CheckRulesForProperty will always attempt to cascade if this property is enabled.
   • We want to honor the advanced CascadeOnDirtyProperties, but from within the batch nature of CheckRules.
2. Run all rules via CheckRulesForProperty, gathering distinct result of affected properties, with no cascading of any sort
   • Current behavior sets cascade property = true
   • New behavior would set cascade property = false
3. For each dirty property, run CheckRulesForProperty again, gathering distinct result of affected properties, with no cascading of any sort
   • Note: Dirty properties are added to the results from affected properties that had a change in value. If an affected property does not change value during the rule execution, it won't be in the dirty properties collection.
   • Cascade property = false
   • Include rules where the dirty property is the primary property
   • Include rules where the dirty property is the input property
   • With the previous behavior, the cascade = true would cause a single recursive level of cascade for both dirty properties and input properties within CheckRulesForProperty. This step does the functional equivalent since CheckRules is not allowing CheckRulesForProperty to do it.
   • This affected / dirty properties that are the output of this step is different from the original set of affected / dirty properties.
4. If CascadeOnDirtyProperties was true, repeat the previous step until no new dirty properties are returned
   • This preserves the behavior when CascadeOnDirtyProperties was enabled at the beginning of CheckRules
   • Include rules where the dirty property is the primary property
   • Include rules where the dirty property is the input property
   • This is the functional equivalent of the block in CheckRulesForProperty that handles when the CascadeOnDirtyProperties is true.
5. Restore CascadeOnDirtyProperties value from the local variable in the first step.

The code details within CheckRules will be very similar to the code within CheckRulesForProperty. The difference is that CheckRuelsForProperty is recursive per property, while CheckRules is batch oriented.

Rules example scenario for desk check

This example is for an order scenario and has a sample of the complexity I deal with on a regular basis.

This scenario has a common that is used by multiple properties

• SignAgreement rule
  • Has a primary property and an array of input properties
  • Rule execution confirms that all properties have the same sign and sets validation status of the property

Here are the Involved properties and the rules assigned to each one as the primary property. All rules have equal priority.

• Customer credit limit property
  • No rules
• Customer current credit balance property
  • No rules
• On credit hold property
  • SetCheckoutStatus rule
    • Has no input properties other than the primary property
    • Has affected property for can check out property
    • Execution adds out value for can check out to be the opposite of the on credit hold property value
• Order total property
  • Sum rule with input properties for product amount property, shipping charge property and sales tax property
  • SetCreditHold rule
    • Has input properties of customer credit limit, current balance, and on credit hold property
    • Has affected property for credit hold property
    • Execution adds out value for credit hold property based on order total + credit balance > credit limit
• Product amount property
  • SignAgreement rule with input properties of sales tax and sign of freight charge
• Freight charge property
  • SignAgreement rule with input properties of sales tax and sign of product amount
• Sales tax property
  • SignAgreement rule with input properties of freight charge and sign of product amount
• Can check out property
  • No rules

Current behavior

Because a change to the product amount affects the order total, and this then affects the credit hold status, and that then affects the can check out property, this business object uses the advanced feature to cascade on dirty properties. Presume that on credit hold and can check out properties are virtual and both initialized to false. Also presume that the order amounts are such that the order would be considered to be on credit hold. All other properties were initialized with LoadProperty.

This list attempts to show the recursive nature of how the rules are being executed.

• On credit hold SetCheckoutStatus rule runs
  • Can check out property is changed from false to true
  • No cascade calls because can check out has no rules and is not used as an input property.
• Order total Sum rule runs and Order total SetCreditHold rule runs
  • On credit hold property changes from false to true, triggering 1st-level cascade for on credit hold primary rules and no rules based on input properties
  • On credit hold SetCheckoutStatus rule runs - 2nd time
    • Can check out property is changed from true to false
    • No more cascade calls because can check out has no rules and is not used as an input property
• Product amount SignAgreement rule runs
  • Order total sum rule runs - 2nd time
    • No more cascade
• Freight charge SignAgreement rule runs
  • Order total sum rule runs - 3rd time
    • No more cascade
• Sales tax SignAgreement rule runs
  • Order total sum rule runs - 4th time
    • No more cascade

New behavior

• Advanced cascade is turned off
• All rules run with no cascade
  • On credit hold SetCheckoutStatus rule runs and adds dirty property for Can check out property (it went from false to true)
  • Order total Sum rule runs with no dirty properties generated
  • Order total SetCreditHold rule runs and adds dirty property for can On credit hold property (it went from false to true)
  • Product amount SignAgreement rule runs with no dirty properties generated
  • Freight charge SignAgreement rule runs with no dirty properties generated
  • Sales tax SignAgreement rule runs with no dirty properties generated
    Note: If we stopped here, the object does not have a valid state because on credit hold and can check out are both true.
• Run rules for dirty properties with no cascade
  • Can check out property has no rules
  • On credit hold SetCheckoutStatus rule runs (2nd time) and it adds a dirty property for Can check out property (it went from true to false)
• Advanced cascade was enabled, so repeat until no more dirty properties
  • Can check out property has no rules
  • No new dirty properties since no rules were run

A couple rules are still run multiple times, but this is unavoidable when you have actual complex rule dependencies that cause property changes. In many scenarios where there are many complex dependencies but not affected properties, CheckRules will simply run every rule a single time and have no more work to do.

[rockfordlhotka]

Captured in #3736

[rockfordlhotka]

At this point I am strongly leaning toward putting the ideas @hurcane documented into the CSLA 9 release. I think it is a fantastic change, but I'd like to allow time to make any deep changes required, get thorough testing (because the impact may be large), etc. I'm concerned that there's too much time pressure right now to get CSLA 8 out (so we support Blazor 8).