ardalis / Specification

Base class with tests for adding specifications to a DDD model
MIT License
1.95k stars 245 forks source link

How to combine specifications? #379

Closed LiptonDev closed 11 months ago

LiptonDev commented 12 months ago

Hello. How to combine 2 or more specification in 1 specification? I just don't know what to do with my specs if I want to reuse existing specs like:

I want use this like:

UserSpec.FindById(1) && UserSpec.NotBlocked && UserSpec.HasRole("admin")

Where UserSpec.HasRole is:

public class UserHasRoleSpecification : Specification<ApplicationUser>
{
    public UserHasRoleSpecification(string role)
    {
        Query.Where(x => x.Roles.Contains(role)); //roles - other table in db
    }
}

How to combine these specs into one or this library doesn't know how to do that?

p.s. - sorry for my English

jvmlet commented 11 months ago

I second this question

ardalis commented 11 months ago

We don't recommend or support combining specifications, because doing so moves query logic out of the domain model and into the consumer (typically UI) layer. Instead, if you have predicates you want to share, these can be shared between Specifications by simply creating (static) properties of type Func<T,bool> and then using these within your well-named specifications.

One (other) reason we don't support arbitrarily combining specs is that they often include more than just a predicate. So if you have a specification that includes OrderBy, and a second one that orders by something else, which one would "win"? The right approach is to just extract the predicate logic (if needed) and reuse that between specifications but within your domain model.

Here's a quick example:

internal static class CustomerPredicates
{
    public static Func<Customer, bool> IsAdult => customer => customer.Age >= 18;
    public static Func<Customer, int, bool> IsAtLeastYearsOld => (customer, years) => customer.Age >= years;
    public static Func<Customer, string, bool> NameIncludes => 
        (customer, nameString) => customer.Name.Contains(nameString, StringComparison.CurrentCultureIgnoreCase);
}

using Ardalis.Specification;

namespace Ardalis.Sample.Domain.Specs;

public class AdultCustomersByNameSpec : Specification<Customer>
{
    public AdultCustomersByNameSpec(string nameSubstring)
    {
        Query.Where(c => CustomerPredicates.IsAdult(c) &&
             CustomerPredicates.NameIncludes(c, nameSubstring));
    }
}

public class Customer : IAggregateRoot
{
    public int Id { get; set; }
    public required string Name { get; set; }
    public required int Age { get; set; }

    public List<Address> Addresses { get; set; } = new();
}

And of course if you preferred you could use extension methods instead of predicates which might be more concise.

jvmlet commented 11 months ago

Thanks for the answer, but I'm not sure how combing predicates moves logic out from domain, can we at least combine the predicates (Query) from 2 specs?

new Spec1().
WithQueryFrom(new Spec2()).
WithQueryFrom(new Spec3())
ardalis commented 11 months ago

Yes, see my updated comment above.

jvmlet commented 11 months ago

I meant to suggest to implement the WithQueryFrom method in this library (part of ISpecificationinterface)

ardalis commented 11 months ago

Right but that would still encourage client code to compose data access logic from some arbitrary combination of specifications, possibly involving AND, OR, and NOT. At which point you might end up with arbitrarily complex logic that isn't in the domain model but is instead in the UI layer where it doesn't belong.

If you need Spec1 with filters from Spec2 and Spec3 then my recommendation is to move those filters to predicates and then reference those predicates from any/all Specifications that need them.

We can't necessarily just say UseQueryFrom because it could include Skip, Take, OrderBy and other elements that don't combine, in addition to the aforementioned issue with not keeping query logic encapsulated in the domain model in specific specifications.

Hope that makes sense and even if you don't agree, you can see where we're coming from.

fiseni commented 11 months ago

Hi @LiptonDev, @jvmlet

I've elaborated on this topic on many occasions. Please refer to this answer, there are a few suggestions on how to handle this type of scenario. If we come up with a consistent API and an idea of how to implement all that in a non-ambiguous way then maybe in the future?! But, we do not recommend that approach. Instead, we tried to make the infrastructure as extensible as possible. So, you can easily write specification builder extensions and then compose functionalities in the specifications.

The example above with predicates unfortunately won't work if you plan to use the specification to retrieve data from DB. The expression itself will contain a "Method Call" node, and that will confuse and break EF. The canonical way of doing it in this library is as follows:

