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

Return a copy of the cache entry when local_cache exists: #36656

Merged
merged 1 commit into from
Jul 15, 2019
Merged

Return a copy of the cache entry when local_cache exists: #36656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion activesupport/lib/active_support/cache/strategy/local_cache.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@ def delete_entry(key, options)
end

def fetch_entry(key, options = nil) # :nodoc:
@data.fetch(key) { @data[key] = yield }
entry = @data.fetch(key) { @data[key] = yield }
dup_entry = entry.dup
Copy link
Member

Choose a reason for hiding this comment

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

Why you need to dup the entry itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

the @data hash keeps a reference to the entry, if I don't dup it, user can modify the underlying value which would affect the cache.

dup_entry&.dup_value!
Copy link
Contributor

Choose a reason for hiding this comment

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

How could dup_entry ever be nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

in case you try to access a key that doesn't exist

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I'm stuck in the past, turns out nil.dup actually exist on modern rubies TIL.

Copy link

Choose a reason for hiding this comment

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

I think this would have to be a deep copy. .dup alone will still let you modify nested objects if present

image

Copy link
Member Author

Choose a reason for hiding this comment

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

No because dup_value! does a Marshal.load

rails/activesupport/lib/active_support/cache.rb

Line 793 in 457c815

@value = Marshal.load(Marshal.dump(@value))

dup_entry
end
end

Expand Down
9 changes: 9 additions & 0 deletions activesupport/test/cache/behaviors/local_cache_behavior.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,15 @@ def test_local_cache_of_write
end
end

def test_local_cache_of_read_returns_a_copy_of_the_entry
@cache.with_local_cache do
@cache.write(:foo, type: "bar")
value = @cache.read(:foo)
assert_equal("bar", value.delete(:type))
assert_equal({ type: "bar" }, @cache.read(:foo))
end
end

def test_local_cache_of_read
@cache.write("foo", "bar")
@cache.with_local_cache do
Expand Down