Skip to content

Constructor relations writtable #2178

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

Merged
merged 6 commits into from
Aug 31, 2018
Merged

Constructor relations writtable #2178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e1733fd
test relations with writable constructor parameters
komik966 Aug 26, 2018
4309ffe
in swagger mark property as readOnly when is not writable and initial…
komik966 Aug 26, 2018
4a80efa
mark method internal
komik966 Aug 29, 2018
508e6ee
fix typo in test
komik966 Aug 29, 2018
34cd246
Merge branch 'master' into constructor-relations-writtable
mysiar Aug 30, 2018
a143f0b
Merge branch 'master' into constructor-relations-writtable
mysiar Aug 31, 2018
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
187 changes: 187 additions & 0 deletions features/serializer/vo_relations.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
Feature: Value object as ApiResource
In order to keep ApiResource immutable
As a client software developer
I need to be able to use class without setters as ApiResource

@createSchema
Scenario: Create Value object resource
When I add "Content-Type" header equal to "application/ld+json"
And I send a "POST" request to "/vo_dummy_cars" with body:
"""
{
"mileage": 1500,
"bodyType": "suv",
"make": "CustomCar",
"insuranceCompany": {
"name": "Safe Drive Company"
},
"drivers": [
{
"firstName": "John",
"lastName": "Doe"
}
]
}
"""
Then the response status code should be 201
And the JSON should be equal to:
"""
{
"@context": "/contexts/VoDummyCar",
"@id": "/vo_dummy_cars/1",
"@type": "VoDummyCar",
"mileage": 1500,
"bodyType": "suv",
"inspections": [],
"make": "CustomCar",
"insuranceCompany": {
"@id": "/vo_dummy_insurance_companies/1",
"@type": "VoDummyInsuranceCompany",
"name": "Safe Drive Company"
},
"drivers": [
{
"@id": "/vo_dummy_drivers/1",
"@type": "VoDummyDriver",
"firstName": "John",
"lastName": "Doe"
}
]
}
"""
And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"

Scenario: Create Value object with IRI and nullable parameter
When I add "Content-Type" header equal to "application/ld+json"
And I send a "POST" request to "/vo_dummy_inspections" with body:
"""
{
"accepted": true,
"car": "/vo_dummy_cars/1"
}
"""
Then the response status code should be 201
And the JSON should be valid according to this schema:
"""
{
"type": "object",
"required": ["accepted", "performed", "car"],
"properties": {
"accepted": {
"enum":[true]
},
"performed": {
"format": "date-time"
},
"car": {
"enum": ["/vo_dummy_cars/1"]
}
}
}
"""

Scenario: Update Value object with writable and non writable property
When I add "Content-Type" header equal to "application/ld+json"
And I send a "PUT" request to "/vo_dummy_inspections/1" with body:
"""
{
"performed": "2018-08-24 00:00:00",
"accepted": false
}
"""
Then the response status code should be 200
And the JSON should be equal to:
"""
{
"@context": "/contexts/VoDummyInspection",
"@id": "/vo_dummy_inspections/1",
"@type": "VoDummyInspection",
"accepted": true,
"car": "/vo_dummy_cars/1",
"performed": "2018-08-24T00:00:00+00:00",
"id": 1
}
"""

@createSchema
Scenario: Create Value object without required params
When I add "Content-Type" header equal to "application/ld+json"
And I send a "POST" request to "/vo_dummy_cars" with body:
"""
{
"mileage": 1500,
"make": "CustomCar",
"insuranceCompany": {
"name": "Safe Drive Company"
}
}
"""
Then the response status code should be 400
And the JSON should be valid according to this schema:
"""
{
"type": "object",
"properties": {
"@context": {
"enum": ["/contexts/Error"]
},
"type": {
"enum": ["hydra:Error"]
},
"hydra:title": {
"enum": ["An error occurred"]
},
"hydra:description": {
"enum": ["Cannot create an instance of ApiPlatform\\Core\\Tests\\Fixtures\\TestBundle\\Entity\\VoDummyCar from serialized data because its constructor requires parameter \"drivers\" to be present."]
}
}
}
"""
And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"

