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

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

Open
probably-not opened this issue Oct 21, 2024 · 3 comments

Comments

@probably-not
Copy link

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
Copy link
Owner

whitfin commented Oct 21, 2024

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
Copy link
Author

probably-not commented Oct 22, 2024

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.

@BradS2S
Copy link
Contributor

BradS2S commented Dec 7, 2024

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.

Could a runtime check raise a more helpful error message than the default Protocol.UndefinedError message?

def check_key(key) do
    try do
      String.Chars.impl_for!(key)
      key
    rescue
      e in Protocol.UndefinedError ->
        raise ArgumentError, "Key must implement String.Chars protocol"
    end
  end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants