Always treat id and custom primary keys as proper attributes

Not doing do was leading to change tracking not being applied in some
circumstances.

With a custom primary key, id is now treated as a formal alias. This
alters the behaviour slightly but setting an attribute with id= and
then getting a totally different value back with id is just as useless
as it is confusing. Some tests have had to be adjusted accordingly.

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/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
       #

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
+              # Undo any previous aliasing.
+              if attribute_aliases.reject! { |_, v| v == value.to_s }
+                attribute_method_matchers.each do |matcher|
+                  method_name = matcher.method_name(value)
+                  undef_method method_name if method_defined?(method_name)
+                end
+              end
+            else
+              alias_attribute :id, value
+            end
 
-          return @_her_primary_key unless value
-          @_her_primary_key = value.to_sym
+            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

spec/model/dirty_spec.rb

@@ -109,7 +109,7 @@
     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
@@ -120,6 +120,19 @@
         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

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,12 +182,10 @@ 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
     end