Fix overwriting updated_at with $set

[guram-vashakidze]

Hello,

1. Found unexpected behaviour when I'm adding updated_at into $set in Eloquent update. For example:

MyModel::query()
    ->where('foo', 'bar')
    ->update(['$set' => ['...' => '...', 'updated_at' => Carbon::now()->toAtomString()]]);

In this case current implementation is overwrite updated_at

2. The $dateFormat is ignored when created_at and updated_at are added automatically (when model is saving) and every time set UTCDateTime as format for value in DB. Added check - if dateFormat is set in model -> Date::now() is returned from freshTimestamp instead of UTCDateTime(Date::now())

[guram-vashakidze]

UPD: 2nd commit is not solution. My fault.

[masterbater]

how about just create a new field if you have custom updated timestamp like modified_date while leaving default behavior of timestamps?

or modify the constant of UPDATED_AT
const UPDATED_AT = null; // Disables automatic updating of the updated_at column
const UPDATED_AT = 'modified_date'; // Use 'modified_date' instead of 'updated_at'

or if its on specific Model just use withoutTimestamps

[guram-vashakidze]

Hello @masterbater,
Not sure about which issue you're answering.

Turning off timestamp - it's not solution for first issue. I still need updated_at with autofill, but sometimes I need to set this value manually. As result:

• with turning off timestamp I'll lose autofill
• changing UPDATED_AT const - also not solution, I'll have same same issue but under another column.

If it's related to 2nd issue, not sure that it's good to ignore property from parent class (trait).
This property means:

 /**
     * The storage format of the model's date columns.
     *
     * @var string
     */
    protected $dateFormat;

I guessed better to not ignore this property, I faced unexpected behaviour, and it's true, doesn't it?

[guram-vashakidze]

Hello @jmikola,

Can you look at it, pls?

[masterbater]

Hello @masterbater,
Not sure about which issue you're answering.

Turning off timestamp - it's not solution for first issue. I still need updated_at with autofill, but sometimes I need to set this value manually. As result:

• with turning off timestamp I'll lose autofill
• changing UPDATED_AT const - also not solution, I'll have same same issue but under another column.

If it's related to 2nd issue, not sure that it's good to ignore property from parent class (trait).
This property means:

 /**
     * The storage format of the model's date columns.
     *
     * @var string
     */
    protected $dateFormat;

I guessed better to not ignore this property, I faced unexpected behaviour, and it's true, doesn't it?

I meant is you have inconsistent date format for updated_at String and Date might as well create a new field modified_at for custom updated_at to have better consistency. Saving everything as UtcDate is better and easy converting to different timezones. If you have need to convert some updated_at into AtomString and you have other fields that tracks it like is_date_string

Example

protected function updatedAt(): Attribute
{
return Attribute::make(
get: fn (string $value, $attributes) => $attributes[''is_date_string]? Carbon::parse($value)->toAtomString : $value,
);
}

I will share to you that inconsistent updated_at type in database like String and Date it will fail when sorting and doing filters your string will get ignored when filtering date

I mean it can be done in aggregate but you need to query all,.transform them to all dates and its going to suffer performance

[GromNaN]

Hello,

Thanks for opening a PR, we really appreciate people who take the time to dig out the bugs and come up with fixes. When you fix a bug, we require a non-regression test to be added. This is not the case here, which makes it hard to understand the bug despite the description.

1. Found unexpected behaviour when I'm adding updated_at into $set in Eloquent update. For example:

MyModel::query()
    ->where('foo', 'bar')
    ->update(['$set' => ['...' => '...', 'updated_at' => Carbon::now()->toAtomString()]]);

In Laravel, it's possible to force a different value for the updated_at field. The code check s if the value is set before setting it. But in the Laravel-MongoDB package, the values in the root of the update array replace the ones in $set.

laravel-mongodb/src/Query/Builder.php