public static class CustomerSpecExtensions
{
    public static ISpecificationBuilder<Customer> IsAdult(this ISpecificationBuilder<Customer> builder)
        => builder.Where(x => x.Age >= 18);

    public static ISpecificationBuilder<Customer> IsAtLeastYearsOld(this ISpecificationBuilder<Customer> builder, int years)
        => builder.Where(x => x.Age >= years);
}

public class AdultCustomersByNameSpec : Specification<Customer>
{
    public AdultCustomersByNameSpec(string nameSubstring)
    {
        Query.IsAdult()
            .Where(x => x.Name.Contains(nameSubstring));
    }
}

PS. If you don't like all of this, you can still create your own composite specifications. The current infrastructure allows you to do that. In the recent versions, we exposed the builder publicly, so you can keep building the spec after its creation. It was done to allow consumers for through customizations. Yes, you'll have to deal with expression building and all of that stuff, but it's possible if you're keen to have composite specs.

jvmlet commented 11 months ago

I probably did not express myself properly, by query I meant where clause only, which is combined by AND. From client perspective, the SearchCriteria UI component might have area 2 that powers Spec2, area 3 that powers Spec3 that might be used independently, and global area that combines both UI components. *Your last comment probability answers what I'm after

fiseni commented 11 months ago

Having extensions to the specification builder will be a good fit for that. We have more examples under "samples" folder in the repository. We tried to cover multiple scenarios in those examples. Let us know if you need further assistance.

jvmlet commented 11 months ago

Please criticize :

  public static ISpecification<T> WithFiltersOf<T>(
        this ISpecification<T> spec, ISpecification<T> filtersSpec) {
        var targetWhereExpressions = (List<WhereExpressionInfo<T>>) spec.WhereExpressions;

        foreach (var whereExpressionInfo in filtersSpec.WhereExpressions) {
            targetWhereExpressions.Add(whereExpressionInfo);
        }

        var targetSearchExpressions = (List<SearchExpressionInfo<T>>) spec.SearchCriterias;

        foreach (var searchExpressionInfo in filtersSpec.SearchCriterias) {
            targetSearchExpressions.Add(searchExpressionInfo);
        }

        return spec;
    }
fiseni commented 11 months ago

Hey @jvmlet,

Yes, that's the idea. But, as I mentioned, we have exposed the builder publicly, so you don't have to deal with internals and you can write it more cleanly.

public static class SpecExtensions
{
    public static ISpecification<T> AddFiltersFrom<T>(
          this ISpecification<T> spec, ISpecification<T> otherSpec) where T : class
    {
        foreach (var whereExpressions in otherSpec.WhereExpressions)
        {
            spec.Query.Where(whereExpressions.Filter);
        }

        foreach (var searchExpressions in otherSpec.SearchCriterias)
        {
            spec.Query.Search(searchExpressions.Selector, searchExpressions.SearchTerm, searchExpressions.SearchGroup);
        }
        return spec;
    }
}

The usage will be as follows. I also changed the method name to AddFiltersFrom to indicate that the original specification will be updated.

var spec1 = new Spec1();
var spec2 = new Spec2();

spec1.AddFiltersFrom(spec2);

So, as shown above, it's trivial to implement such composition (at least combining, e.g., AND logic). We do not necessarily want this out of the box, since we do believe it will create ambiguity and a lot of implicit conventions. But, we won't prevent you from doing it by yourself and will provide the necessary mechanisms. I think it's a fair compromise.

PS. Bear in mind that all search expressions within the same group are OR'ed.

jvmlet commented 11 months ago

Thanks a lot for your support, @fiseni, very much appreciated.

fiseni commented 11 months ago

Hi @LiptonDev,

The example you provided UserSpec.FindById(1) && UserSpec.NotBlocked && UserSpec.HasRole("admin") doesn't make much sense to me. This is not about combining/composing specs, but you want some arbitrary behavior on top of a given spec.

Please check the answers on this thread, and let us know if you have further questions.

jvmlet commented 11 months ago

@fiseni , one more question. What would be the right way to combine specifications with OR ?

spec A : field1.Equals(1) and field2.Equals(3) spec B : field3.Equals(3) and field4.Equals(4)

