Patched Marshal#load to work with constant autoloading (active_support/dependecies.rb) (issue #8167)

activesupport/CHANGELOG.md

@@ -1,5 +1,10 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   Patched Marshal#load to work with constant autoloading.
+    Fixes autoloading with cache stores that relay on Marshal(MemCacheStore and FileStore). [fixes #8167]
+
+    *Uriel Katz*
+
 *   Make `Time.zone.parse` to work with JavaScript format date strings. *Andrew White*
 
 *   Add `DateTime#seconds_until_end_of_day` and `Time#seconds_until_end_of_day`

activesupport/lib/active_support/cache/file_store.rb

@@ -1,3 +1,4 @@
+require 'active_support/core_ext/marshal'
 require 'active_support/core_ext/file/atomic'
 require 'active_support/core_ext/string/conversions'
 require 'uri/common'

activesupport/lib/active_support/cache/mem_cache_store.rb

@@ -6,6 +6,7 @@ rescue LoadError => e
 end
 
 require 'digest/md5'
+require 'active_support/core_ext/marshal'
 
 module ActiveSupport
   module Cache

activesupport/lib/active_support/core_ext/marshal.rb

@@ -0,0 +1,21 @@
+module Marshal
+  class << self
+    def load_with_autoloading(source)
+      begin
+        load_without_autoloading(source)
+      rescue ArgumentError, NameError => exc
+        if exc.message.match(%r|undefined class/module (.+)|)
+          # try loading the class/module
+          $1.constantize
+          # if it is a IO we need to go back to read the object
+          source.rewind if source.respond_to?(:rewind)
+          retry
+        else
+          raise exc
+        end
+      end
+    end
+
+    alias_method_chain :load, :autoloading
+  end
+end

activesupport/test/caching_test.rb

@@ -1,6 +1,7 @@
 require 'logger'
 require 'abstract_unit'
 require 'active_support/cache'
+require 'dependecies_test_helpers'
 
 class CacheKeyTest < ActiveSupport::TestCase
   def test_entry_legacy_optional_ivars
@@ -562,6 +563,45 @@ module LocalCacheBehavior
   end
 end
 
+module AutoloadingCacheBehavior
+  include DependeciesTestHelpers
+  def test_simple_autoloading
+    with_autoloading_fixtures do
+      @cache.write('foo', E.new)
+    end
+
+    remove_constants(:E)
+    ActiveSupport::Dependencies.clear
+
+    with_autoloading_fixtures do
+      assert_kind_of E, @cache.read('foo')
+    end
+
+    remove_constants(:E)
+    ActiveSupport::Dependencies.clear
+  end
+
+  def test_two_classes_autoloading
+    with_autoloading_fixtures do
+      @cache.write('foo', [E.new, ClassFolder.new])
+    end
+
+    remove_constants(:E, :ClassFolder)
+    ActiveSupport::Dependencies.clear
+
+    with_autoloading_fixtures do
+      loaded = @cache.read('foo')
+      assert_kind_of Array, loaded
+      assert_equal 2, loaded.size
+      assert_kind_of E, loaded[0]
+      assert_kind_of ClassFolder, loaded[1]
+    end
+
+    remove_constants(:E, :ClassFolder)
+    ActiveSupport::Dependencies.clear
+  end
+end
+
 class FileStoreTest < ActiveSupport::TestCase
   def setup
     Dir.mkdir(cache_dir) unless File.exist?(cache_dir)
@@ -585,6 +625,7 @@ class FileStoreTest < ActiveSupport::TestCase
   include LocalCacheBehavior
   include CacheDeleteMatchedBehavior
   include CacheIncrementDecrementBehavior
+  include AutoloadingCacheBehavior
 
   def test_clear
     filepath = File.join(cache_dir, ".gitkeep")
@@ -745,6 +786,7 @@ class MemCacheStoreTest < ActiveSupport::TestCase
   include LocalCacheBehavior
   include CacheIncrementDecrementBehavior
   include EncodedKeyCacheBehavior
+  include AutoloadingCacheBehavior
 
   def test_raw_values
     cache = ActiveSupport::Cache.lookup_store(:mem_cache_store, :raw => true)

activesupport/test/core_ext/marshal_test.rb

@@ -0,0 +1,124 @@
+require 'abstract_unit'
+require 'active_support/core_ext/marshal'
+require 'dependecies_test_helpers'
+
+class MarshalTest < ActiveSupport::TestCase
+  include ActiveSupport::Testing::Isolation
+  include DependeciesTestHelpers
+
+  def teardown
+    ActiveSupport::Dependencies.clear
+    remove_constants(:E, :ClassFolder)
+  end
+
+  test "that Marshal#load still works" do
+    sanity_data = ["test", [1, 2, 3], {a: [1, 2, 3]}, ActiveSupport::TestCase]
+    sanity_data.each do |obj|
+      dumped = Marshal.dump(obj)
+      assert_equal Marshal.load_without_autoloading(dumped), Marshal.load(dumped)
+    end
+  end
+
+  test "that a missing class is autoloaded from string" do
+    dumped = nil
+    with_autoloading_fixtures do
+      dumped = Marshal.dump(E.new)
+    end
+
+    remove_constants(:E)
+    ActiveSupport::Dependencies.clear
+
+    with_autoloading_fixtures do
+      assert_kind_of E, Marshal.load(dumped)
+    end
+  end
+
+  test "that classes in sub modules work" do
+    dumped = nil
+    with_autoloading_fixtures do
+      dumped = Marshal.dump(ClassFolder::ClassFolderSubclass.new)
+    end
+
+    remove_constants(:ClassFolder)
+    ActiveSupport::Dependencies.clear
+
+    with_autoloading_fixtures do
+      assert_kind_of ClassFolder::ClassFolderSubclass, Marshal.load(dumped)
+    end
+  end
+
+  test "that more than one missing class is autoloaded" do
+    dumped = nil
+    with_autoloading_fixtures do
+      dumped = Marshal.dump([E.new, ClassFolder.new])
+    end
+
+    remove_constants(:E, :ClassFolder)
+    ActiveSupport::Dependencies.clear
+
+    with_autoloading_fixtures do
+      loaded = Marshal.load(dumped)
+      assert_equal 2, loaded.size
+      assert_kind_of E, loaded[0]
+      assert_kind_of ClassFolder, loaded[1]
+    end
+  end
+
+  test "that a real missing class is causing an exception" do
+    dumped = nil
+    with_autoloading_fixtures do
+      dumped = Marshal.dump(E.new)
+    end
+
+    remove_constants(:E)
+    ActiveSupport::Dependencies.clear
+
+    assert_raise(NameError) do
+      Marshal.load(dumped)
+    end
+  end
+
+  test "when first class is autoloaded and second not" do
+    dumped = nil
+    class SomeClass
+    end
+
+    with_autoloading_fixtures do
+      dumped = Marshal.dump([E.new, SomeClass.new])
+    end
+
+    remove_constants(:E)
+    self.class.send(:remove_const, :SomeClass)
+    ActiveSupport::Dependencies.clear
+
+    with_autoloading_fixtures do
+      assert_raise(NameError) do
+        Marshal.load(dumped)
+      end
+
+      assert_nothing_raised("E failed to load while we expect only SomeClass to fail loading") do
+        E.new
+      end
+
+      assert_raise(NameError, "We expected SomeClass to not be loaded but it is!") do
+        SomeClass.new
+      end
+    end
+  end
+
+  test "loading classes from files trigger autoloading" do
+    Tempfile.open("object_serializer_test") do |f|
+      with_autoloading_fixtures do
+        Marshal.dump(E.new, f)
+      end
+
+      f.rewind
+      remove_constants(:E)
+      ActiveSupport::Dependencies.clear
+
+      with_autoloading_fixtures do
+        assert_kind_of E, Marshal.load(f)
+      end
+    end
+  end
+end

activesupport/test/dependecies_test_helpers.rb

@@ -0,0 +1,27 @@
+module DependeciesTestHelpers
+  def with_loading(*from)
+    old_mechanism, ActiveSupport::Dependencies.mechanism = ActiveSupport::Dependencies.mechanism, :load
+    this_dir = File.dirname(__FILE__)
+    parent_dir = File.dirname(this_dir)
+    path_copy = $LOAD_PATH.dup
+    $LOAD_PATH.unshift(parent_dir) unless $LOAD_PATH.include?(parent_dir)
+    prior_autoload_paths = ActiveSupport::Dependencies.autoload_paths
+    ActiveSupport::Dependencies.autoload_paths = from.collect { |f| "#{this_dir}/#{f}" }
+    yield
+  ensure
+    $LOAD_PATH.replace(path_copy)
+    ActiveSupport::Dependencies.autoload_paths = prior_autoload_paths
+    ActiveSupport::Dependencies.mechanism = old_mechanism
+    ActiveSupport::Dependencies.explicitly_unloadable_constants = []
+  end
+
+  def with_autoloading_fixtures(&block)
+    with_loading 'autoloading_fixtures', &block
+  end
+
+  def remove_constants(*constants)
+    constants.each do |constant|
+      Object.send(:remove_const, constant) if Object.const_defined?(constant)
+    end
+  end
+end

activesupport/test/dependencies_test.rb

@@ -1,6 +1,7 @@
 require 'abstract_unit'
 require 'pp'
 require 'active_support/dependencies'
+require 'dependecies_test_helpers'
 
 module ModuleWithMissing
   mattr_accessor :missing_count
@@ -19,25 +20,7 @@ class DependenciesTest < ActiveSupport::TestCase
     ActiveSupport::Dependencies.clear
   end
 
-  def with_loading(*from)
-    old_mechanism, ActiveSupport::Dependencies.mechanism = ActiveSupport::Dependencies.mechanism, :load
-    this_dir = File.dirname(__FILE__)
-    parent_dir = File.dirname(this_dir)
-    path_copy = $LOAD_PATH.dup
-    $LOAD_PATH.unshift(parent_dir) unless $LOAD_PATH.include?(parent_dir)
-    prior_autoload_paths = ActiveSupport::Dependencies.autoload_paths
-    ActiveSupport::Dependencies.autoload_paths = from.collect { |f| "#{this_dir}/#{f}" }
-    yield
-  ensure
-    $LOAD_PATH.replace(path_copy)
-    ActiveSupport::Dependencies.autoload_paths = prior_autoload_paths
-    ActiveSupport::Dependencies.mechanism = old_mechanism
-    ActiveSupport::Dependencies.explicitly_unloadable_constants = []
-  end
-
-  def with_autoloading_fixtures(&block)
-    with_loading 'autoloading_fixtures', &block
-  end
+  include DependeciesTestHelpers
 
   def test_depend_on_path
     skip "LoadError#path does not exist" if RUBY_VERSION < '2.0.0'

guides/source/active_support_core_extensions.md

@@ -3720,6 +3720,27 @@ The auxiliary file is written in a standard directory for temporary files, but y
 
 NOTE: Defined in `active_support/core_ext/file/atomic.rb`.
 
+Extensions to `Marshal`
+--------------------
+
+### `load`
+
+Unpatched Marshal#load doesn't call constant_missing so in order to support ActiveSupport constant autoloading load is patched using alias_method_chain.
+
+The method accepts the same arguments as unpatched Marshal#load and the result of calling it will be the same.
+
+For example, ActiveSupport uses this method to read from cache(in FileStore):
+
+```ruby
+File.open(file_name) { |f| Marshal.load(f) }
+```
+
+If Marshal#load didn't support constant autoloading then various caching stores wouldn't too.
+
+WARNING. If a IO (e.g. a file) is used as source it needs to respond to rewind (which a normal file does) because if an exception is raised calling the original Marshal#load the file will be exhausted.
+
+NOTE: Defined in `active_support/core_ext/marshal.rb`.
+
 Extensions to `Logger`
 ----------------------