Some refactor and heckle-proofing lazy_load and others

lib/dm-core/model.rb

@@ -192,6 +192,10 @@ def repositories
     def properties(repository_name = default_repository_name)
       @properties[repository_name]
     end
+
+    def eager_properties(repository_name = default_repository_name)
+      properties(repository_name).reject {|p| p.lazy? }
+    end
 
     # @api private
     def properties_with_subclasses(repository_name = default_repository_name)

lib/dm-core/property.rb

@@ -372,21 +372,15 @@ def custom?
     #-
     # @api private
     def get(resource)
-      new_record = resource.new_record?
-
-      lazy_load(resource) unless new_record || resource.attribute_loaded?(name)
+      lazy_load(resource)
 
       value = get!(resource)
 
-      if !resource.original_values.key?(name) && track.in?(:get, :hash)
-        val = value.try_dup
-        val = val.hash if track == :hash
-        resource.original_values[name] = val
-      end
+      set_original_value(resource, value)
 
-      # Why do we care whether options[:default] is nil. The default value of nil will be
-      # applied either way
-      if value.nil? && new_record && !resource.attribute_loaded?(name)
+      # [YK] Why did we previously care whether options[:default] is nil. 
+      # The default value of nil will be applied either way
+      if value.nil? && resource.new_record? && !resource.attribute_loaded?(name)
         value = default_for(resource)
         set(resource, value)
       end
@@ -398,21 +392,39 @@ def get!(resource)
       resource.instance_variable_get(instance_variable_name)
     end
 
+    def set_original_value(resource, val)
+      unless resource.original_values.key?(name)
+        val = val.try_dup
+        val = val.hash if track == :hash
+        resource.original_values[name] = val
+      end
+    end
+
     # Provides a standardized setter method for the property
     #
     # @raise <ArgumentError> "+resource+ should be a DataMapper::Resource, but was ...."
     #-
     # @api private
     def set(resource, value)
-      lazy_load(resource) unless resource.new_record? || resource.attribute_loaded?(name)
+      # [YK] We previously checked for new_record? here, but lazy loading
+      # is blocked anyway if we're in a new record by by 
+      # Resource#reload_attributes. This may eventually be useful for
+      # optimizing, but let's (a) benchmark it first, and (b) do
+      # whatever refactoring is necessary, which will benefit from the
+      # centralize checking
+      lazy_load(resource)
+
       new_value = typecast(value)
       old_value = get!(resource)
 
       # skip setting the property if the new value is equal
       # to the old value, and the old value was defined
-      return if new_value == old_value && resource.attribute_loaded?(name)
+      # ---
+      # [YK] Why bother? Does this change the result at all?
+      # ---
+      # return if new_value == old_value && resource.attribute_loaded?(name)
 
-      resource.original_values[name] = old_value unless resource.original_values.has_key?(name)
+      set_original_value(resource, old_value)
 
       set!(resource, new_value)
     end
@@ -425,14 +437,11 @@ def set!(resource, value)
     #-
     # @api private
     def lazy_load(resource)
-      # TODO: refactor this section
-      contexts = if lazy?
-        name
-      else
-        model.properties(resource.repository.name).reject do |property|
-          property.lazy? || resource.attribute_loaded?(property.name)
-        end
-      end
+      return if resource.attribute_loaded?(name)
+      # If we're trying to load a lazy property, load it. Otherwise, lazy-load
+      # any properties that should be eager-loaded but were not included
+      # in the original :fields list
+      contexts = lazy? ? name : model.eager_properties(resource.repository.name)
       resource.send(:lazy_load, contexts)
     end
 

spec/integration/repository_spec.rb

@@ -42,7 +42,7 @@ class SerialFinderSpec
       sfs = @repository.read_one(@query.merge(:fields => [ :id ], :limit => 1))
       sfs.should be_a_kind_of(@model)
       sfs.should_not be_a_new_record
-
+    
       sfs.attribute_loaded?(:sample).should be_false
       sfs.sample.should_not be_nil
     end

spec/spec_helper.rb

@@ -13,6 +13,7 @@
 
 # setup mock adapters
 DataMapper.setup(:default, "sqlite3::memory:")
+DataMapper.setup(:default2, "sqlite3::memory:")
 
 [ :mock, :legacy, :west_coast, :east_coast ].each do |repository_name|
   DataMapper.setup(repository_name, "mock://localhost/#{repository_name}")

spec/unit/property_spec.rb

@@ -184,18 +184,35 @@ class Tomato
   end
 
   describe '#set' do
-    before do
+    before(:each) do
       Zoo.class_eval do
         property :name, String
         property :age, Integer
+        property :description, String, :lazy => true
       end
+      Zoo.auto_migrate!
+      Zoo.create(:name => "San Diego Zoo", :age => 888, 
+        :description => "Great Zoo")
       @resource = Zoo.new
     end
 
     it 'should typecast the value' do
       @resource.age = "888"
       @resource.age.should == 888
     end
+
+    it "should lazy load itself first" do
+      resource = Zoo.first
+      resource.description = "Still a Great Zoo"
+      resource.original_values[:description].should == "Great Zoo"
+    end
+
+    it "should only set original_values once" do
+      resource = Zoo.first
+      resource.description = "Still a Great Zoo"
+      resource.description = "What can I say. This is one great Zoo"
+      resource.original_values[:description].should == "Great Zoo"
+    end
   end
 
   describe '#set!' do
@@ -248,6 +265,36 @@ class Tomato
     # Do we mean for attribute_loaded? to be public?
     zoo.attribute_loaded?(:id).should == true
   end
+
+  it "should lazily load other non-loaded, non-lazy fields" do
+    # This somewhat contorted setup is to successfully test that 
+    # the list of eager properties to be loaded when it's initially
+    # missing is, in fact, repository-scoped
+    Zoo.class_eval do
+      property :id, DataMapper::Types::Serial
+      property :name, String, :lazy => true
+      property :address, String, :lazy => true
+
+      repository(:default2) do
+        property :name, String
+        property :address, String
+      end
+    end
+
+    repository(:default2) do 
+      Zoo.auto_migrate!
+      Zoo.create(:name => "San Diego Zoo", :address => "San Diego")
+    end
+    repository(:default2) do
+      zoo = Zoo.first(:fields => [:id])
+
+      zoo.attribute_loaded?(:name).should == false
+      zoo.attribute_loaded?(:address).should == false
+      zoo.name
+      zoo.attribute_loaded?(:name).should == true
+      zoo.attribute_loaded?(:address).should == true
+    end
+  end
 
   it "should use a custom type Name property" do
     Zoo.class_eval do

spec/unit/resource_spec.rb

@@ -54,7 +54,7 @@ class Planet
   property :age, Integer
   property :core, String, :accessor => :private
   property :type, Discriminator
-  property :data, Object
+  property :data, Object, :track => :get
 
   repository(:legacy) do
     property :cowabunga, String
@@ -109,7 +109,7 @@ class Cyclist
 
 describe DataMapper::Resource do
 
-  before do
+  before(:each) do
     Planet.auto_migrate!
     Cyclist.auto_migrate!
   end
@@ -119,7 +119,7 @@ class Cyclist
   end
 
   describe '#save' do
-    before do
+    before(:each) do
       @adapter = repository(:default).adapter
     end