@createSchema
Scenario: Create Value object without default param
When I add "Content-Type" header equal to "application/ld+json"
And I send a "POST" request to "/vo_dummy_cars" with body:
"""
{
"mileage": 1500,
"make": "CustomCar",
"insuranceCompany": {
"name": "Safe Drive Company"
},
"drivers": [
{
"firstName": "John",
"lastName": "Doe"
}
]
}
"""
Then the response status code should be 201
And the JSON should be equal to:
"""
{
"@context": "/contexts/VoDummyCar",
"@id": "/vo_dummy_cars/1",
"@type": "VoDummyCar",
"mileage": 1500,
"bodyType": "coupe",
"inspections": [],
"make": "CustomCar",
"insuranceCompany": {
"@id": "/vo_dummy_insurance_companies/1",
"@type": "VoDummyInsuranceCompany",
"name": "Safe Drive Company"
},
"drivers": [
{
"@id": "/vo_dummy_drivers/1",
"@type": "VoDummyDriver",
"firstName": "John",
"lastName": "Doe"
}
]
}
"""
And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"
123 changes: 101 additions & 22 deletions src/Serializer/AbstractItemNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
use Symfony\Component\PropertyInfo\Type;
use Symfony\Component\Serializer\Exception\MissingConstructorArgumentsException;
use Symfony\Component\Serializer\Exception\RuntimeException;
use Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactoryInterface;
use Symfony\Component\Serializer\NameConverter\NameConverterInterface;
use Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer;
Expand Down Expand Up @@ -127,6 +129,93 @@ public function denormalize($data, $class, $format = null, array $context = [])
return parent::denormalize($data, $class, $format, $context);
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

Please mark it as @internal because we'll remove it at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* Method copy-pasted from symfony/serializer.
* Remove it after symfony/serializer version update @link https://github.com/symfony/symfony/pull/28263.
*
* {@inheritdoc}
*
* @internal
*/
protected function instantiateObject(array &$data, $class, array &$context, \ReflectionClass $reflectionClass, $allowedAttributes, string $format = null)
{
if ($this->classDiscriminatorResolver && $mapping = $this->classDiscriminatorResolver->getMappingForClass($class)) {
if (!isset($data[$mapping->getTypeProperty()])) {
throw new RuntimeException(sprintf('Type property "%s" not found for the abstract object "%s"', $mapping->getTypeProperty(), $class));
}

$type = $data[$mapping->getTypeProperty()];
if (null === ($mappedClass = $mapping->getClassForType($type))) {
throw new RuntimeException(sprintf('The type "%s" has no mapped class for the abstract object "%s"', $type, $class));
}

$class = $mappedClass;
$reflectionClass = new \ReflectionClass($class);
}

if (null !== $object = $this->extractObjectToPopulate($class, $context, static::OBJECT_TO_POPULATE)) {
unset($context[static::OBJECT_TO_POPULATE]);

return $object;
}

$constructor = $this->getConstructor($data, $class, $context, $reflectionClass, $allowedAttributes);
if ($constructor) {
$constructorParameters = $constructor->getParameters();

$params = [];
foreach ($constructorParameters as $constructorParameter) {
$paramName = $constructorParameter->name;
$key = $this->nameConverter ? $this->nameConverter->normalize($paramName) : $paramName;

$allowed = false === $allowedAttributes || \in_array($paramName, $allowedAttributes, true);
$ignored = !$this->isAllowedAttribute($class, $paramName, $format, $context);
if ($constructorParameter->isVariadic()) {
if ($allowed && !$ignored && (isset($data[$key]) || array_key_exists($key, $data))) {
if (!\is_array($data[$paramName])) {
throw new RuntimeException(sprintf('Cannot create an instance of %s from serialized data because the variadic parameter %s can only accept an array.', $class, $constructorParameter->name));
}

$params = array_merge($params, $data[$paramName]);
}
} elseif ($allowed && !$ignored && (isset($data[$key]) || array_key_exists($key, $data))) {
$params[] = $this->createConstructorArgument($data[$key], $key, $constructorParameter, $context, $format);

// Don't run set for a parameter passed to the constructor
unset($data[$key]);
} elseif (isset($context[static::DEFAULT_CONSTRUCTOR_ARGUMENTS][$class][$key])) {
$params[] = $context[static::DEFAULT_CONSTRUCTOR_ARGUMENTS][$class][$key];
} elseif ($constructorParameter->isDefaultValueAvailable()) {
$params[] = $constructorParameter->getDefaultValue();
} else {
throw new MissingConstructorArgumentsException(
sprintf(
'Cannot create an instance of %s from serialized data because its constructor requires parameter "%s" to be present.',
Copy link
Contributor

Choose a reason for hiding this comment

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

can this error be more consumer friendly on serialization? it's exposing implementation detail to the consumer which they don't care about: the class name, overly verbose message etc... all the consumer needs to know is that they didn't provide the some parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code comes from symfony/serializer (see method comment). So I'm not sure if we should change it. But I've similar opinion. Ideally would be to show such errors the same way as validation errors (as map: property => error message). I think that non nullable and without default value constructor parameters are equivalent to class attributes with NotBlank constraint (with some exceptions) and errors should be presented the same way.

$class,
$constructorParameter->name
)
);
}
}

if ($constructor->isConstructor()) {
return $reflectionClass->newInstanceArgs($params);
}

return $constructor->invokeArgs(null, $params);
}

return new $class();
}

