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

MemoryStore#fetch with 7.1 defaults can't set expires_at via block opts #49796

Closed
mroach opened this issue Oct 26, 2023 · 3 comments
Closed

MemoryStore#fetch with 7.1 defaults can't set expires_at via block opts #49796

mroach opened this issue Oct 26, 2023 · 3 comments

Comments

@mroach
Copy link

mroach commented Oct 26, 2023

Steps to reproduce

In a Rails app, use the following config:

config.cache = :memory_store
config.load_defaults(7.1)

Now, use Rails.cache.fetch passing a block and try to set options.expires_at. An ArgumentError is raised:

ArgumentError: Either :expires_in or :expires_at can be supplied, but not both

Additionally, the cache store's options are mutated with the expries_at option set in the block

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
end

require "active_support"
require "active_support/core_ext/object/blank"
require "minitest/autorun"

class TestApp < Rails::Application
  config.root = "/tmp"
  config.eager_load = false
  config.cache = :memory_store
  config.load_defaults(7.1)

  Rails.application.initialize!
end

class BugTest < Minitest::Test
  def test_memory_store_fetch_opts
    Rails.cache.fetch(SecureRandom.alphanumeric) do |_key, opts|
      opts.expires_at = 1.minute.from_now
      Time.now.to_s
    end
  ensure
    puts "Cache options are now #{Rails.cache.options}"
  end
end

Expected behavior

Cache is written with the given expires_at value.

Actual behavior

An ArgumentError is raised saying that we're trying to set two options. Rails.cache options are mutated with the options from the block.

# Running:

Cache options are now {:compress=>true, :compress_threshold=>1024, :expires_at=>Thu, 26 Oct 2023 12:58:56.191690312 UTC +00:00}
E

Error:
BugTest#test_memory_store_fetch_opts:
ArgumentError: Either :expires_in or :expires_at can be supplied, but not both

System configuration

Rails version: main

Ruby version: 3.2.2

@mroach mroach changed the title MemoryStore#fetch with 7.1 defaults can't set expires_at MemoryStore#fetch with 7.1 defaults can't set expires_at via block opts Oct 26, 2023
@HolyWalley
Copy link
Contributor

If no one minds, I'll check it out

@HolyWalley
Copy link
Contributor

HolyWalley commented Oct 26, 2023

I don't find this issue is related to config.load_defaults(7.1), you can try with 7.0, 6.0 or other, it's reproducible on main.

The problem is in this merge

when we delete expires_at from hash here we delete it from call_options, which is different object from options (which still contains expires_at), so, merge leads to this behaviour

I'll work on fix

@HolyWalley
Copy link
Contributor

HolyWalley commented Oct 26, 2023

Well, turns out the actual problem is a bit more serious (but easy to fix)

method merge_options in case if no call_options were specified returns Store@options as a result of merge, this way when we change our options in block, what we actually do - we change global rails cache options. I'll make a PR with a fix soon

jonathanhefner added a commit that referenced this issue Oct 26, 2023
…rwriten

[Fix #49796] Prevent global cache options being overwritten
jonathanhefner added a commit that referenced this issue Oct 26, 2023
…rwriten

[Fix #49796] Prevent global cache options being overwritten

(cherry picked from commit e558c39)
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