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.

Sign up for GitHub

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

Handle null data when retrieving value #33

Merged
merged 6 commits into from
Feb 6, 2025
Merged

Handle null data when retrieving value #33

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6bfae0b
Be explicit about null vs. uninitialized fields
joe-boudreau Nov 14, 2024
f8b85bc
Handle null data when retrieving value
joe-boudreau Nov 14, 2024
fda9bb1
Add test case for unexpected null fields
joe-boudreau Nov 14, 2024
5538283
revert method visibility
joe-boudreau Nov 15, 2024
cc0ee36
fix whitespace
joe-boudreau Nov 18, 2024
42d1435
Merge branch 'main' into handle-null-data-when-retrieving-value
joe-boudreau Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions src/Json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,20 +70,19 @@ public function forProperty(ReflectionProperty $prop): Json
public function retrieveValue(?array $data, ReflectionNamedType|ReflectionUnionType|null $type = null)
{
foreach ($this->path as $pathBit) {
if (! array_key_exists($pathBit, $data)) {
if (is_null($data) || ! array_key_exists($pathBit, $data)) {
return $this->handleMissingValue($data);
}
$data = $data[$pathBit];
}
if (is_null($data)) {
return null;
}

if ($type instanceof ReflectionUnionType) {
$type = RUnionType::getSingleTypeMatch($type, $data);
}

if (is_null($data) && $type && $type->allowsNull()) {
return null;
}

if ($type === null) {
if (isset($this->type)) {
$t = $this->type;
Expand All @@ -100,10 +99,6 @@ public function retrieveValue(?array $data, ReflectionNamedType|ReflectionUnionT
return $data;
}
if (! class_exists($typename) && $typename === 'array' && isset($this->type)) {
if (is_null($data)) {
return $data;
}

if (RClass::make($this->type)->isBackedEnum()) {
return array_map(fn ($d) => $this->type::from($d), $data);
}
Expand Down
79 changes: 59 additions & 20 deletions tests/DeSerializationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@

final class DeSerializationTest extends TestCase
{

const UNINITIALIZED = '@uninitialized';

public function export($value)
{
if (is_null($value)) {
Expand All @@ -53,7 +56,7 @@ public function export($value)
];
foreach ($rc->getProperties() as $prop) {
$prop->setAccessible(true);
$v = $prop->isInitialized($value) ? $prop->getValue($value) : null;
$v = $prop->isInitialized($value) ? $prop->getValue($value) : self::UNINITIALIZED;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied the behaviour from the Php81DeSerializationTest class. I think it's better to be more explicit about the returned value so the tests are self-documenting

$n = $prop->getName();

$data[$n] = $this->export($v);
Expand All @@ -70,12 +73,12 @@ public function testDeserializesSimpleClass()
'id' => 'myid',
'name' => 'Clothes',
'data_name' => null,
'schedule' => null,
'schedule' => self::UNINITIALIZED,
'nullableSchedule' => null,
'schedules' => null,
'schedules' => self::UNINITIALIZED,
'nullableSchedules' => null,
'counts' => [],
'unnamed' => null,
'unnamed' => self::UNINITIALIZED,
'untypedSchedule' => null,
], $this->export($c));
}
Expand All @@ -95,12 +98,12 @@ public function testNullableProperty()
'id' => 'myid',
'name' => 'Clothes',
'data_name' => null,
'schedule' => null,
'schedule' => self::UNINITIALIZED,
'nullableSchedule' => null,
'schedules' => null,
'schedules' => self::UNINITIALIZED,
'nullableSchedules' => null,
'counts' => [],
'unnamed' => null,
'unnamed' => self::UNINITIALIZED,
'untypedSchedule' => null,
], $this->export($c));
$this->assertNull($c->nullableSchedule);
Expand All @@ -123,12 +126,12 @@ public function testOmitsEmptyValues()
'data_name' => null,
'id' => 'myid',
'name' => 'Clothes',
'schedule' => null,
'schedule' => self::UNINITIALIZED,
'nullableSchedule' => null,
'schedules' => null,
'schedules' => self::UNINITIALIZED,
'nullableSchedules' => null,
'counts' => [],
'unnamed' => null,
'unnamed' => self::UNINITIALIZED,
'untypedSchedule' => null,
], $this->export($bc));
}
Expand Down Expand Up @@ -167,10 +170,10 @@ public function testSerializesClassAttributesRecursively()
'end' => 20,
],
'nullableSchedule' => null,
'schedules' => null,
'schedules' => self::UNINITIALIZED,
'nullableSchedules' => null,
'counts' => [],
'unnamed' => null,
'unnamed' => self::UNINITIALIZED,
'untypedSchedule' => null,
], $this->export($c));
}
Expand Down Expand Up @@ -224,7 +227,7 @@ public function testSerializesObjectArrays()
],
'nullableSchedules' => null,
'counts' => [],
'unnamed' => null,
'unnamed' => self::UNINITIALIZED,
'untypedSchedule' => null,
], $this->export($c));
}
Expand All @@ -248,16 +251,16 @@ public function testSerializesScalarArrays()
'id' => 'myid',
'name' => 'Clothes',
'data_name' => null,
'schedule' => null,
'schedule' => self::UNINITIALIZED,
'nullableSchedule' => null,
'schedules' => null,
'schedules' => self::UNINITIALIZED,
'nullableSchedules' => null,
'counts' => [
0 => 1,
1 => 'abc',
2 => 678,
],
'unnamed' => null,
'unnamed' => self::UNINITIALIZED,
'untypedSchedule' => null,
], $this->export($c));
}
Expand All @@ -278,9 +281,9 @@ public function testSerializesWithNoName()
'id' => 'myid',
'name' => 'Clothes',
'data_name' => null,
'schedule' => null,
'schedule' => self::UNINITIALIZED,
'nullableSchedule' => null,
'schedules' => null,
'schedules' => self::UNINITIALIZED,
'nullableSchedules' => null,
'counts' => [],
'unnamed' => 'bob',
Expand Down Expand Up @@ -308,9 +311,9 @@ public function testSerializesWithUntypedProp()
'id' => 'myid',
'name' => 'Clothes',
'data_name' => null,
'schedule' => null,
'schedule' => self::UNINITIALIZED,
'nullableSchedule' => null,
'schedules' => null,
'schedules' => self::UNINITIALIZED,
'nullableSchedules' => null,
'counts' => [],
'unnamed' => 'bob',
Expand Down Expand Up @@ -672,4 +675,40 @@ public function testUnionTypesUsingArrayAndCustomObject()
],
]);
}

