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

Improve API for "fetch" methods #510

Open
dylanahsmith opened this issue Dec 20, 2021 · 1 comment
Open

Improve API for "fetch" methods #510

dylanahsmith opened this issue Dec 20, 2021 · 1 comment

Comments

@dylanahsmith
Copy link
Contributor

dylanahsmith commented Dec 20, 2021

The fetch methods (e.g. fetch_#{association_name}) for loading or accessing an already loaded cached record(s) seems inappropriate because

  1. It doesn't make it clear to those unfamiliar with this library that the data could come from the cache.
  2. It makes it easy for those using this library (naturally for performance reasons) to introduce uncached N+1 queries through lazy loading

Problem 1 seems like it came from the fetch method name being taken from Rails.cache.fetch, but the cache context has since been lost. This problem could be avoided by prefixing fetch methods with cache_ (e.g. cache_fetch_#{association_name}), where alias_method could be used for backwards compatibility.

Problem 2 would benefit from having an alternative method that raises instead of lazily loading when the data isn't already loaded (e.g. cacheable_#{association_name}). This alternative method should be used if the data is expected to already be loaded so that it can be used in a loop. Note that the accessed value may not have come from the cache or a cache_fetch_* method, given the Active Record association fallback (hence the suggested cacheable_ prefix).

@dylanahsmith
Copy link
Contributor Author

Note that recursively embedded associations don't even fallback to loading from the cache, which makes the API even more confusing. That also means that if we introduced cache_fetch_#{association_name} methods for those associations, they would be expected to not just be an alias to the existing fetch_#{association_name} accessors that always delegate to the active record association, but would instead be expected to behave more like the prefetch_associations method that loads the owner record from the cache, if the association hasn't already been loaded.

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

1 participant