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

Prevent deserialisation with missing required field #1090

Closed
bdsl opened this issue Jun 3, 2019 · 5 comments
Closed

Prevent deserialisation with missing required field #1090

bdsl opened this issue Jun 3, 2019 · 5 comments

Comments

@bdsl
Copy link
Contributor

bdsl commented Jun 3, 2019

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no

When deserialising an object, it would be useful to know that all required fields are present,
and the caller hasn't forgotten to set them. I think it might make sense to introduce new
annotations @Serializer\Required and @Serializer\Optional, with a config option to set which one is assumed by default. Optional would be the same as current behaviour.

Related to #821

Steps required to reproduce the problem

Run the following script:

<?php

declare(strict_types=1);
use Doctrine\Common\Annotations\AnnotationRegistry;
use JMS\Serializer\Annotation as Serializer;

$loader = require_once (__DIR__ .'/../vendor/autoload.php');

AnnotationRegistry::registerLoader([$loader, "loadClass"]);

class exampleDto
{
    /** @Serializer\Type("string") */
    public $someProperty;
}

$serializer = JMS\Serializer\SerializerBuilder::create()->build();

$object = $serializer->deserialize(\json_encode([]), exampleDto::class, 'json');

var_dump($object->someProperty);

Expected Result

deserialize should throw some exception if $someProperty is annotated as required, or the serializer is configured to treat properties as required by default

Actual Result

NULL

@bdsl
Copy link
Contributor Author

bdsl commented Jun 3, 2019

This probably could be achieved in the client of this libaray by setting a default pre_deserialize or post_deserialize listener which would reflect on the class and check the data against the annotations.

But I suspect that would be less performant than building the check in to deserialize when annotations have already been read.

@goetas
Copy link
Collaborator

goetas commented Jun 4, 2019

Validation of the deserialized object is a rather complex topic, better to use libs as symfony/validator for such kind of tasks. Adding @Serializer\Required, or @Serializer\Optional or other will lead to adding a bunch of features all more or less similar to what symfony/validator already does

@repli2dev
Copy link

Late to the party, but I disagree with this Slippery slope argument. The reason for Required/Optional requirement inside library is the fact that there is a developer expectation that if I deserialize a DTO I either get a valid DTO or error and no DTO -- here valid means that the encapsulation is not broken, which is by quite broken by leaving random fields uninitialized :/

@scyzoryck
Copy link
Collaborator

Hi!
@repli2dev - can you check using JsonDeserializationStrictVisitor - I guess it should solve the issues with uninitialised data.  

@repli2dev
Copy link

@scyzoryck

I have tried

$serializer = JMS\Serializer\SerializerBuilder::create()
    ->setDeserializationVisitor('json', new \JMS\Serializer\Visitor\Factory\JsonDeserializationVisitorFactory(true))->build();

and it still gets uninitialized when missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants