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

Restore ISingleResultSpecificationOfT #196

Merged
merged 3 commits into from
Dec 31, 2021
Merged

Conversation

vittorelli
Copy link
Contributor

@vittorelli vittorelli commented Dec 28, 2021

The ISingleResultSpecificationOfT seems to be have been removed in 73693ac while being included in the current Nuget package. I assume this was accidental?

Furthermore, I've added two base classes SingleResultSpecification based on the existing Specification classes.

@vittorelli
Copy link
Contributor Author

vittorelli commented Dec 29, 2021

I'm having second thoughts on the SingleResultSpecification classes... On one hand it does conform to the existing Specification base classes and it avoids the nessecity of defining the generic type twice. On the other hand, it creates the situation where developers can create ISingleResultSpecifications in multiple ways, which may lead to inconsistent code.

    interface IRepository<T>
    {
        // doesn't work for SingleCustomerSpecification_3
        T Single(ISingleResultSpecification<T> spec);

        // works for all specifications alternatives
        T SingleWithoutGenericSingleResultSpecification<Spec>(Spec spec)
            where Spec : ISingleResultSpecification, ISpecification<T>;
    }

    // option 1 -- uses the new SingleResultSpecification base class
    class SingleCustomerSpecification_1 : SingleResultSpecification<Customer>
    {
        public SingleCustomerSpecification_1(int id)
        {
            Query.Where(s => s.Id == id);
        }
    }

    // option 2 -- need to define the generic type twice
    class SingleCustomerSpecification_2 : Specification<Customer>, ISingleResultSpecification<Customer>
    {
        public SingleCustomerSpecification_2(int id)
        {
            Query.Where(s => s.Id == id);
        }
    }

    // option 3 -- cleanest declaration imo
    class SingleCustomerSpecification_3 : Specification<Customer>, ISingleResultSpecification
    {
        public SingleCustomerSpecification_3(int id)
        {
            Query.Where(s => s.Id == id);
        }
    }

Reviewing the code above, it seems to me that it might be better to remove the ISingleResultSpecification{T} interface, even though it would mean a breaking change with respect to the current NuGet package. My pull-request would then be unnessecary :-)

What do you guys think?

@devbased
Copy link
Contributor

I don't understand what is the point of ISingleResultSpecification? What does it provide?
Here i've removed it

public interface IReadRepositoryBase<T> where T : class
{
    // other methods
    Task<T?> GetBySpecAsync(ISpecification<T> specification, CancellationToken cancellationToken = default);
}
public abstract class RepositoryBase<T> : IRepositoryBase<T> where T : class
{
    // other methods
    public virtual async Task<T?> GetBySpecAsync(ISpecification<T> specification, CancellationToken cancellationToken = default)
    {
        return await ApplySpecification(specification).FirstOrDefaultAsync(cancellationToken);
    }
}

@vittorelli
Copy link
Contributor Author

vittorelli commented Dec 30, 2021

The ISingleResultSpecification interface is a marker interface to indicate that a specification returns a single result. In LINQ one would use SingleOrDefault, FirstOrDefault, Single or First. For me, this marker interface has two advantages:

  • Help the developer understand the intent of a specification at a glimpse
  • Allow the IDE to identify incorrect usage of specifications.

For the first point, consider the example below. Using the ISingleResultSpecification a developers easily understands that the first specification returns multiple customers while the latter returns a single one -- even without having to read the details in the constructor (parameters, body) or fully analysing the class name.

// The first word in this specification is plural, to indicate multiple customers are returned
public class CustomersWithAddressesAndPurchaseHistory : Specification<Customer>
{
    public CustomersWithAddressesAndPurchaseHistory(bool includeAddress, bool includePurchaseHistory)
    { 
        // Query logic
    }
}

// The first word in the specification name is singular, but this is hard to detect.
// The ISingleResultSpecification marker interface is more obvious, which helps to understand the intent.
public class CustomerWithAddressesAndPurchaseHistory : Specification<Customer>, ISingleResultSpecification
{
    public CustomerWithAddressesAndPurchaseHistory(int id, bool includeAddress, bool includePurchaseHistory)
    { 
        // Query logic
    }
}

