ISingleResultSpecification usage issue.

[fiseni]

Let me start by giving a brief overview of ISingleResultSpecification. This interface does not affect and is not utilized by the specification infrastructure at all. This is just a marker interface used to constrain the specification usage in the repositories, more specifically the GetBySpec repository method. The initial idea was to help users to avoid doing mistakes and passing wrong specifications to GetBySpec. At first glance, it seemed a great idea, but it turned out to be really flawed. Unfortunately, it did more harm than good. Let me summarize the issues:

Issues:

• This is the signature of GetBySpec methods.

Task<T?> GetBySpecAsync<Spec>(Spec specification, CancellationToken cancellationToken = default) where Spec : ISingleResultSpecification, ISpecification<T>;
Task<TResult?> GetBySpecAsync<TResult>(ISpecification<T, TResult> specification, CancellationToken cancellationToken = default);

As we can see, we have no constraint for the GetBySpecAsync<TResult> method. We can't apply the same constraint since we have a method generic parameter too. That said, whenever users mark a given Specification<T, TResult specification with ISingleResultSpecification, the usage can no longer be inferred correctly and the wrong method is called.

public class CustomerNameSpec : Specification<Customer, string>, ISingleResultSpecification
{
    public CustomerNameSpec(int id)
    {
        Query.Select(x => x.Name)
             .Where(x => x.Id == id);
    }
}

var spec = new CustomerNameSpec(1);

// This call will pick the wrong method GetBySpecAsync<Spec>, instead of GetBySpecAsync<TResult>
var result = await customerRepository.GetBySpecAsync(spec)

// To mitigate this, users have to explicitly define the `TResult` all the time, and that's tedious.
var result = await customerRepository.GetBySpecAsync<string>(spec)

We added this interface to help minimize the bugs, but actually, we introduced way more potential bugs on another front. The users get confused why they're getting T instead of TResult even though they have applied Select and working with Specification<T, TResult.

• It gives a wrong impression that applying this interface somehow affects how the results are queried. The method still uses FirstOrDefaultAsync, not using SingleOrDefaultAsync. So, it provides a false sense of security.
• We restricted how users design their specifications. It's not uncommon to have specifications with several constructors. For instance, one accepting int id, and an additional one IEnumerable<int> ids. Now, they must mark it with ISingleResultSpecification, which is confusing.

Proposal:

Perhaps we should admit our mistake and do the following:

• Remove the constraint for the GetBySpec method. This will not be a breaking change, existing code will still work. All calls with Specification<T, TResult will be inferred correctly now again. The methods will have the following signature

Task<T?> GetBySpecAsync(ISpecification<T> specification, CancellationToken cancellationToken = default);
Task<TResult?> GetBySpecAsync<TResult>(ISpecification<T, TResult> specification, CancellationToken cancellationToken = default);

• We won't delete the interface. We'll have ISingleResultSpecification, ISingleResultSpecification<T> and ISingleResultSpecification<T, TResult>. Users can utilize them as they wish.
• If users want to add constrained repository methods, they can be explicit and define the following methods. These methods perhaps can use SingleOrDefaultAsync EF method, and that would be a more correct approach for these cases.

Task<T?> SingleOrDefaultAsync(ISingleResultSpecification<T> specification, CancellationToken cancellationToken = default);
Task<TResult?> SingleOrDefaultAsync<TResult>(ISingleResultSpecification<T, TResult> specification, CancellationToken cancellationToken = default);

For convenience, they can also create the following base specifications too

public class SingleResultSpecification<T> : Specification<T>, ISingleResultSpecification<T>
{
}
public class SingleResultSpecification<T, TResult> : Specification<T, TResult>, ISingleResultSpecification<T, TResult>
{
}

// Then use these base specs as following
public class CustomerNameSpec : SingleResultSpecification<Customer, string>
{
    public CustomerNameSpec(int id)
    {
        Query.Select(x => x.Name)
             .Where(x => x.Id == id);
    }
}

var spec = new CustomerNameSpec(1);
var result = await customerRepository.SingleOrDefaultAsync(spec);

[fiseni]

Alternatively, we can implement the proposal and provide it as part of the library.

Implementation proposal:

• Define the following specification types

public class SingleResultSpecification<T> : Specification<T>, ISingleResultSpecification<T>
{
}
public class SingleResultSpecification<T, TResult> : Specification<T, TResult>, ISingleResultSpecification<T, TResult>
{
}

• I really do advocate for verbose repository method names. So, instead of GetBySpec we can define the following methods:

// These will work with any specification.
Task<T?> FirstOrDefaultAsync(ISpecification<T> specification, CancellationToken cancellationToken = default);
Task<TResult?> FirstOrDefaultAsync<TResult>(ISpecification<T, TResult> specification, CancellationToken cancellationToken = default);

// These will work only with SingleResultSpecification 
Task<T?> SingleOrDefaultAsync(ISingleResultSpecification<T> specification, CancellationToken cancellationToken = default);
Task<TResult?> SingleOrDefaultAsync<TResult>(ISingleResultSpecification<T, TResult> specification, CancellationToken cancellationToken = default);

• Mark the GetBySpec methods as obsolete, then we'll remove them in the next major release.

I think this will be a good approach. The new methods will offer consistent and reliable usage. Users can keep their existing code and have time for the transition until version 7.

[vittorelli]

Maintaining a nuget package for personal and professional use, we're currently using the ISingleResultSpecification interface in some generic repository as GetBySpec does. I must say that I initially loved the concept of the single result specification. Used it, promoted it by introducing it in our package, and now regret having done that. It doesn't make the code cleaner. We're currently also discussing to remove the usage of ISingleResultSpecification in our generic repo.

[ardalis]

Do you think many folks would find these useful?

// These will work with any specification.
Task<T?> FirstOrDefaultAsync(ISpecification<T> specification, CancellationToken cancellationToken = default);
Task<TResult?> FirstOrDefaultAsync<TResult>(ISpecification<T, TResult> specification, CancellationToken cancellationToken = default);

[fiseni]

Me 😆. I almost always use FirstOrDefault, and very rarely have use cases for SingleOrDefault. Let me elaborate with an example. Let's have the following specification

public class CustomerSpec : Specification<Customer>
{
  public CustomerSpec(string firstName, string lastName)
  {
    Query.Where(x => x.FirstName == firstName && x.LastName == lastName);
  }
}

If the business requirement is that we should never have two customers with the same FirstName and LastName, then I'd always check that while creating a new Customer. If by any chance you have persisted in such duplicate, then the damage is already done. No matter what approach you use to read the data, the bottom line is you have inconsistent data. By using SingleOrDefault you're not mitigating the issue, you'll just get an exception while running this query (always).

So, I personally won't use either SingleOrDefault or SingleResultSpecification very often. There are some valid use cases, so yea let's keep them around.

[djeikyb]

I found this issue today looking for something like FirstOrDefaultAsync; look forward to it landing!