Merge pull request #1062 from airblade/various_2018-03-11

Various 2018-03-11

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)
 

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.

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

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

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