[Serializer] Add flag to require all properties to be listed in the input

[christian-kolb]

Q	A
Branch?	6.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #49504
License	MIT
Doc PR	symfony/symfony-docs#17979

The PR #40522 introduced a fallback for nullable properties to be set to null when they aren't provided as parameters.

A drawback of that approach is that it easier for bugs to appear through typos or renamings of those properties. I think the current implementation makes perfect sense as a default. Therefore, this PR introduces a new context flag that prevents that fallback behaviour. This way nothing changes for existing systems, but for people wanting more control, it's possible to set a flag.

Example

final class Product
{
    public function __construct(
        public string $name,
        public ?int $costsInCent,
    ) {
    }
}

// This works and results in $costsInCent as null
$product = $this->serializer->deserialize(
    '{"name": "foo"}', 
    Product::class, 
    JsonEncoder::FORMAT,
);

// When using the flag, only the following JSON is valid
$product = $this->serializer->deserialize(
    '{"name": "foo", "costsInCent": null}',
    Product::class,
    JsonEncoder::FORMAT,
    [
        AbstractNormalizer::PREVENT_NULLABLE_FALLBACK => true,
    ],
);

// This would result in an error due to missing parameters
$product = $this->serializer->deserialize(
    '{"name": "foo"}',
    Product::class,
    JsonEncoder::FORMAT,
    [
        AbstractNormalizer::PREVENT_NULLABLE_FALLBACK => true,
    ],
);

[carsonbot]

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 6.3 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

[christian-kolb]

I don't think the last failed test run had anything to do with this PR. The first time the unit tests run through (only the Psalm run was cancelled) and the only change made afterwards was in the CHANGELOG.md.

[christian-kolb]

@dunglas The history says that I removed a request for you to review the PR. I don' think I did any I don't even think it's possible for me to do so. In any way I can't request a new one. Is this part of the normal process? Sorry, for the stupid questions, it's my first contribution.

[christian-kolb]

@maxbeckers @dunglas This is my first PR. The status is now reviewed. What is left to do for the PR to be merged? Anything I can help with? 🙂

[maxbeckers]

@christian-kolb no todos for you ... will be reviewed and merged as soon as possible

[christian-kolb]

@mtarld @maxbeckers Sorry to bother you guys. I'm getting worried about whether this pull request is getting merged as the 6.3 Milestone is very close now. Who is responsible for merging the PRs and will do this? 🙂

[ro0NL]

i tend to believe for this signature:

    public function __construct(
        public string $name,
        public ?int $costsInCent,
    ) {
    }

null is something you'd have to expect, thus a safe default

to me it hints a NotNull constraint might be missing, or simply drop the nullability

what should be doable is detecting uninitialized (aka not given) without using constructor promotion:

final class Product
{
    public string $name;
    public ?int $requiredNullable;
}

in this case you could detect "given" properties from initialized properties, thus {"name": "foo"} vs {"name": "foo", "requiredNullable": null} vs {"name": "foo", "requiredNullable": 123}

[christian-kolb]

@ro0NL Sorry, I don't think I understand what you want to say.

null is something you'd have to expect, thus a safe default

That's what would still work out of the box with for example the Property Normalizer.

The only thing changing through the PR is that you can supply an additional context to configure that you expect null to be present and throw an error when it's not supplied.

[ro0NL]

pesonally i dont see any value in enforcing {"name": "foo", "costsInCent": null} over {"name": "foo"}

[christian-kolb]

The value is the option to prevent bugs.
Setting a default for a value just because it's nullable is a fallback and a little magic. For example:

We have a class with the property ?int $costsInCent. And the payload would look like this {"costsInCent": 5000}.
Now I rename the property to ?int $totalCostsInCent but forget to update the client and still send {"costsInCent": 5000}.
The API will never inform me of any problems. It doesn't care that there's a payload which doesn't seem to be relevant and that there's a missing property it expects. Because it's nullable it's simply set to null.

This could be covered by more tests, more validation, ... but all of those are then just replacements for the missing control I would also already have with typing.

Personally I think the current behaviour is to loose. There is a concept for missing properties and that's optional values. That could also be typed like ?int $costsInCent = null. But I get the fallback and the default and don't mind that. I just would like the option to increase the strictness when necessary.

In the end the discussion about this is the same as with using typing or not.

I have had exactly the problems described here multiple times in multiple projects. So this is just a way to eliminate one bug category 🙂

[ro0NL]

Right! What about something similar like forms do: https://symfony.com/doc/current/reference/forms/types/form.html#allow-extra-fields

