Support nested patterns and joins within ForAllElement

[a046]

I’m trying to create a ”ForAll” rule using the RuleBuilder API where additional facts are joined onto the fact type used in the ForAll rule.

This can be achieved with Any/None rules by using the GroupBuilder, but the patterns collection on ForAll does not work in the same way. See below:

using NRules.IntegrationTests.TestAssets;
using NRules.RuleModel;
using NRules.RuleModel.Builders;
using System;
using System.Linq.Expressions;
using Xunit;

namespace NRules.IntegrationTests
{
    public class ThreeFactForAllRuleBuilderTest : BaseRuleTestFixture
    {
        [Fact]
        public void ThreeFacts()
        {
            //Arrange
            var builder = new RuleBuilder();
            builder.Name("Test Rule");

            var lhs = builder.LeftHandSide();
            var patternBuilder1 = lhs.Pattern(typeof(FactType1), "fact1");

            Expression<Func<FactType1, FactType2, bool>> fact1_2_Join = (fact1, fact2) => fact2.JoinProperty == fact1.TestProperty;
            
            // Adding this makes it work as a workaround, but exports a FactType2 pattern
            //lhs.Pattern(typeof(FactType2), "f2");
            //
            var patternBuilder2_Base = new PatternBuilder(typeof(FactType2), "fact2");
            patternBuilder2_Base.Condition(fact1_2_Join);

            var patternBuilder2_Actual = new PatternBuilder(typeof(FactType2), "f2"); // Use f2 to avoid collision of names, Fluent also uses different names for BasePattern and Pattern
            Expression<Func<FactType2, bool>> condition21 = f2 => f2.TestProperty.StartsWith("Valid");
            patternBuilder2_Actual.Condition(condition21);

            var patternBuilder3 = new PatternBuilder(typeof(FactType3), "fact3");
            Expression<Func<FactType2, FactType3, bool>> fact2_3_Join = (f2, fact3) => fact3.JoinProperty == f2.TestProperty;
            patternBuilder3.Condition(fact2_3_Join);

            var allBuilder = lhs.ForAll();
            allBuilder.BasePattern(patternBuilder2_Base);
            allBuilder.Pattern(patternBuilder2_Actual);
            allBuilder.Pattern(patternBuilder3); // Commenting out this line, the test passes

            Expression<Action<IContext, FactType1>> action = (context, fact1) => NoOp();
            builder.RightHandSide().Action(action);

            //Act
            var rule = builder.Build();
        }

        [Fact]
        public void ThreeFacts_WorksForExists()
        {
            //Arrange
            var builder = new RuleBuilder();
            builder.Name("Test Rule");

            var lhs = builder.LeftHandSide();
            var patternBuilder1 = lhs.Pattern(typeof(FactType1), "fact1");

            var patternBuilder2 = new PatternBuilder(typeof(FactType2), "fact2");
            Expression<Func<FactType1, FactType2, bool>> fact1_2_Join = (fact1, fact2) => fact2.JoinProperty == fact1.TestProperty;
            Expression<Func<FactType2, bool>> condition21 = fact2 => fact2.TestProperty.StartsWith("Valid");
            patternBuilder2.Condition(fact1_2_Join);
            patternBuilder2.Condition(condition21);

            var patternBuilder3 = new PatternBuilder(typeof(FactType3), "fact3");
            Expression<Func<FactType2, FactType3, bool>> fact2_3_Join = (fact2, fact3) => fact3.JoinProperty == fact2.TestProperty;
            patternBuilder3.Condition(fact2_3_Join);

            var groupBuilder = new GroupBuilder();
            groupBuilder.Pattern(patternBuilder2);
            groupBuilder.Pattern(patternBuilder3);

            var existsBuilder = lhs.Exists();
            existsBuilder.Group(groupBuilder);

            Expression<Action<IContext, FactType1>> action = (context, fact1) => NoOp();
            builder.RightHandSide().Action(action);

            //Act
            var rule = builder.Build();
        }

        public static void NoOp()
        {
        }

        protected override void SetUpRules()
        {
        }

        public class FactType1
        {
            public string TestProperty { get; set; }
        }

        public class FactType2
        {
            public string TestProperty { get; set; }
            public string JoinProperty { get; set; }
        }

        public class FactType3
        {
            public string TestProperty { get; set; }
            public string JoinProperty { get; set; }
        }
    }
}

From investigation, it appears that the ForAll patterns cannot “import” parameters from each other and require their dependent patterns to be added to the left hand side of the rule rather than just the ForAllBuilder.

I’m not sure if this is a bug, feature request or support issue. Am I using the ForAllPattern incorrectly and is there any reason Any/None take a group but ForAll cant?

[a046]

I think due to the implementation of VisitForAll which assumes the patterns are all of the same declaration type as "base", the "work around" will actually just cause downstream issues. Exists/Not seems to work for end-to-end cases however.

I guess the "bug" here is that there's no validation for that at rule compile time. But i'm still not sure how to work around this or how ForAll can be changed to support this setup like Exists/Not can.

[snikolayev]

