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] InteractsWithDatabase::castAsJson($value) incorrectly handles SQLite Database #43863

Closed
stevebauman opened this issue Aug 25, 2022 · 10 comments · Fixed by #44196
Closed
Labels

Comments

@stevebauman
Copy link
Contributor

stevebauman commented Aug 25, 2022

  • Laravel Version:
  • PHP Version: v9.26.0
  • Database Driver & Version: MySQL 8.0 | SQLite

Description:

When using SQLite to test locally, calling InteractsWithDatabase::castAsJson in a test incorrectly casts the value to a raw DB expression, containing CAST($value AS JSON).

Steps To Reproduce:

// SomeTest extends TestCase

$this->assertDatabaseHas(TriggerListenerCondition::class, [
    'group' => 0,
    'operand' => 'and',
    'type' => 'array',
    'attribute' => 'call.answered_by',
    'operator' => 'has_any',
    'value' => $this->castAsJson([$user->id]),
]);
Failed asserting that a row in the table [trigger_listener_conditions] matches the attributes {
    "group": 0,
    "operand": "and",
    "type": "array",
    "attribute": "call.answered_by",
    "operator": "has_any",
    "value": "CAST('[1]' AS JSON)"
}.

Found similar results: [
    {
        "group": "0",
        "operand": "and",
        "type": "array",
        "attribute": "call.answered_by",
        "operator": "has_any",
        "value": "[1]"
    },

When running this with a MySQL connection, the assertion passes fine.

This causes issues since I run my tests locally under SQLite for speed, and then in GitHub Actions with MySQL when pushing to master.


Reference:

public function castAsJson($value)
{
if ($value instanceof Jsonable) {
$value = $value->toJson();
} elseif (is_array($value) || is_object($value)) {
$value = json_encode($value);
}
$value = DB::connection()->getPdo()->quote($value);
return DB::raw("CAST($value AS JSON)");
}

Proposed solution:

public function castAsJson($value)
{
    if ($value instanceof Jsonable) {
        $value = $value->toJson();
    } elseif (is_array($value) || is_object($value)) {
        $value = json_encode($value);
    }

    if (DB::connection() instanceof SQLiteConnection) {
        return $value;
    }

    $value = DB::connection()->getPdo()->quote($value);

    return DB::raw("CAST($value AS JSON)");
}
@driesvints
Copy link
Member

This was originally added here: #34302 by @browner12. I'm not sure what the reasoning for a raw statement was but this indeed seems odd given our multiple DB engine support.

Did you confirm the fix? Are you willing to PR it? (with a test if possible)

@driesvints driesvints added the bug label Aug 26, 2022
@stevebauman
Copy link
Contributor Author

I can for sure submit a PR this weekend with tests. The above posted solution works for me 👍

@browner12
Copy link
Contributor

Sqlite does not support JSON columns at all, which is the reason I stopped using it completely for testing. @stevebauman, I guess I'm confused, how are your tests passing at all? Doesn't Sqlite throw an error when you try to migrate a JSON column?

@stevebauman
Copy link
Contributor Author

@browner12 The Laravel schema builder will create a text column if you add a JSON column while connected to a SQLite instance. Together with Eloquent casts, everything works fine when testing across SQLite and MySQL. Though I of course cannot use MySQL specific JSON where clauses in queries (i.e. where('data->foo', '…')).

@browner12
Copy link
Contributor

yah, this PR is starting to come back to me a little. it's definitely not a perfect solution, as it'd be best to know if the field in question is truly a JSON field. Are there any other DB engines that don't support JSON types? Would we want to be more generic with something like:

public function castAsJson($value)
{
    if ($value instanceof Jsonable) {
        $value = $value->toJson();
    } elseif (is_array($value) || is_object($value)) {
        $value = json_encode($value);
    }

    if (!DB::connection()->supportsJson()) {
        return $value;
    }

    $value = DB::connection()->getPdo()->quote($value);

    return DB::raw("CAST($value AS JSON)");
}

and then adjust the connection classes a little?

@stevebauman
Copy link
Contributor Author

That’s a good point @browner12, I’m not sure. Let me dig into it a bit and see which built-in Laravel DB drivers support JSON columns 👍

@browner12
Copy link
Contributor

MySQL added JSON support in v5.7

https://dev.mysql.com/doc/refman/8.0/en/json.html#:~:text=MySQL%20supports%20a%20native%20JSON,documents%20stored%20in%20JSON%20columns.

PostgresSQL added JSON support in v9.2

https://www.postgresql.org/docs/current/datatype-json.html

SQLServer has no native JSON datatype, but allows you to work with JSON since v2016

https://docs.microsoft.com/en-us/sql/relational-databases/json/json-data-sql-server?view=sql-server-ver16

SQLite also has no native JSON datatype, but actually added JSON support in v3.38

https://tirkarthi.github.io/programming/2022/02/26/sqlite-json-improvements.html


So here lies the issue. We're not only having to check the driver, but also the version being used.

Rather than adjusting the InteractsWithDatabase, I think it might be better to adjust the "Grammar" files to add a castAsJSON() method, so they can each be responsible for themselves. As some of the drivers possibly add native JSON data types, this would allow us to make adjustments easier I believe. Thoughts?

@stevebauman
Copy link
Contributor Author

Yea I agree with this. Having it inside of the grammar makes sense 👍

Would you like me to tackle the PR if you got your hands full at the moment?

@browner12
Copy link
Contributor

sure thing. tag me in it and I'll try and provide some feedback.

@driesvints
Copy link
Member

Going to close this since we can assume SQLite isn't supported yet for this function. Feel free to send in a PR when you can. Thanks all.

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

Successfully merging a pull request may close this issue.

3 participants