Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQLite whereJsonContains grammar not working correctly #50246

Closed
dmtar opened this issue Feb 25, 2024 · 8 comments
Closed

SQLite whereJsonContains grammar not working correctly #50246

dmtar opened this issue Feb 25, 2024 · 8 comments

Comments

@dmtar
Copy link

dmtar commented Feb 25, 2024

Laravel Version

10.44

PHP Version

8.1

Database Driver & Version

SQLite 3.42

Description

I have existing queries with whereJsonContains and some tests using it started failing after I migrated the framework from 9.x to 10.44. Up until now we had a patch that uses PDO sqliteCreateFunction in order to fake the exact same behaviour of the MySQL json_contains into SQLite.

However I saw native support in Laravel 10 added with this PR and tried to use it instead of our internal one. The query is built like bellow:

$query->whereJsonContains('value', ['enabled' => true, 'some_other_flag' => false]);

In MySQL grammar the above compiles to

select * from `settings` where json_contains(`value`, '{\"enabled\":true,\"some_other_flag\":false}');

In SQLite while running tests the same query compiles to

select * from "settings" where exists (select 1 from json_each("value") where "json_each"."value" is 1);

The above has couple of issues

  1. it does not take into consideration enabled and some_other_flag json properties and their values
  2. it will always return 0 rows because of json_each("value"), however if you prefix it with the table name json_each("settings"."value") it works, but of course returns all rows containing any kind of json value that is treated as "truthy"

What I'm expecting to get is something like

select * from "settings" where exists (select 1 from json_each(settings.value, '$."enabled"') where "json_each"."value" is 1) and exists (select 1 from json_each(settings.value, '$."some_other_flag"') where "json_each"."value" is 0)

That would have exactly the same behaviour like in MySQL.
If I change
$query->whereJsonContains('value', ['enabled' => true, 'some_other_flag' => false]);
to

$query->whereJsonContains('value->enabled', true)->whereJsonContains('value-> some_other_flag', false);

I can bypass issue 1, but still I can't get around issue 2. I thought it's because "value" is double quoted, but I tried with json_each('value') and json_each(value) without luck, so its something with the internals of the json_each function and the value column name.

Steps To Reproduce

  1. Create model and table with json column named value
  2. Insert 3 records with the following values {"enabled":true,"some_other_flag":true}, {"enabled":false,"some_other_flag":false}, {"enabled":true,"some_other_flag":false}
  3. With a query builder try to fetch $query->whereJsonContains('value', ['enabled' => true, 'some_other_flag' => false]);
  4. Write unit test that checks the result

In MySQL this will return 1 record, In SQLite they will be 3.

Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@driesvints
Copy link
Member

@danieleambrosino do you might know more about this one?

@dmtar
Copy link
Author

dmtar commented Feb 26, 2024

@danieleambrosino do you might know more about this one?

That would be helpful, I think it's underlying issue with SQLite itself and opened a thread in their forum.
Once I have clear understanding I can open a PR to adjust the Laravel SQLite grammar or wait until SQLite 3.x fix

@danieleambrosino
Copy link
Contributor

Hi @dmtar, thank you very much for reporting the issue.
The indispensable premise to the discussion is that SQLite support for JSON fields is quite limited if compared to PostgreSQL or MySQL, which are capable of way more sophisticated operations when comparing JSON strings.

For what concerns your first issue (passing arrays as the second argument of whereJsonContains), it is no coincidence that the documentation says

If your application uses the MySQL or PostgreSQL databases, you may pass an array of values to the whereJsonContains method

This is because MySQL has a dedicated json_contains function, and PostgreSQL has a dedicated operator to check for containment and it even has a dedicated section in the documentation in which JSON containment is defined with examples.
On the other hand, SQLite supports only basic operations to handle JSON fields, and the only way to check if a given JSON array contains something is to decompose it through the table-valued function json_each and then performing "traditional" checks on the virtual table's fields.
Despite the laudable efforts of a query builder, it is very hard to get a perfect isomorphism between different database engines.

Moreover (I really don't want to look like the "RTFM guy", sorry for that!), the documentation states that:

You may use whereJsonContains to query JSON arrays

and not objects or subdocuments. As I interpret it, an appropriate use of whereJsonContains is to perform filtering like "en" in ["en", "it", "fr"].
In my opinion, the fact that you're querying for a subdocument on a JSON object and it's working with MySQL and PostgreSQL is a "nice quirk" more than expected behaviour: this is because internally both MySQL and PostgreSQL grammars compile the second argument of whereJsonContains to a JSON string and then leverage their implementation-specific "contains" functionalities to perform the check.

With that beings said, If I understood correctly your purpose, a possible approach to improve the cross-compatibility of your query is writing it like that:

$query->where('value->enabled', true)->where('value->some_other_flag', false)

Basic where clauses with the "arrow" syntax (and its underlying JSON path handling mechanisms) are better supported among different DBMS's.

Nonetheless, I think you have spotted an interesting misbehaviour, which occurs in the edge case when the column of the queried table is called "value", that is the same name of the column of the json_each virtual table used to perform the filtering.

I created a SQLite table populating it with the examples that you provided and I can confirm that the following query

select * from "test" where exists (select 1 from json_each("value") where "json_each"."value" is true)

wrongly does not return any rows, while

select * from "test" where exists (select 1 from json_each("test"."value") where "json_each"."value" is true)

returns the two rows containing a "truthy" value as one would expect.

This should be easily fixed in the SQLite grammar by always passing the fully qualified column name (prefixing it with the table name) as the argument of json_each.

@driesvints
Copy link
Member

Thank you very much for that thorough response @danieleambrosino.

We'd appreciate a PR to remedy the issue of the non-qualified column! (if that's possible at all)

@danieleambrosino
Copy link
Contributor

@driesvints I was just taking a look at the SQLiteGrammar code, and in fact if you think about it it's not at all obvious to impose a fixed prefix for the column name (how should the query builder know a priori which table we are imposing the constraint on?)

A possible way to handle this problem could be to somehow point out this quirk in the documentation or somewhere else, and suggest specifying the table name when imposing a constraint on a column named "value" on a SQLite DB (even if this is a very specific corner case and I don't know if it's worth reporting it in the documentation).

In fact, the previously mentioned query can be created without problems with

DB::table('test')->whereJsonContains('test.value', true)

@driesvints
Copy link
Member

Ah oh, it's only with "value"? In that case I don't think any immediate action is needed. It's very unlikely lots of people will stumble upon this one.

Gonna close this one then, I appreciate the help here from you both.

@dmtar
Copy link
Author

dmtar commented Feb 26, 2024

@danieleambrosino Thank you very much for the detailed response.

Completely agree with you that arrow syntax is the correct way. Since I'm upgrading a huge Laravel project from 9.x to 10 and these queries are way more complex than the example, I'm was more like "get all tests passing without code changes".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants