Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evaluate Specification for one Entity #111

Closed
JelmarVA opened this issue Mar 20, 2021 · 14 comments · Fixed by #193
Closed

Evaluate Specification for one Entity #111

JelmarVA opened this issue Mar 20, 2021 · 14 comments · Fixed by #193
Milestone

Comments

@JelmarVA
Copy link

JelmarVA commented Mar 20, 2021

It would be nice to use a specification to make sure an object is still valid.
I've given an example of why you'd want to share this logic in one place.

@fiseni proposed the following:

I was thinking of this, and we might introduce a new type of specification, or perhaps just a specific builder for this purpose. Conceptually, all we need is the entity as a parameter, and bool as a result, right? So, we might hold Func<T, bool> delegates.
We can have something as follows:

public class ValidVerificationCodeSpec : Specification<VerificationCode>
{
    public ValidVerificationCodeSpec()
    {
        Validators.Add(entity =>
        {
            if (entity.DateUsed == null && entity.DateCreated.AddMinutes(30) > DateTime.Now) return true;

            return false;
        }
    }
}

And the specification might expose IsValid method as follows

	
public bool IsValid(T entity)
{
    bool result = true;
    foreach (var validator in specification.validators)
    {
        result = result && validator(entity);
    }

    return result;
}

This is still not refined, but it might go along with this idea.

Let's say we add a Validator builder. This should be translatable to a query on the database right?
Or do you guys see it so they live together next to each other as following?

public class ValidVerificationCodeSpec : Specification<VerificationCode>
{
    public ValidVerificationCodeSpec()
    {
        Query
            .Where(vc => vc.DateUsed == null)
            .Where(vc => vc.DateCreated.AddMinutes(30) > DateTime.Now);

        Validators.Add(entity =>
        {
            if (entity.DateUsed == null && entity.DateCreated.AddMinutes(30) > DateTime.Now) return true;

            return false;
        }
    }
}

That will result in some duplication of the logic.

@fiseni
Copy link
Collaborator

fiseni commented Mar 20, 2021

Let's start by clarifying few concepts and give some context here.

Specifications, as the name suggests, are just constructs that hold some information for a given purpose. The specifications by themselves don't bother how this information will be utilized, but the main focus is just to structure the "knowledge" it contains.
Having said that, we can identify two types of specifications in our case

  • Query specification - It holds information for querying a set/collection of data. The constructed query can be executed against some database, or against some in-memory collection, nevertheless. The important point is that these specifications have to do with some sort of filtering operation.
  • Validation specification - This specification holds information for a particular business rule. By leveraging this information we try to validate a given entity or any construct against those rules. So, this type of specification has to do with validation and should operate against in-memory data that you might have. Its job is not to retrieve data, but to ensure the correctness of the data you already have.

Once we categorize it this way, then we should keep these concerns a bit separate, and allow them to evolve independently. I'd rather have duplicated logic (anyhow, you can keep both in the same spec) than to be constrained to the "cross-section" of these concerns. For an instance, in your example, what if tomorrow you want to add some additional condition for the validation logic, which has nothing to do with the query itself?

Initially, I see two options here.

Option 1

We can simply have a new interface, let's say IValidationSpecification, which defines bool Validate() method. The entity to be validated along with any other inputs that the validation logic might require will be defined as constructor parameters

public interface IValidationSpecification
{
    public bool Validate();
}

public class CompanySpecification : Specification<Company>, IValidationSpecification
{
    private readonly Company company;
    private readonly int someOtherParameter;

    // This will be used for query information
    public CompanySpecification()
    {
        Query.Include(x => x.Stores);
    }

    // This will be used for validation logic
    public CompanySpecification(Company company, int someOtherParameter)
    {
        this.company = company;
        this.someOtherParameter = someOtherParameter;
    }

    public bool Validate()
    {
        // Do some validation logic here against company and someOtherParameter fields.
        return true;
    }
}

The reason I don't like this approach is that it keeps the state in a very verbose way. The specification no longer feels simple, but it gives an impression of some complex construct with states and stuff. The other reason is that this can lead to improper usages. In this case, you can create an instance with the parameterless constructor, and then try to use the Validate method. Nothing prevents you from doing that, right? We can define the entity input as part of the method, like bool Validate(T entity), but still you might require additional conditions, and you'll need a constructor for that.

Option 2

We keep validation logic as a delegate Func<T, bool> Validate. I'm not sure if we need a list, or maybe just one delegate is enough.

public class ValidVerificationCodeSpec : Specification<VerificationCode>
{
    public ValidVerificationCodeSpec()
    {
        Query
            .Where(vc => vc.DateUsed == null)
            .Where(vc => vc.DateCreated.AddMinutes(30) > DateTime.Now);
    }
	
