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

introduce requireAllRequiredProperties context #1554

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

idbentley
Copy link
Contributor

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
Copy link
Contributor Author

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

@idbentley idbentley mentioned this pull request Jul 26, 2024
@goetas
Copy link
Collaborator

goetas commented Aug 21, 2024

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

@idbentley idbentley force-pushed the deserialize-required-properties branch from e334a2b to d8c5ee5 Compare August 26, 2024 13:51
@idbentley idbentley force-pushed the deserialize-required-properties branch from 009bbdf to 4bad485 Compare August 26, 2024 14:14
@idbentley
Copy link
Contributor Author

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
Copy link
Collaborator

goetas commented Aug 27, 2024

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
Copy link
Contributor Author

idbentley commented Aug 27, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@idbentley
Copy link
Contributor Author

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

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

Successfully merging this pull request may close these issues.

2 participants