whitfin / cachex

A powerful caching library for Elixir with support for transactions, fallbacks and expirations
https://hexdocs.pm/cachex/
MIT License
1.61k stars 105 forks source link

Document that keys must implement `String.Chars` when using `Cachex.Router.Ring` #386

Open probably-not opened 1 month ago

probably-not commented 1 month ago

The specs for all of the Cachex functions all specify that the key is an any(), however, when using the Cachex.Router.Ring router module (which seems to be recommended in the latest docs), the key is actually required to be a ExHashRing.Hash.hashable(), which is essentially an alias for String.Chars.t().

whitfin commented 1 month ago

Yeah, you're not wrong!

I guess I'm not entirely sure on the best approach here, because routers are third-party pluggable, and I can't control their types. So by that logic Cachex's main interface can't do much beyond any(); is it sufficient to add ExHashRing.Hash.hashable() to the typing on the router implementation itself?

I assume you found this via some type checking utility, would this work for that case?

probably-not commented 1 month ago

I actually don't think that type checkers can find this issue - since the router is dynamically dispatched because the module is decided at runtime, then tools like Dialyzer can't know that the type will cause a failure. I ran into this at runtime when testing out my cache, I was receiving a Protocol Not Implemented exception.

I'm not entirely sure if there's a good solution because of the dynamic dispatching thing, but I would definitely update the specs on the implementation and update the implementation's docs, and ideally wherever you recommend the Cachex.Router.Ring router in the docs (it's recommended in a few of the guides) I would add a note explaining the limitation just to ensure that people reading the guides also see the note.