    public ValidVerificationCodeSpec(int minutes)
    {
        Validator(entity =>
        {
            if (entity.DateUsed == null && entity.DateCreated.AddMinutes(minutes) > DateTime.Now) return true;

            return false;
        };
    }
}

The base specification will expose IsValid(T entity) method as follows

public bool IsValid(T entity)
{
	if (Validate== null) return false;
	
	return Validate(entity);
}

If we have not defined a validator, it should always return false or throw an exception. So, in case we instantiate it with the parameterless ctor ValidVerificationCodeSpec, it will never return true. Maybe throwing an exception is a better way.

Note I still don't like either of these approaches. It feels clumsy somehow :)

@fiseni
Copy link
Collaborator

fiseni commented Mar 20, 2021

As for your original idea of utilizing the Evaluate feature for this purpose, you can easily do that even today.

All you need is to create your own base specification as follows. Then, you use it as a base for all your specs in the application.

public class AppSpecification<T> : Specification<T>
{
    public bool Evaluate(T source)
    {
        return base.Evaluate(new[] { source }).Any();
    }
}

@JelmarVA
Copy link
Author

JelmarVA commented Mar 21, 2021

If we have two types of specifications in our case (Query specification, Validation specification) maybe we can introduce these.

QuerySpecification

It holds information for querying a set/collection of data.

public class ValidVerificationCodeSpec: QuerySpecification<VerificationCode>
{
    public ValidVerificationCodeSpec()
    {
        Query
            .Where(vc => vc.DateUsed == null)
            .Where(vc => vc.DateCreated.AddMinutes(30) > DateTime.Now);
    }
}

ValidationSpecification

This specification holds information for a particular business rule. By leveraging this information we try to validate a given entity or any construct against those rules.

public class ValidVerificationCodeSpec: ValidationSpecification<VerificationCode>
{
    public ValidVerificationCodeSpec()
    {
         Validate
            .That(vc => vc.DateUsed == null)
            .And(vc => vc.DateCreated.AddMinutes(30) > DateTime.Now);
    }
}

The ValidationSpecification has the protected builder Validate so we can use it in the constructor to show our intentions.
The ValidationSpecification exposes an IsValid(T entity) method to check for the validity of an entity.
If the user uses the IsValid method and there are no rules defined it should throw an exception because it doesn't make sense to have a ValidationSpecification without any rules.

The last type of specification could be some super type as lack of better wording.

SuperSpecification

This specification holds information for querying a set/collection of data and has the rules defined for validating the entity.

public class ValidVerificationCodeSpec: Specification<VerificationCode>
    {
        private Expression<Func<VerificationCode, bool>> itIsNotUsed = code => code.DateUsed == null;
        private Expression<Func<VerificationCode, bool>> itIsStillValid = code => code.DateCreated.AddMinutes(30) > DateTime.Now;

