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

Fix forced cache miss for fetch. #24577

Merged
merged 1 commit into from
Apr 18, 2016
Merged

Conversation

mechanicles
Copy link
Contributor

@mechanicles mechanicles commented Apr 16, 2016

As per given document, if we call fetch method with 'force: true' option without
the block, it should force a cache miss. But it was happening properly,
so added this fix.

https://github.com/rails/rails/blob/master/activesupport/lib/active_support/cache.rb#L200-L204

# You may also specify additional options via the +options+ argument.
# Setting <tt>force: true</tt> will force a cache miss:
#
#   cache.write('today', 'Monday')
#   cache.fetch('today', force: true)  # => nil

UPDATE

Based on the discussion with Jeremy. Now we have only raised argument error if no block is passed to fetch method with option force: true is set.

r? @jeremy

@jeremy
Copy link
Member

jeremy commented Apr 17, 2016

Why are we adding force: true to #read? We already skip read_entry in #fetch if force is true.

@mechanicles
Copy link
Contributor Author

@jeremy We are skipping read_entry when the block is passed. If the block is not passed then it goes to else part and code in that section don't consider force: true.

I think I should move handling of force: true from #read method to #fetch method's else part like below

  ...
  else
    options && options[:force] ? nil : read(name, options)
  end

Let me know your thoughts.

@jeremy
Copy link
Member

jeremy commented Apr 17, 2016

I see. That would make sense, yes.

However, does this make sense overall? Forcing a cache miss only makes sense when a block is provided to write the new value. When no block is provided, fetch is a cache read where :force is meaningless.

I think it'd make sense to raise an ArgumentError instead. Note, the docs show an example of calling fetch force: true with no block—let's change that too.

@mechanicles mechanicles force-pushed the fix-fetch-cache-miss branch 4 times, most recently from 36bfd10 to 7c63762 Compare April 18, 2016 09:09
@mechanicles
Copy link
Contributor Author

mechanicles commented Apr 18, 2016

@jeremy Please check. I have raised an argument error if no block is passed to fetch method with option force: true is set. Also, updated doc and tests and refactored the code.

else
read(name, options)
fetch_without_block(name, options)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can get away without extracting separate implementation methods.

if block_given?
  # fetch = write if not present
elsif options[:force]
  raise ArgumentError, "…"
else
  # read …
end

- Raised an argument error if no block is passed to #fetch with
'force: true' option is set.
- Added tests for the same.
@mechanicles
Copy link
Contributor Author

@jeremy I updated the code as per your suggestions.

@jeremy jeremy merged commit a712acc into rails:master Apr 18, 2016
jeremy added a commit that referenced this pull request Apr 18, 2016
Fix forced cache miss for fetch when called without a block.
@mechanicles
Copy link
Contributor Author

Thanks, @jeremy 😃

@mechanicles mechanicles deleted the fix-fetch-cache-miss branch April 18, 2016 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants