This project is archived and is in readonly mode.
active_support/cache/file_store.rb is this correct?
Reported by Jim Wilson | September 10th, 2010 @ 11:41 PM | in 3.0.2
This is crashing on me when I try and run a functional test of a cache sweeper in the test env.
I wonder if this shouldn't be "match.ord.to_s(16)"??
# Translate a file path into a key.
def file_path_key(path)
fname = path[cache_path.size, path.size].split(File::SEPARATOR, 4).last
fname.gsub(UNESCAPE_FILENAME_CHARS){|match| $1.ord.to_s(16)}
end
Comments and changes to this ticket
-
Santiago Pastorino September 11th, 2010 @ 03:22 AM
- Importance changed from “” to “Low”
Can you try to do a failing test case? or at least upload an app with the tests you're mentioning.
Thanks. -
Grzegorz Kazulak September 11th, 2010 @ 09:47 AM
This should be anyways changed to "match.ord.to_s(16)" to preserve consistency with rest of the methods using gsub.
Attached a patch for this. -
Jim Wilson September 11th, 2010 @ 08:37 PM
OK, I dug into this a bit deeper and I think I have both a test case showing failure and a fix. My prior suggested change just prevents a whiny nil dump, but doesn't fix the inherent problem.
I am running ruby 1.9.2-p0, Rails 3.0.0, on a Ubuntu box. I ran into problem as my former (2.3.x) action caching and cache sweeping was failing.
Below is a log showing failure from running "rails c", the fix, and then it apparently working.
Fragments from active_support/cache/file_store.rb. Current code:
UNESCAPE_FILENAME_CHARS = /%[0-9A-F]{2}/ # Translate a file path into a key. def file_path_key(path) fname = path[cache_path.size, path.size].split(File::SEPARATOR, 4).last fname.gsub(UNESCAPE_FILENAME_CHARS){|match| $1.ord.to_s(16)} end jwilson@osprey:~/wg/bridge$ rails c >> t = ActionController::Base.cache_store.send(:key_file_path, "views/index") => "/home/jwilson/wg/bridge/tmp/cache/4E4/5C0/views%2Findex" >> ActionController::Base.cache_store.send(:file_path_key, t) NoMethodError: undefined method `ord' for nil:NilClass from /home/jwilson/.rvm/gems/ruby-1.9.2-p0@cosb/gems/activesupport-3.0.0/lib/active_support/whiny_nil.rb:48:in `method_missing' from /home/jwilson/.rvm/gems/ruby-1.9.2-p0@cosb/gems/activesupport-3.0.0/lib/active_support/cache/file_store.rb:164:in `block in file_path_key' from /home/jwilson/.rvm/gems/ruby-1.9.2-p0@cosb/gems/activesupport-3.0.0/lib/active_support/cache/file_store.rb:164:in `gsub' from /home/jwilson/.rvm/gems/ruby-1.9.2-p0@cosb/gems/activesupport-3.0.0/lib/active_support/cache/file_store.rb:164:in `file_path_key' from (irb):2 from /home/jwilson/.rvm/gems/ruby-1.9.2-p0@cosb/gems/railties-3.0.0/lib/rails/commands/console.rb:44:in `start' from /home/jwilson/.rvm/gems/ruby-1.9.2-p0@cosb/gems/railties-3.0.0/lib/rails/commands/console.rb:8:in `start' from /home/jwilson/.rvm/gems/ruby-1.9.2-p0@cosb/gems/railties-3.0.0/lib/rails/commands.rb:23:in `<top (required)>' from script/rails:6:in `require' from script/rails:6:in `<main>' >>
with potential fix
UNESCAPE_FILENAME_CHARS = /%([0-9A-F]{2})/ # create a capture group # Translate a file path into a key. def file_path_key(path) fname = path[cache_path.size, path.size].split(File::SEPARATOR, 4).last # e.g., convert %(2F) to the character "/" fname.gsub(UNESCAPE_FILENAME_CHARS){|match| $1.to_i(16).chr} end jwilson@osprey:~/wg/bridge$ rails c Loading development environment (Rails 3.0.0) >> t = ActionController::Base.cache_store.send(:key_file_path, "views/index") => "/home/jwilson/wg/bridge/tmp/cache/4E4/5C0/views%2Findex" >> ActionController::Base.cache_store.send(:file_path_key, t) => "views/index" >>
Edited by Rohit Arondekar for formating.
-
Jim Wilson September 11th, 2010 @ 10:55 PM
Wait... Its still not quite right. Let me debug some more.
-
Jim Wilson September 12th, 2010 @ 03:07 PM
Ok, it is working correctly with above patch. I had an incorrect expire_fragment regexp.
I do see that cache fragments are stored in an encoded fashion:
jwilson@osprey:~/wg/bridge$ dir tmp/cache/F88/640/ tmp/cache/F88/640/views%2Flocalhost%3A3000%2Fgames%2Findex%3Fid%3D1
In 2.3.x they would be stored as:
I assume this is by design.tmp/cache/F88/640/views/localhost:3000/games/index...
-
Santiago Pastorino September 14th, 2010 @ 06:22 AM
- Milestone cleared.
- Assigned user set to “Santiago Pastorino”
-
Santiago Pastorino September 14th, 2010 @ 06:24 AM
To apply the provided patch we need tests to show the error.
Please can one of you make a test case, thank you ;). -
Jim Wilson September 14th, 2010 @ 03:43 PM
Added a test that fails without the patch and passes with it. git diff below
git diff -p
diff --git a/activesupport/lib/active_support/cache/file_store.rb b/activesupport/lib/active_support/cache/file_store.rb
index 84f6f29..3bfddae 100644
--- a/activesupport/lib/active_support/cache/file_store.rb +++ b/activesupport/lib/active_support/cache/file_store.rb @@ -12,7 +12,7 @@ module ActiveSupportDIR_FORMATTER = "%03X" ESCAPE_FILENAME_CHARS = /[^a-z0-9_.-]/i
- UNESCAPE_FILENAME_CHARS = /%[0-9A-F]{2}/
-
UNESCAPE_FILENAME_CHARS = /%([0-9A-F]{2})/
def initialize(cache_path, options = nil)
@@ -156,7 +156,7 @@ module ActiveSupportsuper(options)
# Translate a file path into a key. def file_path_key(path) fname = path[cache_path.size, path.size].split(File::SEPARATOR, 4).last
-
fname.gsub(UNESCAPE_FILENAME_CHARS){|match| $1.ord.to_s(16)}
-
diff --git a/activesupport/test/caching_test.rb b/activesupport/test/caching_test.rbfname.gsub(UNESCAPE_FILENAME_CHARS){|match| $1.to_i(16).chr} end # Delete empty directories in the cache.
index 28ef695..05a68e5 100644
--- a/activesupport/test/caching_test.rb +++ b/activesupport/test/caching_test.rb @@ -509,6 +509,11 @@ class FileStoreTest < ActiveSupport::TestCase assert_nil old_cache.read('foo') end end - def test_key_transformation
- key = @cache.send(:key_file_path, "views/index?id=1")
- assert_equal "views/index?id=1", @cache.send(:file_path_key, key)
- end end
class MemoryStoreTest < ActiveSupport::TestCase
-
Jim Wilson September 14th, 2010 @ 03:48 PM
Lighthouse is messing up the formatting. I am not real familiar with Lighthouse...
Here it is attached. -
Reinier de Lange September 21st, 2010 @ 02:05 PM
The patch works fine for me (ruby ee 1.8.7). Like to see this fixed for 3.0.1 as well.. monkey-patched for now.
-
Ryan Bigg October 16th, 2010 @ 02:32 AM
- Tag changed from “sheepskin boots, testing cache sweeper” to “testing cache sweeper”
Automatic cleanup of spam.
-
Santiago Pastorino November 7th, 2010 @ 05:19 PM
Jim Wilson great work but can you provide a patch following http://rails.lighthouseapp.com/projects/8994/sending-patches
I need you to do this to give credits to you for the patch. -
Denis Odorcic November 7th, 2010 @ 05:45 PM
Oh, looks like this is a duplicate of #5850 where I submitted a patch but nobody has touched in awhile. Should take a look at that one as well Santiago as its done a different way and I think the proper way although nobody commented on it.
-
Santiago Pastorino November 7th, 2010 @ 07:02 PM
Denis yeah I know ;).
I wanted to apply this patch before to give some credit to Jim because he worked a lot I guess :) and after that I will apply yours.
BTW please check #5850 I made a comment on it. -
Jim Wilson November 7th, 2010 @ 09:17 PM
I don't need any credit. Just take simpliest approach to fix it. However if you need me to do a patch I can... I am not a very active submitter...
-
Repository November 7th, 2010 @ 10:03 PM
(from [4e0477c9b75683372f662a614ae91b158120ebe8]) Test cache.key_file_path with separators ht. Jim Wilson [#5611] https://github.com/rails/rails/commit/4e0477c9b75683372f662a614ae91...
-
Repository November 7th, 2010 @ 10:03 PM
(from [c7fac8c6ea2590e3cafe50c1e71de78bad9303c1]) Test cache.key_file_path with separators ht. Jim Wilson [#5611] https://github.com/rails/rails/commit/c7fac8c6ea2590e3cafe50c1e71de...
-
Santiago Pastorino November 7th, 2010 @ 10:05 PM
- State changed from “new” to “duplicate”
Duplicate of #5850
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
Attachments
Referenced by
- 5611 active_support/cache/file_store.rb is this correct? (from [4e0477c9b75683372f662a614ae91b158120ebe8]) Test ca...
- 5611 active_support/cache/file_store.rb is this correct? (from [c7fac8c6ea2590e3cafe50c1e71de78bad9303c1]) Test ca...