Add streamlined way for building conditions

jdromeiro commented 1 year ago

Description

Introduces a new way of more fluently building and adding rule conditions:

for a value condition


.WithCondition(rc => rc.Value(ConditionType.ClientType, Operators.Equal, "Premium"))

// or (for a bit more sugar) .WithCondition(ConditionType.ClientType, Operators.Equal, "Premium")


* for a composed condition
```csharp
.WithCondition(rc => rc
    .And(a => a
        .Value(ConditionType.ClientType, Operators.Equal, "Queen")
        .Value(ConditionType.Country, Operators.In, new[] { "Portugal", "Spain" })
        .Or(o => o
            .Value(ConditionType.Country, Operators.Equal, "Brazil")
            .Value(ConditionType.Country, Operators.NotEqual, "United States")
        )
    ))

It also adds a new contructor for the Condition class.

Closes #138 .

Change checklist

[ ] Code follows the code rules guidelines of this project
[ ] Commit messages follow the commit rules of this project
[ ] I have self-reviewed my changes before submitting this pull request
[ ] I have covered new/changed code with new tests and/or adjusted existent ones
[ ] I have made changes necessary to update the documentation accordingly

Please also check the I want to contribute guidelines and make sure you have done accordingly.

Disclaimer

By sending us your contributions, you are agreeing that your contribution is made subject to the terms of our Contributor Ownership Statement

jdromeiro commented 1 year ago

@luispfgarces can you please let me know if you agree with the modifications before changing all the tests and samples to use this streamlined way?

luispfgarces commented 1 year ago

Hi @jdromeiro,

I like your approach, but if you'll allow me, let me suggest taking this even further. I have created a small graph representing the calls to create a rule's conditions.

rule-builder-fluent-api

Basically, it only changes the way we start defining conditions (the root condition) and the composed nodes (and/or), the rest is just naming. Let's get one example:

For the current way of defining the rule.

RuleBuilder.NewRule<ContentTypes, ConditionTypes>()
    .WithName("Benchmark 2 - Bohemian Rapsody")
    .WithDateBegin(DateTime.Parse("2000-01-01"))
    .WithContent(ContentTypes.Songs, "Bohemian Rapsody")
    .WithCondition(x =>
    {
        return x.AsComposed()
            .WithLogicalOperator(LogicalOperators.And)
            .AddCondition(c =>
                c.AsValued(ConditionTypes.Artist)
                    .OfDataType<string>()
                    .WithComparisonOperator(Operators.Equal)
                    .SetOperand("Queen")
                    .Build())
            .AddCondition(c =>
                c.AsValued(ConditionTypes.Lyrics)
                    .OfDataType<string>()
                    .WithComparisonOperator(Operators.Contains)
                    .SetOperand("real life")
                    .Build())
            .AddCondition(c =>
                c.AsValued(ConditionTypes.ReleaseYear)
                    .OfDataType<int>()
                    .WithComparisonOperator(Operators.GreaterThanOrEqual)
                    .SetOperand(1973)
                    .Build())
            .AddCondition(c =>
                c.AsValued(ConditionTypes.ReleaseYear)
                    .OfDataType<int>()
                    .WithComparisonOperator(Operators.LesserThanOrEqual)
                    .SetOperand(1977)
                    .Build())
            .Build();
    })
    .Build();

Now with the mash-up of our suggestions.

RuleBuilder.NewRule<ContentTypes, ConditionTypes>()
    .WithName("Benchmark 2 - Bohemian Rapsody")
    .WithDateBegin(DateTime.Parse("2000-01-01"))
    .WithContent(ContentTypes.Songs, "Bohemian Rapsody")
    .WithCondition(x => 
        x.And(c => c.Value(ConditionTypes.Artist, Operators.Equal, "Queen")
            .Value(ConditionTypes.Lyrics, Operators.Contains, "real life")
            .Value(ConditionTypes.ReleaseYear, Operators.GreaterThanOrEqual, 1973)
            .Value(ConditionTypes.ReleaseYear, Operators.LesserThanOrEqual, 1977)
        )
    )
    .Build();

What are your thoughts on this? Give it a try first if you want to find any flaws in my suggestion, as I have not tested it 🙂

jdromeiro commented 1 year ago

Hello @luispfgarces ! Thanks for your suggestion, I agree it improves the solution. 👌🏼 Considering your snippet, I've managed to achieve the following behaviour:

for cases with a single rule

.WithCondition(BasicConditionType.ClientType, Operators.Equal, "Premium")

for cases with a composed rule

.WithConditions(LogicalOperators.And, x => x
              .Value(BasicConditionType.ClientType, Operators.Equal, "Queen")
              .Value(BasicConditionType.Country, Operators.In, new[] { "Portugal", "Spain" })
              .Or(y => y
                  .Value(BasicConditionType.Country, Operators.Equal, "Brazil")
                  .Value(BasicConditionType.Country, Operators.NotEqual, "United States")
              )
           )

Given that the method in the RuleBuilder (or RuleBuilderExtension) should only accept one composedCondition, didn't manage to make it exactly how you had describe it (because we should only have one LogicalOperator, which in the proposal is being passed as parameter).

I've pushed the new code. Please let me know what you think.

luispfgarces commented 1 year ago

Hi @jdromeiro,

Thank you for taking my input into consideration 👌 if you are interested in resolving that last bit you couldn't resolve, I may have a possible solution for you. The thing is, with the current builders you can't ensure a single "root condition" because on fluent APIs you have to design your interfaces and classes according to the possible state machine to be accomplished. That's why I told you once that fluent APIs require planning representing a state machine on a graph using a tool you'd like (e.g. draw.io) and lots of trial and error sometimes 😄

Here goes my even further improved suggestion:

Create interfaces IRootConditionNodeBuilder<TConditionType> and IConfiguredRootConditionNodeBuilder<TConditionType>.

public interface IRootConditionNodeBuilder<TConditionType>
{
    IConfiguredRootConditionNodeBuilder<TConditionType> And(Func<IComposedConditionNodeBuilder<TConditionType>, IComposedConditionNodeBuilder<TConditionType>> conditionFunc);
    IConfiguredRootConditionNodeBuilder<TConditionType> Or(Func<IComposedConditionNodeBuilder<TConditionType>, IComposedConditionNodeBuilder<TConditionType>> conditionFunc);
    IConfiguredRootConditionNodeBuilder<TConditionType> Value<TDataType>(TConditionType conditionType, Operators condOperator, TDataType operand);
}

public interface IConfiguredRootConditionNodeBuilder<TConditionType>
{
    IConditionNode<TConditionType> GetRootCondition();
}

Create class RootConditionNodeBuilder<TConditionType> that implements both interfaces.

internal class RootConditionNodeBuilder<TConditionType> : IRootConditionNodeBuilder<TConditionType>, IConfiguredRootConditionNodeBuilder<TConditionType>
{
    private IConditionNode<TConditionType> rootCondition;

    public IConfiguredRootConditionNodeBuilder<TConditionType> And(Func<IComposedConditionNodeBuilder<TConditionType>, IComposedConditionNodeBuilder<TConditionType>> conditionFunc)
    {
        // Do your already implemented logic. Assign result to rootCondition.

        return this;
    }

    public IConfiguredRootConditionNodeBuilder<TConditionType> Or(Func<IComposedConditionNodeBuilder<TConditionType>, IComposedConditionNodeBuilder<TConditionType>> conditionFunc)
    {
        // Do your already implemented logic. Assign result to rootCondition.

        return this;
    }
    public IConfiguredRootConditionNodeBuilder<TConditionType> Value<TDataType>(TConditionType conditionType, Operators condOperator, TDataType operand)
    {
        // Do your already implemented logic. Assign result to rootCondition.

        return this;
    }

    public IConditionNode<TConditionType> GetRootCondition()
        => this.rootCondition;
}

This way you only need one extension method on the RuleBuilderExtensions.

public static IRuleBuilder<TContentType, TConditionType> WithCondition<TContentType, TConditionType>(this IRuleBuilder<TContentType, TConditionType> ruleBuilder, Func<IRootConditionNodeBuilder<TConditionType>, IConfiguredRootConditionNodeBuilder<TConditionType>> conditionFunc)

With this, you could accomplish the suggestion I gave you, which has the advantage of defining conditions always the same way (from the consumer's perspective). Maybe this is a bit of nitpicking on my part, but it makes sense to me to offer the best experience possible to the developers that use the fluent API 😄 what do you think?

jdromeiro commented 1 year ago

@luispfgarces , thank you for your suggestions! 👌🏼

I've taken your snippets and adjusted them a bit. I've also created a new entity IChildComposedConditionNodeBuilder (for which the logic could be kept as the IComposedConditionNodeBuilder) because I thought would be easier to separate the old way from the new way of creating the conditions.

If you agree, we can set the old methods as obsolete, and in the future remove them or make them internal.

Let me know of your thoughts regarding the solution as it is now.

jdromeiro commented 1 year ago

Sorry @luispfgarces , decided to also replace all calls to WithContentContainer(). One more 👍🏼 please 😅

Farfetch / rules-framework