Skip to content

Commit d0d5c9a

Browse files
oprypkhantcoprypkhantc
committed
Fix broken callable mapping
1 parent ec0dec6 commit d0d5c9a

7 files changed

Lines changed: 109 additions & 49 deletions

File tree

‎src/Mappers/CannotMapTypeException.php‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -181,18 +181,23 @@ public static function createForNonNullReturnByTypeMapper(): self
181181
return new self('a type mapper returned a GraphQL\Type\Definition\NonNull instance. All instances returned by type mappers should be nullable. It is the role of the NullableTypeMapperAdapter class to make a GraphQL type in a "NonNull". Note: this is an error in the TypeMapper code or in GraphQLite itself. Please check your custom type mappers or open an issue on GitHub if you don\'t have any custom type mapper.');
182182
}
183183

184-
public static function createForUnexpectedCallableParameters(): self
184+
public static function createForUnexpectedCallable(): self
185185
{
186-
return new self('callable() type-hint must not specify any parameters.');
186+
return new self('callable() type-hint is not supported. Use Closure: Closure(): int');
187187
}
188188

189-
public static function createForMissingCallableReturnType(): self
189+
public static function createForUnexpectedClosureParameters(): self
190190
{
191-
return new self('callable() type-hint must specify its return type. For instance: callable(): int');
191+
return new self('Closure() type-hint must not specify any parameters.');
192192
}
193193

194-
public static function createForCallableAsInput(): self
194+
public static function createForMissingClosureReturnType(): self
195195
{
196-
return new self('callable() type-hint can only be used as output type.');
196+
return new self('Closure() type-hint must specify its return type. For instance: Closure(): int');
197+
}
198+
199+
public static function createForClosureAsInput(): self
200+
{
201+
return new self('Closure() type-hint can only be used as output type.');
197202
}
198203
}

src/Mappers/Root/CallableTypeMapper.php renamed to src/Mappers/Root/ClosureTypeMapper.php

Lines changed: 44 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,53 +4,69 @@
44

55
namespace TheCodingMachine\GraphQLite\Mappers\Root;
66

7+
use Closure;
78
use GraphQL\Type\Definition\InputType;
89
use GraphQL\Type\Definition\NamedType;
910
use GraphQL\Type\Definition\OutputType;
1011
use GraphQL\Type\Definition\Type as GraphQLType;
1112
use phpDocumentor\Reflection\DocBlock;
13+
use phpDocumentor\Reflection\Fqsen;
1214
use phpDocumentor\Reflection\Type;
1315
use phpDocumentor\Reflection\Types\Callable_;
16+
use phpDocumentor\Reflection\Types\Compound;
17+
use phpDocumentor\Reflection\Types\Object_;
1418
use ReflectionMethod;
1519
use ReflectionProperty;
1620
use TheCodingMachine\GraphQLite\Mappers\CannotMapTypeException;
1721

