This project is archived and is in readonly mode.

#5611 ✓duplicate
Jim Wilson

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

    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

    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

    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

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

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

  • Jim Wilson

    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

    Santiago Pastorino September 14th, 2010 @ 06:22 AM

    • Milestone cleared.
    • Assigned user set to “Santiago Pastorino”
  • 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

    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

  • Jim Wilson

    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

    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
  • Ryan Bigg

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

    Automatic cleanup of spam.

  • Ryan Bigg

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

    • Tag cleared.

    Automatic cleanup of spam.

  • Santiago Pastorino

    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

    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

    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.

  • Denis Odorcic

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

    Perfect, makes sense, thanks :)

  • Jim Wilson

    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
  • Repository
  • Santiago Pastorino

    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>

Attachments

Referenced by

Pages