The resulting spec should be ( field1.Equals(1) and field2.Equals(3)) OR (field3.Equals(3) and field4.Equals(4))

LiptonDev commented 11 months ago

@fiseni Hello. Okay, I'm testing this. but I think that this will not suit me, most likely. Thanks for answers.

fiseni commented 11 months ago

@jvmlet Well, that's the issue with composite specifications. You start with And, then you need Or, then Not, etc. As I stated in this response, it defies the purpose. Back in the day, it was appealing since we didn't have good expressiveness in languages.

If you start combining the specs all over your code, how is that different than just simply using LINQ all over the place? Not saying that using LINQ everywhere is bad per se, it's your choice. But, probably you started to use specs to avoid doing that in your code. So, it defies the purpose somehow.

Anyhow, here is an example implementation for Or. You can run the example and inspect the content of the newSpec.

using Ardalis.Specification;
using System.Linq.Expressions;
using System.Reflection.Metadata;

var spec1 = new CustomerSpec1();
var spec2 = new CustomerSpec2();

var newSpec = spec1.Or(spec2);

Console.WriteLine();

class CustomerSpec1 : Specification<Customer>
{
    public CustomerSpec1()
    {
        Query.Where(y => y.Field1 == 1)
            .Where(x => x.Field2 == 2);
    }
}

class CustomerSpec2 : Specification<Customer>
{
    public CustomerSpec2()
    {
        Query.Where(y => y.Field3 == 3)
            .Where(x => x.Field4 == 4);
    }
}

class EmptySpecification<T> : Specification<T> { }

class Customer
{
    public int Field1 { get; set; }
    public int Field2 { get; set; }
    public int Field3 { get; set; }
    public int Field4 { get; set; }
}

public static class SpecificationExtensions
{
    public static ISpecification<T> Or<T>(this ISpecification<T> spec, ISpecification<T> otherSpec)
    {
        var newSpec = new EmptySpecification<T>();

        var parameter = Expression.Parameter(typeof(T), "x");

        var exprSpec1 = CombineWhereExpressions(spec.WhereExpressions, parameter);
        var exprSpec2 = CombineWhereExpressions(otherSpec.WhereExpressions, parameter);

        Expression? orExpression = exprSpec1 is not null && exprSpec2 is not null
            ? Expression.OrElse(exprSpec1, exprSpec2)
            : exprSpec1 ?? exprSpec2;

        if (orExpression is null) return newSpec;

        var lambdaExpr = Expression.Lambda<Func<T, bool>>(orExpression, parameter);

        newSpec.Query.Where(lambdaExpr);

        return newSpec;
    }

    public static Expression? CombineWhereExpressions<T>(IEnumerable<WhereExpressionInfo<T>> whereExpressions, ParameterExpression parameter)
    {
        Expression? newExpr = null;
        foreach (var where in whereExpressions)
        {
            var expr = ParameterReplacerVisitor.Replace(where.Filter.Body, where.Filter.Parameters[0], parameter);
            newExpr = newExpr is null ? expr : Expression.AndAlso(newExpr, expr);
        }
        return newExpr;
    }
}

public class ParameterReplacerVisitor : ExpressionVisitor
{
    private readonly Expression _newExpression;
    private readonly ParameterExpression _oldParameter;

    private ParameterReplacerVisitor(ParameterExpression oldParameter, Expression newExpression)
    {
        _oldParameter = oldParameter;
        _newExpression = newExpression;
    }

    internal static Expression Replace(Expression expression, ParameterExpression oldParameter, Expression newExpression)
      => new ParameterReplacerVisitor(oldParameter, newExpression).Visit(expression);

    protected override Expression VisitParameter(ParameterExpression p)
      => p == _oldParameter ? _newExpression : p;
}

The resulting specification will contain one WhereExpression with the following content

Filter = {x => (((x.Field1 == 1) AndAlso (x.Field2 == 2)) OrElse ((x.Field3 == 3) AndAlso (x.Field4 == 4)))}

@LiptonDev Closing the issue since we don't fully understand your requirements here. Feel free to reopen the issue if you have further clarifications.

jvmlet commented 11 months ago

Thanks @fiseni, I'll try to implement the Specification over https://github.com/scottksmith95/LINQKit?tab=readme-ov-file#combining-expressions...