Skip to content

Fixing n+1 relations problem #27

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

Merged
merged 40 commits into from
Sep 11, 2018
Merged

Fixing n+1 relations problem #27

Changes from 1 commit
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
60cdd3f
ignore PhpStorm IDE files
lorado Aug 5, 2018
9ec1416
improve guessing relations
lorado Aug 5, 2018
4c11459
fix errors
lorado Aug 6, 2018
718de93
define Paginable interface for identifying those transformers
lorado Aug 6, 2018
870de54
ListTransformer implements now Paginable interface
lorado Aug 6, 2018
bffe998
rework, how ListTransformer is identified
lorado Aug 6, 2018
f694792
extend DB structure for tests. Tests are still the same
lorado Aug 8, 2018
1051554
sync tables and model definitions
lorado Aug 8, 2018
53db424
rework how relations are identified (much cleaner and correct way)
lorado Aug 8, 2018
1453da4
add missing author parameter to PostDefinition
lorado Aug 8, 2018
171c290
define unit tests for eager loading relations
lorado Aug 8, 2018
035b470
copy-paste travis config
lorado Aug 8, 2018
13cdf22
delete unused imports
lorado Aug 8, 2018
a9beecc
refactor code, so phpmd don't blame on me (CouplingBetweenObjects)
lorado Aug 8, 2018
02cb7e9
calling function recursive make sense only, if relations are found
lorado Aug 8, 2018
88ceb91
save changes after php-cs-fixer
lorado Aug 8, 2018
9088301
remove unnecessary $app variable
lorado Aug 23, 2018
c9fe50e
define fiels selection depth as constant
lorado Aug 24, 2018
68901b2
move relations guessing to static in GraphQL class (will be reused)
lorado Aug 24, 2018
7703678
guess relations also for custom fields
lorado Aug 24, 2018
8bd4965
extend custom query, add test for eager loading in custom queries
lorado Aug 24, 2018
daf6927
add doc to test-query
lorado Aug 24, 2018
06097b4
remove array_filter(), as I see no reason in using it...
lorado Aug 24, 2018
c3bbc1a
extend readme
lorado Aug 24, 2018
55c3c00
fix code styling
lorado Aug 24, 2018
4af8f80
fetch fields ONLY, if returned type is an object
lorado Sep 3, 2018
83b94ea
add new test for fetching simple string from graphql
lorado Sep 3, 2018
fe7fd5e
add authorize check for fields, default is true. update readme
lorado Sep 3, 2018
31cb88b
add tests for authorize checking (test - unauthorized error gets thrown)
lorado Sep 3, 2018
e2a0261
fix definition of method
lorado Sep 3, 2018
dcc86aa
Merge branch 'master' into bug/26
cmizzi Sep 10, 2018
a92e22c
fix(transformers): fix bad merge
cmizzi Sep 10, 2018
12024dd
Merge branch 'master' into bug/26
lorado Sep 10, 2018
7d085f7
fix returned type in docs
lorado Sep 10, 2018
60928bb
adopt EloquentTransformer to fix n+1 problem (at least on read data)
lorado Sep 10, 2018
e227585
fix for linters
lorado Sep 10, 2018
4b52290
add readme for authorization
lorado Sep 10, 2018
5845e35
Merge remote-tracking branch 'origin/bug/26' into bug/26
lorado Sep 10, 2018
8a8eb19
remove unused reflect
lorado Sep 11, 2018
63f6abd
update readme
lorado Sep 11, 2018
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
Prev Previous commit
Next Next commit
fix errors
  • Loading branch information
@lorado
lorado committed Aug 6, 2018
commit 4c114592ece5813f04c4f98059375d146538cd80
10 changes: 7 additions & 3 deletions src/Support/Transformer/Transformer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<?php

namespace StudioNet\GraphQL\Support\Transformer;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsToMany;
use Illuminate\Foundation\Application;
use StudioNet\GraphQL\Support\Definition\Definition;
use GraphQL\Type\Definition\ResolveInfo;
Expand Down Expand Up @@ -126,9 +126,13 @@ public function guessWithRelations(Model $model, array $fields, string $parentRe
foreach ($fields as $key => $field) {
if (is_array($field) && method_exists($model, $key)) {
// get relation name and store it
/** @var BelongsToMany $relation */
$relation = call_user_func([$model, $key]);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened is the $key is delete, update, or whatever that does not relied to relationships ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you do some concrete example?

Method gets called, only if field is an array. Currently even if delete is the key, nothing will happen. $model is just an empty model-object instance, so delete() or update() make no changes. After call, $relation will be boolean or null or whatever these functions return, but definitely not an instance of Illuminate\Database\Eloquent\Relations\Relation, so that code will continue executing and let this field as not a relation.

May be we should add some filter, so that key should not be equal delete or update or what else method of Model are 'critical'? I mean even if you want to have delete to be relation, it is not possible to implement it with Laravel... Also you can't damage the DB somehow with those fields, as model-objects, which gets created in this relation guessing, are empty, and have no connection to any rows in DB. Or am I wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're true. The $model isn't loaded from database (you've used $this->app->make). My bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an idea:

If you wish, I could get all methods of Eloquent-Model class, cache them, and when relation is guessed, those keys that equal the method-names will be just ignored. But I don't really think it is necessary...

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's not necessary. Don't worry, I'm just tired :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok :D

then I am gonna wait till new release with my PR ^_^

$relationNameToStore = $parentRelation ? "{$parentRelation}.{$relation->getRelationName()}" : $relation->getRelationName();
if (method_exists($relation, 'getRelationName')) {
$relationName = $relation->getRelationName();
} else {
$relationName = explode('.', $relation->getQualifiedForeignKeyName())[0];
}
$relationNameToStore = $parentRelation ? "{$parentRelation}.{$relationName}" : $relationName;
$relations[] = $relationNameToStore;
// also guess relations for found relation
$relations = array_merge($relations, $this->guessWithRelations($relation->getModel(), $field, $relationNameToStore));
Expand Down