diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d523fed..22f552b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,6 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/). - PaperTrail now uses `frozen_string_literal`, so you should assume that all strings it returns are frozen. - Using `where_object_changes` to read YAML from a text column will now raise -- [#1051](https://github.com/airblade/paper_trail/issues/1051) Fix `touch_with_version` does not work with conditional options error, was deprecated in 8.1.0. ### Breaking Changes, Minor @@ -44,11 +43,17 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/). ### Fixed +- [#1051](https://github.com/airblade/paper_trail/issues/1051) - `touch_with_version` + should always create a version, regardles of the `:only` option - [#1047](https://github.com/airblade/paper_trail/issues/1047) - A rare issue where `touch_with_version` saved less data than expected, but only when the update callback was not installed, eg. `has_paper_trail(on: [])` - [#1042](https://github.com/airblade/paper_trail/issues/1042) - A rare issue with load order when using PT outside of rails +- [#594](https://github.com/airblade/paper_trail/issues/594) - Improved the + error message for a very rare issue in the experimentatl association tracking + feature involving two has_one associations, referencing STI models with the + same base class, and the same foreign_key. ## 8.1.2 (2017-12-22) diff --git a/README.md b/README.md index cfc541ad..cfe5d8c5 100644 --- a/README.md +++ b/README.md @@ -805,8 +805,8 @@ string, please try the [paper_trail-globalid][37] gem. ### 4.b. Associations -**Experimental feature**, not recommended for production. See known issues -below. +**Experimental feature with many known issues. Not recommended for production.** +See known issues below. PaperTrail can restore three types of associations: Has-One, Has-Many, and Has-Many-Through. In order to do this, you will need to do two things: @@ -913,6 +913,8 @@ 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. [#542](https://github.com/airblade/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/paper_trail/reifiers/has_one.rb b/lib/paper_trail/reifiers/has_one.rb index ad83e7ee..6f9dc62b 100644 --- a/lib/paper_trail/reifiers/has_one.rb +++ b/lib/paper_trail/reifiers/has_one.rb @@ -5,10 +5,42 @@ module PaperTrail # Reify a single `has_one` association of `model`. # @api private module HasOne + # A more helpful error message, instead of the AssociationTypeMismatch + # you would get if, eg. we were to try to assign a Bicycle to the :car + # association (before, if there were multiple records we would just take + # the first and hope for the best). + # @api private + class FoundMoreThanOne < RuntimeError + MESSAGE_FMT = <<~STR + Unable to reify has_one association. Expected to find one %s, + but found %d. + + This is a known issue, and a good example of why association tracking + is an experimental feature that should not be used in production. + + That said, this is a rare error. In spec/models/person_spec.rb we + reproduce it by having two STI models with the same foreign_key (Car + and Bicycle are both Vehicles and the FK for both is owner_id) + + If you'd like to help fix this error, please read + https://github.com/airblade/paper_trail/issues/594 + and see spec/models/person_spec.rb + STR + + def initialize(base_class_name, num_records_found) + @base_class_name = base_class_name.to_s + @num_records_found = num_records_found.to_i + end + + def message + format(MESSAGE_FMT, @base_class_name, @num_records_found) + end + end + class << self # @api private def reify(assoc, model, options, transaction_id) - version = load_version_for_has_one(assoc, model, transaction_id, options[:version_at]) + version = load_version(assoc, model, transaction_id, options[:version_at]) return unless version if version.event == "create" create_event(assoc, model, options) @@ -33,15 +65,29 @@ module PaperTrail # Given a has-one association `assoc` on `model`, return the version # record from the point in time identified by `transaction_id` or `version_at`. # @api private - def load_version_for_has_one(assoc, model, transaction_id, version_at) + def load_version(assoc, model, transaction_id, version_at) + base_class_name = assoc.klass.base_class.name + versions = load_versions(assoc, model, transaction_id, version_at, base_class_name) + case versions.length + when 0 + nil + when 1 + versions.first + else + raise FoundMoreThanOne.new(base_class_name, versions.length) + end + end + + # @api private + def load_versions(assoc, model, transaction_id, version_at, base_class_name) version_table_name = model.class.paper_trail.version_class.table_name model.class.paper_trail.version_class.joins(:version_associations). where("version_associations.foreign_key_name = ?", assoc.foreign_key). where("version_associations.foreign_key_id = ?", model.id). - where("#{version_table_name}.item_type = ?", assoc.klass.base_class.name). + where("#{version_table_name}.item_type = ?", base_class_name). where("created_at >= ? OR transaction_id = ?", version_at, transaction_id). order("#{version_table_name}.id ASC"). - first + load end # @api private diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index 34b387d5..3bc67fb5 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -174,14 +174,6 @@ RSpec.describe Article, type: :model, versioning: true do ).to(eq("Some text here.")) end end - - it "creates a version with touch_with_version & :only option combined" do - article = described_class.create - - expect { article.paper_trail.touch_with_version }.to change { - PaperTrail::Version.count - }.by(+1) - end end context "#destroy" do @@ -193,4 +185,13 @@ RSpec.describe Article, type: :model, versioning: true do expect(article.versions.map(&:event)).to(match_array(%w[create destroy])) end end + + describe "#touch_with_version" do + it "creates a version, ignoring the :only option" do + article = described_class.create + expect { article.paper_trail.touch_with_version }.to change { + ::PaperTrail::Version.count + }.by(+1) + end + end end diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index f596fd23..07f2d3a5 100644 --- a/spec/models/person_spec.rb +++ b/spec/models/person_spec.rb @@ -8,7 +8,7 @@ RSpec.describe Person, type: :model, versioning: true do end describe "#cars and bicycles" do - it "can be reified", skip: "Known Issue #594" do + it "can be reified" do person = Person.create(name: "Frank") car = Car.create(name: "BMW 325") bicycle = Bicycle.create(name: "BMX 1.0") @@ -23,11 +23,16 @@ RSpec.describe Person, type: :model, versioning: true do expect(person.reload.versions.length).to(eq(3)) - # Will fail because the correct sub type of the STI model is not present at query. # See https://github.com/airblade/paper_trail/issues/594 - second_version = person.reload.versions.second.reify(has_one: true) - expect(second_version.car.name).to(eq("BMW 325")) - expect(second_version.bicycle.name).to(eq("BMX 1.0")) + expect { + person.reload.versions.second.reify(has_one: true) + }.to( + raise_error(::PaperTrail::Reifiers::HasOne::FoundMoreThanOne) do |err| + expect(err.message.squish).to match( + /Expected to find one Vehicle, but found 2/ + ) + end + ) end end end