iksaku / laravel-mass-update

Update multiple Laravel Model records, each with its own set of values, sending a single query to your database!
MIT License
126 stars 11 forks source link

Error updating boolean values using PostgreSQL #15

Open javebers opened 2 months ago

javebers commented 2 months ago

Boolean values ​​are being cast to int, wich works with MySQL but throws an error when using PostgreSQL. Postgres accepts '0' and '1' as strings, but not as integers, from what I understand.

Otherwise it works great. Thanks for the work you put into this. πŸ‘

iksaku commented 2 months ago

Thanks for the feedback!

My first idea is that there should be some kind of function to automatically handle primitives. Hopefully in PDO or similar. I'll take a look at it

iksaku commented 2 months ago

Found a built-in method ✨: https://github.com/laravel/framework/blob/v10.13.0/src/Illuminate/Database/Grammar.php#L213

The only problem is that it requires Laravel >=10.13 and we currently bring support for version 8.

Good enough excuse to take a look at v2 of the package. Will look for some time next week to work through this.

iksaku commented 1 month ago

Please checkout the 2.x branch and let me know if this is resolved.

You should be able to install it using:

composer require iksaku/laravel-mass-update:dev-2.x
javebers commented 1 month ago

I just checked out the new version, and it works fine in my tests. πŸŽ‰ The removal of Model::massUpdateQuietly() caught me off guard, but reading the upgrade doc, it makes sense.

Just a suggestion, I think it would be better to call escape directly from connection, since Grammar internally does that.

$escape = function (mixed $value) use ($query)
{
    // ...
    // return $query->getGrammar()->escape($value);
    return $query->getConnection()->escape($value);
};

Not only is it more direct (which doesn't really matter too much to be honest), but it can also avoid problems for those who are extending the Grammar class.

TL;DR Let me explain. After upgrading to the new version, this RuntimeError was thrown:

The database driver's grammar implementation does not support escaping values.

Then I realized that the problem must be related to the fact that I am using an extended version of PostgresGrammar, to handle all timestamps in 'Y-m-d H:i:s.u' format by default. I implemented this the way Carbon recommends in their documentation related to Laravel.

What is not taken into account there is that Laravel when instantiating Grammar, assigns it the Connection instance so that when a method like escape is called, it is able to pass the execution to the Connection instance, which is what it ultimately does.

This is not difficult to fix and I think it is what should be done if the db grammar is being exended, to avoid future problems.

class AppServiceProvider extends ServiceProvider
{
    public function boot()
    {
        $connection = \DB::connection();
        $connection->setQueryGrammar(
            (new MyPostgresGrammar())->setConnection($connection),
        );
    }

I know this has nothing to do with this library, but I just wanted to clarify in case anyone runs into this problem. And also detail why using getConnection()->escape() avoids this error.

Btw, thanks for fixing this so quickly. πŸ‘

iksaku commented 1 month ago

Now that's a really great observation. Thank you! I've not been a user of custom grammars, so I lack the insight you just brought to me, and it makes total sense.

Just pushed out https://github.com/iksaku/laravel-mass-update/commit/7a18113537e6a64815dfd2a6fe2603a1addb170e, that should fix the new issue.

P.S: I'm currently working on ci/databases to expand the test suite to run in multiple DB engines for the v2 release πŸ‘€