Deprecate where_object_changes reading YAML from a text column

[sergey-alekseev]

History

The first problem mention.
The major issue 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
# 1
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

# 2
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

# 3
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
# 4
SELECT "versions".* FROM "versions" WHERE (object @> '{"an_integer":1}')

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

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

The only problematic one is #2. 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

@batter @jaredbeck correct me if I'm wrong.

[jaredbeck]

Hi Sergey, Thanks for the contribution!

It sounds like where_object_changes+YAML has the same issue as where_object_changes+JSON-in-text-column, as described in #803. I think the same solution (#993) would be appropriate here.

What do you think?

[jaredbeck]

It sounds like where_object_changes+YAML has the same issue as where_object_changes+JSON-in-text-column, as described in #803. I think the same solution (#993) would be appropriate here.

To clarify, I am suggesting that we raise an error when someone is storing YAML in a text column and they call where_object_changes. Sound good?

[sergey-alekseev]

@jaredbeck I think applying a "solution" like #979 + #993 is better than not applying it.

Ideally such cases should be fixed, but I'm not interested in a text column.

---

FYI I fixed a failed spec example by changing

let(:int) { column_datatype_override ? 1 : rand(5) + 1 }

to

let(:int) { column_datatype_override ? 1 : rand(5) + 2 }

Initially I didn't take into account rand(5) + 1 may return 1 😳

[jaredbeck]

It sounds like where_object_changes+YAML has the same issue as where_object_changes+JSON-in-text-column, as described in #803. I think the same solution (#993) would be appropriate here.

To clarify, I am suggesting that we raise an error when someone is storing YAML in a text column and they call where_object_changes. Sound good?

@jaredbeck I think applying a "solution" like #979 + #993 is better than not applying it.

OK, raising an error is a breaking change, so we should start with a deprecation warning , just like we did in #979. If you agree, will you please add such a warning, Sergey? Thanks!

[sergey-alekseev]

@jaredbeck I added a deprecation warning. Let me know if you think commits should be squashed.

[jaredbeck]

Looks perfect! Thanks Sergey.

[jaredbeck]

We'll wait a few months after this deprecation warning is released to give people a chance to discuss the issue here. Then, if no concerns are raised we will replace the warning with an error.