Skip to content
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

Fix args in sub queries #327

Merged
merged 27 commits into from
Jun 19, 2019
Merged

Fix args in sub queries #327

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
4867c97
Fix args in sub queries
Jun 15, 2019
2d89c71
Apply fixes from StyleCI
sowork Jun 15, 2019
a5f5995
Merge pull request #1 from sowork/analysis-8LaVMn
sowork Jun 15, 2019
771ff00
update NestedRelationLoadingTest.php
Jun 15, 2019
c7e7cd8
solve the problems cacused by variables
Jun 18, 2019
646920c
Apply fixes from StyleCI
sowork Jun 18, 2019
11375af
Merge pull request #2 from sowork/analysis-qy5N3b
sowork Jun 18, 2019
5750416
fix variables
Jun 18, 2019
cf1f570
Merge remote-tracking branch 'origin/master'
Jun 18, 2019
917b1e5
remote merge
Jun 18, 2019
a6076a9
Apply fixes from StyleCI
sowork Jun 18, 2019
93fea9c
Merge pull request #3 from sowork/analysis-8bWAnl
sowork Jun 18, 2019
f5047ae
x
Jun 18, 2019
ad71319
Merge branch 'master' of https://github.com/sowork/graphql-laravel
Jun 18, 2019
5aefa3e
Merge branch 'master' into master
sowork Jun 19, 2019
76a6a0a
strict mode
Jun 19, 2019
b10017b
Merge branch 'master' of https://github.com/sowork/graphql-laravel
Jun 19, 2019
441df57
Merge remote-tracking branch 'pb/master'
Jun 19, 2019
9add6ac
strict mode
Jun 19, 2019
ff7db90
Merge branch 'master' into master
mfn Jun 19, 2019
8b43a63
Move field and argument extraction logic into separate class
mfn Jun 19, 2019
bd883cf
Remove null coalesce fallback
mfn Jun 19, 2019
2171688
Pass field arguments instead of top level arguments to privacy check …
mfn Jun 19, 2019
d444433
Adapt phpstan ignoreErrors due to code changes
mfn Jun 19, 2019
9bca429
Adapt test description now that the bug is fixed
mfn Jun 19, 2019
12381b1
Update CHANGELOG.md
mfn Jun 19, 2019
204e591
Apply fixes from StyleCI
mfn Jun 19, 2019
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
85 changes: 75 additions & 10 deletions src/Support/SelectFields.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,18 @@

use Closure;
use Illuminate\Support\Arr;
use GraphQL\Language\AST\FieldNode;
use GraphQL\Error\InvariantViolation;
use GraphQL\Language\AST\ArgumentNode;
use GraphQL\Language\AST\VariableNode;
use GraphQL\Type\Definition\UnionType;
use GraphQL\Type\Definition\ResolveInfo;
use GraphQL\Type\Definition\WrappingType;
use GraphQL\Language\AST\SelectionSetNode;
use GraphQL\Language\AST\FragmentSpreadNode;
use GraphQL\Language\AST\InlineFragmentNode;
use GraphQL\Type\Definition\FieldDefinition;
use GraphQL\Language\AST\FragmentDefinitionNode;
use GraphQL\Type\Definition\Type as GraphqlType;
use Illuminate\Database\Eloquent\Relations\HasOne;
use Illuminate\Database\Eloquent\Relations\HasMany;
Expand All @@ -29,6 +36,8 @@ class SelectFields
private $relations = [];
/** @var array */
private static $privacyValidations = [];
/** @var ResolveInfo */
private $info;

const FOREIGN_KEY = 'foreignKey';

Expand All @@ -43,14 +52,65 @@ public function __construct(ResolveInfo $info, GraphqlType $parentType, array $a
$parentType = $parentType->getWrappedType(true);
}

if (! is_null($info->fieldNodes[0]->selectionSet)) {
self::$args = $args;
$this->info = $info;
self::$args = $args;
$fields = self::getSelectableFieldsAndRelations($this->getFieldSelection(5), $parentType);
$this->select = $fields[0];
$this->relations = $fields[1];
}

public function getFieldSelection($depth = 0)
{
$data = [];

if (! is_null($this->info->fieldNodes[0]->selectionSet)) {
/** @var FieldNode $fieldNode */
foreach ($this->info->fieldNodes as $fieldNode) {
if (! is_null($fieldNode->selectionSet)) {
$data = array_merge_recursive($data, $this->foldSelectionSet($fieldNode->selectionSet, $depth));
}
}
}

$fields = self::getSelectableFieldsAndRelations($info->getFieldSelection(5), $parentType);
$fields['fields'] = $data;
$fields['args'] = self::$args;

$this->select = $fields[0];
$this->relations = $fields[1];
return $fields;
}