I believe there is indeed a problem here. A rule element (in the RuleModel) can import declarations from the outer scope and can export declarations to the outer scope. It cannot however just expose declarations to the nested elements w/o also exporting them outside.
ForAllElement cannot export contained patterns to the outer scope, because that would make no sense. NotElement and ExistsElement don't export declarations either, but unlike ForAllElement they can contain GroupElement, which enables you to effectively define a local scope within them.
The fix here will be to allow local declarations within rule elements w/o necessarily exporting them.
I'm going to fix this.

[snikolayev]

@a046 while the above statement about declaration scopes is true, the situation with ForAll element is much deeper. ForAll element states that for all facts that match the base pattern, all of the subsequent patterns must also match. So, the first embedded assumption in its implementation is that all of these patterns are matching the same type of fact, so your example will not work even if scoping issue was somehow resolved. This also pretty much obviates the need to binding the patterns to declarations, since they are of no use, as you cannot join other facts in.
Second, the ForAll element is actually transformed internally to: not(basePattern and not(pattern1) and not(pattern2) .. ). So, basically, instead of asserting the actual condition, it asserts the absence of facts that violate it. But since not(pattern) cannot export any declarations, any join between the ForAll patterns is impossible.
Does this make sense?
Also, to confirm, this is something that never worked for NRules, as opposed to something that broke in 0.9.0, right?

[a046]

Thanks for looking into this! That all does make sense from an implementation point of view.

With how my facts are modeled, I need to be able to have conditions against joined facts for my Exists/ForAll pattern. I can't think of a nice/efficient work around for doing this if ForAll won't work.

As an example, imagine the Customer and Order facts in the RuleBuilder sample was extended to also have an Item fact. A reasonable (albeit contrived) rule setup might be to find customers where all items (an attribute of Order) are InStock so the Orders can be dispatched.

I doubt it worked before 0.9 from what I have seen in RuleTransformation.cs, I don't think multiple patterns are exposed to the Fluent API anyway?

[snikolayev]

I'm not trying to imply it's not a useful feature, just trying to understand if it's a bug/regression or a new feature. This sounds like a new feature.
With that, I'm trying to also separate figuring out what this feature should look like, from trying to unblock you by giving you a way to achieve what you want with the current implementation.
If Items is a property of an order, one idea (and you can translate that to a RuleBuilder) would be to use LINQ All extension:

When()
    .Match<Customer>(() => customer)
    .Match<Order>(() => order, x => x.Customer == customer, x => x.Items.All(i => i.InStock));

If Items is not a property of an order, but instead required a join, you can use a query:

When()
    .Match<Customer>(() => customer)
    .Match<Order>(() => order, x => x.Customer == customer)
    .Query(() =>itemsInStock, q => q
        .Match<Item>(x => x.Order == order)
        .Collect()
        .Where(x => x.All(i => i.InStock))
    );

Would that work?

[a046]

I agree, it seems like it's a feature as it's different to what the ForAllPattern currently does and as far as I'm aware there has been no regression in this behavior.

I think the second solution using a collection might work, but I don't just have the equivalent of Order -> Item. I have some other facts being joined on to my equivalent of Order which would also require conditions too, so i'm not too sure if I can do that and still only return Customers where all Orders (+ joined objects) match some set of conditions?

I think it should also work by adding multiple queries assuming it works with one, but i'm not sure how the second example above actually works to be honest? I'll have to test it, because i'm not as familiar with the "Query" fluent syntax, but at first glance it looks it would match on Customers where at least one Order is in stock rather than Customers where all Items on all Orders are in stock?

On further thought, I also need to make sure some conditions on all Orders for the customer are met too - so I don't think the solution above can work as adding the conditions to the Match for Order would pre-filter the orders, and adding them in a query will cause the same scoping problem discussed earlier.

[a046]

A possible workaround that I dismissed early on was to make two collection patterns of Orders joined to Customers. One set has conditions + joins applied, the other nothing but the Customer -> Order join.

I could then check that the count of the two collections were equal. Without knowing what the resulting internal representations would look like, i'm concerned about the memory usage/performance of that however, but it might work?

[a046]

Hey Sergiy,

I hope you are doing well!

I noticed there was some refactoring work around this area a little while ago, any thoughts on how complicated this feature would be to implement? Is it on your roadmap, or something you'd like help with?

[snikolayev]

Hello, everything is great, thank you! Hope it's all good on your side too.
To be honest I haven't worked on this feature. The refactoring I committed, and what I'm working on right now is geared towards performance. In particular, I'm making changes to facilitate the optimization of the Rete graph topology (e.g. only re-evaluating expressions when there are changes to facts that the expression depends on, which required me to refactor rule model) and adding real joins to the nodes (as opposed to evaluating conditions on cross-joins, which will require drastic changes to the rete algorithm).
Any help is obviously greatly appreciated. I expect that this feature with ForAllElement will require a healthy amount of back and forth, and pretty invasive changes, likely including the rule model and maybe even the rete level. Happy to discuss more if you are considering to tackle it.

[a046]

Thanks, I'm good too!

Those performance improvements sound pretty good, definitely understand that being the priority.

I'll have to look a bit more into how the quantifiers work internally and everything works then and maybe also revisit the workarounds.