        public ValidVerificationCodeSpec()
        {
            Query
                .Where(itIsNotUsed)
                .Where(itIsStillValid);

            Validate
                .That(itIsNotUsed)
                .And(itIsStillValid);
        }
    }

Also here will an IsValid method be exposed that throws an error if it is used when there are nog validation rules defined.

The user can leverage this type to reuse the logic used in the filtering as in the validation and is still able to have different validation rules than filtering rules. Also if they want to have the validation and querying logic in the same specification they could use this type.

I still have mixed feelings about the super type Specification because we are having the querying and validation logic in the same place.

@fiseni
Copy link
Collaborator

fiseni commented Mar 22, 2021

These are valid approaches. But, implementation wise can get tricky. I'll share my concerns here

Separate specifications

What I meant by keeping these concerns independent is to just use different builders (or methods, properties, whatever). It's ok to have separate specification types as long as we keep them completely separate. But, if we define those separate specs and on top of it the "SuperSpecification" too, then the implementation and the maintenance can be a headache. The "SuperSpecification" can inherit either one or another, so we'll have to duplicate the implementation for the other one. There is already some level of complexity just because we have Specification<T, TResult>, and introducing additional complex inheritance chains will just create a mess. Also, we'd like to make the infrastructure extensible. Even at this point, extending it is a bit tricky (just because of the Select feature), so if we complicate even further we can forget of extensibility :)
I vote for having just one specification type, "SuperSpecification" in your case. We'll keep Query as it is, and just introduce a new mechanism to add validation logic.

Validation builder

Your intention here is great. But, trust me, it's a pain to implement that :) You'll start with And, then you will need Or, Andnot, OrNot, Not, etc, then you will define additional sub-builders to group them. It's a rabbit hole. Historically, we did all that in order to implement "composite specifications", which is not our requirement at all here. I personally dislike them and did not have great experience in the past. If you need a combination of several specs, then just write a new one, it makes stuff much cleaner. So, since we're not doing composition, we really have no reason to introduce some complex builder mechanisms. Let the users write whatever they want, we just need the bool as a result, right?

It's important to emphasize that the success and the adoption of any tool is inversely proportional with the learning curve that the tool requires. That's the reason we introduced Query and made the syntax Linq alike, not to introduce new concepts and rely on what users already know. It's the same with the validation logic, we should keep things familiar. In order to make it more flexible, we can start with a simple builder Validator which returns IValidationBuilder, and write an extension method Define which accepts Func<T, bool>. In the future, if we change our minds, we'll just add new extension methods.

So, it would be something like

public class ValidVerificationCodeSpec : Specification<VerificationCode>
{
    public ValidVerificationCodeSpec()
    {
        Query
            .Where(vc => vc.DateUsed == null)
            .Where(vc => vc.DateCreated.AddMinutes(30) > DateTime.Now);
    }
    
    public ValidVerificationCodeSpec(int minutes)
    {
        Validator.Define(entity =>
        {
            if (entity.DateUsed == null && entity.DateCreated.AddMinutes(minutes) > DateTime.Now) return true;

            return false;
        };
    }
}

Of course, users can organize their specs however they want, and based on your example they can write the above spec same as follows

public class ValidVerificationCodeSpec: Specification<VerificationCode>
{
    private Expression<Func<VerificationCode, bool>> itIsNotUsed = code => code.DateUsed == null;
    private Expression<Func<VerificationCode, int, bool>> itIsStillValid = (code, minutes) => code.DateCreated.AddMinutes(minutes) > DateTime.Now;

    public ValidVerificationCodeSpec()
    {
        Query
            .Where(itIsNotUsed)
            .Where(itIsStillValid);
    }
    
    public ValidVerificationCodeSpec(int minutes)
    {
        Validator.Define(entity =>
        {
            if (itIsNotUsed(entity).Compile() 
             && itIsStillValid(entity, minutes).Compile()) 
                  return true;

            return false;
        };
    }
}

@fiseni
Copy link
Collaborator

fiseni commented Mar 23, 2021

Hey @JelmarVA ,
I just created a validations branch. If you wanna work on this, you can PR on that branch. If you agree, we can start only with the Validator builder and the Define extension. Then we'll see how it goes.

@ardalis any thoughts about this thread?

@ardalis
Copy link
Owner

ardalis commented Mar 24, 2021

I've just caught up on this thread and have an alternative design to propose for consideration, though I'm not sure how to structure it. Based on the initial requirement of simply allowing a simpler way to check whether an individual element is returned as by a specification, could we not create a wrapper around a specification that would provide this functionality? It's late and I don't have much time to commit to this but if I were custom coding it myself I could certainly write something like this:

public class Customer
{
  public int OrdersPlaced { get; set; }
}

public class CustomerWithOrderCountSpec : Specification<Customer>
{
  public CustomerWithOrderCountSpec(int minOrderCount)
  {
    Query.Where(c => c.OrdersPlaced >= minOrderCount);
  }
}

public class CustomerValidator
{
  public CustomerValidator(CustomerWithOrderSpec spec)
  {
    _spec = spec;
  }
  private CustomerWithOrderSpec _spec;

