Fix change tracking when fetching from collections or associations

lib/her/model.rb

@@ -54,9 +54,6 @@ module Model
       method_for :destroy, :delete
       method_for :new, :get
 
-      # Define the default primary key
-      primary_key :id
-
       # Define default storage accessors for errors and metadata
       store_response_errors :response_errors
       store_metadata :metadata
@@ -70,6 +67,9 @@ module Model
 
       # Define matchers for attr? and attr= methods
       define_attribute_method_matchers
+
+      # Define the default primary key
+      primary_key :id
     end
   end
 end

lib/her/model/associations/has_many_association.rb

@@ -85,7 +85,7 @@ def create(attributes = {})
         def fetch
           super.tap do |o|
             inverse_of = @opts[:inverse_of] || @parent.singularized_resource_name
-            o.each { |entry| entry.send("#{inverse_of}=", @parent) }
+            o.each { |entry| entry.attributes[inverse_of] = @parent }
           end
         end
 

lib/her/model/attributes.rb

@@ -100,11 +100,6 @@ def get_attribute(attribute_name)
       end
       alias attribute get_attribute
 
-      # Return the value of the model `primary_key` attribute
-      def id
-        @attributes[self.class.primary_key]
-      end
-
       # Return `true` if the other object is also a Her::Model and has matching
       # data
       #
@@ -155,6 +150,7 @@ def instantiate_record(klass, parsed_data)
             attributes = klass.parse(record).merge(_metadata: parsed_data[:metadata],
                                                    _errors: parsed_data[:errors])
             klass.new(attributes).tap do |record|
+              record.instance_variable_set(:@changed_attributes, {})
               record.run_callbacks :find
             end
           end

lib/her/model/paths.rb

@@ -30,12 +30,28 @@ module ClassMethods
         #
         # @param [Symbol] value
         def primary_key(value = nil)
-          @_her_primary_key ||= begin
-            superclass.primary_key if superclass.respond_to?(:primary_key)
-          end
+          if value
+            value = value.to_sym
+
+            if value == :id
+              klass = self
 
-          return @_her_primary_key unless value
-          @_her_primary_key = value.to_sym
+              while (klass = klass.superclass).respond_to?(:primary_key)
+                if klass.primary_key != :id
+                  raise ArgumentError, 'cannot change primary key to :id if ancestors use other name'
+                end
+              end
+            else
+              alias_attribute :id, value
+            end
+
+            attributes value
+            @_her_primary_key = value
+          else
+            @_her_primary_key ||= begin
+              superclass.primary_key if superclass.respond_to?(:primary_key)
+            end
+          end
         end
 
         # Defines a custom collection path for the resource

lib/her/model/relation.rb

@@ -97,7 +97,6 @@ def find(*ids)
           @parent.request(request_params) do |parsed_data, response|
             if response.success?
               resource = @parent.new_from_parsed_data(parsed_data)
-              resource.instance_variable_set(:@changed_attributes, {})
               resource.run_callbacks :find
             else
               return nil

spec/model/dirty_spec.rb

@@ -8,16 +8,24 @@
         builder.use Her::Middleware::FirstLevelParseJSON
         builder.use Faraday::Request::UrlEncoded
         builder.adapter :test do |stub|
+          stub.get("/users")   { [200, {}, [{ id: 1, fullname: "Lindsay Fünke" }, { id: 2, fullname: "Maeby Fünke" }].to_json] }
           stub.get("/users/1") { [200, {}, { id: 1, fullname: "Lindsay Fünke" }.to_json] }
           stub.get("/users/2") { [200, {}, { id: 2, fullname: "Maeby Fünke" }.to_json] }
           stub.get("/users/3") { [200, {}, { user_id: 3, fullname: "Maeby Fünke" }.to_json] }
           stub.put("/users/1") { [200, {}, { id: 1, fullname: "Tobias Fünke" }.to_json] }
           stub.put("/users/2") { [400, {}, { errors: ["Email cannot be blank"] }.to_json] }
           stub.post("/users")  { [200, {}, { id: 1, fullname: "Tobias Fünke" }.to_json] }
