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.

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 alias relationships not firing its own query #612

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ CHANGELOG
--------------

### Added
- Fix aliased relationships not fetching the correct data [\#604 / crissi](https://github.com/rebing/graphql-laravel/pull/604)
- Support Laravel 7 [\#597 / exodusanto](https://github.com/rebing/graphql-laravel/pull/597)
- Add support for custom authorization message [\#578 / Sh1d0w](https://github.com/rebing/graphql-laravel/pull/578)
- Add support for macros on the GraphQL service/facade [\#592 / stevelacey](https://github.com/rebing/graphql-laravel/pull/592)
Expand Down
1 change: 1 addition & 0 deletions phpstan.neon.dist
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ parameters:
- '/Parameter #1 \$haystack of function mb_stripos expects string, \(array\)\|string given/'
- '/Strict comparison using === between null and array will always evaluate to false/'
- '/Cannot access offset . on array\|Closure/'
- '/Function factory invoked with [0-9]+ parameter[s]?, 0 required/'
# \Rebing\GraphQL\Support\SelectFields::handleFields
- '/Parameter #1 \$function of function call_user_func expects callable\(\): mixed, array\(mixed, mixed\) given/'
- '/Binary operation "." between string and array\|string\|null results in an error/'
Expand Down
38 changes: 38 additions & 0 deletions src/Support/AliasedRelationships/AddModelRelationshipScope.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?php

declare(strict_types=1);

namespace Rebing\GraphQL\Support\AliasedRelationships;

use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Scope;

class AddModelRelationshipScope implements Scope
{
/**
* @var array<string,string>
*/
private $relationships;

/**
* @param array<string,string> $relationships
*/
public function __construct(array $relationships = [])
{
$this->relationships = $relationships;
}

public function extend(Builder $builder): void
{
foreach ($this->relationships as [$graphqlAlias, $relationship]) {
$builder->macro($graphqlAlias, function (Builder $builder) use ($relationship) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👀

return $builder->getModel()->{$relationship}();
});
}
}

public function apply(Builder $builder, Model $model)
{
}
}
15 changes: 15 additions & 0 deletions src/Support/AliasedRelationships/GenerateRelationshipKey.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

declare(strict_types=1);

namespace Rebing\GraphQL\Support\AliasedRelationships;

class GenerateRelationshipKey
{
const RELATIONSHIP_PRE_TEXT = '_generated_';

public static function generate(string $key): string
{
return self::RELATIONSHIP_PRE_TEXT.$key;
}
}
20 changes: 20 additions & 0 deletions src/Support/AliasedRelationships/ModelRelationshipAdder.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

declare(strict_types=1);

namespace Rebing\GraphQL\Support\AliasedRelationships;

use Illuminate\Database\Eloquent\Model;

class ModelRelationshipAdder
{
/**
* @param class-string<Model> $model
* @param array<string,string> $relationships
* @return void
*/
public static function add(string $model, array $relationships): void
{
$model::addGlobalScope(new AddModelRelationshipScope($relationships));
Copy link
Collaborator

Choose a reason for hiding this comment

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

👀

Isn't that somehow dangerous (side-effects), permanently mutating the the models global scopes?

Copy link
Contributor Author

@crissi crissi Apr 3, 2020

Choose a reason for hiding this comment

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

I don't feel confident doing a good review on this one.

This shows the absurd complexity we've to put upon us because of the concept of the SelectFields and once more shows me this approach goes into the wrong direction somehow…

Leads me to believe SelectFields should be a standalone component and not in this library TBH 😄

Mutating models scopes, adding macros to builders .. this somehow looks dangerous to me 🤷‍♀️

When I think about I would implement a dedicated PHP server with this library (i.e. never restart, a daemon) I can foresee thus causing problems. Is there no other way to solve this aliasing / double-query problem?

Not really sure how to move forward with this one TBH.

Leads me to believe SelectFields should be a standalone component and not in this library TBH 😄

To clarify this, I believe the proper approach is to use a dataloader concept to solve these things in general (that's what I'm doing in my projects, I'm not using SelectFields)

I love the SelectFields and use it for 99% of my queries, makes it so easy to set up and think it is one of the main "selling points" of the libary

Copy link
Contributor Author

@crissi crissi Apr 3, 2020

Choose a reason for hiding this comment

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

👀

Isn't that somehow dangerous (side-effects), permanently mutating the the models global scopes?

yes it is definitely a side effect, but since SelectFields are made to be used with eloquent's ->with(). The only way to fix this is to generate the relationships.

The side effect is minimized by generating relationships with generated in front of all generated relationships, so I would say this makes it as safe as possible and only applied if alias are actually used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we might need to tore the relationship down if some one uses the same relationship again

}
}
28 changes: 28 additions & 0 deletions src/Support/AliasedRelationships/Resolver.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php

declare(strict_types=1);

namespace Rebing\GraphQL\Support\AliasedRelationships;

use GraphQL\Type\Definition\ResolveInfo;

class Resolver
{
/**
* @param mixed $type
* @param array<string,mixed> $args
* @param mixed $context
* @param ResolveInfo $resolveInfo
* @return mixed
*/
public function __invoke($type, $args, $context, ResolveInfo $resolveInfo)
{
$name = $resolveInfo->fieldName;
$alias = null;
if (isset($resolveInfo->fieldNodes[0]->alias)) {
$alias = GenerateRelationshipKey::generate($resolveInfo->fieldNodes[0]->alias->value);
}

return $type->{$alias ?: $name};
}
}
8 changes: 6 additions & 2 deletions src/Support/ResolveInfoFieldsAndArguments.php
Original file line number Diff line number Diff line change
Expand Up @@ -115,16 +115,20 @@ private function foldSelectionSet(SelectionSetNode $selectionSet, int $descend):
foreach ($selectionSet->selections as $selectionNode) {
if ($selectionNode instanceof FieldNode) {
$name = $selectionNode->name->value;
$alias = $selectionNode->alias->value ?? null;
$key = $alias ?: $name;

$fields[$name] = [
$fields[$key] = [
'name' => $name,
'alias' => $alias,
'args' => [],
'fields' => $descend > 0 && ! empty($selectionNode->selectionSet)
? $this->foldSelectionSet($selectionNode->selectionSet, $descend - 1)
: true,
];

foreach ($selectionNode->arguments ?? [] as $argumentNode) {
$fields[$name]['args'][$argumentNode->name->value] = $this->getValue($argumentNode->value);
$fields[$key]['args'][$argumentNode->name->value] = $this->getValue($argumentNode->value);
}
} elseif ($selectionNode instanceof FragmentSpreadNode) {
$spreadName = $selectionNode->name->value;
Expand Down
50 changes: 46 additions & 4 deletions src/Support/SelectFields.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use GraphQL\Type\Definition\Type as GraphqlType;
use GraphQL\Type\Definition\UnionType;
use GraphQL\Type\Definition\WrappingType;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsTo;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Illuminate\Database\Eloquent\Relations\HasOne;
Expand All @@ -20,6 +21,9 @@
use Illuminate\Database\Eloquent\Relations\Relation;
use Illuminate\Database\Query\Expression;
use Illuminate\Support\Arr;
use Rebing\GraphQL\Support\AliasedRelationships\GenerateRelationshipKey;
use Rebing\GraphQL\Support\AliasedRelationships\ModelRelationshipAdder;
use Rebing\GraphQL\Support\AliasedRelationships\Resolver as AliasedRelationshipsResolver;
use RuntimeException;

class SelectFields
Expand All @@ -28,7 +32,10 @@ class SelectFields
private $select = [];
/** @var array */
private $relations = [];

/**
* @var array<class-string<Model>,array<string,string>>
*/
private static $modelRelationshipsForAlias = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wish this wouldn't be static.

I understand from the code it's cleaned up, still…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah me too

const ALWAYS_RELATION_KEY = 'ALWAYS_RELATION_KEY';

/**
Expand All @@ -49,6 +56,8 @@ public function __construct(ResolveInfo $info, GraphqlType $parentType, array $q
$fields = self::getSelectableFieldsAndRelations($queryArgs, $requestedFields, $parentType, null, true, $ctx);
$this->select = $fields[0];
$this->relations = $fields[1];

$this->createModelRelationshipsForAlias();
}

private function getFieldSelection(ResolveInfo $resolveInfo, array $args, int $depth): array
Expand Down Expand Up @@ -148,6 +157,7 @@ protected static function handleFields(
$parentTable = self::isMongodbInstance($parentType) ? null : self::getTableNameFromParentType($parentType);

foreach ($requestedFields['fields'] as $key => $field) {

// Ignore __typename, as it's a special case
if ($key === '__typename') {
continue;
Expand All @@ -163,7 +173,7 @@ protected static function handleFields(
// If field doesn't exist on definition we don't select it
try {
if (method_exists($parentType, 'getField')) {
$fieldObject = $parentType->getField($key);
$fieldObject = $parentType->getField($field['name'] ?? $key);
} else {
continue;
}
Expand Down Expand Up @@ -204,16 +214,29 @@ protected static function handleFields(
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')
$relationsKey = $fieldObject->config['alias'] ?? $key;
$relationsKey = $fieldObject->config['alias'] ?? $field['name'];
$relation = call_user_func([app($parentType->config['model']), $relationsKey]);

self::handleRelation($select, $relation, $parentTable, $field);

// New parent type, which is the relation
$newParentType = $parentType->getField($key)->config['type'];
$newParentType = $parentType->getField($field['name'])->config['type'];

self::addAlwaysFields($fieldObject, $field, $parentTable, true);

// Check if an graphql alias is being used
// and add the relationship to the model
// so it can be eager loaded.
$isGraphqlAlias = (bool) $field['alias'];

if ($isGraphqlAlias) {
$generatedRelationshipName = GenerateRelationshipKey::generate($field['alias']);
$fieldObject->resolveFn = new AliasedRelationshipsResolver;

self::collectModelRelationshipsForAlias($parentType->config['model'], GenerateRelationshipKey::generate($field['alias']), $relationsKey);
$relationsKey = $generatedRelationshipName;
}

$with[$relationsKey] = self::getSelectableFieldsAndRelations(
$queryArgs,
$field,
Expand Down Expand Up @@ -268,6 +291,25 @@ protected static function handleFields(
}
}

/**
* @param class-string<Model> $modelName
* @param string $graphqlAlias
* @param string $relationsKey
* @return void
*/
public static function collectModelRelationshipsForAlias(string $modelName, string $graphqlAlias, string $relationsKey): void
{
self::$modelRelationshipsForAlias[$modelName][] = [$graphqlAlias, $relationsKey];
}

public function createModelRelationshipsForAlias(): void
{
foreach (self::$modelRelationshipsForAlias as $model => $list) {
ModelRelationshipAdder::add($model, $list);
}
self::$modelRelationshipsForAlias = [];
}

private static function isMongodbInstance(GraphqlType $parentType): bool
{
$mongoType = 'Jenssegers\Mongodb\Eloquent\Model';
Expand Down
35 changes: 35 additions & 0 deletions tests/Database/SelectFields/QueryArgsTest/CommentType.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

declare(strict_types=1);

namespace Rebing\GraphQL\Tests\Database\SelectFields\QueryArgsTest;

use GraphQL\Type\Definition\Type;
use Rebing\GraphQL\Support\Type as GraphQLType;
use Rebing\GraphQL\Tests\Support\Models\Comment;

class CommentType extends GraphQLType
{
/**
* @var array<string, mixed>
*/
protected $attributes = [
'name' => 'Comment',
'model' => Comment::class,
];

public function fields(): array
{
return [
'id' => [
'type' => Type::nonNull(Type::ID()),
],
'body' => [
'type' => Type::string(),
],
'title' => [
'type' => Type::nonNull(Type::string()),
],
];
}
}
52 changes: 52 additions & 0 deletions tests/Database/SelectFields/QueryArgsTest/PostType.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

declare(strict_types=1);

namespace Rebing\GraphQL\Tests\Database\SelectFields\QueryArgsTest;

use GraphQL\Type\Definition\Type;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Rebing\GraphQL\Support\Facades\GraphQL;
use Rebing\GraphQL\Support\Type as GraphQLType;
use Rebing\GraphQL\Tests\Support\Models\Post;

class PostType extends GraphQLType
{
/**
* @var array<string, mixed>
*/
protected $attributes = [
'name' => 'Post',
'model' => Post::class,
];

public function fields(): array
{
return [
'body' => [
'type' => Type::string(),
],
'comments' => [
'type' => Type::nonNull(Type::listOf(Type::nonNull(GraphQL::type('Comment')))),
'args' => [
'flag' => [
Type::boolean(),
],
],
'query' => function (array $args, HasMany $query, $ctx): HasMany {
if (isset($args['flag'])) {
$query->where('comments.flag', '=', $args['flag']);
}

return $query;
},
],
'id' => [
'type' => Type::nonNull(Type::ID()),
],
'title' => [
'type' => Type::nonNull(Type::string()),
],
];
}
}
Loading