Add support for comparing jsonb @> with boolean values

[leandrocp]

The commit 96f70b5 may have introduced a breaking change when dealing with boolean values.

Given the schema

ecto_sql/test/ecto/adapters/postgres_test.exs

Lines 9 to 26 in c22115b

	defmodule Schema do
	use Ecto.Schema
	schema "schema" do
	field :x, :integer
	field :y, :integer
	field :z, :integer
	field :w, {:array, :integer}
	field :meta, :map
	has_many :comments, Ecto.Adapters.PostgresTest.Schema2,
	references: :x,
	foreign_key: :z
	has_one :permalink, Ecto.Adapters.PostgresTest.Schema3,
	references: :y,
	foreign_key: :id
	end
	end

and considering a record with meta stored as {"enabled": true} on v3.7.2 one could write the following query:

     query = Schema |> where([s], s.meta["enabled"] == "true") |> select(true) |> plan()
     assert all(query) == ~s|SELECT TRUE FROM "schema" AS s0 WHERE ((s0."meta"#>'{"enabled"}') = 'true')|

Note that you can't write s.meta["enabled"] == true as that would generate the following invalid query, either on v3.7.2 or v3.8.1:

     query = Schema |> where([s], s.meta["enabled"] == true) |> select(true) |> plan()
     assert all(query) == ~s|SELECT TRUE FROM "schema" AS s0 WHERE ((s0."meta"#>'{"enabled"}') = TRUE)|

So far, so good.

On v3.8.1 the same query s.meta["enabled"] == "true" is now generated as:

     query = Schema |> where([s], s.meta["enabled"] == "true") |> select(true) |> plan()
     assert all(query) == ~s|SELECT TRUE FROM "schema" AS s0 WHERE ((s0."meta"@>'{"enabled": "true"}'))|

And turns out that query won't return the expected records, see a live example at https://dbfiddle.uk/?rdbms=postgres_13&fiddle=9987baf6c8972988319559cb0fafb655

In order to make it work the query should be "meta"@>'{"enabled": true}' (an actual boolean instead of a string) or using the #> operator as before.

Proposed solution

AFAIK boolean values are expected to be transformed to their text representation

ecto_sql/lib/ecto/adapters/postgres/connection.ex

Lines 758 to 759 in c22115b

	defp expr(true, _sources, _query), do: "TRUE"
	defp expr(false, _sources, _query), do: "FALSE"

so using the @> wouldn't work or would introduce a new behavior, right? So the proposed solution is to revert to the old query for bool expr and also deal with true/false to make s.meta["enabled"] == true work.

I'm not sure if we should check for right in ["TRUE", "true", "FALSE", "false"] but I'm concerned because without such guard it will generate the query using @> which would fail to return the expected results silently.

[josevalim]

This is tricky. 🤔 Do we have this issue with integers?

[josevalim]

@leandrocp I think may preferred solution to address this is to actually make == true work. Otherwise == "true" is ambiguous because someone may actually be expecting the string "true".

[leandrocp]

@josevalim seems like the problem goes beyond boolean values:

1. The operator @> works with jsonb fields only
   AFAIK Ecto makes no distinction if the field is json or jsonb, so to support both then that @> operator can't be used.

2. The operator #> returns either json or jsonb
   Comparing with scalar values fails as pointed out by @greg-rychlewski

See this example: https://dbfiddle.uk/?rdbms=postgres_14&fiddle=e55dda0ca5e9d25842d10a1f9a2294a6

I don't know the context of 96f70b5 but a possible solution is to just use the #>> operator by default and fix the comparison with boolean.

[josevalim]

I think that operator is not optimizable, which defeats the purpose of the PR. :( So we have two options:

1. revert
2. provide an option (perhaps true by default) to use optimized jsonb operator. so people have an option to disable. then we fix the boolean handling

[leandrocp]

1. "Revert"
   Even if we revert, I believe we should fix the comparison with boolean and change the default json operator to #>>

2. "Option"
   How the API would like to pass a json operator as an option in a clause like where: t.column["key"] == "value"?

[josevalim]

Wait. I think we documented when we added this syntax that it only works for jsonb. And I think that’s fair. Jsonb is much more efficient and I don’t think it is worth making everything slower for json. Then you can use fragments.

[leandrocp]

Cool, so two places I found a mention on jsonb type are https://hexdocs.pm/ecto/Ecto.Query.API.html#json_extract_path/2-warning-indexes-on-postgresql and https://hexdocs.pm/ecto/embedded-schemas.html

Wdyt about the following plan?

ecto

• Update https://hexdocs.pm/ecto/Ecto.Query.html#where/3 to add an example with jsonb and mention that fragment/1 should be used for json
• Add a boolean case in https://github.com/elixir-ecto/ecto/blob/master/integration_test/cases/type.exs

ecto_sql

• Fix jsonb comparison with boolean on this PR

[josevalim]

I don't think we need to update where. :map/:array already defaults to :jsonb and you will get a query error if you try to use json anyway. :) The other two tasks look great!

[josevalim]

💚 💙 💜 💛 ❤️