add multiple types of parameter resolution #419

sowork · 2019-07-23T09:56:05Z

$args is parsed incorrectly when the parameter passed in is an object or listObject

{
    "query": "{users{id, name blogs(filter:{id:1, desc:\"hello\", ReviewInput:{id: 4}}, field5:[{id:2,desc:\"world\"},{id:3,desc:\"world\", ReviewInput:{id: 4}}]){id text comments{id desc} }}}"
}

$args:   result:
Array
(
    [filter] => Array
        (
            [id] => 1
            [desc] => hello
            [ReviewInput] => Array
                (
                    [id] => 4
                )

        )

    [field5] => Array
        (
            [0] => Array
                (
                    [id] => 2
                    [desc] => world
                )

            [1] => Array
                (
                    [id] => 3
                    [desc] => world
                    [ReviewInput] => Array
                        (
                            [id] => 4
                        )

                )

        )

)

Apply fixes from StyleCI

sowork · 2019-07-23T12:40:49Z

Moved into PR description for clarity

sowork · 2019-07-23T12:46:28Z

looks like something went wrong

Ignored error pattern /Access to an undefined property GraphQL\\Language\\AST\\ValueNode::\$value/ was not matched in reported errors.

what do I need to do?

mfn · 2019-07-23T13:11:11Z

Please:

don't format / change existing / unrelated code
add tests!

Thank you!

sowork · 2019-07-23T13:55:40Z

don't format / change existing / unrelated code

I don't understand what you mean, can you say something specific?

crissi · 2019-07-23T20:26:09Z

src/Support/ResolveInfoFieldsAndArguments.php

        if ($value instanceof VariableNode) {
            $variableName = $value->name->value;

            return $this->info->variableValues[$variableName] ?? null;
        }
+        // Scalar Types
+        if ($value instanceof IntValueNode || $value instanceof FloatValueNode || $value instanceof StringValueNode || $value instanceof BooleanValueNode || $value instanceof EnumValueNode) {


I think this could be shorten to:

if ($value instanceof ValueNode)

Valuenode contains the following types

/**
export type ValueNode = VariableNode
| NullValueNode
| IntValueNode
| FloatValueNode
| StringValueNode
| BooleanValueNode
| EnumValueNode
| ListValueNode
| ObjectValueNode
*/

The parameter is ListValueNode or ObjectValueNode is not applicable.

@sowork @crissi should we change the order of conditions?

check for ListValueNode and ObjectVakueNode

Check for ValueNode

This way we would not be concerned anymore about all the details as they effectively work the same.

Another approach for 2.: we effectively treat the "fallthrough" in this method as always being ValueNode and return $argumentNode->value as it currently is?

Would that make sense?

Theoretically feasible, but there is also a nullValueNode

not sure

but checking for the ListValueNode and ObjectVakueNode and nullValueNode first

and then fallthough to just get the value might work. So custom scalar types is also considered, not sure the current implementation take this in account.

but there is also a nullValueNode

Right! Forgot to list it, but that would apply for it too.

So custom scalar types

Oh, interesting. Didn't think about it and currently don't know what kind of node it exactly is.

We need a test for this too. 😄

custom scalar types

for custom scalar types will still be converted to a ValueNode

mfn · 2019-07-23T20:40:49Z

I don't understand what you mean, can you say something specific?

Please don't just remove the empty lines:

Also this change seems unrelated to any functionality in this PR:

But most importantly would be tests first.

sowork · 2019-07-24T02:30:47Z

I don't understand what you mean, can you say something specific?

Please don't just remove the empty lines:

Also this change seems unrelated to any functionality in this PR:

But most importantly would be tests first.

This may be caused by my editor, I will fix it later and append a test

thank

mfn

Thanks for for, now just waiting for the test(s) :-)

Apply fixes from StyleCI

sowork · 2019-07-31T07:19:18Z

I added a unit test and it should be fine now.

mfn

Many thanks for the PR, I did my first in-depth review today.

I'm in favour of this change and looking forward to include it 👍 but have some feedback regarding style and clarity as well as the tests 🔍

My issue with the test:

