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

Is it possible to use return value in key-generation along with the passed parameters ? #132

Closed
collegeimprovements opened this issue Sep 13, 2021 · 7 comments

Comments

@collegeimprovements
Copy link

  @decorate cacheable(
              cache: Cache,
              key: {__MODULE__, KeyGeneration.generate(returned_value, attrs)},
              opts: [ttl: :timer.seconds(30)]
            )
         def get_entity(attrs) do
               Entity.get_entity(attrs) # => %Entity{...}
         end

Can we use the returned %Entity{} in KeyGeneration along with attrs?

@cabol
Copy link
Owner

cabol commented Sep 13, 2021

Hey 👋 !

I'm not very sure if I understand, but will try to reply. I see a couple of things here.

About your question, no, it is not possible to use the returned value in the key generator because:

  1. The key generation has to run before the function logic/block is executed, so that, the key can be calculated and then check if it does exist in the cache; keep in mind the cacheable annotation implements read-through pattern.
  2. The reason to be of the cachable annotation is precisely to avoid the function logic or block is executed if the key is within the cache, as it is described in the cache read-through pattern. Perhaps because we want to avoid hitting the DB (or whatever SoR you're using) as much as possible for example. Hence, if the returned value is passed to the key generation, the function has to be always evaluated, which is pointless, since it breaks the idea of the annotation completely.

On the other hand, I see you are not using the key generation in the right way, maybe I'm missing something, but the idea is to use the :key_generator option, you can check the docs for more information. Not sure if maybe you wanted to do something like:

@decorate cacheable(
  cache: Cache,
  key_generator: __MODULE__,
  opts: [ttl: :timer.seconds(30)]
)
def get_entity(attrs) do
   Entity.get_entity(attrs) # => %Entity{...}
end

@impl true
def generate(mod, fun, args) do
  # your key generation logic ...
end

Let me know, stay tuned! Thanks!

@collegeimprovements
Copy link
Author

Thanks a lot @cabol for the reply.

I've a module(Entity) for all my CRUD operations.
Then all other modules e.g. User, Country, Company etc. just delegate CRUD creation to Entity module.

I understand why it's not possible to generate key from returned value.
Thanks a lot for explaining it 😃.

I'll try to add some caching related value to attrs of User, Country and so on...and then will generate key from it.
Or maybe I can put cacheable on those functions themselves.

defmodule Cache do
...
  def generate(mod, fun, args) do
        caching_module = Map.get(args, :caching_module, nil)
       case caching_module do 
              nil -> {:error, "don't save the cache"}
             _ ->{  caching_module , :erlang.phash2({caching_module, fun, args})}
       end
  end
end

# Generic CRUD module
defmodule Entity do
  @decorate cacheable(
    cache: Cache,
    opts: [ttl: :timer.seconds(30)]
  )
  def get_entity(attrs) do
     Entity.get_entity(attrs) # => %Entity{...}
  end
end

# Context with many modules
defmodule Accounts do
  def get_user(attrs) do # There are 35 more such entities like User.
      attrs = Map.merge(attrs, %{cache_module: User})
  end
end

Is it possible to save or not-save value/result based on the Cache that's generated by KeyGenerator?
e.g. If KeyGenerator returns nil or :dont_cache or {:error, reason} or something like this then don't save the cache ?

Is is also possible to have a function for cache_evict ?
e.g.

@decorate cacheable(
            cache: Cache,
            keys: CacheEviction.keys(args)
          )
def delete_user() do
end


defmodule CacheEvicition do
   def keys(attrs) do
       # somehow get all the keys with pattern {User, ...} # where attrs.caching_module == User
   end
end

@cabol
Copy link
Owner

cabol commented Sep 13, 2021

Let me first try to answer your inquiries, and then I will give you an example of how you could achieve what you want.

Is it possible to save or not-save value/result based on the Cache that's generated by KeyGenerator? e.g. If KeyGenerator returns nil or :dont_cache or {:error, reason} or something like this then don't save the cache ?

You shouldn't use the key generator result to determine if the value/result has to be cached, the generator has a very simple/single responsibility which is to provide the key under which the result will be cached, that's all. If you have very custom logic, I think you have to use the cache API instead of the annotations. However, I'd suggest you review the documentation examples as well as the Ecto example, perhaps can give you a better idea about how to use the annotations in your contexts.

Is it also possible to have a function for cache_evict ?

No, it is not possible. Normally, when you use a cache is because your data has some kind of unique key or keys, like an id, username, etc. Then, it is clear beforehand what attributes of your data you will use to cache the values, then you can explicitly use the option key: instead of a custom key generator, but it depends on the case.

Now, regarding your scenario, you say you have a kind of generic context with the CRUD functions for all your schemas, consider passing in the attrs a field like schema indicating the schema and also the key you can to cache the result with, let's say all the entities have a field id.

defmodule MyApp.Entity do
  use Nebulex.Caching

  alias MyApp.Cache

  @decorate cacheable(
    cache: Cache,
    key: {schema, id},
    opts: [ttl: :timer.seconds(30)]
  )
  def get_entity(%{schema: schema, id: id} = attrs) do
     Entity.get_entity(attrs) # => %Entity{...}
  end
end

If you have different key fields per entity, you can use pattern-matching, like so:

defmodule MyApp.Entity do
  use Nebulex.Caching

  alias MyApp.Cache

  @decorate cacheable(
              cache: Cache,
              key_generator: __MODULE__,
              opts: [ttl: :timer.seconds(30)]
            )
  def get_entity(attrs) do
    # => %Entity{...}
    Entity.get_entity(attrs)
  end
  
  @decorate cache_evict(
              cache: Cache,
              key_generator: __MODULE__
            )
  def delete_entity(attrs) do
    # => %Entity{...}
    Entity.delete_entity(attrs)
  end

  # Assuming the attrs is the entity/schema
  @impl true
  def generate(__MODULE__, fun, [attrs]) when fun in [:get_entity, :delete_entity] do
    case attrs do
      %User{username: username} -> {User, username}
      %Company{id: id} -> {Company, id}
      %Country{code: code} -> {Country, code}
      # more ...
    end
  end
end

The guard when fun in [:get_entity, :delete_entity] is for filter the functions we want to run the key generation, if you want them all, just remove it and use _ instead.

Please let me know if something like that is what you are trying to achieve, maybe I can help you more, otherwise, try to give me more details, and also check the examples in the doc and the Ecto example I sent you the link. I stay tuned!

@collegeimprovements
Copy link
Author

Thanks a lot @cabol for your reply.
Here is the code that I'm trying to cache.

  def get_entity(query, _attrs = %{id: id}) when is_binary(id) do
    Repo.get(query, id)
  end

  def get_entity(query, attrs) when is_map(attrs) do
   # ... Find Logic ...
  end

  def get_entity(query, %{}) when is_atom(query), do: nil
  def get_entity(%{} = _attrs), do: nil

And my attrs generally looks like this:

%{
  filters: [%{field: :name, query_operation: :contains, search_term: "ind"}],
  limit: 40,
  order_by: [%{field: :name, sort_order: :asc}],
  pagination: %{limit: 30}
}

Since, they are quite dynamic in nature too, it would be hard to cache them.
And since, search query have similar dynamic nature, I guess I'll go with small ttl for caching.

I was actually looking for a way to put caching on list or search operations.
And since it's highly dynamic in natrue, I'll go with small ttl like 2 mins or so.

For eviction -> I was trying to create a mechanism where it will read data from cache but if attrs have fetch_from_cache: false (i.e. attrs = %{fetch_from_cache: false}) then skip the cache. This is the piece that I'm still trying to solve.

Cache based on id, name or single unique parameter has been sorted thanks to your awesome docs 😃

@collegeimprovements
Copy link
Author

collegeimprovements commented Sep 14, 2021

One more question:
I've the following functions to get the value of a resource.
I want to cache the value when i fetch it by name.

defmodule Auth do
  @decorate cacheable(
              cache: Cache,
              key: {AuthRule, attrs.name},
              opts: [ttl: :timer.seconds(30)]
            )
  def get_auth_rule(%{name: name} = attrs) do
       Repo.get_by(AuthRule, name: name)
  end

  def get_auth_rule(%{id: id} = attrs) do
       Repo.get(id)
  end

  def get_auth_rule(attrs), do: Crud.get_entity(AuthRule, attrs)
end

When I run the function, I get the following error:

iex(1)> Auth.get_auth_rule(%{name:  "hello"}) 
%AuthRule{..., name: "hello", ...}

iex(2)>  Auth.get_auth_rule(%{id:  1}) 
** (KeyError) key :name not found in: %{id: 1 }

Am I doing something wrong ?
Is there a way to function with same name and args but different cache-keys like above ?
Another minor thing is, we get a warning variable attrs is unused, as it's only used in cache as cache can't use name from args in function.
Is there a better way to put key as: {AuthRule, name} when function head is get_auth_rule(%{name: name} = attrs?

  @decorate cacheable(
              cache: Cache,
              key: {AuthRule, attrs.name},
              opts: [ttl: :timer.seconds(30)]
            )
  def get_auth_rule(%{name: name} = attrs) do
       Repo.get_by(AuthRule, name: name)
  end

One last question(🙈): Is there a way to get all the available keys in the cache ?

@cabol
Copy link
Owner

cabol commented Sep 14, 2021

Hey, no worries, let me answer your question.

I've the following functions to get the value of a resource.
I want to cache the value when i fetch it by name.

Right, the thing is the annotation covers all function clauses, but nothing it cannot be handled, you can do something like this:

defmodule Auth do
  def get_auth_rule(%{name: name}) do
    get_auth_rule_by_name(name)
  end

  def get_auth_rule(%{id: id}) do
    Repo.get(id)
  end

  def get_auth_rule(attrs), do: Crud.get_entity(AuthRule, attrs)

  @decorate cacheable(
              cache: Cache,
              key: {AuthRule, name},
              opts: [ttl: :timer.seconds(30)]
            )
  defp get_auth_rule_by_name(name) do
    Repo.get_by(AuthRule, name: name)
  end
end

As you can see, for those cases the trick is to create a private function and use the cacheable annotation there, in that way, the cache annotation logic will happen only for the case you want.

Another minor thing is, we get a warning variable attrs is unused, as it's only used in cache as cache can't use name from args in function.
Is there a better way to put key as: {AuthRule, name} when function head is get_auth_rule(%{name: name} = attrs?

I think that is because you are not using the attrs in the first two clauses, it has not to do with the annotation, so try to remove it, check the code I put above, it is removed there, try it out.

One last question(🙈): Is there a way to get all the available keys in the cache ?

Yes, it is, it should be like MyApp.Cache.all(), by default it returns all the keys (you can use the :return option to return the values if you need to). Check the function c:all/2 for more info.

Since, they are quite dynamic in nature too, it would be hard to cache them.
And since, search query have similar dynamic nature, I guess I'll go with small ttl for caching.
I was actually looking for a way to put caching on list or search operations.
And since it's highly dynamic in natrue, I'll go with small ttl like 2 mins or so.

I'd like to start by saying, not all the data is or has to be suitable to cache, and part of the work is actually identifying the data really worth to cache because it will improve the performance by reducing database contention significantly, otherwise, I'd reconsider using a cache. For instance, the search operation you mention, which is very dynamic, maybe it is something not suitable to cache. Overall, caching queries is tricky, if they are known queries like the "best sellers" in a marketplace then is easy, but for unknown ones, you have to identify those common or most hit queries, so that you can then start caching them. My advice would be, if you are going to cache ALL queries for just a few seconds, don't use a cache (unless you know for a fact those few min will be critical to improve the system's performance and reduce contention), start to analyze them, and identify the ones you can cache for long or significant periods of time, then maybe you can pattern-match those cases and use the cache. For example:

def get_entity(query, %{id: id}) when is_binary(id) do
  get_entity_by_id(id)
end

# Maybe an identified common query worth to be cached
# REMEMBER you can also use guards and validate more operation, etc.
def get_entity(query, %{
      filters: [%{field: :name, query_operation: :contains, search_term: "ind"}],
      limit: 40,
      order_by: [%{field: :name, sort_order: :asc}],
      pagination: %{limit: 30}
    } = attrs) do
  get_entity_by_query(attrs)
end

def get_entity(query, _attrs = %{id: id}) when is_binary(id) do
  get_entity_by_id(id)
end

def get_entity(query, attrs) when is_map(attrs) do
  # ... Find Logic ...
end

# BE CAREFUL, the %{} is not matching an empty map, you have to use the guard map_size(map)
def get_entity(query, %{}) when is_atom(query), do: nil

@decorate cacheable(
            cache: Cache,
            key: id,
            opts: [ttl: :timer.seconds(30)]
          )
defp get_entity_by_id(id) do
  Repo.get(query, id)
end

@decorate cacheable(
            cache: Cache,
            key: attrs,
            opts: [ttl: :timer.seconds(30)]
          )
defp get_entity_by_query(attrs) do
  # ... Find Logic ...
end

In the meantime, if you are not sure, just avoid the cache for those "search" or "dynamic queries" cases.

Let me know if that helps, stay tuned, thanks!

@collegeimprovements
Copy link
Author

collegeimprovements commented Sep 14, 2021

Thanks a ton @cabol for your valuable feedback 🙌🏽
The approach you have suggested does solve my issues 😃
And moving the code to private function is a wonderful trick 💡

I'll try to measure and finetune my operations 🎸
Your suggestion and feedback has truly been invaluable. Thanks a lot again.

 @decorate cacheable(
              cache: Cache,
              key: {AuthRule, attrs.name},
              opts: [ttl: :timer.seconds(30)]
            )
  def get_auth_rule(%{name: name} = attrs) do
       Repo.get_by(AuthRule, name: name)
  end

By this code i meant that if we have just one function like above where name variable is coming from pattern-matching,
we can't use that variable in @decorate cacheable(...). @decorate cacheable() can only accept attrs and not name.
Though the idea of extracting out the required functionality to private function solves this issue for me 😃

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

2 participants