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.

Sign up for GitHub

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

Jump to bottom

When using Rails.cache.fetch, Please show warning at mistyping the expires_in option. #39850

Closed
takundao71 opened this issue Jul 15, 2020 · 5 comments · Fixed by #40964
Closed

When using Rails.cache.fetch, Please show warning at mistyping the expires_in option. #39850

takundao71 opened this issue Jul 15, 2020 · 5 comments · Fixed by #40964

Comments

@takundao71
Copy link

Problem

When using Rails.cache.fetch, it is easy to mistype the expires_in option.
Incorrectly specified in expires or expired_in,
I haven't noticed it for a long time.

This article specifies expired_in incorrectly.
This article is at the top of the search results, so I think many people have made the same mistakes.
https://morizyun.github.io/ruby/rails-function-cache-fetch-write-delete.html

Improvement plan

If we specify incorrectly such as expires or expired_in,
Please show warning.
https://github.com/rails/rails/blob/master/activesupport/lib/active_support/cache.rb
@victorperez
Copy link
Contributor

victorperez commented Jul 15, 2020 •
edited

Rails.cache.fetch allow to pass some universal options as expires_in, but it also allows you to pass any other option that will be handled by the specific cache store implementation you are using (including custom cache stores). That makes harder to determinate if an option is valid.

@mrpinsky
Copy link
Contributor

I have also run into this problem. Mistyping expire_in: 2.seconds instead of expires_in: 2.seconds caused us to accidentally cache an endpoint for 6 hours when we wanted 2 seconds. I understand @victorperez's concern of clobbering other keys, but in the case of keys that are 1 character off from global options, I think that's a pretty low risk. What if the cache store was just more forgiving for these easy, dangerous issues by considering them all as aliases for :expires_in?

@rails-bot
Copy link

rails-bot bot commented Dec 27, 2020

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-0-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Dec 27, 2020
@rafaelfranca
Copy link
Member

What if the cache store was just more forgiving for these easy, dangerous issues by considering them all as aliases for :expires_in

That is a good plan. Do any of you are interested in opening a PR to fix this issue adding an alias to expires_in?

@rails-bot rails-bot bot removed the stale label Dec 28, 2020
@mrpinsky
Copy link
Contributor

I am. I’ll get a PR up tomorrow

@mrpinsky mrpinsky mentioned this issue Dec 28, 2020
Merged
mrpinsky pushed a commit to mrpinsky/rails that referenced this issue Dec 29, 2020
Support aliases to expires_in for cache stores
fd8c367 
The `expires_in` option is easy to misremember or mistype as `expire_in`
or `expired_in`, with potentially harmful results. If a developer wants
to cache a value for only 2 seconds but mistakenly types
`expire_in: 2.seconds`, that value will instead be cached for the
default time, likely 6 hours, meaning that users of the site will see
the same data for much longer than they should, and the only recovery
(short of waiting for the 6 hours to elapse) is to manually expire all
relevant keys. This commit allows cache stores to recognize these common
typos as aliases by normalizing them before consuming the options.

In general, we should be careful about adding too many aliases for
options to the cache stores, since each option key used by the base
Cache::Store class is one fewer key that each cache implementation can
customize for itself. This case was approved because of the similarity
of the aliases to the root key and the potential damage caused by
mistaking them.

Fixes rails#39850.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support aliases to expires_in for cache stores mrpinsky/rails
4 participants
@rafaelfranca @victorperez @takundao71 @mrpinsky