Lines 792 to 802 in 2724144

	public function update(array $values, array $options = [])
	{
	// Use $set as default operator for field names that are not in an operator
	foreach ($values as $key => $value) {
	if (is_string($key) && str_starts_with($key, '$')) {
	continue;
	}
	$values['$set'][$key] = $value;
	unset($values[$key]);
	}

If you remove the $set level, you can force a different value:

-     ->update(['$set' => ['...' => '...', 'updated_at' => Carbon::now()->toAtomString()]]);
+     ->update(['...' => '...', 'updated_at' => Carbon::now()->toAtomString()]);

You can update the PR to fix this issue: check if $values['$set']['updated_at'] is set before setting $values['updated_at']; and add a test for this in the ModelTest class.

In this case current implementation is overwrite updated_at

2. The $dateFormat is ignored when created_at and updated_at are added automatically (when model is saving) and every time set UTCDateTime as format for value in DB. Added check - if dateFormat is set in model -> Date::now() is returned from freshTimestamp instead of UTCDateTime(Date::now())

MongoDB has a native date format, storing dates in strings is inefficient and reduces database capacity. The $dateFormat property is inherited from Eloquent SQL and is ignored on MongoDB model classes.

[masterbater]

The $dateFormat is ignored when created_at and updated_at are added automatically (when model is saving) and every time set UTCDateTime as format for value in DB

I think they ignored it because mongodb save date in Date type in the document itself.

The BSON Date type allows for efficient and accurate date and time operations such as sorting, filtering (e.g., $gt, $lt), and aggregation framework operations (e.g., $dateToString, $dayOfMonth).BSON Date type ensures consistent handling and allows client applications to display dates in the desired time zone and index better

Why storing dates as strings is generally discouraged:

Inconsistent Formatting:
String-based dates can lead to inconsistencies if not strictly formatted, making queries and comparisons unreliable.

Inefficient Operations:

Performing date-based operations on string fields can be less efficient as it often requires string comparisons rather than native date comparisons.

Time Zone Challenges:

Handling time zones with string-based dates can be complex and error-prone, potentially leading to incorrect interpretations of time.

[guram-vashakidze]

Hello @GromNaN,
Thank you for answering.

It's just example of update where I use just $set, but in my request can be at the same time:

• $set
• $unset
• $push
• $pull
  To have more readable code I'm using $set:

MyModel::query()
    ->where('foo', 'bar')
    ->update(
        [
            '$set' => [
                '...' => '...',
                'updated_at' => Carbon::now()->toAtomString()
            ],
            '$unset' => [
                '....'
            ],
            '$push' => [
                '....'
            ],
            '$pull' => [
                '....'
            ],
        ]
    );

And I saw in source code even for this example possible to use:

MyModel::query()
    ->where('foo', 'bar')
    ->update(
        [
            '...' => '...',
            'updated_at' => Carbon::now()->toAtomString(),
            '$unset' => [
                '....'
            ],
            '$push' => [
                '....'
            ],
            '$pull' => [
                '....'
            ],
        ]
    );

But you must admit that the first option is more readable than the second 🙂
And you're supporting $set in update request because of it overwriting updated_at in that case looks like a bug for me.

MongoDB has a native date format, storing dates in strings is inefficient and reduces database capacity. The $dateFormat property is inherited from Eloquent SQL and is ignored on MongoDB model classes.

It's clear, but MongoDB is giving us freedom to choose what and how we would like to save data in our storage and just we responsible of it. If I consciously want to save data in a certain format, why not give me that choice?
@masterbater it's also answer on your comment 🙂

[masterbater]

$set
$unset
$push
$pull

These raw operators already have a dedicated query builder methods which is more in line with laravel way of things.

You can check the docs.
Readability are subjective, and writing those as string are more prone with typo you can do enums etc to handle that but methods with ide autocompletion are better and phpstan

[guram-vashakidze]

@masterbater Ofc, has.
But at the same time update method is supported it in raw format. Doesn't it look like a bug - supporting raw format with the ability to use $set and overwriting updated_at?

These raw operators already have a dedicated query builder

How it's possible to use those operators trough queue builder in one query? I don't see ability to run unset()->update()->pull()->push();

UPD:
I see that just unset is returning Builder object and at the same time is running independent request to DB. Just after unset possible to run new request. But even in that case it doesn't look as correct approach instead of 1 call have 2-4 calls.

[masterbater]

@masterbater Ofc, has. But at the same time update method is supported it in raw format. Doesn't it look like a bug - supporting raw format with the ability to use $set and overwriting updated_at?

These raw operators already have a dedicated query builder

How it's possible to use those operators trough queue builder in one query? I don't see ability to run unset()->update()->pull()->push();

UPD: I see that just unset is returning Builder object and at the same time is running independent request to DB. Just after unset possible to run new request. But even in that case it doesn't look as correct approach instead of 1 call have 2-4 calls.

I feel you man, when I started using this package, I keep having issues and understanding do my biggest concern was that package compatibility and the native objectid relationship is not supported. There are times I wanted to fight and override the behavior but I just followed it (string relationships) and @GromNaN is working hard like the package compatibility using DocumentModel trait and the id attribute standardization in v5 was his greatest feature yet that makes it not so painful working with this package. And awesome work with @paulinevos is working on ide experience by adding types. @alcaeus and @jmikola they carry the back of the mongodb driver and do code review here. Hopefully this people would have more strength and dedication to continue improving this package as much as possible remove those pain points that is out of laravels query builder and eloquent compat

[guram-vashakidze]

@masterbater I see here has done big job. It's really flexible for me. As you mentioned - I can use independently:

• ->push()
• ->unset()
• ->pull()
• ->update()

And at the same time I can use all those methods in one update request:

->update(
        [
            '$set' => [
                '....'
             ],
            '$unset' => [
                '....'
            ],
            '$push' => [
                '....'
            ],
            '$pull' => [
                '....'
            ],
        ]
    );

Really nice they left this ability / behaviour - it's flexible.

Not problem for me if someone will close this PR if it's not respond current paradigm. I've found unexpected behaviour and suggested fix that's all 🙂

[GromNaN]

Do you want to fix the issue that way?

You can update the PR to fix this issue: check if $values['$set']['updated_at'] is set before setting $values['updated_at']; and add a test for this in the ModelTest class.

[guram-vashakidze]

@GromNaN this part not so understand... I'm checking it before updated_at is setting, no?

check if $values['$set']['updated_at'] is set before setting $values['updated_at']

yes, I'll add tests.

[masterbater]

@masterbater I see here has done big job. It's really flexible for me. As you mentioned - I can use independently:

• ->push()
• ->unset()
• ->pull()
• ->update()

And at the same time I can use all those methods in one update request:

->update(
        [
            '$set' => [
                '....'
             ],
            '$unset' => [
                '....'
            ],
            '$push' => [
                '....'
            ],
            '$pull' => [
                '....'
            ],
        ]
    );

Really nice they left this ability / behaviour - it's flexible.

Not problem for me if someone will close this PR if it's not respond current paradigm. I've found unexpected behaviour and suggested fix that's all 🙂

😂 sorry bro, it really has an unexpected behavior. Cheers on your PR bro and sorry for contributing noise here 🫶

[GromNaN]

Your first change is indeed correct.

MongoDB is giving us freedom to choose what and how we would like to save data in our storage and just we responsible of it. If I consciously want to save data in a certain format, why not give me that choice?

That's conventions. We try to keep the choice when possible. Laravel is opinionated, for full flexibility you must use raw queries or the MongoDB library directly.

[guram-vashakidze]

@GromNaN done,
pls, check it

[GromNaN]

Thank you @guram-vashakidze

[guram-vashakidze]

@GromNaN thank you too 🤝