From f9a8a777a0e5bd16f696cb421c79ef57802aa41b Mon Sep 17 00:00:00 2001 From: Weston Ganger Date: Fri, 18 May 2018 17:48:48 -0700 Subject: [PATCH] add PaperTrail.config.association_reify_error_behaviour --- CHANGELOG.md | 2 +- README.md | 5 +- .../paper_trail/install_generator.rb | 11 ++-- lib/paper_trail/config.rb | 2 +- lib/paper_trail/reifiers/has_one.rb | 13 ++++- spec/dummy_app/app/models/person.rb | 3 ++ spec/dummy_app/app/models/thing.rb | 2 + .../20110208155312_set_up_test_tables.rb | 1 + spec/models/person_spec.rb | 54 +++++++++++++++++-- spec/paper_trail/config_spec.rb | 8 +++ 10 files changed, 89 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b1c027ae..71eec157d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/). ### Added -- None +- [#1089] Sometimes the has_one association will find more than one possible candidate and will raise a `PaperTrail::Reifiers::HasOne::FoundMoreThanOne` when NOT using STI. You may want to just assume the first result (of multiple) is the correct one and continue. Versions pre v8.1.2 and below did this without error or warning. To do so add the following line to your initializer: `PaperTrail.config.association_reify_error_behaviour = :warn`. Valid options are: `[:error, :warn, :ignore]` ### Fixed diff --git a/README.md b/README.md index 11df70ef2..8eb92e758 100644 --- a/README.md +++ b/README.md @@ -948,8 +948,9 @@ Associations are an **experimental feature** and have the following known issues, in order of descending importance. 1. PaperTrail only reifies the first level of associations. -1. Does not fully support STI (For example, see `spec/models/person_spec.rb` and - `PaperTrail::Reifiers::HasOne::FoundMoreThanOne` error) +1. Sometimes the has_one association will find more than one possible candidate and will raise a `PaperTrail::Reifiers::HasOne::FoundMoreThanOne` error. For example, see `spec/models/person_spec.rb` + - If you are not using STI, you may want to just assume the first result (of multiple) is the correct one and continue. Versions pre v8.1.2 and below did this without error or warning. To do so add the following line to your initializer: `PaperTrail.config.association_reify_error_behaviour = :warn`. Valid options are: `[:error, :warn, :ignore]` + - When using STI, even if you enable :warn you will likely still end up recieving an `ActiveRecord::AssociationTypeMismatch` error in which case your SOL 1. [#542](https://github.com/paper-trail-gem/paper_trail/issues/542) - Not compatible with [transactional tests][34], aka. transactional fixtures. 1. Requires database timestamp columns with fractional second precision. diff --git a/lib/generators/paper_trail/install_generator.rb b/lib/generators/paper_trail/install_generator.rb index 001e5e20c..c964b96bf 100644 --- a/lib/generators/paper_trail/install_generator.rb +++ b/lib/generators/paper_trail/install_generator.rb @@ -43,10 +43,13 @@ def create_migration_file end def create_initializer - create_file( - "config/initializers/paper_trail.rb", - "PaperTrail.config.track_associations = #{!!options.with_associations?}\n" - ) + if options.with_associations? + create_file( + "config/initializers/paper_trail.rb", + "PaperTrail.config.track_associations = true\n", + "PaperTrail.config.association_reify_error_behaviour = :error" + ) + end end def self.next_migration_number(dirname) diff --git a/lib/paper_trail/config.rb b/lib/paper_trail/config.rb index ad4f565a6..5d1e4b5bc 100644 --- a/lib/paper_trail/config.rb +++ b/lib/paper_trail/config.rb @@ -26,7 +26,7 @@ class Config STR include Singleton - attr_accessor :serializer, :version_limit + attr_accessor :serializer, :version_limit, :association_reify_error_behaviour def initialize # Variables which affect all threads, whose access is synchronized. diff --git a/lib/paper_trail/reifiers/has_one.rb b/lib/paper_trail/reifiers/has_one.rb index ec3c89065..d1f982bd6 100644 --- a/lib/paper_trail/reifiers/has_one.rb +++ b/lib/paper_trail/reifiers/has_one.rb @@ -74,7 +74,18 @@ def load_version(assoc, model, transaction_id, version_at) when 1 versions.first else - raise FoundMoreThanOne.new(base_class_name, versions.length) + case PaperTrail.config.association_reify_error_behaviour.to_s + when "warn" + version = versions.first + version.logger&.warn( + format(FoundMoreThanOne::MESSAGE_FMT, base_class_name, versions.length) + ) + version + when "ignore" + versions.first + else # "error" + raise FoundMoreThanOne.new(base_class_name, versions.length) + end end end diff --git a/spec/dummy_app/app/models/person.rb b/spec/dummy_app/app/models/person.rb index 0a0ac912a..5bc42edad 100644 --- a/spec/dummy_app/app/models/person.rb +++ b/spec/dummy_app/app/models/person.rb @@ -12,6 +12,9 @@ class Person < ActiveRecord::Base has_one :car, foreign_key: :owner_id has_one :bicycle, foreign_key: :owner_id + has_one :thing + has_one :thing_2, class_name: "Thing" + if ActiveRecord.gem_version >= Gem::Version.new("5.0") belongs_to :mentor, class_name: "Person", foreign_key: :mentor_id, optional: true else diff --git a/spec/dummy_app/app/models/thing.rb b/spec/dummy_app/app/models/thing.rb index e47aa46e1..6409ce455 100644 --- a/spec/dummy_app/app/models/thing.rb +++ b/spec/dummy_app/app/models/thing.rb @@ -2,4 +2,6 @@ class Thing < ActiveRecord::Base has_paper_trail save_changes: false + + belongs_to :person, optional: true end diff --git a/spec/dummy_app/db/migrate/20110208155312_set_up_test_tables.rb b/spec/dummy_app/db/migrate/20110208155312_set_up_test_tables.rb index dc94468c3..f6985024a 100644 --- a/spec/dummy_app/db/migrate/20110208155312_set_up_test_tables.rb +++ b/spec/dummy_app/db/migrate/20110208155312_set_up_test_tables.rb @@ -238,6 +238,7 @@ def up create_table :things, force: true do |t| t.string :name + t.references :person end create_table :translations, force: true do |t| diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index 852aaab88..52305a547 100644 --- a/spec/models/person_spec.rb +++ b/spec/models/person_spec.rb @@ -7,8 +7,9 @@ expect(Person.new).to be_versioned end - describe "#cars and bicycles" do - it "can be reified" do + # See https://github.com/paper-trail-gem/paper_trail/issues/594 + describe "#association reify error behaviour" do + it "association reify error behaviour = :error" do person = Person.create(name: "Frank") car = Car.create(name: "BMW 325") bicycle = Bicycle.create(name: "BMX 1.0") @@ -23,7 +24,6 @@ expect(person.reload.versions.length).to(eq(3)) - # See https://github.com/paper-trail-gem/paper_trail/issues/594 expect { person.reload.versions.second.reify(has_one: true) }.to( @@ -34,5 +34,53 @@ end ) end + + # it "association reify error behaviour = :warn" do + # #::PaperTrail.config.association_reify_error_behaviour = :warn + + # person = Person.create(name: "Frank") + # thing = Thing.create(name: "BMW 325") + # thing = Thing.create(name: "BMX 1.0") + + # person.thing = thing + # person.thing = thing + # person.update_attributes(name: "Steve") + + # thing.update_attributes(name: "BMW 330") + # thing.update_attributes(name: "BMX 2.0") + # person.update_attributes(name: "Peter") + + # expect(person.reload.versions.length).to(eq(3)) + + # expect(person.versions.first.logger).to( + # receive(:warn).with(/Unable to reify has_one association/).twice + # ) + + # person.reload.versions.second.reify(has_one: true) + # end + + # it "association reify error behaviour = :ignore" do + # #::PaperTrail.config.association_reify_error_behaviour = :ignore + + # person = Person.create(name: "Frank") + # thing = Thing.create(name: "BMW 325") + # thing = Thing.create(name: "BMX 1.0") + + # person.thing = thing + # person.thing = thing + # person.update_attributes(name: "Steve") + + # thing.update_attributes(name: "BMW 330") + # thing.update_attributes(name: "BMX 2.0") + # person.update_attributes(name: "Peter") + + # expect(person.reload.versions.length).to(eq(3)) + + # expect(person.versions.first.logger).to_not( + # receive(:warn).with(/Unable to reify has_one association/) + # ) + + # person.reload.versions.second.reify(has_one: true) + # end end end diff --git a/spec/paper_trail/config_spec.rb b/spec/paper_trail/config_spec.rb index bcaa63c0e..0b38924f4 100644 --- a/spec/paper_trail/config_spec.rb +++ b/spec/paper_trail/config_spec.rb @@ -32,6 +32,14 @@ module PaperTrail end end + # describe "association_reify_error_behaviour" do + # it "accepts a value" do + # config = described_class.instance + # config.association_reify_error_behaviour = :warn + # expect(config.association_reify_error_behaviour).to eq(:warn) + # end + # end + describe ".version_limit", versioning: true do after { PaperTrail.config.version_limit = nil }