Fixing n+1 relations problem #27

lorado · 2018-08-05T23:57:35Z

Fix for #26

As you can see, I increased fields depth, if it is a ListTransformer, because one extra depth-level goes for items {...}. Moreover I pick fields from items, and pass them then to guessing relations.

guessWithRelations() is now recursive and takes 3 params

Eloquent model object
Fields for the given model, so we can check for relations
Parents relation name, as it has to be prepended to currently found relation

What do you think? As Transformers are currently used just by the lib, it has no breaking changes.

lorado · 2018-08-06T00:43:10Z

Huh... I need to test it on my test data for all kinds of relations... PHPUnits passes now, but I still want to test it on my own

lorado · 2018-08-06T00:53:43Z

Most failed builds in Travis for my last commit (fix errors) are actually not errors at all... composer just can't handle dependencies....

lorado · 2018-08-06T07:12:16Z

I just figured out, how I can verify my code with unit test. I'll try to write tests this week and I want to extend test data a little (to test all different relations in laravel)

cmizzi · 2018-08-06T07:16:18Z

Hi ! Thanks for your PR ! I really understand your issue, but the implement seems wrong to me. I think the best way to fix it is to create a Paginable interface to transformer and checks for presence to resolve the depth-level. The code will be more cleaner.

Can you do that ?

cmizzi

#27 (comment)

lorado · 2018-08-06T07:55:38Z

Oh yeah, your approach sounds much better =) Yes, I will try to do it this week.

- added some new entities - added all possible relations - added definitions for new entities - updated usage of extended structure in tests - move registration of definitions to function

lorado · 2018-08-08T16:27:29Z

