8000 [Serializer] deserialization_path missing in custom DenormalizerInterface upon using object's constructor · Issue #44925 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Serializer] deserialization_path missing in custom DenormalizerInterface upon using object's constructor #44925

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

Jump to bottom
Closed
szymat opened this issue Jan 5, 2022 · 7 comments
Closed

[Serializer] deserialization_path missing in custom DenormalizerInterface upon using object's constructor #44925

szymat opened this issue Jan 5, 2022 · 7 comments
Labels
Bug Serializer Status: Needs Review

Comments

@szymat
Copy link
szymat commented Jan 5, 2022

Symfony version(s) affected

5.4 and above

Description

In denormalizing to object by constructor, symfony serializer is not sending deserialization_path in $context.
In AbstractObjectNormalizer there is foreach which goes through all attributes and creating context (symfony/serializer/Normalizer/AbstractObjectNormalizer.php:374)

foreach ($normalizedData as $attribute => $value) {
$attributeContext = $this->getAttributeDenormalizationContext($resolvedClass, $attribute, $context);

private function getAttributeDenormalizationContext(string $class, string $attribute, array $context): array
{
    // (...)
     $context['deserialization_path'] = ($context['deserialization_path'] ?? false) ? $context['deserialization_path'].'.'.$attribute : $attribute;
    // (...)
}

And it works fine when we want to denormalize by properties etc.
Problem occurs if object has only constructor available, then Serializer fires AbstractNormalizer.
In AbstractNormalizer there is foreach which goes through construct arguments
(symfony/serializer/Normalizer/AbstractNormalizer.php:362)

foreach ($constructorParameters as $constructorParameter) {
    $paramName = $constructorParameter->name;
    $key = $this->nameConverter ? $this->nameConverter->normalize($paramName, $class, $format, $context) : $paramName;

key would be equivalent to attribute from AbstractObjectNormalizer, hence the question why not add key to context as deserialization_path as construct arguments needs to be same as class attributes.
When you want to use Collecting Type Errors While Denormalizing path is empty, so you cannot return info which property had incorrect type.

How to reproduce

To reproduce you will need serializer, annotations and property access

composer require symfony/serializer
composer require doctrine/annotations
composer require symfony/property-access

example.php:

<?php
// composer require symfony/serializer
// composer require doctrine/annotations
// composer require symfony/property-access

use Doctrine\Common\Annotations\AnnotationReader;
use Symfony\Component\Serializer\Exception\NotNormalizableValueException;
use Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactory;
use Symfony\Component\Serializer\Mapping\Loader\AnnotationLoader;
use Symfony\Component\Serializer\Normalizer\DenormalizerInterface;
use Symfony\Component\Serializer\Normalizer\ObjectNormalizer;
use Symfony\Component\Serializer\Serializer;
use Symfony\Component\Serializer\Encoder\JsonEncoder;

require 'vendor/autoload.php';

class ExampleObject
{
    private int $number;

    public function __construct(int $number)
    {
        $this->number = $number;
    }
}

class ExampleDTO
{
    private ExampleObject $exampleObject;

    public function __construct(ExampleObject $exampleObject)
    {
        $this->exampleObject = $exampleObject;
    }
}

$classMetadataFactory = new ClassMetadataFactory(new AnnotationLoader(new AnnotationReader()));
$serializer = new Serializer([new ObjectNormalizer($classMetadataFactory)],['json' => new JsonEncoder()]);

try {
    $data = $serializer->deserialize(
        '{"exampleObject":"test"}',
        ExampleDTO::class,
        'json',
        [
            DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS => true
        ]
    );
    print_r($data);
} catch (\Symfony\Component\Serializer\Exception\PartialDenormalizationException $e) {
    // as seen at https://symfony.com/doc/5.4/components/serializer.html#collecting-type-errors-while-denormalizing
    /** @var NotNormalizableValueException */
    foreach ($e->getErrors() as $exception) {
        echo sprintf('The type must be one of "%s" ("%s" given). in: "%s"',
                implode(', ', $exception->getExpectedTypes()),
                $exception->getCurrentType(),
                $exception->getPath()
            ) . PHP_EOL;
        // Outputs: The type must be one of "unknown" ("array" given). in: "" <- no path and expected type is unknown
    }
}

Above example will output The type must be one of "unknown" ("array" given). in: "" <- no path and expected type is unknown

Possible Solution

For the path in \Symfony\Component\Serializer\Normalizer\AbstractNormalizer::instantiateObject add at the beginning of constructor arguments foreach or same context build as in AbstractObjectNormalizer:

$context['deserialization_path'] = $key;

As for the "unknown" I have not looked into it.

Additional Context

I use DTOs in my project which have custom denormalizers which shows same issue.
Also I have noticed issue when collecting errors that despite mismatch of constructor arguments Serializer Component will try to create object resulting in \Symfony\Component\Serializer\Exception\ExceptionInterface when using custom Denormalizer which throws NotNormalizableValueException in denormalize method and I think it is a bit odd.
@szymat szymat added the Bug label Jan 5, 2022
@marliotto
Copy link

The same problem, when I use CamelCaseToSnakeCaseNameConverter. deserialization_path missing the full path when child attribute has name in snake_case, but property of object in camelCase.
I see the problem in

symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

Lines 258 to 266 in 22fd05a

private function getAttributeDenormalizationContext(string $class, string $attribute, array $context): array
{
if (null === $metadata = $this->getAttributeMetadata($class, $attribute)) {
return $context;
}
$context['deserialization_path'] = ($context['deserialization_path'] ?? false) ? $context['deserialization_path'].'.'.$attribute : $attribute;
return array_merge($context, $metadata->getDenormalizationContextForGroups($this->getGroups($context)));

I think it will be ok if deserialization_path will be built before checking metadata.
For example:

private function getAttributeDenormalizationContext(string $class, string $attribute, array $context): array
{
    $context['deserialization_path'] = ($context['deserialization_path'] ?? false) ? $context['deserialization_path'].'.'.$attribute : $attribute;

    if (null === $metadata = $this->getAttributeMetadata($class, $attribute)) {
        return $context;
    }

    return array_merge($context, $metadata->getDenormalizationContextForGroups($this->getGroups($context)));
}

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@majermi4
Copy link

I'm experiencing related issue with attribute Symfony\Component\Serializer\Annotation\Context being ignored during deserialization when object has constructor with promoted properties.

    public function __construct(
        #[Symfony\Component\Serializer\Annotation\Context(context: ['foo' => 'bar'])]
        public MyClass $someObject
    ) {
    }

In this example, the context data is not used during denormalization process.

I think that both my issue and the issue in this ticket could be solved by adding this one line:
$childContext = $this->getAttributeDenormalizationContext($currentClass, $attribute, $context);
to AbstractObjectNormalizer on line 557.

https://github.com/symfony/serializer/blob/6.1/Normalizer/AbstractObjectNormalizer.php#L556-L559
8000

@carsonbot carsonbot removed the Stalled label Jul 15, 2022
@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@szymat
Copy link
Author
szymat commented Mar 2, 2023

Its still relevant, constructor and serializer do not go in pair.

@carsonbot carsonbot removed the Stalled label Mar 2, 2023
@zavitkov
Copy link

Still relevant. What are the possible solutions?

@mtarld
Copy link
Contributor
mtarld commented Nov 22, 2023 •
edited
Loading

In Symfony 6.3, the issue doesn't exist because it has been cover by #46680, but I'll a little fix for 5.4

@mtarld mtarld mentioned this issue Nov 22, 2023
Merged
nicolas-grekas added a commit that referenced this issue Nov 23, 2023
@nicolas-grekas
bug #52683 [Serializer] Fix constructor deserialization path (mtarld)
a560853 
This PR was merged into the 5.4 branch.

Discussion
----------

[Serializer] Fix constructor deserialization path

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #44925
| License       | MIT

This fix doesn't have to be upmerged because it already has been covered by #46680.

Commits
-------

272bc28 [Serializer] Fix constructor deserialization path
@mtarld mtarld mentioned this issue Nov 24, 2023
Merged
xabbuh added a commit that referenced this issue Nov 24, 2023
@xabbuh
bug #52713 [Serializer] Fix deserialization_path missing using contru…
cd98a3f 
…ctor (mtarld)

This PR was merged into the 5.4 branch.

Discussion
----------

[Serializer] Fix deserialization_path missing using contructor

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprec
5E84
ations? | no
| Issues        | Fix #44925 #52683
| License       | MIT

While trying to fix #44925, I used a wrong approach (#52683), and therefore introduced a bug.

This PR fixes it and solve the previous issue in a better way.

Commits
-------

8a33f53 [Serializer] Fix deserialization_path missing using contructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Serializer Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@nicolas-grekas @marliotto @majermi4 @mtarld @carsonbot @szymat @zavitkov
0