private function foldSelectionSet(SelectionSetNode $selectionSet, $descend)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole logic shouldn't be intermixed within this class.

Extracting the field/argument datastructure is a separate concern.

{
$fields = [];

foreach ($selectionSet->selections as $selectionNode) {
if ($selectionNode instanceof FieldNode) {
$fields[$selectionNode->name->value]['fields'] = $descend > 0 && ! empty($selectionNode->selectionSet)
? $this->foldSelectionSet($selectionNode->selectionSet, $descend - 1)
: true;
$fields[$selectionNode->name->value]['args'] = [];
foreach ($selectionNode->arguments as $argument) {
if ($argument->value instanceof VariableNode) {
$value = $this->info->variableValues[$argument->value->name->value] ?? null;
} else {
$value = $argument->value->value;
}
if ($argument instanceof ArgumentNode) {
$fields[$selectionNode->name->value]['args'][$argument->name->value] = $value;
}
}
} elseif ($selectionNode instanceof FragmentSpreadNode) {
$spreadName = $selectionNode->name->value;
if (isset($this->info->fragments[$spreadName])) {
/** @var FragmentDefinitionNode $fragment */
$fragment = $this->info->fragments[$spreadName];
$fields = array_merge_recursive($this->foldSelectionSet($fragment->selectionSet, $descend), $fields);
}
} elseif ($selectionNode instanceof InlineFragmentNode) {
$fields = array_merge_recursive($this->foldSelectionSet($selectionNode->selectionSet, $descend), $fields);
}
}

return $fields;
}

