Cast on Updated At column not applied for rapid saves (i.e. unit tests)

[willpower232]

Laravel Version

10.13.2

PHP Version

8.2.7

Database Driver & Version

No response

Description

apologies for duplicating an unanswered discussion #47506 but I believe this is a bug requiring attention

We are making use of a legacy database written for an ancient framework in Laravel. One "feature" of this framework is that integer unix timestamps are used for the updated_at column instead of datetime strings.

We can deal with this by setting protected $dateFormat = 'U'; on the model classes however ideally we would like to use datetimes for application-specific time-related columns going forward so don't want to force the format on all new columns.

We wrote a cast to juggle the value from an instance of Carbon to $carbon->timestamp and all seemed well until we went to write unit tests for the functionality.

The tests failed because data was being truncated for the updated_at column as it was receiving a datetime string instead of the timestamp from the cast.

Warning: 1265 Data truncated for column 'updated_at' at row 1 (Connection: mysql, SQL: update `customers` set `name` = redacted, `customers`.`updated_at` = 2023-07-18 10:49:06 where `id` = 138)

I believe this is occurring here as it does not seem to check for casts before appending an updated_at value

framework/src/Illuminate/Database/Eloquent/Builder.php

Line 1140 in 31b3d29

[$column => $this->model->freshTimestampString()],

I think that updated_at is not marked as dirty because the value has not changed because the change has been within the same second as the creation or another update.

A quick resolution to this problem within tests is to include $this->travelTo(now()->addMinute()); before every change made to a particular model so that the casted value is picked up with the dirty attributes

framework/src/Illuminate/Database/Eloquent/Model.php

Line 1212 in 31b3d29

$dirty = $this->getDirty();

Obviously this is not possible within the main application so this problem may occur there if there are saves within the same second from different parts of the code.

if you create an entity in the database and then update the entity via a queued job then you encounter this error and must delay the job being dispatched

I am unsure as to whether the resolution is for the builder to cast the value before appending it or for any dirty attributes check to always include the updated_at value if any other changed values are present.

Steps To Reproduce

Have the following database schema and code

CREATE TABLE `customers` (
  `id` int(11) unsigned NOT NULL AUTO_INCREMENT,
  `customer_name` varchar(100) DEFAULT NULL,
  `created_at` int(11) DEFAULT NULL,
  `updated_at` int(11) DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

<?php

namespace App\Models;

use App\Casts\UnixTimeStampToCarbon;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;

class Customer extends Model
{
    use HasFactory;

    protected $casts = [
        'created_at' => UnixTimeStampToCarbon::class,
        'updated_at' => UnixTimeStampToCarbon::class,
    ];
}

<?php

namespace App\Casts;

use Carbon\Carbon;
use Illuminate\Contracts\Database\Eloquent\CastsAttributes;

/**
 * @implements CastsAttributes<Carbon|null, Carbon>
 */
class UnixTimeStampToCarbon implements CastsAttributes
{
    /**
     * Cast the given value.
     *
     * @param  \Illuminate\Database\Eloquent\Model  $model
     * @param  mixed  $value
     * @param  array<mixed>  $attributes
     * @return Carbon|null
     */
    public function get($model, string $key, $value, array $attributes)
    {
        return $value ? Carbon::createFromTimestampUTC($value) : null;
    }

    /**
     * Prepare the given value for storage.
     *
     * @param  \Illuminate\Database\Eloquent\Model  $model
     * @param  mixed $value
     * @param  array<mixed>  $attributes
     * @return int|float|string|null
     */
    public function set($model, string $key, $value, array $attributes)
    {
        if (is_string($value) || is_numeric($value) || ($value instanceof \DateTime && ! $value instanceof Carbon)) {
            $value = Carbon::parse($value);
        }

        return $value ? $value->timestamp : null;
    }
}

<?php

namespace Tests\Feature\Http\Controllers;

use App\Models\Customer;
use App\Models\User;
use Illuminate\Contracts\Auth\Authenticatable;
use Tests\TestCase;

class CustomerControllerTest extends TestCase
{
    public function testUpdate(): void
    {
        /** @var Authenticatable|User $user */
        $user = User::factory()
            ->create();

        $customer = Customer::factory()->create([
            'customer_name' => 'Test Customer',
        ]);

        // this is required because the customer has just been created
        $this->travelTo(now()->addMinute());
        $response = $this
            ->actingAs($user)
            ->put(
                route('customers.update', $customer),
                [
                    'customer_name' => 'Updated Customer',
                ]
            );
        $response->assertSessionHasNoErrors();
        $response->assertRedirect(route('customers.index'));

        $customer->refresh();
        $this->assertSame('Updated Customer', $customer->customer_name);
    }
}

[driesvints]

Thanks. The freshTimestampString indeed seems problematic. Would appreciate a PR to remedy this.

[github-actions]

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

[willpower232]

@driesvints thanks for confirming, would you be able to nominate what you think the correct solution is?

I am unsure as to whether the resolution is for the builder to cast the value before appending it or for any dirty attributes check to always include the updated_at value if any other changed values are present.

I feel like the latter probably makes more sense, if there are dirty attributes present then include the updated_at which already seems to have been through the casts, but happy to look at the other option if that makes more sense to you.

[driesvints]

I'm not sure myself, sorry. I'd say the former because the latter could have side effects that people don't want.

[Mdhesari]

what do you think about this?

@taylorotwell

[amir9480]

@willpower232
How do you implement the controller for customers.update?
Make sure you are not using query builder's update or similar methods instead of Eloquent's update method.

[willpower232]

@amir9480 unfortunately its definitely eloquent related

[timacdonald]

Hey folks, I dived into this one and have a potential fix, but I'm not 100% if it is suitable or how I feel about it - so I wanted to float it with those experiencing the issue.

The first fix would require a new property on the model indicating the format of the "updated_at" timestamp AND the cast in place.

I don't love this.

Another solution, which I prefer at this point, but is a bit more expensive, is to run the new value that the builder generates via the model's cast again. Introducing the if here means that the new model instance will only be created if the updated_at is not dirty, which is probably pretty rare in a real-world scenario - but certainly could happen, and does happen in unit tests.

This approach means that there is no changes in the user model, i.e. there isn't a new "updatedAtFormat" property.

Would love your input on these approaches.

[timacdonald]

We can follow along with this fix I have just created: #47942