introduce requireAllRequiredProperties context

[idbentley]

Q	A
Bug fix?	no
New feature?	yes
Doc updated	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#...
License	MIT

This pull request introduces a new DeserializationContext attribute that allows you to specify that you'd like deserialization to fail if the JSON is missing required properties.

[idbentley]

@goetas this is the second half of #1549 allowing a workaround for deserializing non-discriminated unions.

[goetas]

@idbentley is this still needed given the state of the union discriminator feature recently merged?

[idbentley]

Hi @goetas yes, I still need these changes. They are only incidentally related to unions.

This pull request introduces a DeserializationContext that causes deserialization to fail when non-nullable fields are missing from the incoming JSON.

As a simple example:

use JMS\Serializer\Annotation\Type;

class Author
{
    #[Type(name: 'string')]
    private $name;

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

    public function getName()
    {
        return $this->name;
    }
}

Since the name field is non nullable, so if we attempt to deserialize an empty JSON object, deserialization should fail:

{}

With this PR in place, and the appropriate Context provided:

$this->deserialize('{}', Author::class, DeserializationContext::create()->setRequireAllRequiredProperties(true))

The serializer will throw an PropertyMissingException.

[goetas]

This feature is crossing the delicate ground between serialization and validation. In the past I had similar proposals and I always tried to keep them outside of the serializer because is validation is a complex thing (see just how big is the symfony validator comoponent).

because of it I would really like to keep this outside of the serializer

[idbentley]

I agree that increasing the complexity of validation in the serializer is a slippery slope, but I would like to argue that this is actually more of a type check consideration, which the serializer already does.

If I try to deserialize:

class SimpleObject
{
    /**
     * @Type("string")
     */
    #[Type(name: 'string')]
    private $foo;
}

With the wrong data type:

{"foo": 123}

The system will throw an exception: RuntimeException: Type int cannot be visited as string.

This is much the same - the expected data type is Non nullable, and the visited type is null (or unset which amounts to the same).

While it would be possible for me to add a validator to the json data, I would be doing that solely for this null type check, and it feels very onerous to add a whole validation system to support a Type Error.

[idbentley]

@goetas I'm gonna take another shot at convincing you that this is worth keeping in the seializer.

1. As I've pointed out before - it is consistent with the other type of "validation" in the serializer - it only validates that the type of data matches the specification.
2. It doesn't introduce much complexity - the majority of the lines of code in the PR are testing & documentation
3. The alternative is both complex and unperformant. In order to accomplish the same task outside of the serializer I need to:
   a) parse the JSON a second time
   b) introduce a validation library, and populate it with the annotations/metadata to match jms/serializer annotations
   c) iterate the JSON tree a second time, validating each node

I hope you'll reconsider :)

[idbentley]

@goetas I wonder if you'd had a chance to consider this?

[idbentley]

Hi @goetas Have you had a chance to read my posts?

[goetas]

I would really like to keep the graph visitor as simple as possible (and to keep validation outside of the serializer), so I'm going to close this pull request (I owe you a clear action, without gigling around it).

[goetas]

Long time ago i worked on #1005 to handle explicitly null... then i worked also on other stuff that never made it to github (like a @Required annotation), but every time i tried it was a can of worms.

[idbentley]

I would really like to keep the graph visitor as simple as possible (and to keep validation outside of the serializer)

This statement is very confusing to me. The graph visitor already does type validation. The current behaviour of the deserializer is inconsistent and it's handling of null values is a type checking bug. The serializer type checks of every piece of data it deserializes except null values. Deserializing a null value into an integer should be a type failure, they are not the same type.

I put the bug fix behind a configuration flag because I didn't want to introduce a regression, but this PR is a bug fix.

It's a shame that you close it without any justification beyond "too complicated".