/**
Expand Down Expand Up @@ -95,9 +155,9 @@ public static function getSelectableFieldsAndRelations(array $requestedFields, G
if ($topLevel) {
return [$select, $with];
} else {
return function ($query) use ($with, $select, $customQuery) {
return function ($query) use ($with, $select, $customQuery, $requestedFields) {
if ($customQuery) {
$query = $customQuery(self::$args, $query);
$query = $customQuery($requestedFields['args'] ?? self::$args, $query);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}

$query->select($select);
Expand All @@ -118,7 +178,7 @@ protected static function handleFields(array $requestedFields, GraphqlType $pare
{
$parentTable = self::isMongodbInstance($parentType) ? null : self::getTableNameFromParentType($parentType);

foreach ($requestedFields as $key => $field) {
foreach ($requestedFields['fields'] as $key => $field) {
// Ignore __typename, as it's a special case
if ($key === '__typename') {
continue;
Expand Down Expand Up @@ -155,7 +215,7 @@ protected static function handleFields(array $requestedFields, GraphqlType $pare
self::handleFields($field, $fieldObject->config['type']->getWrappedType(), $select, $with);
}
// With
elseif (is_array($field) && $queryable) {
elseif (is_array($field['fields']) && $queryable) {
if (isset($parentType->config['model'])) {
// Get the next parent type, so that 'with' queries could be made
// Both keys for the relation are required (e.g 'id' <-> 'user_id')
Expand Down Expand Up @@ -193,7 +253,7 @@ protected static function handleFields(array $requestedFields, GraphqlType $pare
$segments = explode('.', $foreignKey);
$foreignKey = end($segments);
if (! array_key_exists($foreignKey, $field)) {
$field[$foreignKey] = self::FOREIGN_KEY;
$field['fields'][$foreignKey] = self::FOREIGN_KEY;
}
}

Expand Down Expand Up @@ -357,4 +417,9 @@ public function getRelations(): array
{
return $this->relations;
}

public function getResolveInfo(): ResolveInfo
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't necessary: ResolveInfo is solely here consumed by the class; any query/mutation resolver it as a separate argument anyway

{
return $this->info;
}
}
86 changes: 4 additions & 82 deletions tests/Database/SelectFields/NestedRelationLoadingTests/NestedRelationLoadingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,8 +630,8 @@ public function testQuerySelectAndWithAndSubArgs(): void

$this->assertSqlQueries(<<<'SQL'
select "users"."id", "users"."name" from "users" order by "users"."id" asc;
select "posts"."body", "posts"."id", "posts"."title", "posts"."user_id" from "posts" where "posts"."user_id" in (?, ?) order by "posts"."id" asc;
select "comments"."body", "comments"."id", "comments"."title", "comments"."post_id" from "comments" where "comments"."post_id" in (?, ?, ?, ?) order by "comments"."id" asc;
select "posts"."body", "posts"."id", "posts"."title", "posts"."user_id" from "posts" where "posts"."user_id" in (?, ?) and "posts"."flag" = ? order by "posts"."id" asc;
select "comments"."body", "comments"."id", "comments"."title", "comments"."post_id" from "comments" where "comments"."post_id" in (?, ?) order by "comments"."id" asc;
SQL
);

Expand Down Expand Up @@ -659,23 +659,6 @@ public function testQuerySelectAndWithAndSubArgs(): void
],
],
],
[
'body' => $users[0]->posts[1]->body,
'id' => (string) $users[0]->posts[1]->id,
'title' => $users[0]->posts[1]->title,
'comments' => [
[
'body' => $users[0]->posts[1]->comments[0]->body,
'id' => (string) $users[0]->posts[1]->comments[0]->id,
'title' => $users[0]->posts[1]->comments[0]->title,
],
[
'body' => $users[0]->posts[1]->comments[1]->body,
'id' => (string) $users[0]->posts[1]->comments[1]->id,
'title' => $users[0]->posts[1]->comments[1]->title,
],
],
],
],
],
[
Expand All @@ -699,23 +682,6 @@ public function testQuerySelectAndWithAndSubArgs(): void
],
],
],
[
'body' => $users[1]->posts[1]->body,
'id' => (string) $users[1]->posts[1]->id,
'title' => $users[1]->posts[1]->title,
'comments' => [
[
'body' => $users[1]->posts[1]->comments[0]->body,
'id' => (string) $users[1]->posts[1]->comments[0]->id,
'title' => $users[1]->posts[1]->comments[0]->title,
],
[
'body' => $users[1]->posts[1]->comments[1]->body,
'id' => (string) $users[1]->posts[1]->comments[1]->id,
'title' => $users[1]->posts[1]->comments[1]->title,
],
],
],
],
],
],
Expand Down Expand Up @@ -793,8 +759,8 @@ public function testQuerySelectAndWithAndNestedSubArgs(): void

$this->assertSqlQueries(<<<'SQL'
select "users"."id", "users"."name" from "users" order by "users"."id" asc;
select "posts"."body", "posts"."id", "posts"."title", "posts"."user_id" from "posts" where "posts"."user_id" in (?, ?) order by "posts"."id" asc;
select "comments"."body", "comments"."id", "comments"."title", "comments"."post_id" from "comments" where "comments"."post_id" in (?, ?, ?, ?) order by "comments"."id" asc;
select "posts"."body", "posts"."id", "posts"."title", "posts"."user_id" from "posts" where "posts"."user_id" in (?, ?) and "posts"."flag" = ? order by "posts"."id" asc;
select "comments"."body", "comments"."id", "comments"."title", "comments"."post_id" from "comments" where "comments"."post_id" in (?, ?) and "comments"."flag" = ? order by "comments"."id" asc;
SQL
);

Expand All @@ -815,28 +781,6 @@ public function testQuerySelectAndWithAndNestedSubArgs(): void
'id' => (string) $users[0]->posts[0]->comments[0]->id,
'title' => $users[0]->posts[0]->comments[0]->title,
],
[
'body' => $users[0]->posts[0]->comments[1]->body,
'id' => (string) $users[0]->posts[0]->comments[1]->id,
'title' => $users[0]->posts[0]->comments[1]->title,
],
],
],
[
'body' => $users[0]->posts[1]->body,
'id' => (string) $users[0]->posts[1]->id,
'title' => $users[0]->posts[1]->title,
'comments' => [
[
'body' => $users[0]->posts[1]->comments[0]->body,
'id' => (string) $users[0]->posts[1]->comments[0]->id,
'title' => $users[0]->posts[1]->comments[0]->title,
],
[
'body' => $users[0]->posts[1]->comments[1]->body,
'id' => (string) $users[0]->posts[1]->comments[1]->id,
'title' => $users[0]->posts[1]->comments[1]->title,
],
],
],
],
Expand All @@ -855,28 +799,6 @@ public function testQuerySelectAndWithAndNestedSubArgs(): void
'id' => (string) $users[1]->posts[0]->comments[0]->id,
'title' => $users[1]->posts[0]->comments[0]->title,
],
[
'body' => $users[1]->posts[0]->comments[1]->body,
'id' => (string) $users[1]->posts[0]->comments[1]->id,
'title' => $users[1]->posts[0]->comments[1]->title,
],
],
],
[
'body' => $users[1]->posts[1]->body,
'id' => (string) $users[1]->posts[1]->id,
'title' => $users[1]->posts[1]->title,
'comments' => [
[
'body' => $users[1]->posts[1]->comments[0]->body,
'id' => (string) $users[1]->posts[1]->comments[0]->id,
'title' => $users[1]->posts[1]->comments[0]->title,
],
[
'body' => $users[1]->posts[1]->comments[1]->body,
'id' => (string) $users[1]->posts[1]->comments[1]->id,
'title' => $users[1]->posts[1]->comments[1]->title,
],
],
],
],
Expand Down