Deprecate where_object_changes reading YAML from text column

* clarify the issue with `where_object_changes` for numeric values

History
---
[The first problem mention](1ad7838).
[The major issue](#803) with relevant discussions.

Details
---
I see the problem only with one specific case – when the method is `where_object_changes` and the serializer is YAML. Which is a default configuration.

I see there are 6 types of queries with a numeric value for `where_object_changes` and `where_object` (grabbed from `version_spec.rb`):
```
.where_object_changes
N1
SELECT "versions".* FROM "versions" WHERE "versions"."item_id" = 1 AND "versions"."item_type" = 'Widget' AND (object_changes @> '{"an_integer":[1]}')  ORDER BY "versions"."created_at" ASC, "versions"."id" ASC

N1
SELECT "versions".* FROM "versions" WHERE "versions"."item_id" = 1 AND "versions"."item_type" = 'Widget' AND ("versions"."object_changes" ILIKE '%
an_integer:
- 1
%' OR "versions"."object_changes" ILIKE '%
an_integer:
-%
- 1
%')  ORDER BY "versions"."created_at" ASC, "versions"."id" ASC

N3
SELECT "versions".* FROM "versions" WHERE "versions"."item_id" = 1 AND "versions"."item_type" = 'Widget' AND (((object_changes->>'an_integer' ILIKE '[1,%') OR (object_changes->>'an_integer' ILIKE '[%,1]%')))  ORDER BY "versions"."created_at" ASC, "versions"."id" ASC

.where_object
N4
SELECT "versions".* FROM "versions" WHERE (object @> '{"an_integer":1}')

N5
SELECT "versions".* FROM "versions" WHERE (object->>'an_integer' = '1')

N6
SELECT "versions".* FROM "versions" WHERE ("versions"."object" ILIKE '%
an_integer: 1
%')
```

The only problematic one is N2. It incorrectly matches `object_changes` like
```
---
name:
-
- Kimberli
an_integer:
-
- 0
created_at:
-
- 2017-09-28 18:30:13.369889000 Z
updated_at:
-
- 2017-09-28 18:30:13.369889000 Z
id:
-
- 1
---
name:
- foobar
- Hayes
an_integer:
- 77
- 1
updated_at:
- 2017-09-28 18:30:13.383966000 Z
- 2017-09-28 18:30:13.395539000 Z
```

CHANGELOG.md

@@ -11,7 +11,8 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/).
 
 ### Added
 
