Skip to content

Fix incorrect behaviour when comparing castable attributes in originalIsEquivalent method #3439

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: 5.x
Choose a base branch
from
Open

Fix incorrect behaviour when comparing castable attributes in originalIsEquivalent method #3439

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c6147c9
Fix overwriting `updated_at` when `$set` is used
guram-vashakidze Jul 24, 2025
74d2cb5
Merge branch 'mongodb:5.x' into 5.x
guram-vashakidze Aug 7, 2025
f027089
fix incorrect behaviour when comparing castable attributes in `origin…
guram-vashakidze Aug 7, 2025
40ecfd0
fix phpDoc return type
guram-vashakidze Aug 7, 2025
8e808a3
comparing castable attributes just by serialization
guram-vashakidze Aug 8, 2025
13d48d4
add one more "assert" into tests
guram-vashakidze Aug 8, 2025
7358dcc
change comparing by serialization to different comparing type (relate…
guram-vashakidze Aug 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Eloquent/DocumentModel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
use function is_string;
use function ltrim;
use function method_exists;
use function serialize;
use function sprintf;
use function str_contains;
use function str_starts_with;
Expand Down Expand Up @@ -377,6 +378,10 @@ public function originalIsEquivalent($key)
$this->castAttribute($key, $original);
}

if ($this->isClassCastable($key)) {
return serialize($attribute) === serialize($original);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you checking the result of serialize here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because of using ===

$b1 = new Binary(hex2bin('0c103357380648c9a84b867dcb625cfb'), Binary::TYPE_UUID);
$b2 = new Binary(hex2bin('0c103357380648c9a84b867dcb625cfb'), Binary::TYPE_UUID);

$b1 === $b2; //false
serialize($b1) === serialize($b2); //true

Copy link
Member

@alcaeus alcaeus Aug 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I suspected. Using serialize is not the right way to go about this. See my comment above about using == when comparing objects. Strictly speaking you should be comparing the BSON representation, but since that's not always easily possible I think a loose object comparison (which will leverage a class' compare_objects handler) is a safe middle ground.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, not sure I understood correctly, but why serialization is not right way here, can you explain a bit more, pls.
Example from your previous thread works correct. I guess there is even possible to leave just serialization. This example also working correctly:

$d1 = new Int64(12);
$d2 = new Int64(12);

$d1 === $d2; //false
serialize($d1) === serialize($d2); //true

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=== is an identity check, meaning that values have to be identical. For scalar values this works fine, but it breaks down for objects as PHP will compare the internal object IDs as you've shown in the previous example.
== is an equality check, meaning there is some advanced comparison logic happening for objects. For objects that have a compare_objects handler implemented (which can only be done for internal classes such as MongoDB\BSON\Binary, it will call the first handler it finds (first checking the object on the left side of the operation, then that on the right) and return that result. If there is no compare_objects handler, it falls back to a property equality check for objects of the same class and returns false when the objects are of different classes.

This equality check is what we want here -- loosely speaking, when two values are equal (ignoring PHP's type coercion shenanigans around numeric strings) we can assume that they will map to the same representation in the database. serialize is an unnecessary complication at that point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The theory of differences ==/=== i know but with all due respect to your work and your knowledge, I cannot agree with you that serialize is unnecessary complication. What you described much more complicate for me... at first check both values is scalar or no, next compare this values by ===. If it's not scalar - compare objects == . In most cases first compare will work. As I said before the easiest way here - leave just serialization (even remove first if) which will work as expected for all types. Also comparing objects trough serialization is giving us feature to affect compare behaviour (not sure how it can be used :D).
It's just my thought, you're deciding is it respond your repo paradigm or no 🤝

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alcaeus I left comparing just by serialization. I faced an issue with comparing after castAttribute. The root in Laravel here. When I'm getting result from castAttribute the result the same for $original and for $attribute in every cases.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on the issue you're facing? I'm not super sure what you mean.

I replaced your comparison with this and your test case was green

if ($this->isClassCastable($key)) {
    return match (gettype($attribute)) {
        'object' => $attribute == $original,
         default => $attribute === $original,
    };
}

Also doesn't seem to complicate things, unless I'm missing something. I agree with @alcaeus that serializing doesn't seem right here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulinevos hello,
when I'm using serialization - it works for all variable types, without additional type definition and different comparing type.
Additionally I don't really like to use NOT strict comparing.

For this PR I changed usage to check variable type.

}

return is_numeric($attribute) && is_numeric($original)
&& strcmp((string) $attribute, (string) $original) === 0;
}
Expand Down
14 changes: 14 additions & 0 deletions tests/ModelTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
use MongoDB\Laravel\Tests\Models\Item;
use MongoDB\Laravel\Tests\Models\MemberStatus;
use MongoDB\Laravel\Tests\Models\NonIncrementing;
use MongoDB\Laravel\Tests\Models\Options;
use MongoDB\Laravel\Tests\Models\Soft;
use MongoDB\Laravel\Tests\Models\SqlUser;
use MongoDB\Laravel\Tests\Models\User;
Expand Down Expand Up @@ -1075,6 +1076,19 @@ public function testGetDirtyDates(): void
$this->assertEmpty($user->getDirty());
}

public function testGetDirtyObjects(): void
{
$user = new User();
$user->options = new Options();
$this->assertNotEmpty($user->getDirty());

$user->save();
$this->assertEmpty($user->getDirty());

$user->options = (new Options())->setOption1('Value1');
$this->assertNotEmpty($user->getDirty());
}

public function testChunkById(): void
{
User::create(['name' => 'fork', 'tags' => ['sharp', 'pointy']]);
Expand Down
37 changes: 37 additions & 0 deletions tests/Models/Options.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php

declare(strict_types=1);

namespace MongoDB\Laravel\Tests\Models;

class Options
{
private string $option1;
private string $option2;

public function setOption1(string $option1): self
{
$this->option1 = $option1;
return $this;
}

public function setOption2(string $option2): self
{
$this->option2 = $option2;
return $this;
}

public function serialize(): object
{
$result = [];
if (isset($this->option1)) {
$result['option1'] = $this->option1;
}

if (isset($this->option2)) {
$result['option2'] = $this->option2;
}

return (object) $result;
}
}
40 changes: 40 additions & 0 deletions tests/Models/OptionsCast.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

declare(strict_types=1);

namespace MongoDB\Laravel\Tests\Models;

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

class OptionsCast implements CastsAttributes
{
public function get(Model $model, string $key, mixed $value, array $attributes): Options
{
$attributes = new Options();
if (! empty($value['option1'])) {
$attributes->setOption1($value['option1']);
}

if (! empty($value['option2'])) {
$attributes->setOption2($value['option2']);
}

return $attributes;
}

/**
* @param Model $model
* @param string $key
* @param Options|null $value
* @param array $attributes
*
* @return null[]|object[]
*/
public function set(Model $model, string $key, mixed $value, array $attributes): array
{
return [
$key => $value?->serialize(),
];
}
}
2 changes: 2 additions & 0 deletions tests/Models/User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
* @property Carbon $updated_at
* @property string $username
* @property MemberStatus member_status
* @property Options $options
*/
class User extends Model implements AuthenticatableContract, CanResetPasswordContract
{
Expand All @@ -44,6 +45,7 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon
'birthday' => 'datetime',
'entry.date' => 'datetime',
'member_status' => MemberStatus::class,
'options' => OptionsCast::class,
];

protected $fillable = [
Expand Down
Loading