Skip to content
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

Jump to bottom

[4.x] use mongodb driver to check for dirtiness #1990

Closed
wants to merge 1 commit into from
Closed

[4.x] use mongodb driver to check for dirtiness #1990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 6 additions & 13 deletions src/Jenssegers/Mongodb/Eloquent/Model.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
use MongoDB\BSON\Binary;
use MongoDB\BSON\ObjectID;
use MongoDB\BSON\UTCDateTime;
use function MongoDB\BSON\fromPHP;
use MongoDB\Driver\Exception\UnexpectedValueException;

abstract class Model extends BaseModel
{
Expand Down Expand Up @@ -239,20 +241,11 @@ public function originalIsEquivalent($key, $current)
return false;
}

if ($this->isDateAttribute($key)) {
$current = $current instanceof UTCDateTime ? $this->asDateTime($current) : $current;
$original = $original instanceof UTCDateTime ? $this->asDateTime($original) : $original;

return $current == $original;
}

if ($this->hasCast($key)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The only problem I see is a casted attributes, probably we need to check after keys was casted back to their values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@divine Looking at the code, I don't see it is required to check for casts. The input values passed by getDirty() are uncasted:

    public function getDirty()
    {
        $dirty = [];

        foreach ($this->getAttributes() as $key => $value) {
            if (! $this->originalIsEquivalent($key, $value)) {
                $dirty[$key] = $value;
            }
        }

        return $dirty;
    }
    public function getAttributes()
    {
        return $this->attributes;
    }

Besides, I don't see why one should use Laravel <=6 casting system for MongoDB, I don't see a test for it in the repository either. Laravel 7 custom casts are a different story. I hope they won't make things go wrong. I've already had my custom cast implementation along side this PR in Larave 5.8 production without an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, we're dropping support for laravel 6 (no other way to drop object id auto-casting) and this branch will be focused for versions > 7.0, so that's the reason why I've asked this question.

This might be an issue with Laravel 7 when it wouldn't check dirtiness for casted objects.

Yes, there are no tests, I think a lot of tests are missing, might take a look myself (for tests).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@divine Then I should send this PR on top of your own PR #1988 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably no, it's not ready yet, still have something to do. I will let you know, ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@divine I have already done some checks and coding for Laravel 7. Sounds like casting in Laravel 7 also doesn't break things.
I'll send you that PR anyway. I'll wait for you to let me work on my unset-field PR #1667 after this is done.

return $this->castAttribute($key, $current) ===
$this->castAttribute($key, $original);
try {
return fromPHP([$current]) === fromPHP([$original]);
} catch (UnexpectedValueException $e) {
return false;
}

return is_numeric($current) && is_numeric($original)
&& strcmp((string) $current, (string) $original) === 0;
}

/**
Expand Down
41 changes: 40 additions & 1 deletion tests/ModelTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,13 +555,52 @@ public function testMultipleLevelDotNotation(): void
public function testGetDirtyDates(): void
{
$user = new User();
$user->setRawAttributes(['name' => 'John Doe', 'birthday' => new DateTime('19 august 1989')], true);
$user->name = 'John Doe';
$user->birthday = new DateTime('19 august 1989');
$user->syncOriginal();
$this->assertEmpty($user->getDirty());

$user->birthday = new DateTime('19 august 1989');
$this->assertEmpty($user->getDirty());
}

public function testGetDirty(): void
{
$ids = [
new ObjectId(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Add value for ObjectId please

new ObjectId(),
];
$item = new Item([
'numbers' => [1, 2, 3],
'number' => 4,
'ids' => $ids,
'nullable' => 'value',
'fix' => 'fix',
]);
$item->date = $item->fromDateTime(new DateTime('19 august 1989'));
$item->dates = [$item->fromDateTime(new DateTime('19 august 1989'))];
$item->save();

$item = $item->fresh();

$item->numbers = [1, 2, '3'];
$item->nullable = null;
$item->new_val = 'new';
$item->number = '4';
$item->ids = [
Copy link
Contributor

@Smolevich Smolevich Mar 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like strange, i suggest to replace on

$item->ids = $ids;

or

$item->ids = [
    new ObjectId('576c25db6118fd406e6e6471'),
    new ObjectId('576c25db6118fd406e6e6472')
];

new ObjectId((string) $ids[0]),
new ObjectId((string) $ids[1]),
];
$item->date = $item->fromDateTime(new DateTime('19 august 1989'));
$item->dates = [$item->fromDateTime(new DateTime('19 august 1989'))];
$this->assertEquals([
'numbers' => [1, 2, '3'],
'number' => '4',
'nullable' => null,
'new_val' => 'new',
], $item->getDirty());
}

public function testChunkById(): void
{
User::create(['name' => 'fork', 'tags' => ['sharp', 'pointy']]);
Expand Down