Related relations #257

jschlies · 2017-06-25T17:09:06Z

This pull request is my initial attempt to audit the relationships of a particular model. Please comment but I am unwilling to push this intil I deal with the 3 Todo's.

Be warned: This can have negative impact on DB performance. Be very cautious to not use this feature on too many models (and those models relationships). This PR offers controls on both.

Todo:
Create table related_relations and use this rather that audits:related_relation_json
Integrate this into $model->audits()
Better unit tests

Todo (far future)
Make this Event/Queue friendly to try to deal w/ the DB load this this can generate.

To use:

Run the migration adding related_relation_json to audits table

All models for which you want to track related_relations need:

A public property of $model->auditIncludeRelated set to true
These public methods: $model->getBelongsToArr(), $model->HasManyArr,$model->GetBelongsToManyArr(), $model->getHasOneArr(). These should return an array of strings that indicate the names of the Eloquent properties (relations) you wish to track.
If you are auditing the related models of $a (say $b and $c), you minimally need to audit $b and $c.

Nitpick-CI · 2017-06-25T17:09:16Z

src/Auditable.php

+        $related_relations_arr        = [];
+        $broad_relationship_types_arr = ['BelongsTo', 'HasMany', 'BelongsToMany', 'HasOne'];
+
+        if ( ! $this->auditIncludeRelated)


Expected 0 spaces after opening bracket; 1 found

Expected 1 space after closing parenthesis; found 9

Nitpick-CI · 2017-06-25T17:09:16Z

src/Auditable.php

+        {
+            return $related_relations_arr;
+        }
+        foreach ($broad_relationship_types_arr as $broad_relationship)


Expected 1 space after closing parenthesis; found 9

Nitpick-CI · 2017-06-25T17:09:16Z

src/Auditable.php

+            return $related_relations_arr;
+        }
+        foreach ($broad_relationship_types_arr as $broad_relationship)
+        {


Expected 1 newline after opening brace; 2 found

Nitpick-CI · 2017-06-25T17:09:17Z

src/Auditable.php

+        {
+
+            $broad_relationship_method = 'get'.ucfirst($broad_relationship) . 'Arr';
+            if ( ! method_exists($this, $broad_relationship_method))


Expected 0 spaces after opening bracket; 1 found

Expected 1 space after closing parenthesis; found 13

Nitpick-CI · 2017-06-25T17:09:17Z

src/Auditable.php

+            }
+            $related_relations_arr[$broad_relationship] = [];
+            $relationship_arr                           = $this->$broad_relationship_method();
+            foreach ($relationship_arr as $relationship)


Expected 1 space after closing parenthesis; found 13

Nitpick-CI · 2017-06-25T17:09:17Z

src/Auditable.php

+            $relationship_arr                           = $this->$broad_relationship_method();
+            foreach ($relationship_arr as $relationship)
+            {
+                if ( ! method_exists($this, $relationship))


Expected 0 spaces after opening bracket; 1 found

Expected 1 space after closing parenthesis; found 17

Nitpick-CI · 2017-06-25T17:09:17Z

src/Auditable.php

+                    continue;
+                }
+                $RelationshipObjArr = $this->$relationship;
+                if ( ! is_iterable($RelationshipObjArr))


Expected 0 spaces after opening bracket; 1 found

Expected 1 space after closing parenthesis; found 17

Nitpick-CI · 2017-06-25T17:09:17Z

src/Auditable.php

+                {
+                    $RelationshipObjArr = [$RelationshipObjArr];
+                }
+                foreach ($RelationshipObjArr as $RelationshipObj)


Expected 1 space after closing parenthesis; found 17

Nitpick-CI · 2017-06-25T17:09:18Z

src/Auditable.php

+                        'source_relation'    => get_class($this),
+                        'source_relation_id' => $this->{$this->primaryKey},
+                    ];
+                    if($RelationshipObj == null)


Expected 1 space after IF keyword; 0 found

Expected 1 space after closing parenthesis; found 21

Nitpick-CI · 2017-06-25T17:09:18Z

src/Auditable.php

+                         * this can happen when a foreign key constraint referances the self-same table.
+                         */
+                        $x=1;
+                    }


Expected 1 space after closing brace; newline found

Nitpick-CI · 2017-06-25T17:09:18Z

src/Auditable.php

+                         */
+                        $x=1;
+                    }
+                    else


Expected 1 space after ELSE keyword; newline found

Nitpick-CI · 2017-06-25T17:13:59Z

src/Auditable.php

+        $related_relations_arr        = [];
+        $broad_relationship_types_arr = ['BelongsTo', 'HasMany', 'BelongsToMany', 'HasOne'];
+
+        if (! property_exists($this,'auditIncludeRelated') ||  ! $this->auditIncludeRelated)


No space found after comma in function call

Expected 1 space after closing parenthesis; found 9

Nitpick-CI · 2017-06-25T17:14:00Z

src/Auditable.php

+                        /**
+                         * this can happen when a foreign key constraint referances the self-same table.
+                         */
+                    }


Expected 1 space after closing brace; newline found