For the second point, consider the example below. When used in combination with carefully designed repository interfaces, one can generate compile time errors for when a developer accidentally uses the wrong specification. Imagine you have two specifications with similar names and untrivial constructor parameters, where one returns a list of entities and the other a single entity. We can then use the compiler to generate errors if one accidentally picked the wrong specification.

    interface IRepository<T>
    {
        T List(ISpecification<T> spec);

        T Single<Spec>(Spec spec)
            where Spec : ISingleResultSpecification, ISpecification<T>;
    }

Of course, finding good naming conventions and/or well chosen constructors also help in avoiding these kind of mistakes. And even though some might argue that marker interfaces shouldn't be used because they don't actually do anything, I still believe the examples shown here here illustrate that using marker this interfaces is in fact beneficial.

@vittorelli
Copy link
Contributor Author

vittorelli commented Dec 30, 2021

The generic ISingleResultSpecification<T> interface is included in version 5.2.0 NuGet package; simply removing it in the next package creates an unannounced breaking change. I haven't modified my pull request, but my suggestion is to mark the generic interface as obsolete, leaving only ISingleResultSpecification.

@devbased
Copy link
Contributor

@vittorelli Thank you for explanation. But is it really a specification responsibility to define resulting object? Would you create marker interfaces for TwoResults, ThreeResults, NResults, IAsyncEnumerableResult or ListResult, or other complex type specifications? If so, should ISomeResultSpecification create that result and not just mark that it 'wants' it? IMHO Specification builds query and not materializes it (or tells you how to materialize).

Sorry if this is not a right place for such discussion 😅

@fiseni
Copy link
Collaborator

fiseni commented Dec 30, 2021

Hi,

The ISingleResultSpecification interface has no importance in the specification infrastructure, nor it affects how they're built. Ultimately, it's utilized by the repository implementation and offers help if you need to restrict the way how a given specification can be materialized. I personally am not using it. Not rarely my specs may contain several constructors (accepting Id, collection of Ids, and possibly a filter object), so this interface does not make sense for my use-cases. But, the community asked for it, so it's a thing now. I do admit, including the interface out of the box and restricting the GetBySpec repository method was an opinionated action. As I said, it's too late now, and taking it away will be worse. Anyhow, the built-in repository implementation is just for reference, users can have their own implementation or use the specs without repositories (directly with dbContext). Also, some users have created interesting usages around this interface, so we'll keep it. @vittorelli thank you for spotting this, it seems is being removed by mistake.

As for the additional base class SingleResultSpecification, I'm not for it honestly. Not sure what @ardalis thinks about this, but I think libraries shouldn't expose too many "notions". In our case, all that users have to remember is that they need to extend the base Specification class and use Query property to start building the specification. Once we include new "starting points", we introduce dilemmas somehow, should include more detailed clarifications, etc. Users can build their own infrastructure on top of the base one provided in the library. It's very common to have project-specific base specifications, which may include some generic actions like pagination. It's up to the users.

EDIT: Just to clarify, I don't have a strong opinion here. If the community wants it OOTB, sure we can add it.

@vittorelli vittorelli changed the title Restore ISingleResultSpecificationOfT + Provide base class for SingleResultSpecification Restore ISingleResultSpecificationOfT Dec 31, 2021
@vittorelli
Copy link
Contributor Author

Ok, I agree. Better to keep the package clean and leave it up to end users if they want to use ISingleResultSpecification and/or create SingleResultSpecification base classes :-)

I've updated the PR so it only restores the generic interface so as to not have breaking changes in the next NuGet release.

@fiseni
Copy link
Collaborator

fiseni commented Dec 31, 2021

Thank you @vittorelli. We appreciate your help.

@fiseni fiseni merged commit d0b0194 into ardalis:main Dec 31, 2021
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 this pull request may close these issues.

3 participants