  public bool IsValid(Customer customer)
  {
    return _spec.Evaluate(new[] { customer}).Any();
  }
}

Now the trick to make this part of the framework would be to give CustomerValidator a constraint of some sort that made its design enforce the implicit rules given here, mainly that you pass a valid spec into it via its constructor.

Another option would be an extension method that would do the same work.

Both of these seem cleaner than adding a whole new concept of validators into the base spec class itself, to me. I don't know that too many people will use this feature instead of just using a more full-featured library like FluentValidation.

Thoughts?

@fiseni
Copy link
Collaborator

fiseni commented Mar 24, 2021

Actually, that was the original request by @JelmarVA. In one of my answers, I provided the following.

As for your original idea of utilizing the Evaluate feature for this purpose, you can easily do that even today.

All you need is to create your own base specification as follows. Then, you use it as a base for all your specs in the application.

public class AppSpecification<T> : Specification<T>
{
    public bool Evaluate(T source)
    {
        return base.Evaluate(new[] { source }).Any();
    }
}

You can name it IsValid, and then use it like this (you don't even need a separate validator class, right?)

new CustomerSpec().IsValid(customer);

The arguments against implementing this as part of the package are:

  • The evaluator will evaluate also Take, Skip, Order, and you might get inconsistent results. It's a subtle issue and it's hard to spot it. So, you'll end up with some implicit rules "Ok, if this spec will be used for validation, then you should remember to use only Where".
  • Validations are in-memory actions. They operate against some in-memory constructs and can contain complex rules, right? But, in this case, you'll have to be careful what you're writing in Query.Where, since if you write something complex the ORM won't be able to evaluate it. So, either you'll restrict yourself to only simple rules, or you'll have another implicit rule "Ok, you can write complex rules here, but you should remember not to use this spec with your repository".
  • I guess conceptually is wrong. Where is a filtering operation against collections. We can use it, but it's just a hack, right? Do we wanna embed it in the package?

To summarize, it's OK approach as long you are aware of these caveats. So, users can adopt it in their codebases if they want (simple base class as shown above). But, I'm not so sure of including it as part of the package. It will confuse and create ambiguity for the rest 90% of the users.

EDIT: As you suggested, an extension method can be even better than a base class.

public static bool IsValid<T>(this Specification<T> spec, T entity)
{
    return spec.Evaluate(new[] { entity }).Any();
}

@JelmarVA
Copy link
Author

I get @fiseni's arguments against implementing it in the package. For me, it will be clear because I introduced this hack and know the caveats. For other users (and colleagues), it would create ambiguity indeed.

If I want to remove the ambiguity in my own solutions about the validation I can introduce the FluentValidation library.
I can have an abstract class ValidationSpecification

 public abstract class ValidationSpecification<T> : IValidationSpecification<T>
{
    public Specification<T> Specification { get; internal set; }
    public AbstractValidator<T> Validator { get; internal set; }

    protected ValidationSpecification(Specification<T> specification, AbstractValidator<T> validator)
    {
        Specification = specification;
        Validator = validator;
    }

    public ValidationResult Validate(T entity)
    {
        return Validator.Validate(new ValidationContext<T>(entity));
    }

    public IEnumerable<T> Evaluate(IEnumerable<T> entities)
    {
        return Specification.Evaluate(entities);
    }
}

With the ValidationSpecification class, I can hold both a complex validation and the Specification I want to use against the ORM.
This way I can leverage both powerful libraries.

public class ValidVerificationCodeValidationSpec : ValidationSpecification<VerificationCode>
{
    private static int minutes = 30;
    public ValidVerificationCodeValidationSpec() :
        base(
            new ValidVerificationCodeSpec(minutes),
            new ValidVerificationCodeValidator(minutes)
            )
    { }
}

Let me know how you guys feel about this design.

I don't know if this should be implemented in the package because it would become dependant on the FluentValidator library. But it can be a nonissue.

@fiseni
Copy link
Collaborator

fiseni commented Mar 25, 2021

I have no strong opinion on this. Honestly, I don't see myself using this feature very much. Usually, I use different constructs for validations, I need error messages, localizations, etc. So, whatever the community likes 😀
Actually, we can publish all this as a separate addon package Ardalis.Specifications.Validations. So whoever wants it, can pick it up explicitly.
@ardalis has the last call, I'm OK either way.

@ardalis
Copy link
Owner

ardalis commented Mar 26, 2021

I'd be fine with having a separate package as @fiseni suggests, which could simply hold the types shown in @JelmarVA 's last comment. It can live in this repo but have a separate nuget package and build pipeline.

@pauldbentley
Copy link

While I don't use this for validation, I do have a similar use-case for evaluating a specification in memory for a single entity.

I have an extension method similar to what was suggested:

public static bool IsSatisfiedBy<T>(this ISpecification<T> specification, T entity) =>
    specification.Evaluate(new[] { entity }).Any();

So I can do things like:

var spec = new PremiumCustomerSpecification();

if (spec.IsSatisfiedBy(customer))
{
    ShowOfferOnlyPremiumCustomersGet();
}

I'm not sure if I'm doing something completely wrong here but just wanted to add this to the discussion.

@fiseni
Copy link
Collaborator

fiseni commented Sep 10, 2021

Nothing wrong here. This is a perfectly valid approach. 👍

@fiseni
Copy link
Collaborator

fiseni commented Dec 15, 2021

We're getting this request quite often in one form or another. So, let's try to implement it. But, instead of utilizing the above-mentioned hack, we should try to do it properly. We can use the predicates directly, and not apply it through LINQ Where at all.

The API should be simple, you provide the entity and get back information whether it satisfies the specification. We can discuss the name, it can be named "IsSatisfiedBy" or "IsValid", or anything else.

public interface ISpecification<T>
{
	...
	
	bool IsSatisfiedBy(T entity);
}

As discussed in several threads, our specifications primarily are intended for query generation and don't hold only criteria information. So, during this validation, we have to account only for the state which represents a "criteria" (Where and Search expressions in our case). We already have something similar for the specification evaluator (used for the CountAsync action), and we can extend the same idea for the in-memory evaluators too. To recap, we have two types of evaluators

  • ISpecificationEvaluator which iterates through a list of partial IEvaluator evaluators, and is intended to be used for query generation through various ORMs.
  • IInMemorySpecificationEvaluator which iterates through a list of partial IInMemoryEvaluator evaluators, which evaluates the provided in-memory collections.
public interface ISpecificationEvaluator
{
    IQueryable<TResult> GetQuery<T, TResult>(IQueryable<T> inputQuery, ISpecification<T, TResult> specification) where T : class;
    IQueryable<T> GetQuery<T>(IQueryable<T> inputQuery, ISpecification<T> specification, bool evaluateCriteriaOnly = false) where T : class;
}
public interface IEvaluator
{
    bool IsCriteriaEvaluator { get; }

    IQueryable<T> GetQuery<T>(IQueryable<T> query, ISpecification<T> specification) where T : class;
}

public interface IInMemorySpecificationEvaluator
{
    IEnumerable<TResult> Evaluate<T, TResult>(IEnumerable<T> source, ISpecification<T, TResult> specification);
    IEnumerable<T> Evaluate<T>(IEnumerable<T> source, ISpecification<T> specification);
}
public interface IInMemoryEvaluator
{
    IEnumerable<T> Evaluate<T>(IEnumerable<T> query, ISpecification<T> specification);
}

The idea is to extend the IInMemorySpecificationEvaluator as follows. The Evaluate method will iterate and check against all partial evaluators which are defined as criteria evaluators.

public interface IInMemorySpecificationEvaluator
{
    IEnumerable<TResult> Evaluate<T, TResult>(IEnumerable<T> source, ISpecification<T, TResult> specification);
    IEnumerable<T> Evaluate<T>(IEnumerable<T> source, ISpecification<T> specification);
    bool Evaluate<T>(T entity, ISpecification<T> specification);
}
public interface IInMemoryEvaluator
{
    bool IsCriteriaEvaluator { get; }
    IEnumerable<T> Evaluate<T>(IEnumerable<T> query, ISpecification<T> specification);
    bool Evaluate<T>(T entity, ISpecification<T> specification);
}

Then, the IsSatisfiedBy implementation in the specification becomes quite trivial

public virtual bool IsSatisfiedBy(T entity)
{
    return Evaluator.Evaluate(entity, this);
}

And the implementation for InMemorySpecificationEvaluator.Evaluate()

public virtual bool Evaluate<T>(T entity, ISpecification<T> specification)
{
    foreach (var partialEvaluator in evaluators.Where(x=>x.IsCriteriaEvaluator))
    {
        if (partialEvaluator.Evaluate(entity, specification) == false) return false;
    }

    return true;
}

@fiseni fiseni added this to the 6.0 milestone Dec 19, 2021
@fiseni
Copy link
Collaborator

fiseni commented Dec 22, 2021

This is implemented a bit differently. Utilizing and extending the IInMemoryEvaluator won't be nice, since we will force the omitted evaluators to implement bool Evaluate<T>(T entity, ISpecification<T> specification);. Instead, we'll use a separate infrastructure for this feature. We'll have ISpecificationValidator construct which will iterate through a list of partial IValidator validators. Thus, we'll be able to define only the required validators, in this case for Where and Search. More details in the related PR.

@JelmarVA This is exactly what you wanted. You can use the same specification without the need to duplicate your expressions. The same spec can be utilized for query generation, in-memory evaluation, and also for entity validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants