Skip to content

Commit b0680f1

Browse files
PowerKiKiPowerKiKi
committed
Absence of default value was incorrectly exposed as a default value of null
This went unnoticed until now because the custom assertion used in our tests was incorrect.
1 parent 7ab89af commit b0680f1

16 files changed

+242
-147
lines changed

‎src/Annotation/AbstractAnnotation.php‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace GraphQL\Doctrine\Annotation;
6+
7+
use GraphQL\Type\Definition\Type;
8+
9+
/**
10+
* Abstract annotation with common logic for Argument and Field
11+
*/
12+
abstract class AbstractAnnotation
13+
{
14+
/**
15+
* The name of the argument, it must matches the actual PHP argument name
16+
*
17+
* @var null|string
18+
* @Required
19+
*/
20+
private $name;
21+
22+
/**
23+
* FQCN of PHP class implementing the GraphQL type
24+
*
25+
* @var null|string
26+
*/
27+
private $type;
28+
29+
/**
30+
* Instance of the GraphQL type
31+
*
32+
* @var null|Type
33+
*/
34+
private $typeInstance;
35+
36+
/**
37+
* @var null|string
38+
*/
39+
private $description;
40+
41+
/**
42+
* @var mixed
43+
*/
44+
private $defaultValue;
45+
46+
/**
47+
* @var bool
48+
*/
49+
private $hasDefaultValue = false;
50+
51+
public function __construct($values = [])
52+
{
53+
foreach ($values as $key => $value) {
54+
$setter = 'set' . ucfirst($key);
55+
$this->$setter($value);
56+
}
57+
}
58+
59+
public function toArray(): array
60+
{
61+
$data = [
62+
'name' => $this->getName(),
63+
'type' => $this->getTypeInstance(),
64+
'description' => $this->getDescription(),
65+
];
66+
67+
if ($this->hasDefaultValue()) {
68+
$data['defaultValue'] = $this->getDefaultValue();
69+
}
70+
71+
return $data;
72+
}
73+
74+
/**
75+
* @return null|string
76+
*/
77+
public function getName(): ?string
78+
{
79+
return $this->name;
80+
}
81+
82+
/**
83+
* @param null|string $name
84+
*/
85+
public function setName(?string $name): void
86+
{
87+
$this->name = $name;
88+
}
89+
90+
/**
91+
* @return null|string
92+
*/
93+
public function getType(): ?string
94+
{
95+
return $this->type;
96+
}
97+
98+
/**
99+
* @param null|string $type
100+
*/
101+
public function setType(?string $type): void
102+
{
103+
$this->type = $type;
104+
}
105+
106+
/**
107+
* @return null|string
108+
*/
109+
public function getDescription(): ?string
110+
{
111+
return $this->description;
112+
}
113+
114+
/**
115+
* @param null|string $description
116+
*/
117+
public function setDescription(?string $description): void
118+
{
119+
$this->description = $description;
120+
}
121+
122+
/**
123+
* @return mixed
124+
*/
125+
public function hasDefaultValue()
126+
{
127+
return $this->hasDefaultValue;
128+
}
129+
130+
/**
131+
* @return mixed
132+
*/
133+
public function getDefaultValue()
134+
{
135+
return $this->defaultValue;
136+
}
137+
138+
/**
139+
* @param mixed $defaultValue
140+
*/
141+
public function setDefaultValue($defaultValue): void
142+
{
143+
$this->defaultValue = $defaultValue;
144+
$this->hasDefaultValue = true;
145+
}
146+
147+
/**
148+
* @return null|Type
149+
*/
150+
public function getTypeInstance(): ?Type
151+
{
152+
return $this->typeInstance;
153+
}
154+
155+
/**
156+
* @param null|Type $typeInstance
157+
*/
158+
public function setTypeInstance(?Type $typeInstance): void
159+
{
160+
$this->typeInstance = $typeInstance;
161+
}
162+
}

‎src/Annotation/Argument.php‎

Lines changed: 10 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -7,51 +7,20 @@
77
/**
88
* Annotation used to override values for an field argument in GraphQL.
99
*
10-
* All values are optional and should only be used to override
10+
* The name of the argument is required and must match the actual PHP argument name.
11+
*
12+
* All other values are optional and should only be used to override
1113
* what is declared by the original argument of the method.
1214
*
1315
* @Annotation
1416
* @Target({"ANNOTATION"})
17+
* @Attributes({
18+
* @Attribute("name", required=true, type="string"),
19+
* @Attribute("type", required=false, type="string"),
20+
* @Attribute("description", required=false, type="string"),
21+
* @Attribute("defaultValue", required=false, type="mixed"),
22+
* })
1523
*/
16-
class Argument
24+
class Argument extends AbstractAnnotation
1725
{
18-
/**
19-
* The name of the argument, it must matches the actual PHP argument name
20-
*
21-
* @var string
22-
* @Required
23-
*/
24-
public $name;
25-
26-
/**
27-
* FQCN of PHP class implementing the GraphQL type
28-
*
29-
* @var string
30-
*/
31-
public $type;
32-
33-
/**
34-
* @var string
35-
*/
36-
public $description;
37-
38-
/**
39-
* @var mixed
40-
*/
41-
public $defaultValue;
42-
43-
public function toArray(): array
44-
{
45-
$data = [
46-
'name' => $this->name,
47-
'type' => $this->type,
48-
'description' => $this->description,
49-
];
50-
51-
if ($this->defaultValue !== null) {
52-
$data['defaultValue'] = $this->defaultValue;
53-
}
54-
55-
return $data;
56-
}
5726
}

‎src/Annotation/Input.php‎

Lines changed: 10 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@
44

55
namespace GraphQL\Doctrine\Annotation;
66

7+
use Doctrine\Common\Annotations\Annotation\Attribute;
8+
use Doctrine\Common\Annotations\Annotation\Attributes;
9+
710
/**
811
* Annotation used to override values for an input field in GraphQL.
912
*
@@ -12,43 +15,13 @@
1215
*
1316
* @Annotation
1417
* @Target({"METHOD"})
18+
* @Attributes({
19+
* @Attribute("name", required=false, type="string"),
20+
* @Attribute("type", required=false, type="string"),
21+
* @Attribute("description", required=false, type="string"),
22+
* @Attribute("defaultValue", required=false, type="mixed"),
23+
* })
1524
*/
16-
class Input
25+
class Input extends AbstractAnnotation
1726
{
18-
/**
19-
* @var string
20-
*/
21-
public $name;
22-
23-
/**
24-
* FQCN of PHP class implementing the GraphQL type
25-
*
26-
* @var string
27-
*/
28-
public $type;
29-
30-
/**
31-
* @var string
32-
*/
33-
public $description;
34-
35-
/**
36-
* @var mixed
37-
*/
38-
public $defaultValue;
39-
40-
public function toArray(): array
41-
{
42-
$data = [
43-
'name' => $this->name,
44-
'type' => $this->type,
45-
'description' => $this->description,
46-
];
47-
48-
if ($this->defaultValue !== null) {
49-
$data['defaultValue'] = $this->defaultValue;
50-
}
51-
52-
return $data;
53-
}
5427
}

‎src/Factory/AbstractFieldsConfigurationFactory.php‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,15 @@
99
use Doctrine\Common\Persistence\Mapping\Driver\AnnotationDriver;
1010
use Doctrine\ORM\EntityManager;
1111
use Doctrine\ORM\Mapping\ClassMetadata;
12+
use GraphQL\Doctrine\Annotation\AbstractAnnotation;
1213
use GraphQL\Doctrine\Annotation\Exclude;
1314
use GraphQL\Doctrine\Exception;
1415
use GraphQL\Doctrine\Types;
1516
use GraphQL\Type\Definition\InputType;
1617
use GraphQL\Type\Definition\NonNull;
1718
use GraphQL\Type\Definition\Type;
1819
use GraphQL\Type\Definition\WrappingType;
20+
use ReflectionClass;
1921
use ReflectionMethod;
2022
use ReflectionParameter;
2123
use ReflectionProperty;
@@ -367,41 +369,40 @@ private function getTypeFromRegistry(string $type, bool $isEntityId): Type
367369
/**
368370
* Input with default values cannot be non-null
369371
*
370-
* @param Type $type
371-
* @param mixed $defaultValue
372-
*
373-
* @return Type
372+
* @param AbstractAnnotation $annotation
374373
*/
375-
protected function nonNullIfHasDefault(?Type $type, $defaultValue): ?Type
374+
protected function nonNullIfHasDefault(AbstractAnnotation $annotation): void
376375
{
377-
if ($type instanceof NonNull && $defaultValue !== null) {
378-
return $type->getWrappedType();
376+
$type = $annotation->getTypeInstance();
377+
if ($type instanceof NonNull && $annotation->hasDefaultValue()) {
378+
$annotation->setTypeInstance($type->getWrappedType());
379379
}
380-
381-
return $type;
382380
}
383381

384382
/**
385383
* Throws exception if argument type is invalid
386384
*
387385
* @param ReflectionParameter $param
388-
* @param Type $type
389-
* @param string $annotation
386+
* @param AbstractAnnotation $annotation
390387
*
391388
* @throws Exception
392389
*/
393-
protected function throwIfNotInputType(ReflectionParameter $param, ?Type $type, string $annotation): void
390+
protected function throwIfNotInputType(ReflectionParameter $param, AbstractAnnotation $annotation): void
394391
{
392+
$type = $annotation->getTypeInstance();
393+
$class = new ReflectionClass($annotation);
394+
$annotationName = $class->getShortName();
395+
395396
if (!$type) {
396-
throw new Exception('Could not find type for parameter `$' . $param->name . '` for method ' . $this->getMethodFullName($param->getDeclaringFunction()) . '. Either type hint the parameter, or specify the type with `@API\\' . $annotation . '` annotation.');
397+
throw new Exception('Could not find type for parameter `$' . $param->name . '` for method ' . $this->getMethodFullName($param->getDeclaringFunction()) . '. Either type hint the parameter, or specify the type with `@API\\' . $annotationName . '` annotation.');
397398
}
398399

399400
if ($type instanceof WrappingType) {
400401
$type = $type->getWrappedType(true);
401402
}
402403

403404
if (!($type instanceof InputType)) {
404-
throw new Exception('Type for parameter `$' . $param->name . '` for method ' . $this->getMethodFullName($param->getDeclaringFunction()) . ' must be an instance of `' . InputType::class . '`, but was `' . get_class($type) . '`. Use `@API\\' . $annotation . '` annotation to specify a custom InputType.');
405+
throw new Exception('Type for parameter `$' . $param->name . '` for method ' . $this->getMethodFullName($param->getDeclaringFunction()) . ' must be an instance of `' . InputType::class . '`, but was `' . get_class($type) . '`. Use `@API\\' . $annotationName . '` annotation to specify a custom InputType.');
405406
}
406407
}
407408
}

0 commit comments

Comments
 (0)