public function testNonNullableFieldWithNullValue()
{
$c = Category::fromJsonString('{"next_schedule": null}');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only field that's being set in JSON is not being tested in the output

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, it is - the next_schedule JSON field maps to the $schedule class property:
https://github.com/square/pjson/blob/main/tests/Definitions/Category.php#L23

$this->assertEquals([
'@class' => Category::class,
'id' => null,
'name' => null,
'data_name' => null,
'schedule' => self::UNINITIALIZED, // should stay uninitialized because it's a non-nullable field
'nullableSchedule' => null,
'schedules' => self::UNINITIALIZED,
'nullableSchedules' => null,
'counts' => [],
'unnamed' => self::UNINITIALIZED,
'untypedSchedule' => null,
], $this->export($c));
}

public function testNullParentObjectForNestedPath()
{
$c = Category::fromJsonString('{"data": null}');
$this->assertEquals([
'@class' => Category::class,
'id' => null,
'name' => null,
'data_name' => null, // set to null because the parent object is null and it can be null
'schedule' => self::UNINITIALIZED,
'nullableSchedule' => null,
'schedules' => self::UNINITIALIZED,
'nullableSchedules' => null,
'counts' => [],
'unnamed' => self::UNINITIALIZED,
'untypedSchedule' => null,
], $this->export($c));
}
}
6 changes: 4 additions & 2 deletions tests/php81/Php81DeSerializationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

final class Php81DeSerializationTest extends TestCase
{
const UNINITIALIZED = '@uninitialized';

public function export($value)
{
if (is_null($value)) {
Expand All @@ -33,7 +35,7 @@ public function export($value)
'@class' => get_class($value),
];
foreach ($rc->getProperties() as $prop) {
$v = $prop->isInitialized($value) ? $prop->getValue($value) : '@uninitialized';
$v = $prop->isInitialized($value) ? $prop->getValue($value) : self::UNINITIALIZED;
$n = $prop->getName();

$data[$n] = $this->export($v);
Expand Down Expand Up @@ -67,7 +69,7 @@ public function testBackedEnum()
"value" => "ON",
],
"optional" => null,
"size" => '@uninitialized',
"size" => self::UNINITIALIZED,
], $this->export($w));
}

Expand Down
Loading