Nebulex.Caching.SimpleKeyGenerator should produce a unique key

[nmbrone]

Hi there 👋

An example:

defmodule MyApp.A do
  @decorate cacheable(cache: MyApp.Cache)
  def get_something do
    #...
  end
end

defmodule MyApp.B do
  @decorate cacheable(cache: MyApp.Cache)
  def get_something do
    #...
  end
end

In version 2.0 the code above will result in having two entries in the cache with the (unique) keys

:erlang.phash2({MyApp.A, :get_something}) and
:erlang.phash2({MyApp.B, :get_something}),

but version 2.1 will end up with the key 0 in both cases and one entry will overwrite another. This could lead to very unexpected results for the one who already did the upgrade.

I guess the fix is pretty straightforward and changing Nebulex.Caching.SimpleKeyGenerator should be enough.

Also, the option :key_generatorwould make so much sense here:

use Nebulex.Cache,
    otp_app: :my_app,
    adapter: Nebulex.Adapters.Local,
    key_generator: MyApp.CacheKeyGen

[cabol]

Hey 👋 !

Yeah, in version 2.1 that part changed a bit, the SimpleKeyGenerator is based on the default key generation in the Spring Cache Abstraction

Let me check if maybe makes more sense to change it and make it similar to how it was before. In the meantime, you can also provide a very simple generator like:

defmodule MyApp.Cache.KeyGenerator do
  @behaviour Nebulex.Caching.KeyGenerator

  @impl true
  def generate(mod, fun, args), do: :erlang.phash2({mod, fun, args})
end

On the other hand, regarding the suggestion about the :key_generator option, makes sense, I will check it out too!

Thanks 👍 !!

[cabol]

Hi @nmbrone 👋

1. Regarding Nebulex.Caching.SimpleKeyGenerator should produce a unique key,). Image I have a simple scenario with a module like this:

defmodule Example do
  use Nebulex.Caching

  @decorate cacheable(cache: Cache)
  def get_something(key) do
    # get logic
  end

  @decorate cache_evict(cache: Cache)
  def del_something(key) do
    # delete logic
  end
end

If the generator includes the module and function to compute the key, the code above wouldn't work, because you will get different keys for each function, so for the most basic and simplest scenarios, it wouldn't work. On the other hand, with the current implementation of Nebulex.Caching.SimpleKeyGenerator the code above does work fine because the key is computed only based on the arguments.

The idea behind Nebulex.Caching.SimpleKeyGenerator is to provide a very generic/simple implementation, of course, not to cover all scenarios nor even most of them, it is just to cover a few, the simplest/basic/generic ones. Then, to cover other scenarios (more specific maybe), the idea is you to provide the generator based on your needs, taking advantage it is very easy to implement a custom generator (as it is shown in the example in the previous comment).

2. About the suggestion of setting the key generator at compile time when the cache is defined, that makes sense, but not to replace the current :key_generator option in the annotations, but for allowing changing the default key generator, and in that way avoiding to pass the :key_generator option on every annotation when we want a different generator (in case we only have one generator). So the :key_generator option remains, giving us the flexibility of changing the generator at any time when using the annotations overriding the default one. I created a separate issue explaining the idea, and also pushed the solution for it (including documentation) – you can check the master branch. Please take a look at it and let me know your thoughts.

Thanks.

[nmbrone]

Hi @cabol,

First of all, big thanks for your time.

I really love your idea of adding the Nebulex.Caching.KeyGenerator behaviour. My only concern was that its default implementation Nebulex.Caching.SimpleKeyGenerator is producing a different key compared to the previous release in case of the very basic usage when you not providing your own :key to the @decorate.

Given the previous default key implementation https://github.com/cabol/nebulex/blob/v2.0.0/lib/nebulex/caching.ex#L407-L412

key_var =        
  Keyword.get(          
    attrs,          
    :key,          
    quote(do: :erlang.phash2({unquote(context.module), unquote(context.name)}))        
  )

I would expect Nebulex.Caching.SimpleKeyGenerator to be something like

defmodule Nebulex.Caching.SimpleKeyGenerator do
  @behaviour Nebulex.Caching.KeyGenerator

  @impl true  
  def generate(mod, fun, _args), do: :erlang.phash2({mod, fun}) 
end

But since it's different, a blind update from v2.0 to v2.1 would have broken our app (and who knows how many others). It isn't something you would expect from a minor version.

Obviously, it was easy to fix by providing our own key generator, but it was pretty annoying that we would have to provide it as the option to each @decorate. And that's why I suggested the default key generator option for use Nebulex.Cache. My bad, I didn't describe the idea clear enough, but you get it just like I meant it 😁.

Anyway, I hope this conversation will help someone save hours of debugging.

Please, feel free to close the issue whenever you like.

And once again, thanks for the great work!

[cabol]

Hey 👋 !!

First of all, thanks for your feedback, it has been very helpful. About the default generator, I agree with you in the sense the version 2.1 broke the previous one, that's my bad 🤦 . I should have provided documentation about the breaking changes. Overall, I think the issue was more about lack of documentation, which I will try to fix (adding some release notes or migration guide from 2.0 to 2.1). But the implementation, one of the things I wanted to address was precisely the default key generation, I think the default shouldn't be computed based on the function name at least, because this will avoid using it in the cacheable functions as well as the eviction ones, which doesn't make much sense. Anyway, for now, I will try to add some documentation about these breaking changes introduced by the key generator and the default behavior.

Finally, thanks a lot for the suggestion about the key generator option at compile-time, I think it is very useful, especially for avoiding adding the generator on each cache annotation, which is very tedious ugly (since we end up adding the same option to all annotated functions). This feature will avoid that.

PD: I will close the issue once I push some documentation about the key generation changes as I mentioned earlier, so it can help others, along with this conversation as you mentioned, to save up time debugging trying to the issue.

Thanks.

[tomtaylor]

@nmbrone @cabol thanks for clarifying this - this really caught us out and would have caused security issues if we hadn't caught it in CI. We didn't expect to need to provide a key to @cachable decorators, this change resulted in multiple modules returning the same cache result for their /0 arity functions. It would be great if breaking changes like this could be released in a major version next time.

[cabol]

@tomtaylor absolutely 💯 , this kind of breaking changes will be in a major version next time, thanks 👍 !!