Skip to content

Fix possible compilation error for variables expecting any kind of cache store#32

Merged
mamantoha merged 1 commit intocrystal-cache:mainfrom
ellmetha:ellmetha/fix_possible_compilation_error_when_expecting_any_kind_of_cache_store
Apr 12, 2023
Merged

Fix possible compilation error for variables expecting any kind of cache store#32
mamantoha merged 1 commit intocrystal-cache:mainfrom
ellmetha:ellmetha/fix_possible_compilation_error_when_expecting_any_kind_of_cache_store

Conversation

@ellmetha
Copy link
Contributor

@ellmetha ellmetha commented Apr 12, 2023

Description

I encountered a possible compilation error with the Cache::Store class when defining variables accepting any kind of Cache::Store(String, String) object.

For example:

require "cache"

class Test
  @cache_store : Cache::Store(String, String)? = nil
  
  property cache_store
end

test = Test.new
test.cache_store = Cache::MemoryStore(String, String).new(expires_in: 15.minutes)
test.cache_store.not_nil!.fetch("foo") { "bar" }

This snippet fails with the following compilation error:

Error: instantiating 'Cache::Store(String, String)+#fetch(String)'


In lib/cache/src/cache/store.cr:35:40

 35 | def fetch(key : K, *, expires_in = @expires_in, &block)
                                         ^----------
Error: can't infer the type of instance variable '@expires_in' of Cache::Store(String, String)

Could you add a type annotation like this

    struct Cache::Store(String, String)
      @expires_in : Type
    end

replacing `Type` with the expected type of `@expires_in`?

It seems that this is related to the use of the @expires_in instance variable in the Cache::Store abstract class: this instance variable is not defined by this class directly but is referenced nonetheless.

To fix this, I ensured that a nilable @expires_in instance variable is defined in the Cache::Store abstract class. This also led me to modify the Cache::Entry struct so that its @expires_at instance is nilable as well (I imagine this makes sense given that some entries could not have expiry times).

@mamantoha
Copy link
Member

mamantoha commented Apr 12, 2023

Thanks!

@mamantoha mamantoha merged commit b9087c2 into crystal-cache:main Apr 12, 2023
@mamantoha mamantoha mentioned this pull request Apr 12, 2023
Merged
@mamantoha
Copy link
Member

mamantoha commented Apr 12, 2023

@ellmetha I apologize, but I had to revert your changes because I don't think it's a good idea to make expires_in nullable. This would result in a breaking change to the code.

@mamantoha
Copy link
Member

mamantoha commented Apr 12, 2023

@ellmetha By adding this code to abstract struct Store(K, V), the example you provided in the description is now fixed. However, I'll need to investigate further to ensure everything is working properly.

def initialize(@expires_in : Time::Span)
end

@ellmetha ellmetha deleted the ellmetha/fix_possible_compilation_error_when_expecting_any_kind_of_cache_store branch April 13, 2023 00:16
@ellmetha
Copy link
Contributor Author

ellmetha commented Apr 13, 2023

I apologize, but I had to revert your changes because I don't think it's a good idea to make expires_in nullable. This would result in a breaking change to the code.

No worries! Avoiding a breaking change seems sensible to me.

Regarding expires_in I still think that it should be nullable ideally. Most caching solutions like ActiveSupport don't enforce the presence of expiry times by default and most caching backends are able to evict old keys automatically. Defining non-expirable entries seem like a valid use case.

Out of curiosity, what was the rationale for enforcing expires_in when initializing cache stores and when calling methods like #fetch?

@mamantoha
Copy link
Member

mamantoha commented Apr 13, 2023

The concept of expires_in is fundamental to the design of this library.

The rationale behind enforcing expires_in when initializing cache stores and calling methods like fetch is to ensure that the cached data does not remain in the cache indefinitely and becomes stale.

By default, when initializing cache stores, the expires_in parameter sets the default value for the entire cache. This default value can be overridden when calling methods like fetch, write to set a specific expiration time for a particular item.

I'm not opposed to expires_in being nullable, but I'm not sure of the reasoning behind it.

@ellmetha
Copy link
Contributor Author

ellmetha commented Apr 13, 2023

I'm not opposed to expires_in being nullable, but I'm not sure of the reasoning behind it.

I agree that an expiry time will be specified in most situations (like probably 99% of the time). Though I've seen cases where specifying an expiry explicitly didn't really make sense for the task at hand (eg. storing timestamp versions for resources in a Memcached store that were then used as part of cache keys in order to handle some cache invalidation strategies with some API clients, storing a resource in a memory store for the whole duration of the worker process, etc). Giving the option to the users of this shard would be nice IMHO!

@mamantoha
Copy link
Member

mamantoha commented Apr 14, 2023 •
edited
Loading

@ellmetha
Could you please create a PR as you might have a better understanding of your requirements? I'll gladly review and merge it.
In any case, I'm planning to introduce some changes that won't be backward compatible with the current version.
Thanks.

@mamantoha mamantoha mentioned this pull request Apr 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ellmetha @mamantoha