-- None
+- [#997](https://github.com/airblade/paper_trail/pull/997)
+ Deprecate `where_object_changes` when reading YAML from a text column
 
 ### Fixed
 

README.md

@@ -248,9 +248,11 @@ version.index
 version.event
 
 # Query the `versions.object` column (or `object_changes` column), by
-# attributes, using the SQL LIKE operator. Known issue: inconsistent results for
-# numeric values due to limitations of SQL wildcard matchers against the
-# serialized objects.
+# attributes, using the SQL LIKE operator.
+#
+# Known issue: `where_object_changes` with the default YAML serializer and
+# an `object_changes` text column may return incorrect results for numeric values
+# due to limitations of SQL wildcard matchers against the serialized objects.
 PaperTrail::Version.where_object(attr1: val1, attr2: val2)
 PaperTrail::Version.where_object_changes(attr1: val1)
 ```

lib/paper_trail/serializers/yaml.rb

@@ -4,6 +4,13 @@ module PaperTrail
  module Serializers
  # The default serializer for, e.g. `versions.object`.
  module YAML
+ E_WHERE_OBJ_CHANGES = <<-STR.squish.freeze
+ where_object_changes has a known issue. When reading YAML from a text
+ column, it may return more records than expected. Instead of a warning,
+ this method may raise an error in the future. Please join the discussion
+ at https://github.com/airblade/paper_trail/pull/997
+ STR
+
  extend self # makes all instance methods become module methods as well
 
  def load(string)
@@ -23,6 +30,8 @@ def where_object_condition(arel_field, field, value)
  # Returns a SQL LIKE condition to be used to match the given field and
  # value in the serialized `object_changes`.
  def where_object_changes_condition(arel_field, field, value)
+ ::ActiveSupport::Deprecation.warn(E_WHERE_OBJ_CHANGES)
+
  # Need to check first (before) and secondary (after) fields
  m1 = "%\n#{field}:\n- #{value}\n%"
  m2 = "%\n#{field}:\n-%\n- #{value}\n%"

spec/models/version_spec.rb

@@ -107,6 +107,10 @@ module PaperTrail
 
  column_overrides.shuffle.each do |column_datatype_override|
  context "with a #{column_datatype_override || 'text'} column" do
+ let(:widget) { Widget.new }
+ let(:name) { FFaker::Name.first_name }
+ let(:int) { column_datatype_override ? 1 : rand(5) + 2 }
+
  before do
  if column_datatype_override
  # In rails < 5, we use truncation, ie. there is no transaction
@@ -147,10 +151,6 @@ module PaperTrail
  end
 
  describe "#where_object", versioning: true do
- let(:widget) { Widget.new }
- let(:name) { FFaker::Name.first_name }
- let(:int) { rand(10) + 1 }
-
  before do
  widget.update_attributes!(name: name, an_integer: int)
  widget.update_attributes!(name: "foobar", an_integer: 100)
@@ -172,6 +172,9 @@ module PaperTrail
  end
 
  it "locates versions according to their `object` contents" do
+ expect(
+ PaperTrail::Version.where_object(an_integer: int)
+ ).to eq([widget.versions[1]])
  expect(
  PaperTrail::Version.where_object(name: name)
  ).to eq([widget.versions[1]])
@@ -191,6 +194,9 @@ module PaperTrail
  end
 
  it "locates versions according to their `object` contents" do
+ expect(
+ PaperTrail::Version.where_object(an_integer: int)
+ ).to eq([widget.versions[1]])
  expect(
  PaperTrail::Version.where_object(name: name)
  ).to eq([widget.versions[1]])
@@ -206,13 +212,9 @@ module PaperTrail
  end
 
  describe "#where_object_changes", versioning: true do
- let(:widget) { Widget.new }
- let(:name) { FFaker::Name.first_name }
- let(:int) { rand(5) + 2 }
-
  before do
  widget.update_attributes!(name: name, an_integer: 0)
- widget.update_attributes!(name: "foobar", an_integer: 77)
+ widget.update_attributes!(name: "foobar", an_integer: 100)
  widget.update_attributes!(name: FFaker::Name.last_name, an_integer: int)
  end
 
@@ -226,22 +228,36 @@ module PaperTrail
  end
 
  context "YAML serializer" do
+ before do
+ unless column_datatype_override
+ allow(ActiveSupport::Deprecation).to receive(:warn)
+ end
+ end
+
  it "locates versions according to their `object_changes` contents" do
  expect(
  widget.versions.where_object_changes(name: name)
  ).to eq(widget.versions[0..1])
  expect(
- widget.versions.where_object_changes(an_integer: 77)
+ widget.versions.where_object_changes(an_integer: 100)
  ).to eq(widget.versions[1..2])
  expect(
  widget.versions.where_object_changes(an_integer: int)
  ).to eq([widget.versions.last])
+ unless column_datatype_override
+ expect(ActiveSupport::Deprecation).to have_received(:warn).
+ exactly(3).times.with(/^where_object_changes/)
+ end
  end
 
  it "handles queries for multiple attributes" do
  expect(
- widget.versions.where_object_changes(an_integer: 77, name: "foobar")
+ widget.versions.where_object_changes(an_integer: 100, name: "foobar")
  ).to eq(widget.versions[1..2])
+ unless column_datatype_override
+ expect(ActiveSupport::Deprecation).to have_received(:warn).
+ at_least(:once).with(/^where_object_changes/)
+ end
  end
  end
 
@@ -257,7 +273,7 @@ module PaperTrail
  widget.versions.where_object_changes(name: name)
  ).to eq(widget.versions[0..1])
  expect(
- widget.versions.where_object_changes(an_integer: 77)
+ widget.versions.where_object_changes(an_integer: 100)
  ).to eq(widget.versions[1..2])
  expect(
  widget.versions.where_object_changes(an_integer: int)
@@ -266,7 +282,7 @@ module PaperTrail
 
  it "handles queries for multiple attributes" do
  expect(
- widget.versions.where_object_changes(an_integer: 77, name: "foobar")
+ widget.versions.where_object_changes(an_integer: 100, name: "foobar")
  ).to eq(widget.versions[1..2])
  end