-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix overwriting updated_at
with $set
#3433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
UPD: 2nd commit is not solution. My fault.
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
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?
Hello @jmikola,
Can you look at it, pls?
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
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.
- 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
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
- The
$dateFormat
is ignored whencreated_at
andupdated_at
are added automatically (when model is saving) and every time setUTCDateTime
as format for value in DB. Added check - ifdateFormat
is set in model -> Date::now() is returned fromfreshTimestamp
instead ofUTCDateTime(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.
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.
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 🙂
$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
@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 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 overwritingupdated_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 afterunset
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
@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 🙂
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 theModelTest
class.
@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 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 🫶
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@GromNaN done,
pls, check it
updated_at
when $set
is used. Fix ignoring dateFromat
(削除ここまで)updated_at
with $set
(追記ここまで)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @guram-vashakidze
@GromNaN thank you too 🤝
Hello,
updated_at
into$set
in Eloquent update. For example:In this case current implementation is overwrite
updated_at
$dateFormat
is ignored whencreated_at
andupdated_at
are added automatically (when model is saving) and every time setUTCDateTime
as format for value in DB. Added check - ifdateFormat
is set in model -> Date::now() is returned fromfreshTimestamp
instead ofUTCDateTime(Date::now())