[christian-kolb]

That's an awesome idea. That would fix the second part of my example I didn't even think to solve yet.

The two problems in my example are:

1. There is a value for a property expected, that isn't supplied
2. There is an additional value supplied, that is not needed on the current object level

There is a value for a property expected, that isn't supplied

This could happen through:

• A client not implementing a new endpoint correctly.
• A client renaming something without changing the endpoint.
• A client not implementing a new property.

It would be more likely to catch those issues when an endpoint is only used in one place, but it's possible that it's used in for example 5 places in the client but only adapted in 4 of them. Which becomes more relevant, when we're not talking about properties on the first level, but the second or third what we might have with value objects (I'm using those a lot).

There is an additional value supplied, that is not needed on the current object level

This could happen through:

• A client not implementing a new endpoint correctly.
• A client renaming something without changing the endpoint.

---

And we must not forget that the serializer is not just used for client / API combinations. I for example use it also as part of custom doctrine types to store and retrieve value objects in JSON. Or to return read models directly from custom queries (again with multiple levels of object structures) or to dump and retrieve aggregates in an event sourcing context for snapshotting.

This pull requests solves the first of the described problems. I'm not sure if the Serializer structure allowed for an allow_extra_fields configuration, as it's based primarily on the data it would denormalize (at least I think it is). But I will look into it and supply a separate pull request to solve the second batch of problems. Thanks for the input 🙂

[christian-kolb]

@chalasr @nicolas-grekas Sorry to bother you. It's my first contribution to Symfony and I don't know how to get this merged. I'm hanging here for two months. Could you please help me here or point me to someone who can?

[dunglas]

The implementation looks good to me. I'm not sure about the name of the option though. Maybe FORCE_PASS_ALL_PROPS? Or NO_DEFAULT_VALUES?

[christian-kolb]

The implementation looks good to me. I'm not sure about the name of the option though. Maybe FORCE_PASS_ALL_PROPS? Or NO_DEFAULT_VALUES?

@dunglas Thanks for the additional approval. I'm not 100% about the naming myself. But I don't think your suggestions are better. FORCE_PASS_ALL_PROPS looks more like the allow_extra_fields idea from the last comment which I would like to supply as a separate PR and I would interpret NO_DEFAULT_VALUES as the a flag that would ignore property defaults likebool $isEnabled = true. It took me an hour just to land on PREVENT_NULLABLE_FALLBACK.

My thoughts where:

• It has to change the default behaviour (that does something special). (prevent)
• It only applies to nullable properties. (nullable)
• It only happens when they are not supplied. (fallback)

It would be way easier with the nullable fallback being the default 🙂

What just came to mind: Would it be reasonable to switch the logic. So that the default of the context is true and only with it the fallback would be used? Then I could change the name to ALLOW_NULLABLE_FALLBACK. This could then be set to false to have the same result. But it would also kind of question the default used now and I'm not sure it can go this far.

I really don't want to make it miss the 6.3 milestone because of it. Anyone around who could bless that change?

[christian-kolb]

@dunglas Ok, sorry scratch that. The flags have to be provided specifically and there is not a single context flag that works like that. So that's no way ether. Still happy to hear other namings 🙂

Otherwise very happy when it would get merged 🙂

[mtarld]

I'd go with ALLOW_NULLABLE_FALLBACK, it does make sense to me indeed.

[christian-kolb]

@mtarld That's unfortunately not possible. That's what I meant in my last comment. ALLOW_NULLABLE_FALLBACK is the current behaviour. So introducing a flag wouldn't change anything. Or would change the current behaviour which would be a breaking change. And that is if the relevant people agree that the default behaviour should change.
I don't mind introducing a second PR that targets Symfony 7 and switches the default. But it's not possible for the 6.3 to change it that way.

[nicolas-grekas]

REQUIRE_ALL_PROPERTIES?
STRICT_PROPERTIES?

[mtarld]

Sounds great yes

[christian-kolb]

@nicolas-grekas @mtarld Ok, then I will change it to REQUIRE_ALL_PROPERTIES

[christian-kolb]

@nicolas-grekas @mtarld Ok, then I will change it to REQUIRE_ALL_PROPERTIES

@nicolas-grekas Updated it to REQUIRE_ALL_PROPERTIES. The related documentation PR is also already updated.
The test failures don't seem to be related to my changes.

Anything left for me to do to get it merged and released with 6.3? 🙂

[nicolas-grekas]

LGTM, just minor things

[nicolas-grekas]

Thank you @christian-kolb.