it's slapped on top of an existing but absolutely unrelated test
all types supported in \Rebing\GraphQL\Support\ResolveInfoFieldsAndArguments::getValue should be tested for (currently it misses at least the null type and the variable node; the latter was already there but I think it's the right time to add this)
there are no real assertions on the new code: rather it implicitly observes the output of the GraphQL result. That's great and useful for many applications, but I think in this particular one it should be more explicit

My suggestion is:

it should be a standalone test, i.e. create a new directory/namespace in tests/Database/SelectFields
the (new) FilterInput should cover all types supported in getValue
the assertions should be done when the 'query' receives the arguments, i.e. with the types the full $args could be compared to a expected array using \PHPUnit\Framework\Assert::assertSame => see \Rebing\GraphQL\Tests\Database\SelectFields\ValidateFieldTests\PostType::fields for an example. Having to add fixtures / results for a test just bloats for not obvious reasons. A assertion directly where we receive the data ASAP we transformed it seems most appropriate for me.
- As such I would suggest to remove all the newly added models, types, etc. and only concentrate on the essential as outlined above

What do you think?

mfn · 2019-07-31T20:17:01Z

src/Support/ResolveInfoFieldsAndArguments.php

+            return $this->getInputListObjectValue($value);
+        }
+
+        throw new \Exception('Failed to resolve unknown ValueNode type');


If you don't add a specific exception, please use at least RuntimeException (and import it)

How can this be triggered with the current code?

Would like to see a test for this.

mfn · 2019-07-31T20:20:10Z

src/Support/ResolveInfoFieldsAndArguments.php

@@ -76,6 +85,7 @@ public function __construct(ResolveInfo $info)
     * @param  int  $depth  How many levels to include in output
     * @return array
     * @see \GraphQL\Type\Definition\ResolveInfo::getFieldSelection
+     * @throws \Exception


Please remove all the @throws you added in this PR

They are deliberately not added because it's a PITA to maintain them. Code changes, exception changes and before you notice it, it's out of date.

mfn · 2019-07-31T20:21:46Z

src/Support/ResolveInfoFieldsAndArguments.php

+        if ($value instanceof IntValueNode || $value instanceof FloatValueNode || $value instanceof StringValueNode || $value instanceof BooleanValueNode || $value instanceof EnumValueNode) {
+            return $value->value;
+        }
+        // null Type


I think the code speaks for itself, remove the comment (but leave a blank line)

mfn · 2019-07-31T20:21:52Z

src/Support/ResolveInfoFieldsAndArguments.php

+        if ($value instanceof NullValueNode) {
+            return;
+        }
+        // object Type


I think the code speaks for itself, remove the comment (but leave a blank line)

mfn · 2019-07-31T20:21:58Z

src/Support/ResolveInfoFieldsAndArguments.php

+        if ($value instanceof ObjectValueNode) {
+            return $this->getInputObjectValue($value);
+        }
+        // list object Type


I think the code speaks for itself, remove the comment (but leave a blank line)

mfn · 2019-07-31T20:25:48Z

src/Support/ResolveInfoFieldsAndArguments.php

+     * @return array
+     * @throws \Exception
+     */
+    private function getInputListObjectValue(ListValueNode $listValueNode)


add return type decl :array

mfn · 2019-07-31T20:37:56Z

src/Support/ResolveInfoFieldsAndArguments.php

