[8.x] Inverse morphable type and id filter statements to prevent SQL errors

[damiencriado]

There is an SQL bug when using a morph relation when a morphable id column has rows with mixed integer and UUID values.

Setup

Here is my notifications table (note that we use varchar column because there are tables with primary key as UUID and others with primary key as integer):

CREATE TABLE `notifications` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `notifiable_type` varchar(128) NOT NULL DEFAULT '',
  `notifiable_id` varchar(36) NOT NULL DEFAULT '',
  `data` longtext DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1;

Now let's say that we have 2 rows in the notifications table, one with a notifiable_id as integer and the other with a notifiable_id as string.

INSERT INTO `notifications` (`notifiable_id`, `notifiable_type`, `data`) VALUES ('1', 'App\\Models\\User', 'bar');
INSERT INTO `notifications` (`notifiable_id`, `notifiable_type`, `data`) VALUES ('15ae6dab-ac4b-494e-bf28-0bfa068f378f', 'App\\Models\\Guest', 'bar');

Here is my User model:

class User extends Model
{
    public function notifications()
    {
        return $this->morphMany(Notification::class, 'notifiable');
    }
}

Now, let's reproduce the bug

1. We want to update the notifications relation from a User model:

App\Models\User::find(1)->notifications()->update(['data' => 'foo']);

2. This query will trigger an SQL error:

Truncated incorrect DOUBLE value: '15ae6dab-ac4b-494e-bf28-0bfa068f378f'
Error code 1292.

3. Behind the scene, Eloquent is executing this query to update the table:

UPDATE
    notifications
SET
    data = "foo"
WHERE
    notifiable_id = 1	
    AND notifiable_id IS NOT NULL	
    AND notifiable_type = "App\Models\User"

Explanation

The statement notifiable_id = 1 where 1 is an integer, will force MySQL to cast the entire column notifiable_id as integer, and then try to find the record 1 in dataset. But UUID values can't be casted...

One solution is to force quoting the value like notifiable_id = '1' but it will create performance issues.

The other solution (and the retained here) is to inverse the notifiable_type and notifiable_id statements.

It is probably safe to assume that a table's primary key is either an integer or a string, but not both.
By filtering by notifiable_type before notifiable_id, MySQL constrains his dataset to records with only valid integer ids and will not try to cast uuid as integer.

Here is the final query executed by MySQL:

UPDATE
    notifications
SET
    data = "foo"
WHERE
    notifiable_type = "App\Models\User"	
    AND notifiable_id = 1	
    AND notifiable_id IS NOT NULL

Finally

• This allows developers to have a morphable id column with multiple data types
• This will not break anything as the changes are very small

[taylorotwell]

Looks like a test is broken. Marking as draft until fixed and passing.

[damiencriado]

Looks like a test is broken. Marking as draft until fixed and passing.

Sorry for the mistake, tests passed now.

[taylorotwell]

Does this PR actually solve the problem on your machine. I'm actually pretty surprised this works.

[damiencriado]

Does this PR actually solve the problem on your machine. I'm actually pretty surprised this works.

We have done several tests on our end with multiple SQL engines (MariaDB, MySQL), and swapping the statements solves the problem. It's very easy to reproduce.