From cd5efbb24586aa8fbfb5ca29d6edccf97a74b6c0 Mon Sep 17 00:00:00 2001 From: James Le Cuirot Date: Fri, 14 Nov 2014 16:27:08 +0000 Subject: [PATCH 1/3] Fix change tracking when fetching from collections or associations --- lib/her/model/attributes.rb | 1 + lib/her/model/relation.rb | 1 - spec/model/dirty_spec.rb | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/lib/her/model/attributes.rb b/lib/her/model/attributes.rb index 9bd95ecf..e5a99b98 100644 --- a/lib/her/model/attributes.rb +++ b/lib/her/model/attributes.rb @@ -155,6 +155,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 diff --git a/lib/her/model/relation.rb b/lib/her/model/relation.rb index 6fb22723..cdd63beb 100644 --- a/lib/her/model/relation.rb +++ b/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 diff --git a/spec/model/dirty_spec.rb b/spec/model/dirty_spec.rb index f51399e2..bba89a67 100644 --- a/spec/model/dirty_spec.rb +++ b/spec/model/dirty_spec.rb @@ -8,16 +8,23 @@ 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/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,6 +81,24 @@ 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 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") } it "has changes" do @@ -87,6 +112,13 @@ expect(user).not_to be_changed 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 # From 5ca2e0897aec36df76833a5243899490f7d78e88 Mon Sep 17 00:00:00 2001 From: James Le Cuirot Date: Fri, 10 Jun 2016 12:41:17 +0100 Subject: [PATCH 2/3] Fix change tracking when fetching from an association collection --- lib/her/model/associations/has_many_association.rb | 2 +- spec/model/dirty_spec.rb | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/her/model/associations/has_many_association.rb b/lib/her/model/associations/has_many_association.rb index 837e0409..23ed797a 100644 --- a/lib/her/model/associations/has_many_association.rb +++ b/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 diff --git a/spec/model/dirty_spec.rb b/spec/model/dirty_spec.rb index bba89a67..f0efd22b 100644 --- a/spec/model/dirty_spec.rb +++ b/spec/model/dirty_spec.rb @@ -15,6 +15,7 @@ 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 @@ -89,6 +90,14 @@ 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 From e0e41f54a3912f13d31a018b724df3b6223777f2 Mon Sep 17 00:00:00 2001 From: James Le Cuirot Date: Mon, 22 Jan 2018 14:34:31 +0000 Subject: [PATCH 3/3] 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. Note that you cannot set the primary key back to :id if it was set to something else in an ancestor class. Undoing attribute aliases leads to infinite loops. --- lib/her/model.rb | 6 +++--- lib/her/model/attributes.rb | 5 ----- lib/her/model/paths.rb | 26 +++++++++++++++++++++----- spec/model/dirty_spec.rb | 15 ++++++++++++++- spec/model/orm_spec.rb | 2 +- spec/model/paths_spec.rb | 18 ++++++++++++++++-- 6 files changed, 55 insertions(+), 17 deletions(-) diff --git a/lib/her/model.rb b/lib/her/model.rb index 43929e5e..8e058149 100644 --- a/lib/her/model.rb +++ b/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 diff --git a/lib/her/model/attributes.rb b/lib/her/model/attributes.rb index e5a99b98..87995b80 100644 --- a/lib/her/model/attributes.rb +++ b/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 # diff --git a/lib/her/model/paths.rb b/lib/her/model/paths.rb index f4a9bc22..a24b2d2c 100644 --- a/lib/her/model/paths.rb +++ b/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 diff --git a/spec/model/dirty_spec.rb b/spec/model/dirty_spec.rb index f0efd22b..746e6d37 100644 --- a/spec/model/dirty_spec.rb +++ b/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 diff --git a/spec/model/orm_spec.rb b/spec/model/orm_spec.rb index 001683c6..9050b44d 100644 --- a/spec/model/orm_spec.rb +++ b/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) diff --git a/spec/model/paths_spec.rb b/spec/model/paths_spec.rb index 5b96b5f8..5fc54a31 100644 --- a/spec/model/paths_spec.rb +++ b/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