ardalis / Specification

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

Allow combination of Specification with different projections #389

Open gabrielheming opened 8 months ago

gabrielheming commented 8 months ago

Hello there,

I am using the Specification in a few projects that I am working on, and one of my projects came to the point that it has multiple Specifications only to represent different projections, using the same Where/Order expressions and/or Search Criteria.

This project in question is a WebAPI without any UI. At this moment, I am tweaking the current approach to map domain entities to their endpoints (VMs/DTOs). As some particular entities are loaded in different scenarios with partial data, we decided to map directly on the projection instead of loading the entity to memory and then mapping it to its VM/DTO.

My questioning came to the point that if I have to create a Specification per mapping/projection, is it still an advantage over the direct usage of DBContext? Of course, I am exaggerating it here, but it is only to make my point.

As Specification is easy to extend (extra points for you guys) I have created an extension method to combine a Specification with different projections.

My take on it is understanding whether it is useful/helpful or if it's just a load of BS making the purpose of Specification nonexistent.

In a nutshell, I created the IProjectionSpecification interface as below:

public interface IProjectionSpecification<T, TResult>
{
    public Expression<Func<T, TResult>>? Selector { get; }
    public void Select(Expression<Func<T, TResult>> selector);
}

And being able to use the extension method .ToProjection.

// VMs used as ex
public record StoreVm
{
    public int Id { get; set; }
    public string? Name { get; set; }
}

public record StoreWithStreetVm
{
    public int Id { get; set; }
    public string? Name { get; set; }
    public string? Street { get; set; }
}

// Store and StoreByIdSpec are concrete classes from Ardalis.Specification.UnitTests
ISpecification<Store> storeByIdSpec = new StoreByIdSpec(1);
ISpecification<Store, StoreVm> mySpecWithProjection = storeByIdSpec.ToProjection(new MyStoreToStoreVm());
ISpecification<Store, StoreWithStreetVm> mySpecWithProjection2 = storeByIdSpec.ToProjection(new MyStoreToStoreWithStreetVm());

I don't think it's necessary to go into implementation details right now, but I'm interested in hearing your thoughts.

If you believe it would be useful, I can submit it as a pull request or create an extension package.

fiseni commented 8 months ago

Hi @gabrielheming,

We already support projections in the specs, you have to inherit from Specification<T, TResult>. If you want to have a single specification per entity, and reuse the same spec for arbitrary projections, then you may want to check our sample apps. More specifically the sample3. In this sample, we're combining specs with Automapper for projections, and the usage becomes super simple. Of course, you can use other mapping libraries too.

// Projecting directly to response DTOs. In addition the response is paginated and wrapped in PagedResponse<T>.
app.MapGet("/customers", async (IReadRepository<Customer> repo,
                                [AsParameters] CustomerFilter filter,
                                CancellationToken cancellationToken) =>
{
    var spec = new CustomerSpec(filter);
    var result = await repo.ProjectToListAsync<CustomerDto>(spec, filter, cancellationToken);
    return Results.Ok(result);
});
fiseni commented 8 months ago

Btw, if you prefer doing the mapping manually, then you may create a spec extension for common criteria and create projection specification per result type. So, in this case, the projection specs will act as mappers (since you're doing it manually).

public static class CustomerSpecExtensions
{
    public static ISpecificationBuilder<Customer> ApplyCommon(this ISpecificationBuilder<Customer> builder)
    {
        // Apply your common queries for Customer here
        builder.OrderBy(x => x.Name);
        return builder;
    }
}

public class CustomerDtoSpec : Specification<Customer, CustomerDto>
{
    public CustomerDtoSpec()
    {
        Query.ApplyCommon();
        Query.Select(x => new CustomerDto(
            x.Id, 
            x.Name, 
            x.Age, 
            x.Addresses.Select(a => new AddressDto(a.Id, a.Street, a.CustomerId)).ToList()));
    }
}
gabrielheming commented 8 months ago

Hi @fiseni, thank you for your feedback.

We already support projections in the specs, you have to inherit from Specification<T, TResult>.

I thought it was clear that I was already using it, and it led to the creation of the ToProjection extension method solution to solve (through composition) the inheritance issue. However, I have to admit that I tend to go naturally to a composition approach when a virtual member is called in a constructor. On the other hand, mapping to a common projection with different criteria will lead to duplication. For instance: 2 different Specs but projects the same output.

My point is a similar approach to your example from sample3 without having to rely on an external library and using only the Specification library. That is the general question that I will rephrase: Do you think it might be useful for the community as a implementation or do you rather not have such implementation on the package?

Your example with ApplyCommon keeps the inheritance problem that sample3 solves.

On my take, I didn't want to add more methods on the already existing IReadRepository<T> and IRepository<T>. Though I will check the approach from sample3 to have a check on performance. I am testing part of my implementation using source generator to check any improvement, and I will see the results.

Thanks again for your feedback, it gave me more approaches to check on.

fiseni commented 8 months ago

@gabrielheming @ardalis Yes, I do understand your points. But, adding another type IProjectionSpecification<T, TResult> might not be the best way to move forward. We try to minimize the number of types that the library exposes. Also, we already have a construct ISpecification<T, TResult> that represents the same thing. So, we should find a way how to integrate that one into this usage. Roughly, I'm thinking in this direction

ISpecification<Customer> spec = new CustomerSpec();
ISpecification<Customer, CustomerDto> projectionSpec = new CustomerDtoSpec();

// Get a new spec by combining the state from the base spec.
var newSpec = projectionSpec.Combine(spec);

If we put it this way, then it's clear that we're trying to implement composite specifications. There have been thorough discussions on this topic, about the pros and cons of the composite specs. So, if we want to implement this feature then it should be done a bit more carefully and considering the broader scope, not just this specific case. Perhaps it's time we re-evaluate composite specs and the best way to move forward in that regard.

gabrielheming commented 8 months ago

@fiseni

we already have a construct ISpecification<T, TResult> that represents the same thing

Perhaps it's time we re-evaluate composite specs and the best way to move forward in that regard.

True, and I agree with you.

On my scenario, I've used a new type to take advantage of source generator as it specifies that the direction will be from ISpecification<T> to IProjectionSpecification<T, TResult> resulting an ISpecification<T, TResult>, without generating code for something that is not going to be use. Though it could probably be done using an attribute and possibly some configuration.

Nevertheless, I would argue that the specification composition should be strict for the criteria, while projection an extension on top of the specification.

Furthermore, I agree with you that discussing composition in details could be the proper approach.

fiseni commented 8 months ago

Ok, we'll plan this. But, before starting this work, we need substantial refactoring of the internals. It's something I've been planning for quite some time. Only then, we can start implementing this feature.