+        }
+        // null Type
+        if ($value instanceof NullValueNode) {
+            return;


Shouldn't this be more explicit and actually return null?

Originally I returned null, but styleci changed to this way by default.

How strange, thanks!

mfn · 2019-07-31T20:41:35Z

src/Support/ResolveInfoFieldsAndArguments.php

+    {
+        $value = [];
+        foreach ($objectValueNode->fields->getIterator() as $item) {
+            if ($item instanceof ObjectFieldNode) {


Not sure about this.

The code makes it look like $item could be something else. But reading \GraphQL\Language\AST\ObjectValueNode::$fields it's clear this is not intended.

How can we improve this? Should we remove the check? Or add an } else { and throw an exception?

Because as it is right now, if that case ever would happen, we silently would carry on.

Currently, item only handles objects of type ObjectFieldNode

If the current item object contains other scalar types or listNode and so on, the resolution continues recursively

mfn · 2019-07-31T20:52:22Z

phpstan.neon.dist

@@ -42,8 +42,6 @@ parameters:
        - '/Cannot call method set\(\) on Illuminate\\Config\\Repository\|null/'
        - '/Parameter #4 \$currentPage of class Illuminate\\Pagination\\LengthAwarePaginator constructor expects int\|null, float\|int given/'
        - '/Parameter #1 \$offset of method Illuminate\\Support\\Collection::slice\(\) expects int, float\|int given/'
-        # \Rebing\GraphQL\Support\ResolveInfoFieldsAndArguments::getValue
-        - '/Access to an undefined property GraphQL\\Language\\AST\\ValueNode::\$value/'


This pretty cool and a very clear indicator that the current code was broken :/ Great 👍

mfn · 2019-07-31T20:54:02Z

src/Support/ResolveInfoFieldsAndArguments.php

        if ($value instanceof VariableNode) {
            $variableName = $value->name->value;

            return $this->info->variableValues[$variableName] ?? null;
        }
+        // Scalar Types
+        if ($value instanceof IntValueNode || $value instanceof FloatValueNode || $value instanceof StringValueNode || $value instanceof BooleanValueNode || $value instanceof EnumValueNode) {


Please break the lines: there are so many ORs, they really need to be better readable.

For simplicity, I suggest each one gets its own line:

if ($value instanceof IntValueNode || $value instanceof FloatValueNode || …

Apply fixes from StyleCI

mfn

Thanks, looking very good 👍

mfn · 2019-08-16T21:55:51Z

src/Support/ResolveInfoFieldsAndArguments.php

+        }
+        // null Type
+        if ($value instanceof NullValueNode) {
+            return;


How strange, thanks!

mfn · 2019-08-16T21:57:44Z

src/Support/ResolveInfoFieldsAndArguments.php

+     * @param ValueNode $value
+     * @return mixed
+     * @throws \Exception
+     */


mfn · 2019-08-16T21:57:58Z

src/Support/ResolveInfoFieldsAndArguments.php

+     * @param ObjectValueNode $objectValueNode
+     * @return array
+     * @throws \Exception
+     */


mfn · 2019-08-16T21:58:30Z

src/Support/ResolveInfoFieldsAndArguments.php

+     * @param ListValueNode $listValueNode
+     * @return array
+     * @throws \Exception
+     */


mfn · 2019-08-16T21:59:12Z

tests/Database/SelectFields/ValidateDiffNodeTests/ValidateDiffNodeTests.php

+    use SqlAssertionTrait;
+
+    /**
+     * Fixed with https://github.com/rebing/graphql-laravel/pull/419.


I removed this reference; it would only have made sense for code which existing prior to this PR.

xingshenqiang@uniondrug.cn and others added 4 commits July 23, 2019 17:15

add multiple parameter type parsing

377b94a

Apply fixes from StyleCI

67ab0a5

Merge pull request #4 from sowork/analysis-qBpyw9

5111514

Apply fixes from StyleCI

Merge branch 'master' into master

c82fb19

phpstan fix error

3ad9592

crissi reviewed Jul 23, 2019

View reviewed changes

restore original format code

b8ebf74

mfn requested changes Jul 24, 2019

View reviewed changes

mfn added the waiting:feedback label Jul 27, 2019

xingshenqiang@uniondrug.cn and others added 4 commits July 31, 2019 14:06

Add unit tests that take arguments as objects or lists

9047073

Apply fixes from StyleCI

07835f3

Merge pull request #5 from sowork/analysis-8KbQl9

265dc9c

Apply fixes from StyleCI

Merge branch 'master' into master

2bec80e

mfn removed the waiting:feedback label Jul 31, 2019

mfn requested changes Jul 31, 2019

View reviewed changes

crissi mentioned this pull request Aug 9, 2019

Nested filter #440

Closed

xingshenqiang@uniondrug.cn and others added 7 commits August 12, 2019 20:28

Merge remote-tracking branch 'pb/master'

0fa10db

update tests

329b4af

Apply fixes from StyleCI

50cfe57

Merge pull request #6 from sowork/analysis-qJogYQ

acaf93f

Apply fixes from StyleCI

update import assert

6b33458

Apply fixes from StyleCI

b5f4964

Merge pull request #7 from sowork/analysis-XaBgZx

5994077

Apply fixes from StyleCI

mfn added 5 commits August 16, 2019 23:51

Import class

7631c5d

Remove docblocks which don't add anything

a5b4458

Remove reference as it was not prior code

6b2626e

Merge branch 'master' into sowork-master

272f2b7

Update changelog

edc0bd2

mfn approved these changes Aug 16, 2019

View reviewed changes

mfn merged commit edc0bd2 into rebing:master Aug 16, 2019

add multiple types of parameter resolution #419

add multiple types of parameter resolution #419

Conversation

sowork commented Jul 23, 2019 • edited by mfn Loading

sowork commented Jul 23, 2019 • edited by mfn Loading

sowork commented Jul 23, 2019

mfn commented Jul 23, 2019

sowork commented Jul 23, 2019

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

mfn commented Jul 23, 2019

sowork commented Jul 24, 2019

mfn left a comment

Choose a reason for hiding this comment

sowork commented Jul 31, 2019

mfn left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfn left a 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

sowork commented Jul 23, 2019 •

edited by mfn

Loading

sowork commented Jul 23, 2019 •

edited by mfn

Loading