/**
* {@inheritdoc}
*/
protected function createConstructorArgument($parameterData, string $key, \ReflectionParameter $constructorParameter, array &$context, string $format = null)
{
return $this->createAttributeValue($constructorParameter->name, $parameterData, $format, $context);
}

/**
* {@inheritdoc}
*
Expand Down Expand Up @@ -167,6 +256,11 @@ protected function getAllowedAttributes($classOrObject, array $context, $attribu
* {@inheritdoc}
*/
protected function setAttributeValue($object, $attribute, $value, $format = null, array $context = [])
{
$this->setValue($object, $attribute, $this->createAttributeValue($attribute, $value, $format, $context));
}

private function createAttributeValue($attribute, $value, $format = null, array $context = [])
{
if (!\is_string($attribute)) {
throw new InvalidValueException('Invalid value provided (invalid IRI?).');
Expand All @@ -176,44 +270,29 @@ protected function setAttributeValue($object, $attribute, $value, $format = null
$type = $propertyMetadata->getType();

if (null === $type) {
// No type provided, blindly set the value
$this->setValue($object, $attribute, $value);

return;
// No type provided, blindly return the value
return $value;
}

if (null === $value && $type->isNullable()) {
$this->setValue($object, $attribute, $value);

return;
return $value;
}

if (
$type->isCollection() &&
null !== ($collectionValueType = $type->getCollectionValueType()) &&
null !== $className = $collectionValueType->getClassName()
) {
$this->setValue(
$object,
$attribute,
$this->denormalizeCollection($attribute, $propertyMetadata, $type, $className, $value, $format, $context)
);

return;
return $this->denormalizeCollection($attribute, $propertyMetadata, $type, $className, $value, $format, $context);
}

if (null !== $className = $type->getClassName()) {
$this->setValue(
$object,
$attribute,
$this->denormalizeRelation($attribute, $propertyMetadata, $className, $value, $format, $this->createChildContext($context, $attribute))
);

return;
return $this->denormalizeRelation($attribute, $propertyMetadata, $className, $value, $format, $this->createChildContext($context, $attribute));
}

$this->validateType($attribute, $type, $value, $format);
$this->setValue($object, $attribute, $value);

return $value;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/Swagger/Serializer/DocumentationNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ private function getPropertySchema(PropertyMetadata $propertyMetadata, \ArrayObj
{
$propertySchema = new \ArrayObject($propertyMetadata->getAttributes()['swagger_context'] ?? []);

if (false === $propertyMetadata->isWritable()) {
if (false === $propertyMetadata->isWritable() && !$propertyMetadata->isInitializable()) {
$propertySchema['readOnly'] = true;
}

Expand Down
Loading