Rails Ruby on Rails → 3.0.2

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.

• You flagged this item as spam.
  

  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.

  • consistency.diff 1 KB

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  Jim Wilson September 11th, 2010 @ 10:55 PM

  Wait... Its still not quite right. Let me debug some more.

• You flagged this item as spam.
  

  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:
  

  tmp/cache/F88/640/views/localhost:3000/games/index...
  

  I assume this is by design.
• 

  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 ;).

• You flagged this item as spam.
  

  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 ActiveSupport

     DIR_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)

     super(options)
    

    @@ -156,7 +156,7 @@ module ActiveSupport

     # 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)}
    

  •    fname.gsub(UNESCAPE_FILENAME_CHARS){|match| $1.to_i(16).chr}
     end
    
     # Delete empty directories in the cache.
    

    diff --git a/activesupport/test/caching_test.rb b/activesupport/test/caching_test.rb
    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

• You flagged this item as spam.
  

  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.

  • patch 1.5 KB

• You flagged this item as spam.
  

  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.

• 

  Jeremy Kemper October 15th, 2010 @ 11:02 PM

  • Milestone set to “3.0.2”

  [bulk edit]

• 

  Ryan Bigg October 16th, 2010 @ 02:32 AM

  • Tag changed from “sheepskin boots, testing cache sweeper” to “testing cache sweeper”

  Automatic cleanup of spam.

• 

  Ryan Bigg October 19th, 2010 @ 08:36 AM

  • Tag cleared.

  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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  Denis Odorcic November 7th, 2010 @ 08:04 PM

  Perfect, makes sense, thanks :)

• You flagged this item as spam.
  

  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