Heh, as I expected, I messed up =(

Currently working on better solution (found already one). I also write UnitTests for all possible relations.

- each type of relation is tested separately - at the end one test that uses deep relation (3 levels)

- also fixing problem "$relationNameToStore might not be defined" - overlooked...

lorado · 2018-08-08T22:04:03Z

Sooooo I made some changes:

as you wished, I create Paginable interface, and ListTransformer implements it. So in Transformer::getResolverCallable() I check if Paginable was implemented. And so on. BUT! I really don't know, how I can force using {pagination: {...}, items: {...}} structure via Interface. Any idea? Or just ignore at this stage?
I also reworked, how relations are identified - it works now perfectly.
Most changes are in tests folder. I extended test DB and entities definition, so I could define all possible relations
Refactored tests a little
Created new tests for eager loading
AND I copy-pasted your travis config from another pull-request, so travis can give me this - ✅

What do you think? Is it good now? If you have any improvements - just tell me.

cmizzi · 2018-08-13T06:31:43Z

src/Support/Transformer/Transformer.php

+
+			// check, if Paginable interface was implemented by given Transformer
+			// Paginable interface has an extra wrapper for data-fields
+			$isPaginable = $this instanceof Paginable;


Paginable doesn't seem to be imported with use ?

As I know there is no need to import classes with use, if they are in the same Namespace. PhpStorm don't show any errors, and the code is also executable.

If I add use StudioNet\GraphQL\Support\Transformer\Paginable; on top, PhpStorm adds notice: "Import is not necessary"

Yeah, I didn't notice that ;)

cmizzi · 2018-08-13T06:33:54Z

src/Support/Transformer/Transformer.php

-				array_push($relations, $key);
+			if (is_array($field) && method_exists($model, $key)) {
+				// verify, that given method returns relation
+				$relation = call_user_func([$model, $key]);


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

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?

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

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

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

ok :D

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

- getResolver() of custom field is now called not directly from webonyx, but from this library with some additional infos (similar to transformers) - getResolver() gets now just one argument - an array with arguments from original webonyx caller, and additional `fields` and `with` - with-relations will be guessed only, if field defines a class name of source in new `getSource()` method. Otherwise with will be empty (as before)

lorado · 2018-08-24T01:46:49Z

Hi again =)

I am using your library more and more in my new project. As I wanted to create a custom query, I noticed directly, that in custom queries there is also no possibility to do eager loading depending on query. So I moved some code around and extended Field class. Now it is used similar to transformers - getResolver() becomes now one argument as array with different elements within. Amongst other things there is now also with array, that contains all relations, that can be eager loaded. To make this relation identifying work, I had to add new method to Field class - getSource() (similar to transformers).

Anyway, with all my changes under the hood, there is no breaking changes. Just an extra feature, that can be used.

Let me know, if there is something wrong in code. I would be happy, if PR will be merged ^_^

this test was implemented, as I found a bug in my last implementation. So I decided to create a test for this case

cmizzi · 2018-09-10T08:16:57Z

tests/GraphQL/Query/Unauthorized.php

@@ -6,7 +6,7 @@
 use StudioNet\GraphQL\Support\Definition\Query;

 class Unauthorized extends Query {
-	protected function authorize() {
+	protected function authorize(array $args) {


You should use commentary here to pass unit tests, such as

/** * @params array $args * @returns bool * * @SuppressWarnings(PHPMD.UnusedFormalParameter) */

Ahhhh thanks! gonna fix it

cmizzi · 2018-09-10T08:33:23Z

I've merged #24 , so a conflict was created. I tried to solve it, if you can re-test your code in order to merge this PR, thanks 😉

cmizzi · 2018-09-10T09:14:24Z

Also, can you describe changes into the README.md file like authorization (#28) ?

# Conflicts: # .travis.yml # src/Support/Transformer/Eloquent/ListTransformer.php # src/Support/Transformer/Transformer.php

lorado · 2018-09-10T22:47:51Z

Hi. It is my first collab in github in open-source... sorry, I didn't notice, you already merged new master in this branch, so I merged it again... 😅

I tried to understand, how pipes works and what happens there... I understood it for about 80%... anyway, it seems like your StoreTransformer is not completely refactored. After I figured out, where I should add ->with([...]) to actually fix this n+1 problem, I found out, that it don't work well with mutations.

There are some mutation test, that fail, when ->with() is used. Here is the explanation of flow for GraphQLMutationTest::testNestedMutation():

Factory creates some user objects, that have no posts
GraphQL mutation updates the user with ID 1, and creates one post for him
Result is then sent back to GraphQL-lib, that then tries to fetch fields from mutation. And here is the trap... When we don't use ->with(...) on query creation, then GraphQL fetch posts lazy, so it queries a DB and gets post, that was created right now. But when we define ->with(...) in EloquentTransformer, which looks good place for me, then DB query for relations is already done. Model knows, that there are no posts, and after inserting new posts, and still don't know, that there is a new post for this user. So GraphQL then returns an empty array for posts, what is actually wrong!

Generally it concerns all types of graphql mutations - create and update of deep relations, that get created/updated step by step. We have definitely to refactor StoreTransformer, and update model relation entities, after that gets stored to DB. Eloquent has helpful methods for this situation, like $model->relation->save($relatioObj), $model->push etc. May be we can use some of them in StoreTransformer.

For now, I simply ignore adding ->with(...), if StoreTransformer is used. It solves the compatibility problem, but don't solve initial n+1 problem.
Anyway, let us merge this into master. I would wait, till you or me refactor StoreTransformer, and then after this, I would get back to the n+1 problem on StoreTransformer.

What do you think?

cmizzi · 2018-09-11T07:43:23Z

src/Support/Transformer/Transformer.php

@@ -69,19 +70,25 @@ public function transform(Definition $definition) {
 	 * @return callable
 	 */
 	public function getResolverCallable(Definition $definition) {
-		$app = $this->app;
+		return function ($root, array $args, $context, ResolveInfo $info) use ($definition) {
+			$reflect = new \ReflectionClass($this);


$reflect does not seem to be used ?

Ouch, you are right! I removed it.

This was a part of your old code, that you removed in your branch. That was there:

$reflect = new \ReflectionClass($this); $definition->assertAcl( str_replace("transformer", "", strtolower($reflect->getShortName())), [ "fields" => $fields, "context" => $context, "args" => $args, 'info' => $info, ] );

Dunno, what Definition::accertAcl() was doing, but currently this method is not used at all. May be delete this method completely?

cmizzi · 2018-09-11T07:48:49Z

So ! I don't really understand your problematic with the StoreTransformer. Is there a code example ?

I'll merge this branch when units will pass. We'll manage your problematic in a ticket ;)

lorado · 2018-09-11T10:07:39Z

Ok... I will create an example for you, and create an issue for it later, so you can see, what am I talking about ;)

lorado added 3 commits August 6, 2018 01:26

ignore PhpStorm IDE files

60cdd3f

improve guessing relations

9ec1416

fix errors

4c11459

cmizzi suggested changes Aug 6, 2018

View reviewed changes

lorado added 4 commits August 7, 2018 01:47

define Paginable interface for identifying those transformers

718de93

ListTransformer implements now Paginable interface

870de54

rework, how ListTransformer is identified

bffe998

extend DB structure for tests. Tests are still the same

f694792

- added some new entities - added all possible relations - added definitions for new entities - updated usage of extended structure in tests - move registration of definitions to function

lorado added 9 commits August 8, 2018 23:37

sync tables and model definitions

1051554

rework how relations are identified (much cleaner and correct way)

53db424

add missing author parameter to PostDefinition

1453da4

define unit tests for eager loading relations

171c290

- each type of relation is tested separately - at the end one test that uses deep relation (3 levels)

copy-paste travis config

035b470

delete unused imports

13cdf22

refactor code, so phpmd don't blame on me (CouplingBetweenObjects)

a9beecc

calling function recursive make sense only, if relations are found

02cb7e9

- also fixing problem "$relationNameToStore might not be defined" - overlooked...

save changes after php-cs-fixer

88ceb91

cmizzi reviewed Aug 13, 2018

View reviewed changes

lorado added 4 commits August 24, 2018 01:51

remove unnecessary $app variable

9088301

define fiels selection depth as constant

c9fe50e

move relations guessing to static in GraphQL class (will be reused)

68901b2

lorado added 4 commits August 24, 2018 03:33

add doc to test-query

daf6927

remove array_filter(), as I see no reason in using it...

06097b4

extend readme

c3bbc1a

fix code styling

55c3c00

lorado added 4 commits September 4, 2018 00:22

fetch fields ONLY, if returned type is an object

4af8f80

add new test for fetching simple string from graphql

83b94ea

this test was implemented, as I found a bug in my last implementation. So I decided to create a test for this case

add authorize check for fields, default is true. update readme

fe7fd5e

add tests for authorize checking (test - unauthorized error gets thrown)

31cb88b

lorado mentioned this pull request Sep 3, 2018

Implementing kind of auth middleware #28

Closed

fix definition of method

e2a0261

cmizzi suggested changes Sep 10, 2018

View reviewed changes

Merge branch 'master' into bug/26

dcc86aa

fix(transformers): fix bad merge

a92e22c

lorado added 6 commits September 10, 2018 22:23

Merge branch 'master' into bug/26

12024dd

# Conflicts: # .travis.yml # src/Support/Transformer/Eloquent/ListTransformer.php # src/Support/Transformer/Transformer.php

fix returned type in docs

7d085f7

adopt EloquentTransformer to fix n+1 problem (at least on read data)

60928bb

fix for linters

e227585

add readme for authorization

4b52290

Merge remote-tracking branch 'origin/bug/26' into bug/26

5845e35

# Conflicts: # .travis.yml # src/Support/Transformer/Eloquent/ListTransformer.php # src/Support/Transformer/Transformer.php

cmizzi reviewed Sep 11, 2018

View reviewed changes

remove unused reflect

8a8eb19

update readme

63f6abd

cmizzi approved these changes Sep 11, 2018

View reviewed changes

cmizzi merged commit 4cc1501 into studio-net:master Sep 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing n+1 relations problem #27

Fixing n+1 relations problem #27

lorado commented Aug 5, 2018

lorado commented Aug 6, 2018

lorado commented Aug 6, 2018

lorado commented Aug 6, 2018

cmizzi commented Aug 6, 2018

cmizzi left a comment

lorado commented Aug 6, 2018

lorado commented Aug 8, 2018

lorado commented Aug 8, 2018 •

edited

Loading

cmizzi Aug 13, 2018

lorado Aug 13, 2018

cmizzi Aug 13, 2018

cmizzi Aug 13, 2018

lorado Aug 13, 2018

cmizzi Aug 13, 2018

lorado Aug 13, 2018

cmizzi Aug 14, 2018

lorado Aug 14, 2018

lorado commented Aug 24, 2018

cmizzi Sep 10, 2018

lorado Sep 10, 2018

cmizzi commented Sep 10, 2018

cmizzi commented Sep 10, 2018

lorado commented Sep 10, 2018

cmizzi Sep 11, 2018

lorado Sep 11, 2018

cmizzi commented Sep 11, 2018

lorado commented Sep 11, 2018

Fixing n+1 relations problem #27

Fixing n+1 relations problem #27

Conversation

lorado commented Aug 5, 2018

lorado commented Aug 6, 2018

lorado commented Aug 6, 2018

lorado commented Aug 6, 2018

cmizzi commented Aug 6, 2018

cmizzi left a comment

Choose a reason for hiding this comment

lorado commented Aug 6, 2018

lorado commented Aug 8, 2018

lorado commented Aug 8, 2018 • 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

lorado commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmizzi commented Sep 10, 2018

cmizzi commented Sep 10, 2018

lorado commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmizzi commented Sep 11, 2018

lorado commented Sep 11, 2018

lorado commented Aug 8, 2018 •

edited

Loading