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 Reifiers
  # 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 @@ def create_event(assoc, model, options)
  # 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 @@
  ).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 @@
  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 @@
  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 @@
 
  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