Merge pull request #338 from marshall-lee/attributes_misbehavior

:attributes class method misbehavior

lib/her/model.rb

@@ -67,6 +67,9 @@ module Model
       # Configure ActiveModel callbacks
       extend ActiveModel::Callbacks
       define_model_callbacks :create, :update, :save, :find, :destroy, :initialize
+
+      # Define matchers for attr? and attr= methods
+      define_attribute_method_matchers
     end
   end
 end

lib/her/model/attributes.rb

@@ -158,6 +158,22 @@ def hash
         @attributes.hash
       end
 
+      # Assign attribute value (ActiveModel convention method).
+      #
+      # @private
+      def attribute=(attribute, value)
+        @attributes[attribute] = nil unless @attributes.include?(attribute)
+        self.send(:"#{attribute}_will_change!") if @attributes[attribute] != value
+        @attributes[attribute] = value
+      end
+
+      # Check attribute value to be present (ActiveModel convention method).
+      #
+      # @private
+      def attribute?(attribute)
+        @attributes.include?(attribute) && @attributes[attribute].present?
+      end
+
       module ClassMethods
         # Initialize a collection of resources with raw data from an HTTP request
         #
@@ -175,6 +191,25 @@ def new_from_parsed_data(parsed_data)
           new(parse(parsed_data[:data]).merge :_metadata => parsed_data[:metadata], :_errors => parsed_data[:errors])
         end
 
+        # Define attribute method matchers to automatically define them using ActiveModel's define_attribute_methods.
+        #
+        # @private
+        def define_attribute_method_matchers
+          attribute_method_suffix '='
+          attribute_method_suffix '?'
+        end
+
+        # Create a mutex for dynamically generated attribute methods or use one defined by ActiveModel.
+        #
+        # @private
+        def attribute_methods_mutex
+          @attribute_methods_mutex ||= if generated_attribute_methods.respond_to? :synchronize
+                                         generated_attribute_methods
+                                       else
+                                         Mutex.new
+                                       end
+        end
+
         # Define the attributes that will be used to track dirty attributes and validations
         #
         # @param [Array] attributes
@@ -184,22 +219,8 @@ def new_from_parsed_data(parsed_data)
         #     attributes :name, :email
         #   end
         def attributes(*attributes)
-          define_attribute_methods attributes
-
-          attributes.each do |attribute|
-            unless method_defined?(:"#{attribute}=")
-              define_method("#{attribute}=") do |value|
-                @attributes[:"#{attribute}"] = nil unless @attributes.include?(:"#{attribute}")
-                self.send(:"#{attribute}_will_change!") if @attributes[:"#{attribute}"] != value
-                @attributes[:"#{attribute}"] = value
-              end
-            end
-
-            unless method_defined?(:"#{attribute}?")
-              define_method("#{attribute}?") do
-                @attributes.include?(:"#{attribute}") && @attributes[:"#{attribute}"].present?
-              end
-            end
+          attribute_methods_mutex.synchronize do
+            define_attribute_methods attributes
           end
         end
 

spec/json_api/model_spec.rb

@@ -99,11 +99,11 @@
 
     end
 
-    spawn_model("Foo::User", Her::JsonApi::Model)
+    spawn_model("Foo::User", type: Her::JsonApi::Model)
   end
 
   it 'allows configuration of type' do
-    spawn_model("Foo::Bar", Her::JsonApi::Model) do
+    spawn_model("Foo::Bar", type: Her::JsonApi::Model) do
       type :foobars
     end
 

spec/model/attributes_spec.rb

@@ -299,5 +299,75 @@ def document?
       user.assign_attributes(fullname: 'Tobias Fünke')
       user.fullname?.should be_truthy
     end
+
+    context "when attribute methods are already defined" do
+      before do
+        class AbstractUser
+          attr_accessor :fullname
+
+          def fullname?
+            @fullname.present?
+          end
+        end
+        @spawned_models << :AbstractUser
+
+        spawn_model 'Foo::User', super_class: AbstractUser do
+          attributes :fullname
+        end
+      end
+
+      it "overrides getter method" do
+        Foo::User.generated_attribute_methods.instance_methods.should include(:fullname)
+      end
+
+      it "overrides setter method" do
+        Foo::User.generated_attribute_methods.instance_methods.should include(:fullname=)
+      end
+
+      it "overrides predicate method" do
+        Foo::User.generated_attribute_methods.instance_methods.should include(:fullname?)
+      end
+
+      it "defines setter that affects @attributes" do
+        user = Foo::User.new
+        user.fullname = 'Tobias Fünke'
+        user.attributes[:fullname].should eq('Tobias Fünke')
+      end
+
+      it "defines getter that reads @attributes" do
+        user = Foo::User.new
+        user.attributes[:fullname] = 'Tobias Fünke'
+        user.fullname.should eq('Tobias Fünke')
+      end
+
+      it "defines predicate that reads @attributes" do
+        user = Foo::User.new
+        user.fullname?.should be_falsey
+        user.attributes[:fullname] = 'Tobias Fünke'
+        user.fullname?.should be_truthy
+      end
+    end
+
+    if ActiveModel::VERSION::MAJOR < 4
+      it "creates a new mutex" do
+        expect(Mutex).to receive(:new).once.and_call_original
+        spawn_model 'Foo::User' do
+          attributes :fullname
+        end
+        Foo::User.attribute_methods_mutex.should_not eq(Foo::User.generated_attribute_methods)
+      end
+    else
+      it "uses ActiveModel's mutex" do
+        Foo::User.attribute_methods_mutex.should eq(Foo::User.generated_attribute_methods)
+      end
+    end
+
+    it "uses a mutex" do
+      spawn_model 'Foo::User'
+      expect(Foo::User.attribute_methods_mutex).to receive(:synchronize).once.and_call_original
+      Foo::User.class_eval do
+        attributes :fullname, :documents
+      end
+    end
   end
 end

spec/support/macros/model_macros.rb

@@ -3,13 +3,20 @@ module Testing
     module Macros
       module ModelMacros
         # Create a class and automatically inject Her::Model into it
-        def spawn_model(klass, model_type=Her::Model, &block)
+        def spawn_model(klass, options={}, &block)
+          super_class = options[:super_class]
+          model_type = options[:type] || Her::Model
+          new_class = if super_class
+                        Class.new(super_class)
+                      else
+                        Class.new
+                      end
           if klass =~ /::/
             base, submodel = klass.split(/::/).map{ |s| s.to_sym }
             Object.const_set(base, Module.new) unless Object.const_defined?(base)
             Object.const_get(base).module_eval do
               remove_const submodel if constants.map(&:to_sym).include?(submodel)
-              submodel = const_set(submodel, Class.new)
+              submodel = const_set(submodel, new_class)
               submodel.send(:include, model_type)
               submodel.class_eval(&block) if block_given?
             end