Nitpick-CI · 2017-06-25T17:14:00Z

src/Auditable.php

+                         * this can happen when a foreign key constraint referances the self-same table.
+                         */
+                    }
+                    else


Expected 1 space after ELSE keyword; newline found

quetzyg · 2017-06-25T18:40:31Z

Hi @jschlies,

Thanks for trying to come up with a solution to the many-to-many relation auditing.

On your TODO, you state that there should be a related_relations table, instead of a new audits column (related_relation_json in this case), why so?

Also, the logic you're proposing is using relation types (BelongsTo, and so on) instead of events, why?

And how do you deal with a sync() call that would remove/add lots of relations? Would you create an audit record for each, or do you create a single one with all the changes?

jschlies · 2017-06-25T19:15:41Z

@quetzyg

On your TODO, you state that there should be a related_relations table, instead of a new audits column (related_relation_json in this case), why so?

It would be extremely hard to extract the data needed to run $somethingRelatedToodel->audits() for not only the object in question but it's relations. As I stated above, this fix runs the risk of overloading the DB w/ insert requests. Perhaps there is a way to bundle these inserts or get them offline somehow.

Also, the logic you're proposing is using relation types (BelongsTo, and so on) instead of events, why?

This is our current implementation and it serves us well. I will explore using events as you indicate. Can you point me in a direction on this?

And how do you deal with a sync() call that would remove/add lots of relations? Would you create an audit record for each, or do you create a single one with all the changes?

I don't but if I did, (and I will research doing so), each relationship would get it's own related_relation record. As I stated above, this fix runs the risk of overloading the DB w/ insert requests. Perhaps there is a way to bundle these inserts or get them offline somehow.

-=-=-=-=-=-=-
Thanks for the input and will explore the issues you point out. Keep the comments coming

jschlies · 2017-06-25T19:21:38Z

AuditableTrait.php.txt

See attached for our current implementation of $this->getAuditArr(). Ignore the sorting stuff.

quetzyg · 2017-06-25T19:23:38Z

This is our current implementation and it serves us well. I will explore using events as you indicate. Can you point me in a direction on this?

In regards to the usage of events, I commented in the open issue #58. The idea would be to fire an event from a repository and that would kickstart the audit process.

quetzyg · 2017-06-25T19:29:40Z

You totally lost me with the AuditableTrait.php.txt

jschlies · 2017-06-25T19:32:16Z

I think that a great idea (events and potentially queuing) and will implement but I think that a separate topic to be taken up later.

jschlies · 2017-06-25T19:33:58Z

AuditableTrait.php.txt contains our implementation of getAuditArr().
Can you elaborate?

quetzyg · 2017-06-25T19:37:43Z

The code, is confusing.

jschlies · 2017-06-25T19:53:41Z

I'll spend some time commenting.
Keep in mind:

The table I propose, related_relations, is called audit_relations in our implementation.
$generations keeps our use of recursion under control.
If you think of a beter nomenclature than 'source' and 'related', please let me know.
In our implementation, we only really care about belongsTo.
ignore the sorting stuff

quetzyg · 2017-07-02T20:06:58Z

Closing it, since we're currently working on version 4.1.

jschlies added 3 commits June 24, 2017 22:04

related_relations

42659d5

related_relations

ce33572

related_relations cleanup

99c39ca

Nitpick-CI reviewed Jun 25, 2017

View reviewed changes

src/Auditable.php Outdated

*/

$x=1;

}

else

Copy link

Nitpick-CI Jun 25, 2017

Choose a reason for hiding this comment

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

Expected 1 space after ELSE keyword; newline found

related_relations property_exists($this,'auditIncludeRelated')

24106bf

Nitpick-CI reviewed Jun 25, 2017

View reviewed changes

related_relations style issues

9bdae2e

related_relations unittest issues

1371ab7

quetzyg closed this Jul 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Related relations #257

Related relations #257

jschlies commented Jun 25, 2017 •

edited

Loading

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

Nitpick-CI Jun 25, 2017

quetzyg commented Jun 25, 2017

jschlies commented Jun 25, 2017

jschlies commented Jun 25, 2017 •

edited

Loading

quetzyg commented Jun 25, 2017

quetzyg commented Jun 25, 2017

jschlies commented Jun 25, 2017

jschlies commented Jun 25, 2017

quetzyg commented Jun 25, 2017

jschlies commented Jun 25, 2017

quetzyg commented Jul 2, 2017

Related relations #257

Related relations #257

Conversation

jschlies commented Jun 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quetzyg commented Jun 25, 2017

jschlies commented Jun 25, 2017

jschlies commented Jun 25, 2017 • edited Loading

quetzyg commented Jun 25, 2017

quetzyg commented Jun 25, 2017

jschlies commented Jun 25, 2017

jschlies commented Jun 25, 2017

quetzyg commented Jun 25, 2017

jschlies commented Jun 25, 2017

quetzyg commented Jul 2, 2017

jschlies commented Jun 25, 2017 •

edited

Loading

jschlies commented Jun 25, 2017 •

edited

Loading