22+
use function count;
23+
use function iterator_to_array;
24+
1825
/**
1926
* This mapper maps callable types into their return types, so that fields can defer their execution.
2027
*/
21-
class CallableTypeMapper implements RootTypeMapperInterface
28+
class ClosureTypeMapper implements RootTypeMapperInterface
2229
{
30+
private Object_ $closureType;
31+
2332
public function __construct(
2433
private readonly RootTypeMapperInterface $next,
2534
private readonly RootTypeMapperInterface $topRootTypeMapper,
2635
) {
36+
$this->closureType = new Object_(new Fqsen('\\' . Closure::class));
2737
}
2838

2939
public function toGraphQLOutputType(Type $type, OutputType|null $subType, ReflectionMethod|ReflectionProperty $reflector, DocBlock $docBlockObj): OutputType&GraphQLType
3040
{
31-
if (! $type instanceof Callable_) {
41+
// This check exists because any string may be a callable (referring to a global function),
42+
// so if a string that looks like a callable is returned from a resolver, it will get wrapped
43+
// in `Deferred`, even though it wasn't supposed to be a deferred value. This could be fixed
44+
// by combining `QueryField`'s resolver and `CallableTypeMapper` into one place, but
45+
// that's not currently possible with GraphQLite's design.
46+
if ($type instanceof Callable_) {
47+
throw CannotMapTypeException::createForUnexpectedCallable();
48+
}
49+
50+
if (! $type instanceof Compound || ! $type->contains($this->closureType)) {
3251
return $this->next->toGraphQLOutputType($type, $subType, $reflector, $docBlockObj);
3352
}
3453

35-
if ($type->getParameters()) {
36-
throw CannotMapTypeException::createForUnexpectedCallableParameters();
54+
$allTypes = iterator_to_array($type);
55+
56+
if (count($allTypes) !== 2) {
57+
return $this->next->toGraphQLOutputType($type, $subType, $reflector, $docBlockObj);
3758
}
3859

39-
$returnType = $type->getReturnType();
60+
$callableType = $this->findCallableType($allTypes);
61+
$returnType = $callableType?->getReturnType();
4062

4163
if (! $returnType) {
42-
throw CannotMapTypeException::createForMissingCallableReturnType();
64+
throw CannotMapTypeException::createForMissingClosureReturnType();
4365
}
4466

45-
// It would also be a good idea to check if the type-hint is actually `Closure(): something`,
46-
// not `callable(): something`, because the latter is currently not supported. But to do so,
47-
// `phpDocumentor` would need to pass in the type of callable, which it doesn't. All
48-
// types that look like callables - are reported as `callable` by phpDocumentor.
49-
// The reason for such a check is that any string may be a callable (referring to a global function),
50-
// so if a string that looks like a callable is returned from a resolver, it will get wrapped
51-
// in `Deferred`, even though it wasn't supposed to be a deferred value. This could be fixed
52-
// by combining `QueryField`'s resolver and `CallableTypeMapper` into one place, but
53-
// that's not currently possible with GraphQLite's design.
67+
if ($callableType->getParameters()) {
68+
throw CannotMapTypeException::createForUnexpectedClosureParameters();
69+
}
5470

5571
return $this->topRootTypeMapper->toGraphQLOutputType($returnType, null, $reflector, $docBlockObj);
5672
}
@@ -61,11 +77,23 @@ public function toGraphQLInputType(Type $type, InputType|null $subType, string $
6177
return $this->next->toGraphQLInputType($type, $subType, $argumentName, $reflector, $docBlockObj);
6278
}
6379

64-
throw CannotMapTypeException::createForCallableAsInput();
80+
throw CannotMapTypeException::createForClosureAsInput();
6581
}
6682

6783
public function mapNameToType(string $typeName): NamedType&GraphQLType
6884
{
6985
return $this->next->mapNameToType($typeName);
7086
}
87+
88+
/** @param array<int, Type> $types */
89+
private function findCallableType(array $types): Callable_|null
90+
{
91+
foreach ($types as $type) {
92+
if ($type instanceof Callable_) {
93+
return $type;
94+
}
95+
}
96+
97+
return null;
98+
}
7199
}

‎src/QueryField.php‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,6 @@
2121
use TheCodingMachine\GraphQLite\Parameters\ParameterInterface;
2222
use TheCodingMachine\GraphQLite\Parameters\SourceParameter;
2323

24-
use function is_callable;
25-
2624
/**
2725
* A GraphQL field that maps to a PHP method automatically.
2826
*

‎src/SchemaFactory.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
use TheCodingMachine\GraphQLite\Mappers\PorpaginasTypeMapper;
3636
use TheCodingMachine\GraphQLite\Mappers\RecursiveTypeMapper;
3737
use TheCodingMachine\GraphQLite\Mappers\Root\BaseTypeMapper;
38-
use TheCodingMachine\GraphQLite\Mappers\Root\CallableTypeMapper;
38+
use TheCodingMachine\GraphQLite\Mappers\Root\ClosureTypeMapper;
3939
use TheCodingMachine\GraphQLite\Mappers\Root\CompoundTypeMapper;
4040
use TheCodingMachine\GraphQLite\Mappers\Root\EnumTypeMapper;
4141
use TheCodingMachine\GraphQLite\Mappers\Root\FinalRootTypeMapper;
@@ -400,7 +400,7 @@ public function createSchema(): Schema
400400
$lastTopRootTypeMapper = new LastDelegatingTypeMapper();
401401
$topRootTypeMapper = new NullableTypeMapperAdapter($lastTopRootTypeMapper);
402402
$topRootTypeMapper = new VoidTypeMapper($topRootTypeMapper);
403-
$topRootTypeMapper = new CallableTypeMapper($topRootTypeMapper, $lastTopRootTypeMapper);
403+
$topRootTypeMapper = new ClosureTypeMapper($topRootTypeMapper, $lastTopRootTypeMapper);
404404

405405
$errorRootTypeMapper = new FinalRootTypeMapper($recursiveTypeMapper);
406406
$rootTypeMapper = new BaseTypeMapper($errorRootTypeMapper, $recursiveTypeMapper, $topRootTypeMapper);

‎tests/AbstractQueryProvider.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@
4141
use TheCodingMachine\GraphQLite\Mappers\Parameters\ResolveInfoParameterHandler;
4242
use TheCodingMachine\GraphQLite\Mappers\RecursiveTypeMapper;
4343
use TheCodingMachine\GraphQLite\Mappers\Root\BaseTypeMapper;
44-
use TheCodingMachine\GraphQLite\Mappers\Root\CallableTypeMapper;
44+
use TheCodingMachine\GraphQLite\Mappers\Root\ClosureTypeMapper;
4545
use TheCodingMachine\GraphQLite\Mappers\Root\CompoundTypeMapper;
4646
use TheCodingMachine\GraphQLite\Mappers\Root\EnumTypeMapper;
4747
use TheCodingMachine\GraphQLite\Mappers\Root\FinalRootTypeMapper;
@@ -360,7 +360,7 @@ protected function buildRootTypeMapper(): RootTypeMapperInterface
360360
$lastTopRootTypeMapper = new LastDelegatingTypeMapper();
361361
$topRootTypeMapper = new NullableTypeMapperAdapter($lastTopRootTypeMapper);
362362
$topRootTypeMapper = new VoidTypeMapper($topRootTypeMapper);
363-
$topRootTypeMapper = new CallableTypeMapper($topRootTypeMapper, $lastTopRootTypeMapper);
363+
$topRootTypeMapper = new ClosureTypeMapper($topRootTypeMapper, $lastTopRootTypeMapper);
364364

365365
$errorRootTypeMapper = new FinalRootTypeMapper($this->getTypeMapper());
366366
$rootTypeMapper = new BaseTypeMapper(

‎tests/Integration/IntegrationTestCase.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@
4141
use TheCodingMachine\GraphQLite\Mappers\RecursiveTypeMapper;
4242
use TheCodingMachine\GraphQLite\Mappers\RecursiveTypeMapperInterface;
4343
use TheCodingMachine\GraphQLite\Mappers\Root\BaseTypeMapper;
44-
use TheCodingMachine\GraphQLite\Mappers\Root\CallableTypeMapper;
44+
use TheCodingMachine\GraphQLite\Mappers\Root\ClosureTypeMapper;
4545
use TheCodingMachine\GraphQLite\Mappers\Root\CompoundTypeMapper;
4646
use TheCodingMachine\GraphQLite\Mappers\Root\EnumTypeMapper;
4747
use TheCodingMachine\GraphQLite\Mappers\Root\FinalRootTypeMapper;
@@ -297,7 +297,7 @@ public function createContainer(array $overloadedServices = []): ContainerInterf
297297
);
298298
},
299299
RootTypeMapperInterface::class => static function (ContainerInterface $container) {
300-
return new CallableTypeMapper(
300+
return new ClosureTypeMapper(
301301
new VoidTypeMapper(
302302
new NullableTypeMapperAdapter(
303303
$container->get('topRootTypeMapper')

tests/Mappers/Root/CallableTypeMapperTest.php renamed to tests/Mappers/Root/ClosureTypeMapperTest.php

Lines changed: 48 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace TheCodingMachine\GraphQLite\Mappers\Root;
44

5+
use Closure;
56
use Generator;
67
use GraphQL\Type\Definition\InputType;
78
use GraphQL\Type\Definition\NamedType;
@@ -10,10 +11,12 @@
1011
use GraphQL\Type\Definition\StringType;
1112
use GraphQL\Type\Definition\Type as GraphQLType;
1213
use phpDocumentor\Reflection\DocBlock;
14+
use phpDocumentor\Reflection\Fqsen;
1315
use phpDocumentor\Reflection\Type;
1416
use phpDocumentor\Reflection\Types\Array_;
1517
use phpDocumentor\Reflection\Types\Callable_;
1618
use phpDocumentor\Reflection\Types\CallableParameter;
19+
use phpDocumentor\Reflection\Types\Compound;
1720
use phpDocumentor\Reflection\Types\Nullable;
1821
use phpDocumentor\Reflection\Types\Object_;
1922
use phpDocumentor\Reflection\Types\String_;
@@ -25,9 +28,9 @@
2528
use TheCodingMachine\GraphQLite\Fixtures\TestObject2;
2629
use TheCodingMachine\GraphQLite\Mappers\CannotMapTypeException;
2730

28-
#[CoversClass(CallableTypeMapper::class)]
31+
#[CoversClass(ClosureTypeMapper::class)]
2932
#[CoversClass(CannotMapTypeException::class)]
30-
class CallableTypeMapperTest extends AbstractQueryProvider
33+
class ClosureTypeMapperTest extends AbstractQueryProvider
3134
{
3235
public function testMapsCallableReturnTypeUsingTopRootMapper(): void
3336
{
@@ -42,51 +45,77 @@ public function testMapsCallableReturnTypeUsingTopRootMapper(): void
4245
->with($returnType, null, $reflection, $docBlock)
4346
->willReturn(GraphQLType::string());
4447

45-
$mapper = new CallableTypeMapper(
48+
$mapper = new ClosureTypeMapper(
4649
$this->createMock(RootTypeMapperInterface::class),
4750
$topRootMapper,
4851
);
4952

50-
$result = $mapper->toGraphQLOutputType(new Callable_(returnType: $returnType), null, $reflection, $docBlock);
53+
$type = new Compound([
54+
new Callable_(returnType: $returnType),
55+
new Object_(new Fqsen('\\' . Closure::class))
56+
]);
57+
58+
$result = $mapper->toGraphQLOutputType($type, null, $reflection, $docBlock);
5159

5260
$this->assertSame(GraphQLType::string(), $result);
5361
}
5462

55-
public function testThrowsWhenUsingCallableWithParameters(): void
63+
public function testThrowsWhenUsingCallable(): void
5664
{
57-
$this->expectExceptionObject(CannotMapTypeException::createForUnexpectedCallableParameters());
65+
$this->expectExceptionObject(CannotMapTypeException::createForUnexpectedCallable());
5866

59-
$mapper = new CallableTypeMapper(
67+
$mapper = new ClosureTypeMapper(
6068
$this->createMock(RootTypeMapperInterface::class),
6169
$this->createMock(RootTypeMapperInterface::class)
6270
);
6371

64-
$type = new Callable_(
65-
parameters: [
66-
new CallableParameter(new String_())
67-
]
72+
$mapper->toGraphQLOutputType(new Callable_(), null, new ReflectionMethod(__CLASS__, 'testSkipsNonCallables'), new DocBlock());
73+
}
74+
75+
public function testThrowsWhenUsingClosureWithParameters(): void
76+
{
77+
$this->expectExceptionObject(CannotMapTypeException::createForUnexpectedClosureParameters());
78+
79+
$mapper = new ClosureTypeMapper(
80+
$this->createMock(RootTypeMapperInterface::class),
81+
$this->createMock(RootTypeMapperInterface::class)
6882
);
6983

84+
$type = new Compound([
85+
new Callable_(
86+
parameters: [
87+
new CallableParameter(new String_())
88+
],
89+
returnType: new String_()
90+
),
91+
new Object_(new Fqsen('\\' . Closure::class))
92+
]);
93+
7094
$mapper->toGraphQLOutputType($type, null, new ReflectionMethod(__CLASS__, 'testSkipsNonCallables'), new DocBlock());
7195
}
7296

73-
public function testThrowsWhenUsingCallableWithoutReturnType(): void
97+
public function testThrowsWhenUsingClosureWithoutReturnType(): void
7498
{
75-
$this->expectExceptionObject(CannotMapTypeException::createForMissingCallableReturnType());
99+
$this->expectExceptionObject(CannotMapTypeException::createForMissingClosureReturnType());
76100

77-
$mapper = new CallableTypeMapper(
101+
$mapper = new ClosureTypeMapper(
78102
$this->createMock(RootTypeMapperInterface::class),
79103
$this->createMock(RootTypeMapperInterface::class)
80104
);
81105

82-
$mapper->toGraphQLOutputType(new Callable_(), null, new ReflectionMethod(__CLASS__, 'testSkipsNonCallables'), new DocBlock());
106+
$type = new Compound([
107+
new Callable_(),
108+
new Object_(new Fqsen('\\' . Closure::class))
109+
]);
110+
111+
$mapper->toGraphQLOutputType($type, null, new ReflectionMethod(__CLASS__, 'testSkipsNonCallables'), new DocBlock());
83112
}
84113

85-
public function testThrowsWhenUsingCallableAsInputType(): void
114+
public function testThrowsWhenUsingClosureAsInputType(): void
86115
{
87-
$this->expectExceptionObject(CannotMapTypeException::createForCallableAsInput());
116+
$this->expectExceptionObject(CannotMapTypeException::createForClosureAsInput());
88117

89-
$mapper = new CallableTypeMapper(
118+
$mapper = new ClosureTypeMapper(
90119
$this->createMock(RootTypeMapperInterface::class),
91120
$this->createMock(RootTypeMapperInterface::class)
92121
);
@@ -115,7 +144,7 @@ public function testSkipsNonCallables(callable $createType): void
115144
->with('Name')
116145
->willReturn(GraphQLType::float());
117146

118-
$mapper = new CallableTypeMapper($next, $this->createMock(RootTypeMapperInterface::class));
147+
$mapper = new ClosureTypeMapper($next, $this->createMock(RootTypeMapperInterface::class));
119148

120149
$this->assertSame(GraphQLType::string(), $mapper->toGraphQLOutputType($type, null, $reflection, $docBlock));
121150
$this->assertSame(GraphQLType::int(), $mapper->toGraphQLInputType($type, null, 'arg1', $reflection, $docBlock));

0 commit comments

Comments
 (0)