Allow deserialization of nullable arrays#108
Conversation
agustingomes
force-pushed
the
issues/107
branch
from
b5253e6 to
cb8a519
Compare
agustingomes
force-pushed
the
issues/107
branch
2 times, most recently
from
ef2d951 to
b2358fc
Compare
The following minimal reproduction case currently ends in failure:
```php
<?php
declare(strict_types=1);
use Crell\Serde\Attributes\SequenceField;
use Crell\Serde\SerdeCommon;
require_once dirname(__DIR__) . '/vendor/autoload.php';
$crellSerde = new SerdeCommon();
$nullableArray = <<<'JSON'
{
"data": null
}
JSON;
final class ListItem {
}
final class MyList {
#[SequenceField(arrayType: ListItem::class)]
public readonly array|null $data;
}
$crellSerde->deserialize(
serialized: $nullableArray,
from: 'json',
to: MyList::class,
);
```
This test addition aims to confirm the bug, and serves as automated test to fix the implementation to achieve the desired behavior.
agustingomes
force-pushed
the
issues/107
branch
from
ef2d951 to
74608f0
Compare
| ### Objects without constructors | ||
|
|
||
| > [!WARNING] | ||
| > When deserializing nullable properties in objects without constructors, You must indicate the default value with the #[Field] attribute, otherwise properties may remain uninitialized after deserialization. |
There was a problem hiding this comment.
The constructor isn't actually relevant here. It's that the constructor uses promoted properties and provides a default. That's not a flattening-specific issue.
There was a problem hiding this comment.
indeed, and is the emphasis I try to make here, since unlike with promoted properties, we cannot give a default value to a readonly property that is not within a constructor promoted property.
Perhaps I should rephrase it a bit better to make the emphasis a bit clear, or you feel this addition does not bring enough value here?
There was a problem hiding this comment.
Calling out more strongly that Serde could leave an object with uninitialized properties is fine, but it's not specific to missing constructors, nor to flattening. There's already another argument to treat that as an error condition: https://github.com/Crell/Serde#requirevalue-bool-default-false
If we add a stronger warning about it, it should go there, or in some more general location. It's also not specific to nullable arrays, so maybe yet another split-off PR. 😄
There was a problem hiding this comment.
I see. Since requireValues part already describes the consequence of not having a default value, I think this addition I've proposed can be reverted.
|
|
||
| if ($this->phpType === $valueType) { | ||
| if ($this->phpType === 'array' && $this->nullable && $value === null) { | ||
| return true; |
There was a problem hiding this comment.
Why is this not just setting $valid = true, like everything else? Returning here bypasses the TypeField check at the end of the method.
There was a problem hiding this comment.
The main reason is that simply setting $valid = true, would still run the $this->typeField?->validate($value), which in cases of a nullable list, would still end in exception.
I quickly tried to adopt your suggestion, but I can confirm that the tests would fail as I expected:
? Round trip with nullable_list_with_constructor
?
? TypeError: array_is_list(): Argument #1 ($array) must be of type array, null given
?
? /home/agustingomes/Repositories/github/Crell/Serde/src/Attributes/SequenceField.php:83
? /home/agustingomes/Repositories/github/Crell/Serde/src/Attributes/Field.php:421
? /home/agustingomes/Repositories/github/Crell/Serde/src/PropertyHandler/ObjectImporter.php:71
? /home/agustingomes/Repositories/github/Crell/Serde/src/PropertyHandler/ObjectImporter.php:39
? /home/agustingomes/Repositories/github/Crell/Serde/src/Deserializer.php:31
? /home/agustingomes/Repositories/github/Crell/Serde/src/Serde.php:114
? /home/agustingomes/Repositories/github/Crell/Serde/tests/SerdeTestCases.php:224
?
I agree the early return may mask other issues. I tried moving the check towards the end of the method in f92fb13
Is not elegant, but it could help towards the next steps. What do you think?
There was a problem hiding this comment.
Hrm. Does that mean the validate() method needs to be updated as well?
If we have to go with a return here, it should at least be commented as to why. But I'd prefer not to if we can avoid it.
There was a problem hiding this comment.
Hrm. Does that mean the validate() method needs to be updated as well?
The challenge I see, is that updatingvalidate, may entail a breaking change for the interface, depending on exactly how we could update it.
If we have to go with a return here, it should at least be commented as to why. But I'd prefer not to if we can avoid it.
Absolutely! I will add some extra content around the new check for future reference, in case we end up sticking with the current solution.
This was a case encountered when dealing with an external system as part of a project I was working on.
With this fix, the following minimal reproduction case can work:
```php
<?php
declare(strict_types=1);
use Crell\Serde\Attributes\SequenceField;
use Crell\Serde\SerdeCommon;
require_once dirname(__DIR__) . '/vendor/autoload.php';
$crellSerde = new SerdeCommon();
$nullableArray = <<<'JSON'
{
"data": null
}
JSON;
final class ListItem {
}
final class MyList {
#[SequenceField(arrayType: ListItem::class)]
public readonly array|null $data;
}
$crellSerde->deserialize(
serialized: $nullableArray,
from: 'json',
to: MyList::class,
);
```
We're returning early under this conditions due to the fact that a SequenceField instance call to `validate` with a null value would throw an exception. Unfortunately at this moment, only the value can be passed to the `validate`, meaning we cannot check `$this->nullable`, which would allow us to have this check inside the `validate` method of the SequenceField instance.
2 participants
Description
Allows deserialization of nullable arrays
Motivation and context
This was a case encountered when dealing with an external system as part of a project I was working on. With this change, the library can now deserialize nullable arrays.
closes #107
How has this been tested?
Automated test based on the reproduction example linked to the issue were added covering the round trip of serializing/deserializing a nullable array.
Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:Checklist: