ardalis / Specification

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

Simplify Cache usage #370

Closed LionelDimitrijevic closed 8 months ago

LionelDimitrijevic commented 8 months ago

Hello, i'm new to this library and find it just fine. Nonetheless, reading the documentation on the caching part, i find it too "complicated" to use the cache. What i mean is that you have to explicitely define the cache key. Would it be possible to override the EnableCache method without any parameter. The key would be computed by the GetHashCode() method. Overriding would allow to keep compatibility or special cases. I can manage the change if you explain how to do it and what are the rules. Regards

ardalis commented 8 months ago

I'd be somewhat concerned about the performance of GetHashCode (not sure if that concern is justified without measuring) and its ability to avoid collisions. I'd hate to ship an "in the box" caching solution that resulted in incorrect cache hits.

LionelDimitrijevic commented 8 months ago

I would not be much concerned about performance if you keep the way cache works today and just offer the possibility to have a shorter way but with a slightly slower execution. The dev would choose what is the best for his project. Performance is not always an issue. It is sometimes acceptable to loose performance if you gain developping time and thus the cost of the software you are building. About collisions it is a point i did not take care because i'm on a POC. But you are totally right there might be collisions (though not often). Thanks a lot for your reply.

fiseni commented 8 months ago

Hi @LionelDimitrijevic,

The EnableCache feature just provides the ability to set a cache key, that you might utilize in your caching implementation. That's all that it is. The idea is that since the specifications have the knowledge of all parameters used for the query, it might be the appropriate place to generate a unique key.

The GetHashCode() won't work, since that would require all parameters to be saved as a state. And we can't force the consumers into that. Imagine the following specification. In this case, the id is a crucial parameter to guarantee the uniqueness of the cache key. The consumers may not save this value to a private/public field/property, hence the value is lost at the end of the constructor, and GetHashCode can't utilize it.

The classic usage is as follows. What would you like to change here?

public class CustomerSpec : Specification<Customer>
{
    public CustomerSpec(int id)
    {
        Query.Where(x => x.Id == id);

        Query.EnableCache(nameof(CustomerSpec), id);

        // The string is arbitrary, you can add whatever you want.
        Query.EnableCache("asdfghjk", id);
    }
}
LionelDimitrijevic commented 8 months ago

Thaks @fiseni i've seen that in the code. What i meant was that instead of giving an arbitrary key, we could rely on an HashCode, that is in the constructor of the spec just write Query.EnableCache, and i wanted the key to be autocomputed with the GetHashCode of the spec object. But as @ardalis stated there is a collision risk.

fiseni commented 8 months ago

Yes, the collisions are a possible consequence. But, as stated in the previous message, the main issue is that you can't auto-compute the HashCode. The parameters are not part of the specification state.

Sorry, not much that can be done here. Closing the issue.