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

[9.x] Add whereJsonContainsKey to query builder #41756

Closed
wants to merge 2 commits into from
Closed

[9.x] Add whereJsonContainsKey to query builder #41756

wants to merge 2 commits into from

Conversation

amir9480
Copy link
Contributor

This PR adds a new function to the query builder to check JSON column type has the given key or not.
The syntax for keys is exactly like whereJsonContains.

Sample codes:

$users = DB::table('users')
                ->whereJsonContainsKey('options->languages')
                ->get();
$users = DB::table('users')
                ->whereJsonDoesntContainKey('options->language->primary')
                ->get();

@taylorotwell
Copy link
Member

taylorotwell commented Mar 31, 2022

What if the user does whereJsonContainsKey('options->foo->0') ... where options->foo is an array like ['first', 'second', 'third']?

I believe we recently worked on a similar issue with another JSON clause builder /cc @derekmd

@derekmd
Copy link
Contributor

derekmd commented Mar 31, 2022

TLDR; I may need to submit another PR before this one is merged. Here's a preliminary dev branch to fix Postgres support of JSON array indexes: https://github.com/laravel/framework/compare/9.x...derekmd:fix-postgres-json-paths-with-array-index?expand=1

Now this repo's GitHub Actions have Postgres & SQL Server test runs, some https://github.com/laravel/framework/tree/9.x/tests/Integration/Database cases should probably be added to execute actual database queries?

My PR #38391 fixed allowing array index references:

DB::table('users')
    ->whereJsonDoesntContainKey('options->languages[0]')
    ->get();

The new methods calling wrapJsonFieldAndPath() should be good to support this however I'm only now noticing that my previous PR didn't fix the Postgres driver path. PostgresGrammer@wrapJsonSelector() overrides the base implementation and doesn't even compile SQL to the JSON path API, instead using native json/jsonb operators for querying.

This isn't a huge deal since this compiles fine:

DB::table('users')
    ->whereJsonDoesntContainKey('options->languages->0')
    ->get();

This PR's PostgresGrammar@compileJsonContainsKey() method calling $this->wrap() combined with my pending branch should ensure the correct SQL is generated for the first PHP snippet:

-- unexpected wrapping of array key inside the last segment
SELECT * FROM "users"
WHERE coalesce(("options"')::jsonb ?? 'language[0]', false)
-- correct
SELECT * FROM "users"
WHERE coalesce(("options"->'language')::jsonb ?? 0, false)

@amir9480
Copy link
Contributor Author

amir9480 commented Apr 1, 2022

@taylorotwell I'm confused about implementing this feature for the arrays as whereJsonContains doesn't support array keys on all database drivers.

@derekmd
Copy link
Contributor

derekmd commented Apr 1, 2022

Here's a sample integration test for Postgres where 3 of the 6 data sets currently don't pass: derekmd@fcb626f

/**
 * @dataProvider jsonContainsKeyDataProvider
 */
public function testWhereJsonContainsKey($count, $column)
{
    DB::table('json_table')->insert([
        ['json_col' => '{"foo":{"bar":["baz"]}}'],
        ['json_col' => '{"foo":{"bar":false}}'],
        ['json_col' => '{"foo":{}}'],
    ]);

    $this->assertSame($count, DB::table('json_table')->whereJsonContainsKey($column)->count());
}

public function jsonContainsKeyDataProvider()
{
    return [
        'key exists' => [3, 'json_col->foo'],
        'nested key exists' => [2, 'json_col->foo->bar'],
        'array key exists with arrow' => [1, 'json_col->foo->bar->0'],
        'array key exists with braces' => [1, 'json_col->foo->bar[0]'],
        'array key not exists' => [0, 'json_col->foo->bar[1]'],
        'key not exists' => [0, 'json_col->none'],
    ];
}

  1. ✔️ Test passes
    DB::table('json_table')->whereJsonContainsKey('json_col->foo')->count();
    where coalesce(("json_col")::jsonb ?? 'foo', false)
  2. ✔️ Test passes
    DB::table('json_table')->whereJsonContainsKey('json_col->foo->bar')->count();
    where coalesce(("json_col"->'foo')::jsonb ?? 'bar', false)
  3. ❌ Test fails, Postgres operator ?? / ? is for object string keys so it won't match array index integers
    DB::table('json_table')->whereJsonContainsKey('json_col->foo->bar->0')->count();
    where coalesce(("json_col"->'foo'->'bar')::jsonb ?? '0', false)
  4. ❌ Test fails, array index 0 must be matched
    DB::table('json_table')->whereJsonContainsKey('json_col->foo->bar[0]')->count();
    where coalesce(("json_col"->'foo')::jsonb ?? 'bar[0]', false)
  5. ❌ Test passes but for a different missing key. SQL must compile the same as example 4.
    DB::table('json_table')->whereJsonContainsKey('json_col->foo->bar[1]')->count();
    where coalesce(("json_col"->'foo')::jsonb ?? 'bar[1]', false)
  6. ✔️ Test passes
    DB::table('json_table')->whereJsonContainsKey('json_col->none')->count();
    where coalesce(("json_col")::jsonb ?? 'none', false)

For PostgresGrammar, SQL in this new method may need to use parts of wrapJsonPathAttributes().

  1. avoid wrapping integer keys in quotes
  2. PHP string json_col->key[0][1] must be compiled to SQL ("json_col"->'key'->0)::jsonb ?? 1. The $this->wrapJsonPathAttributes(["json_col", "key[0][1]"]) collection pipeline with collapse() reformats the JSON segments array like this:
    ["json_col", "key[0][1]"]
    [["json_col", ["key", "0", "1"]]
    ["json_col", "key", "0", "1"]
    ["'json_col'", "'key'", "0", "1"] // or ['"json_col"', '"key"', "0", "1"] with arg $quote === "\""

@taylorotwell
Copy link
Member

taylorotwell commented Apr 1, 2022

Is it even possible to support numeric keys with this method across all drivers?

@derekmd
Copy link
Contributor

derekmd commented Apr 2, 2022

Experimental commits with integration tests covering array integer keys: https://github.com/derekmd/framework/commits/query-where-json-contains-key

MySQL/MariaDB

👍 Supported and very straightforward with MySQL's json_contains_path() function.

SQL Server

👍 Method SqlServerGrammar@compileJsonContainsKey() needs additional handling to extract the array index from the column string and avoid wrapping it in quotes. However select [key] from openjson() does support array integer keys.

PostgreSQL

👍-ish, this is the most complicated implementation since Postgres functions differentiate between JSONB objects and arrays. Like SQL Server, the array index must be extracted from the column string. Also:

  • check for object string keys using the ? operator (this PR's implementation)
  • array integer keys call functions jsonb_typeof() and jsonb_array_length()
    • this checks the given index is within the (0 to n-1) the range.
    • Postgres also supports negative array indexes, counting from the array end
    • conditional case when ... then ... else false end must used to avoid calling jsonb_array_length() on a non-array

SQLite

👍 Not implemented in this PR, but my branch shows SQLite is as straightforward as MySQL. json_type(column, path) IS NOT NULL is true when the JSON path exists. If that JSON path's actual value is null, json_type() returns string 'null'.

@taylorotwell taylorotwell marked this pull request as draft April 2, 2022 15:27
@taylorotwell
Copy link
Member

Thanks @derekmd

Getting a bit lost though, so this PR is not ready to merge?

@derekmd
Copy link
Contributor

derekmd commented Apr 2, 2022

This PR is 80% there, I can finish the last 20% in another commit that can be merged into this.

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

Successfully merging this pull request may close these issues.

3 participants