+          stub.get("/users/1/posts") { [200, {}, [{ id: 1, user_id: 1, body: "Hello" }].to_json] }
+          stub.get("/users/1/posts/1") { [200, {}, { id: 1, user_id: 1, body: "Hello" }.to_json] }
         end
       end
 
+      spawn_model "Foo::Post" do
+        belongs_to :user
+        attributes :body
+      end
       spawn_model "Foo::User" do
+        has_many :posts
         attributes :fullname, :email
       end
       spawn_model "Dynamic::User" do
@@ -74,8 +82,34 @@
       end
     end
 
+    context "for an existing resource from an association" do
+      let(:post) { Foo::User.find(1).posts.find(1) }
+      it "has no changes" do
+        expect(post.changes).to be_empty
+        expect(post).to_not be_changed
+      end
+    end
+
+    context "for an existing resource from an association collection" do
+      let(:post) { Foo::User.find(1).posts.first }
+      it "has no changes" do
+        expect(post.changes).to be_empty
+        expect(post).to_not be_changed
+      end
+    end
+
+    context "for resources from a collection" do
+      let(:users) { Foo::User.all.fetch }
+      it "has no changes" do
+        users.each do |user|
+          expect(user.changes).to be_empty
+          expect(user).to_not be_changed
+        end
+      end
+    end
+
     context "for new resource" do
-      let(:user) { Foo::User.new(fullname: "Lindsay Fünke") }
+      let(:user) { Foo::User.new(id: 1, fullname: "Lindsay Fünke") }
       it "has changes" do
         expect(user).to be_changed
       end
@@ -86,6 +120,26 @@
         user.save
         expect(user).not_to be_changed
       end
+      it "tracks a dirty :id primary key" do
+        expect(user.changes).to eq('id' => [nil, 1], "fullname" => [nil, "Lindsay Fünke"])
+        user.id = 2
+        expect(user.changes).to eq('id' => [nil, 2], "fullname" => [nil, "Lindsay Fünke"])
+      end
+      it "tracks a dirty custom primary key" do
+        user = Dynamic::User.new(user_id: 1, fullname: "Lindsay Fünke")
+        expect(user.changes).to eq('user_id' => [nil, 1], "fullname" => [nil, "Lindsay Fünke"])
+        user.user_id = 2
+        expect(user.changes).to eq('user_id' => [nil, 2], "fullname" => [nil, "Lindsay Fünke"])
+        user.id = 3
+        expect(user.changes).to eq('user_id' => [nil, 3], "fullname" => [nil, "Lindsay Fünke"])
+      end
+    end
+
+    context "for a new resource from an association" do
+      let(:post) { Foo::User.find(1).posts.build }
+      it "has changes" do
+        expect(post).to be_changed
+      end
     end
   end
 end

spec/model/orm_spec.rb

@@ -58,7 +58,7 @@
     end
 
     it "handles new resource with custom primary key" do
-      @new_user = Foo::AdminUser.new(fullname: "Lindsay Fünke", id: -1)
+      @new_user = Foo::AdminUser.new(fullname: "Lindsay Fünke")
       expect(@new_user).to be_new
 
       @existing_user = Foo::AdminUser.find(1)

spec/model/paths_spec.rb

@@ -182,14 +182,28 @@ class User < Foo::Model; end
       describe "#request_path" do
         it "uses the correct primary key attribute" do
           expect(User.new(UserId: "foo").request_path).to eq("users/foo")
-          expect(User.new(id: "foo").request_path).to eq("users")
         end
 
         it "replaces :id with the appropriate primary key" do
           expect(Customer.new(customer_id: "joe").request_path).to eq("customers/joe")
-          expect(Customer.new(id: "joe").request_path).to eq("customers")
         end
       end
+
+      it 'prevents subclasses from using :id again' do
+        expect {
+          Class.new(User) do
+            primary_key :id
+          end
+        }.to raise_error(ArgumentError, /:id/)
+      end
+
+      it 'allow subclasses to use yet another primary key name' do
+        expect {
+          Class.new(User) do
+            primary_key :subclass_id
+          end
+        }.to_not raise_error
+      end
     end
   end