laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.32k stars 10.95k forks source link

updateRaw, please #32568

Closed 0x42h closed 4 years ago

0x42h commented 4 years ago

"laravel/framework": "5.8.*"

Case

$update = $statement->update([
  'data' => DB::raw('JSON_SET(data, "$.title", ?)')
]);

Question

how to add the parameter (safely)?

Not the solutions

$update->addBinding("hello"); 
// because update executes immediately; addBinding on boolean
$statement->addBinding("hello")->update([
  'data' => DB::raw('JSON_SET(data, "$.title", ?)')
]); 
// because God knows (and the Illuminate g(uy|irl)s)
// what happens to the order of the parameters in 
// your query, but debugging shows, not much of 
// what you might be wishing for.

The hack

That works, because join-parameters are the first accepted parameters in the query for update.

$statement->addBinding(["hello"], 'join')->update([
  'data' => DB::raw('JSON_SET( `data`, "$.title", ?)')
]);

Suggestions

in case I'm not simply missing something

$statement->updateRaw([
  'data' => JSON_SET( `data`, "$.title", ?)
], [ "hello" ])

or

$statement->update([
  'data' => DB::raw('JSON_SET( `data`, "$.title", ?)', ["hello"])
])

or something better?

driesvints commented 4 years ago

Hi there,

Thanks for reporting but it looks like this is a question which can be asked on a support channel. Please only use this issue tracker for reporting bugs with the library itself. If you have a question on how to use functionality provided by this repo you can try one of the following channels:

However, this issue will not be locked and everyone is still free to discuss solutions to your problem!

Thanks.

0x42h commented 4 years ago

Just read the docs... 😬

https://laravel.com/docs/5.8/queries#updating-json-columns

zmorris commented 3 years ago

I don't think that this question should have been closed until it's answered. Note that it's actually about an inconsistency in Laravel's query builder, not JSON fields specifically.

There's currently a bug/feature of Eloquent where DB::update() accepts a DB::raw() query or string, but Model::update() does not:

php artisan tinker
# these work:

DB::update("update users set id = 1 where id = 1;");

DB::update(DB::raw("update users set id = 1 where id = 1;"));
# these could/should work (although I'm not sure exactly what the syntax would look like, which is probably the real source of this inconsistency):

DB::table('users')->update("set id = 1 where id = 1;");
TypeError: Argument 1 passed to Illuminate/Database/Query/Builder::update() must be of the type array, string given on line 1

DB::table('users')->update(DB::raw("set id = 1 where id = 1;"));
TypeError: Argument 1 passed to Illuminate/Database/Query/Builder::update() must be of the type array, object given on line 1

App\User::where('id', 1)->update('set id = 1 where id = 1;');
TypeError: Argument 1 passed to Illuminate/Database/Eloquent/Builder::update() must be of the type array, string given on line 1

App\User::where('id', 1)->update(DB::raw('set id = 1 where id = 1;'));
TypeError: Argument 1 passed to Illuminate/Database/Eloquent/Builder::update() must be of the type array, object given on line 1

So I agree with 0x42h's Suggestions section above, where they correctly point out that the ::update(<array>) method variants should also accept a second parameter array.

I'm not going to open another issue for this, because Laravel's been around long enough that I feel this is more of a conceptual problem than an implementation problem. Here are some modern approaches to prevent these issues in the future:

Short of that stuff, you also need to provide a way to quote identifiers like table and field names, because right now a lot of people are having to write raw SQL queries by hand when they hit these inconsistencies. That opens up a whole class of security bugs because they have to understand things like quoting database parameters.

Pretty much every mature Laravel project is going to have a workaround similar to this: https://stackoverflow.com/a/13448326 https://dev.mysql.com/doc/refman/8.0/en/identifiers.html

Unfortunately PDO has its own conceptual flaws, it needs a new method or a PDO::PARAM_IDENTIFIER option for PDO::quote(), but that's outside the scope of Laravel: https://stackoverflow.com/a/36283238 https://www.php.net/manual/en/pdo.quote.php https://www.php.net/manual/en/pdo.constants.php

xdroidteam commented 3 years ago

We using Singlestore (aka MemSQL) in our projects, with a custom connector. You only need to extend the \Illuminate\Database\Query\Builder class, this way:

<?php

namespace Your\CustomConnector\Query;

class Builder extends \Illuminate\Database\Query\Builder
{

    /**
     * The current query value bindings.
     *
     * @var array
     */
    public $bindings = [
        'select' => [],
        'from' => [],
        'update' => [],
        'join' => [],
        'where' => [],
        'groupBy' => [],
        'having' => [],
        'order' => [],
        'union' => [],
        'unionOrder' => [],
    ];

...

    /**
     * Update a record in the database, with DB::raw expression
     *
     * @param  array  $values
     * @param  array  $bindings
     * @return int
     */

    public function updateRaw(array $values, array $bindings){
        $this->addBinding($bindings, 'update');

        return $this->update($values);
    }

}

Now you can use, this way:

$statement->updateRaw([
  'data' => DB::raw('JSON_SET( `data`, "$.title", ?)')
], ["hello"])