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

Why it is not possible to add null items in the cache via IAppCache.Add ? #155

Open
EnricoMassone opened this issue Jun 12, 2021 · 9 comments

Comments

@EnricoMassone
Copy link

EnricoMassone commented Jun 12, 2021

Can you please explain why adding a null item to the cache via IAppCache.Add is disallowed ?

I'm asking because the underlying ASP.NET core memory cache allows to add null items to the cache via IMemoryCache.Set.

Is this design choice intended to avoid a situation like the following one, where the semantic of the null reference is somewhat ambiguous ?

class Program
{
    static void Main(string[] args)
    {
      var options = new MemoryCacheOptions();
      var cache = new MemoryCache(options);

      cache.Set<string>("the-key", null, DateTimeOffset.UtcNow.AddDays(1));

      var item = cache.Get<string>("the-key");
      var otherItem = cache.Get<string>("other-key");

      // both item and otherItem are null references, but item has been retrieved from the cache
    }
}

Thanks for the clarification.

@alastairtree
Copy link
Owner

Exactly as you describe - if you allow someone to cache null, they cannot determine when doing a Get if the item was removed from the cache or if they cached null.

If you really want to do weird things with null then you should cache a Nullable<MyThing> and then it is explicit when you are deliberately caching a null.

@EnricoMassone
Copy link
Author

EnricoMassone commented Jun 18, 2021

Exactly as you describe - if you allow someone to cache null, they cannot determine when doing a Get if the item was removed from the cache or if they cached null.

If you really want to do weird things with null then you should cache a Nullable<MyThing> and then it is explicit when you are deliberately caching a null.

Thanks for the clarification, now your design decision is clear to me.

Sometimes caching null items can be useful. I know that using nulls as return values from methods is quite controversial, but in real-world code bases there are plenty of methods such as the following one:

public interface IProductRepository 
{
  // returns null if a product having the specified id doesn't exist
  Product GetById(int id);
} 

Maybe you want to cache the result of calling IProductRepository.GetById, even in the case when null is returned. The workaround of defining a null object totally makes sense to me. The only point that I have with this solution is that, in a certain sense, makes the usage of LazyCache a little more uncomfortable. The code without nulls is safer for sure, but a little more complex.

Maybe there is an alternative. The IAppCache interface defines the following method:

bool TryGetValue<T>(string key, out object value);

By using this method an item is fetched from the cache by key and the bool value returned indicates whether the item was available in the cache. In this case the semantic is clearer than the one in the IAppCache.Get, because there is an explicit boolean value to discriminate between a cache hit and a cache miss.

So a possible change can be allowing to store null values via IAppCache.Add and suggesting users to fetch items via IAppCache.TryGetValue in order to avoid ambiguity. There is probably the need to augment the IAppCache interface with a new TryGetValueAsync method to address the point described in #154. Maybe with such a change the two methods IAppCache.Get and IAppCache.GetAsync could be removed entirely to avoid any ambiguity for the API users.

The main point for this idea is that caching null values is disallowed when using IAppCache.Add, but at the same time you can endup caching null values when using IAppCache.GetOrAddAsync. So, basically, there is a mixture: in one case you cannot cache null items, but in the other one you can.

To summarize it's a tradeoff between having a safer API which tries to avoid working with null and an easier to use API, which allows the user to choose whether or not working with null. I totally agree that relying on null to express a semantic is a bad practice, but at the same time we still not have a simple tool to completely eradicate it from the language.

@alastairtree
Copy link
Owner

Good point about being able to cache a null with the GetOrAdd methods, that is also inconsistent. As you say managing nulls is always imperfect in c#due to the language design although recent null annotations do make it better.

Part of me wonders if it's better to give up and let developers just cache a null if they really want to, and remove the null check in Add. What do you think?

I have to admit I never use the Get, TryGet or the Add methods, I only ever use the GetOrAdd flavour since I want my operation to only execute once which is the main benefit of the package, so that's probably why I haven't run into it before.

@EnricoMassone
Copy link
Author

EnricoMassone commented Jun 18, 2021

Unfortunately the whole concept of null reference is deeply rooted into the language. I have seen various examples of functional libraries attempting (among the other things) to eradicate the null reference from the language, but all of them add a lot of burden. I tend to prefer simplicity.

I wrote myself a simple wrapper class attempting to remove the usage of null as a way to express the absence of a result. But again this adds burden: to be honest after migrating to .NET core 3.1 I prefer using nullable reference types instead of my own wrapper.

To summarize, I would allow null values to be cached via IAppCache.Add.

You can also consider a couple of changes to IAppCache interface:

  • change the signature bool TryGetValue<T>(string key, out object value); to bool TryGetValue<T>(string key, out T value); . In the current implementation the generic argument is actually not used. This goes against the expectation of a caller using a generic method.
  • consider a review of the implementation of TryGetValue to align it to the implementation of Get. The current implementation of TryGetValue does not handle Lazy<T>, probably the right thing to do here is reusing GetValueFromLazy.
  • if the previous point makes sense to you, there is also the case to try get a value added with an async factory to the cache. So maybe also introducing TryGetValueAsync makes sense.

All of these is a breaking change of the public API, so I think it goes to the roadmap for the next major release. Maybe I can help with a pull request if you think that all of this makes sense.

@alastairtree
Copy link
Owner

Yeah that all makes a lot of sense to me and would tidy things up - I would accept a PR based on those changes with supporting unit tests. Although technically breaking API changes, they are all actually very minor breaking changes - I doubt any of them would break users apps, and so I am not even sure they would warrant a major version.

@EnricoMassone
Copy link
Author

EnricoMassone commented Jun 20, 2021

Yeah that all makes a lot of sense to me and would tidy things up - I would accept a PR based on those changes with supporting unit tests. Although technically breaking API changes, they are all actually very minor breaking changes - I doubt any of them would break users apps, and so I am not even sure they would warrant a major version.

Hello, I have manged to fork the repo. I'll produce a single pull request containing all of the changes, is it fine for you ?

@EnricoMassone
Copy link
Author

Hi @alastairtree did you take a chance to evaluate my pull request ?

@jstutson-rel
Copy link

Hi any chance this can get some more priority?

Just want to re-iterate NULLs are not going anywhere in C# as a representation for the absence of data. The new nullability features in newer c# versions are geared towards more null safety, not for any alteration in their meaning or frequency of use. They only ask for more consideration as to whether you really want to use NULL or should expect NULL at any given point.

@paschalisTo
Copy link

Hi, what is the status of this feature? I see there is a PR for almost three years now but no updates of its status.

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

No branches or pull requests

4 participants