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

halaei · 2020-03-06T09:50:34Z

Reopening of #1664

To check if a field is dirty we can simply use fromPHP() function of the mongodb driver. Besides, we shouldn't use the SQL-specific logic of Illuminate\Eloquent\Model::originalIsEquivalent() for mongodb. In sql, fields keep their schema data types and changing integer 1 to string '1' actually does nothing. However, in mongodb fields accept their types as they are inserted/updated. Therefore, if we change value of a field from (int)1 to (string)'1' we actually must regard it as dirty. This change is backward incompatible. Don't know if anyone likes the current behaviour more.
--

jenssegers · 2020-03-06T09:52:24Z

Codecov Report

Merging #1990 into develop will decrease coverage by 0.08%.
The diff coverage is 33.33%.

@@              Coverage Diff              @@
##             develop    #1990      +/-   ##
=============================================
- Coverage      82.80%   82.72%   -0.09%     
+ Complexity       531      526       -5     
=============================================
  Files             27       27              
  Lines           1210     1204       -6     
=============================================
- Hits            1002      996       -6     
  Misses           208      208

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b379bd3...9e7866a. Read the comment docs.

codecov-io · 2020-03-06T09:52:34Z

Codecov Report

Merging #1990 into develop will decrease coverage by 0.08%.
The diff coverage is 33.33%.

@@              Coverage Diff              @@
##             develop    #1990      +/-   ##
=============================================
- Coverage       82.8%   82.72%   -0.09%     
+ Complexity       531      526       -5     
=============================================
  Files             27       27              
  Lines           1210     1204       -6     
=============================================
- Hits            1002      996       -6     
  Misses           208      208

Impacted Files	Coverage Δ	Complexity Δ
src/Jenssegers/Mongodb/Eloquent/Model.php	`80.98% <33.33%> (-0.78%)`	`75 <0> (-5)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b379bd3...9e7866a. Read the comment docs.

divine

@halaei first of all, good work! I think we need to add a test with casted attribute to check if it's working correctly as expected.

divine · 2020-03-06T11:11:09Z

src/Jenssegers/Mongodb/Eloquent/Model.php

-            return $current == $original;
-        }
-
-        if ($this->hasCast($key)) {


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

@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.

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).

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

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

@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.

halaei · 2020-03-06T11:53:10Z

@jenssegers Nice coverage tool. But I feel I decreased the source code complexity in terms of both lines of code and branches. I also added new tests. How come the coverage is decreased? I think the report is somehow wrong.

halaei · 2020-03-06T12:01:31Z

I see the report now. I didn't test for when exception is thrown. I don't quite remember. Maybe it has to do with my next pull request regarding unset field #1667

Smolevich · 2020-03-06T13:40:15Z

tests/ModelTest.php

+    public function testGetDirty(): void
+    {
+        $ids = [
+            new ObjectId(),


Add value for ObjectId please

Smolevich · 2020-03-06T13:47:02Z

tests/ModelTest.php

+        $item->nullable = null;
+        $item->new_val = 'new';
+        $item->number = '4';
+        $item->ids = [


Looks like strange, i suggest to replace on

$item->ids = $ids;

or

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

divine · 2023-02-19T20:30:49Z

Closing in favor of #2515 after 3 years...

Thank you @halaei !

use mongodb driver to check for dirtiness

9e7866a

divine reviewed Mar 6, 2020

View reviewed changes

Smolevich reviewed Mar 6, 2020

View reviewed changes

tests/ModelTest.php

public function testGetDirty(): void

{

$ids = [

new ObjectId(),

Copy link

Contributor

Smolevich Mar 6, 2020

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

Smolevich reviewed Mar 6, 2020

View reviewed changes

Smolevich added the Needs work label Mar 10, 2020

Smolevich assigned halaei Mar 10, 2020

divine mentioned this pull request Feb 19, 2023

feat!: use mongodb function to check for dirtiness #2515

Open

divine closed this Feb 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

halaei commented Mar 6, 2020

jenssegers commented Mar 6, 2020

codecov-io commented Mar 6, 2020 •

edited

divine left a comment

divine Mar 6, 2020

halaei Mar 6, 2020

divine Mar 6, 2020

halaei Mar 6, 2020

divine Mar 6, 2020

halaei Mar 6, 2020

halaei commented Mar 6, 2020

halaei commented Mar 6, 2020

Smolevich Mar 6, 2020

Smolevich Mar 6, 2020 •

edited

divine commented Feb 19, 2023

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

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

Conversation

halaei commented Mar 6, 2020

jenssegers commented Mar 6, 2020

Codecov Report

codecov-io commented Mar 6, 2020 • edited

Codecov Report

divine left a comment

Choose a reason for hiding this comment

divine Mar 6, 2020

Choose a reason for hiding this comment

halaei Mar 6, 2020

Choose a reason for hiding this comment

divine Mar 6, 2020

Choose a reason for hiding this comment

halaei Mar 6, 2020

Choose a reason for hiding this comment

divine Mar 6, 2020

Choose a reason for hiding this comment

halaei Mar 6, 2020

Choose a reason for hiding this comment

halaei commented Mar 6, 2020

halaei commented Mar 6, 2020

Smolevich Mar 6, 2020

Choose a reason for hiding this comment

Smolevich Mar 6, 2020 • edited

Choose a reason for hiding this comment

divine commented Feb 19, 2023

codecov-io commented Mar 6, 2020 •

edited

Smolevich Mar 6, 2020 •

edited