Skip to content

Commit

Permalink
clarify the issue with where_object_changes for numeric values
Browse files Browse the repository at this point in the history
History
---
[The first problem mention](paper-trail-gem@1ad7838).
[The major issue](paper-trail-gem#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
```
  • Loading branch information
sergey-alekseev committed Oct 22, 2017
1 parent 0bc9eb1 commit 485af88
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 16 deletions.
8 changes: 5 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
```
Expand Down
28 changes: 15 additions & 13 deletions spec/models/version_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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]])
Expand All @@ -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]])
Expand All @@ -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

Expand All @@ -231,7 +233,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)
Expand All @@ -240,7 +242,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
end
Expand All @@ -257,7 +259,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)
Expand All @@ -266,7 +268,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

Expand Down

0 comments on commit 485af88

Please sign in to comment.