Fix args in sub queries #327
Conversation
|
@sowork thanks for the pull request! Some tests are still failing, can you take a look? |
Apply fixes from StyleCI
Apply fixes from StyleCI
Codecov Report
@@ Coverage Diff @@
## master #327 +/- ##
===========================================
+ Coverage 71.44% 71.85% +0.4%
- Complexity 318 333 +15
===========================================
Files 21 22 +1
Lines 837 874 +37
===========================================
+ Hits 598 628 +30
- Misses 239 246 +7
Continue to review full report at Codecov.
|
|
Nice, tests are looking good! I hope to find time for the review soon! |
|
@sowork I see you are still working on the PR. Please ping me once you're done. Oh, and don't worry too much about the static analyzer errors, I can take care of them. |
|
@mfn tks, nested parameter problem solved, leaving some static analysis errors |
- there's always an 'args' (it may be empty) - it's logically wrong to provide a nested query with the top level arguments; that's the bug we're trying to fix here
…methods It's the same as with arguments for custom queries, the fallback to the top level arguments are logically wrong. This removes the ::$args field for good.
src/Support/SelectFields.php
Outdated
| if ($customQuery) { | ||
| $query = $customQuery(self::$args, $query); | ||
| $query = $customQuery($requestedFields['args'] ?? self::$args, $query); |
There was a problem hiding this comment.
The fallback to self::$args doesn't look right.
First: in what case when ['args'] be null (or not present)? Given the getFieldSelection code, I don't think it's possible.
Second: why should this default to self::$args? This is in fact the bug we're trying to fix; passing none or wrong arguments to the custom query.
IMHO the whole ?? part isn't necessary.
src/Support/SelectFields.php
Outdated
| return $fields; | ||
| } | ||
|
|
||
| private function foldSelectionSet(SelectionSetNode $selectionSet, $descend) |
There was a problem hiding this comment.
This whole logic shouldn't be intermixed within this class.
Extracting the field/argument datastructure is a separate concern.
src/Support/SelectFields.php
Outdated
| @@ -357,4 +417,9 @@ public function getRelations(): array | |||
| { | |||
| return $this->relations; | |||
| } | |||
|
|
|||
| public function getResolveInfo(): ResolveInfo | |||
There was a problem hiding this comment.
This isn't necessary: ResolveInfo is solely here consumed by the class; any query/mutation resolver it as a separate argument anyway
|
@sowork many thanks for your contribution, solid! 👍 |
This _probably_ got broken after #327 landed. There were no tests before, so it didn't got noticed.
This _probably_ got broken after #327 landed. There were no tests before, so it didn't got noticed.
This _probably_ got broken after #327 landed. There were no tests before, so it didn't got noticed.
This _probably_ got broken after #327 landed. There were no tests before, so it didn't got noticed.
This _probably_ got broken after rebing/graphql-laravel#327 landed. There were no tests before, so it didn't got noticed.
This PR fixes nested field arguments being properly passed to custom queries.
Previously the query arguments where used everywhere, no matter the nesting, for custom queries or privacy callbacks => now the field local ones are used.
Fixes #314