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

Jump to bottom

Union types complex #1549

Closed
wants to merge 20 commits into from
Closed

Union types complex #1549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
41d1789
non disciminated working and disciminated not working.
idbentley Jul 4, 2024
d2a5090
iter
idbentley Jul 8, 2024
5a4128c
phpcs
idbentley Jul 8, 2024
1013bd8
iter
idbentley Jul 9, 2024
27adca8
add tests for serialization
idbentley Jul 9, 2024
8fe8a7c
iter
idbentley Jul 9, 2024
08f9152
iter
idbentley Jul 10, 2024
72a2124
handle deserialization failures.
idbentley Jul 15, 2024
a86562c
better handle non discriminated unions, and primitives
idbentley Jul 16, 2024
0cd13a6
better handling of primitives
idbentley Jul 16, 2024
f96f768
style
idbentley Jul 17, 2024
4f4e238
stan
idbentley Jul 17, 2024
75c2aff
fix tests
idbentley Jul 17, 2024
f92f276
add tests for mapped union discriminators
idbentley Jul 17, 2024
af18df8
missed chnages
idbentley Jul 17, 2024
c0f8cbc
remnants
idbentley Jul 17, 2024
15b3244
code quality
idbentley Jul 17, 2024
186d526
more code cleanup
idbentley Jul 17, 2024
405bf93
revert debug change
idbentley Jul 17, 2024
2ed4057
clean
idbentley Jul 17, 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
26 changes: 26 additions & 0 deletions src/Annotation/UnionDiscriminator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

declare(strict_types=1);

namespace JMS\Serializer\Annotation;

/**
* @Annotation
* @Target({"PROPERTY"})
*/
#[\Attribute(\Attribute::TARGET_PROPERTY)]
final class UnionDiscriminator implements SerializerAttribute
{
use AnnotationUtilsTrait;

/** @var array<string> */
public $map = [];

/** @var string */
public $field = 'type';

public function __construct(array $values = [], string $field = 'type', array $map = [])
{
$this->loadAnnotationParameters(get_defined_vars());
}
}
9 changes: 9 additions & 0 deletions src/Exception/PropertyMissingException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php

declare(strict_types=1);

namespace JMS\Serializer\Exception;

final class PropertyMissingException extends RuntimeException
{
}
27 changes: 27 additions & 0 deletions src/GraphNavigator/DeserializationGraphNavigator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use JMS\Serializer\Exception\ExpressionLanguageRequiredException;
use JMS\Serializer\Exception\LogicException;
use JMS\Serializer\Exception\NotAcceptableException;
use JMS\Serializer\Exception\PropertyMissingException;
use JMS\Serializer\Exception\RuntimeException;
use JMS\Serializer\Exception\SkipHandlerException;
use JMS\Serializer\Exclusion\ExpressionLanguageExclusionStrategy;
Expand Down Expand Up @@ -197,6 +198,7 @@ public function accept($data, ?array $type = null)

$this->visitor->startVisitingObject($metadata, $object, $type);
foreach ($metadata->propertyMetadata as $propertyMetadata) {
$allowsNull = null === $propertyMetadata->type ? true : $this->allowsNull($propertyMetadata->type);
if (null !== $this->exclusionStrategy && $this->exclusionStrategy->shouldSkipProperty($propertyMetadata, $this->context)) {
continue;
}
Expand All @@ -212,12 +214,21 @@ public function accept($data, ?array $type = null)
$this->context->pushPropertyMetadata($propertyMetadata);
try {
$v = $this->visitor->visitProperty($propertyMetadata, $data);

$this->accessor->setValue($object, $v, $propertyMetadata, $this->context);
} catch (NotAcceptableException $e) {
if (true === $propertyMetadata->hasDefault) {
$cloned = clone $propertyMetadata;
$cloned->setter = null;
$this->accessor->setValue($object, $cloned->defaultValue, $cloned, $this->context);
} elseif (!$allowsNull && $this->visitor->getRequireAllRequiredProperties()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if we should handle this here... could you please explain why do you need this check at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if we should handle this here... could you please explain why do you need this check at this point?

I added this check to make the normal deserialization process is less permissive, resulting in more accurate type detection.

Specifically, if the object you are deserializing into contains a required field, and the JSON does not contain that field, the existing deserializer will deserialize without complaint. While, that behaviour could be desirable in some cases, and presumably must be maintained for backwards compatibility, it's undesirable for deserializing non-discriminated unions.

For non-discriminated unions, we use a "guess and check" approach to deserialization, and the absence of a required field is a strong signal that the data we're deserializing may better match a different type.

class SimplePerson {
    public string $name;
    public ?string $nickname;
}

class SimpleCar {
    public string $model;
    public ?string $nickname;
}

class WrapperClass {
  public SimplePerson|SimpleCar $entity
}

If we want to deserialize the following data:

{
  "entity": {
    "name": "ian"
  }
}

Without such a rule, the resulting class is determined by the order of application. If you consider SimplePerson first, then all is well. If you consider SimpleCar first, then the deserialization model skips name because it doesn't exist in the class, and deserialization will succeed returning a null SimpleCar.

By adding the rule, when SimpleCar is tested, the required model attribute is not present, so deserialization will fail.

I agree that the implementation is awkward, and would be happy to refactor!

Copy link
Collaborator

@goetas goetas Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would really avoid implementing such usecase. It is too specific and I'm really not sure if it worth the complexity.

{
  "entity": {
    "type": "simple_person",
    "name": "ian"
  }
}

A type filed should be always present in order to match efficiently the class name to use.

The same type of discussion were taken when implementing the @DiscrminatorMap for inheritance and even in that case any other solution was just not worth the effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's worth the effort, and I don't think it's a very specific usecase.

While, of course deserializing discriminated unions is easier, it's not the only type of data that exists. It's really easy to say "just change the data", but in many cases, that isn't possible - for example: if someone's deserializing data from an external API. Anecdotally in my situation, this case comes up regularly.

In order to provide good support for PHP 8, jms/serializer should have good support for union types,.

I'll note that other deserialization libraries do provide this functionality: dataclasses_json, pydantic, zod are just three examples.

I've provided an implementation that is opt-out (by not using the UnionHandler altogether, or you can construct the UnionHandler with requireAllProperties=false to avoid the new failure logic). Is the concern the added code complexity? I'm happy to refactor the code.

However, if you still think this isn't an appropriate feature fro the jms/serializer - I would ask that at least we provide a workaround for users. If we could include the ability to requireAllRequiredFields when deserializing, it would allow me to write a custom UnionHandler outside of jms/serializer.

$this->visitor->endVisitingObject($metadata, $data, $type);

throw new PropertyMissingException('Property ' . $propertyMetadata->name . ' is missing from data');
} elseif ($this->visitor->getRequireAllRequiredProperties()) {
$this->visitor->endVisitingObject($metadata, $data, $type);

throw $e;
}
}

Expand All @@ -231,6 +242,22 @@ public function accept($data, ?array $type = null)
}
}

private function allowsNull(array $type)
{
$allowsNull = false;
if ('union' === $type['name']) {
foreach ($type['params'] as $param) {
if ('NULL' === $param['name']) {
$allowsNull = true;
}
}
} elseif ('NULL' === $type['name']) {
$allowsNull = true;
}

return $allowsNull;
}

/**
* @param mixed $data
*/
Expand Down
14 changes: 14 additions & 0 deletions src/GraphNavigator/SerializationGraphNavigator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,20 @@ public function accept($data, ?array $type = null)

throw new RuntimeException($msg);

case 'union':
if (null !== $handler = $this->handlerRegistry->getHandler(GraphNavigatorInterface::DIRECTION_SERIALIZATION, $type['name'], $this->format)) {
try {
return \call_user_func($handler, $this->visitor, $data, $type, $this->context);
} catch (SkipHandlerException $e) {
// Skip handler, fallback to default behavior
} catch (NotAcceptableException $e) {
$this->context->stopVisiting($data);

throw $e;
}
}

break;
default:
if (null !== $data) {
if ($this->context->isVisiting($data)) {
Expand Down
116 changes: 98 additions & 18 deletions src/Handler/UnionHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@

use JMS\Serializer\Context;
use JMS\Serializer\DeserializationContext;
use JMS\Serializer\Exception\NonFloatCastableTypeException;
use JMS\Serializer\Exception\NonIntCastableTypeException;
use JMS\Serializer\Exception\NonStringCastableTypeException;
use JMS\Serializer\Exception\NonVisitableTypeException;
use JMS\Serializer\Exception\PropertyMissingException;
use JMS\Serializer\Exception\RuntimeException;
use JMS\Serializer\GraphNavigatorInterface;
use JMS\Serializer\SerializationContext;
Expand All @@ -15,6 +20,12 @@
final class UnionHandler implements SubscribingHandlerInterface
{
private static $aliases = ['boolean' => 'bool', 'integer' => 'int', 'double' => 'float'];
private bool $requireAllProperties;

public function __construct(bool $requireAllProperties = false)
{
$this->requireAllProperties = $requireAllProperties;
}

/**
* {@inheritdoc}
Expand Down Expand Up @@ -47,46 +58,115 @@ public function serializeUnion(
mixed $data,
array $type,
SerializationContext $context
) {
return $this->matchSimpleType($data, $type, $context);
): mixed {
if ($this->isPrimitiveType(gettype($data))) {
return $this->matchSimpleType($data, $type, $context);
} else {
$resolvedType = [
'name' => get_class($data),
'params' => [],
];

return $context->getNavigator()->accept($data, $resolvedType);
}
}

public function deserializeUnion(DeserializationVisitorInterface $visitor, mixed $data, array $type, DeserializationContext $context)
public function deserializeUnion(DeserializationVisitorInterface $visitor, mixed $data, array $type, DeserializationContext $context): mixed
{
if ($data instanceof \SimpleXMLElement) {
throw new RuntimeException('XML deserialisation into union types is not supported yet.');
}

return $this->matchSimpleType($data, $type, $context);
}

private function matchSimpleType(mixed $data, array $type, Context $context)
{
$dataType = $this->determineType($data, $type, $context->getFormat());
$alternativeName = null;

if (isset(static::$aliases[$dataType])) {
$alternativeName = static::$aliases[$dataType];
}

foreach ($type['params'] as $possibleType) {
if ($possibleType['name'] === $dataType || $possibleType['name'] === $alternativeName) {
return $context->getNavigator()->accept($data, $possibleType);
$propertyMetadata = $context->getMetadataStack()->top();
$finalType = null;
if (null !== $propertyMetadata->unionDiscriminatorField) {
if (!array_key_exists($propertyMetadata->unionDiscriminatorField, $data)) {
throw new NonVisitableTypeException('Union Discriminator Field \'' . $propertyMetadata->unionDiscriminatorField . '\' not found in data');
}

$lkup = $data[$propertyMetadata->unionDiscriminatorField];
if (!empty($propertyMetadata->unionDiscriminatorMap)) {
if (array_key_exists($lkup, $propertyMetadata->unionDiscriminatorMap)) {
$finalType = [
'name' => $propertyMetadata->unionDiscriminatorMap[$lkup],
'params' => [],
];
} else {
throw new NonVisitableTypeException('Union Discriminator Map does not contain key \'' . $lkup . '\'');
}
} else {
$finalType = [
'name' => $lkup,
'params' => [],
];
}
}

if (null !== $finalType && null !== $finalType['name']) {
return $context->getNavigator()->accept($data, $finalType);
} else {
try {
$previousVisitorRequireSetting = $visitor->getRequireAllRequiredProperties();
if ($this->requireAllProperties) {
$visitor->setRequireAllRequiredProperties($this->requireAllProperties);
}

if ($this->isPrimitiveType($possibleType['name']) && (is_array($data) || !$this->testPrimitive($data, $possibleType['name'], $context->getFormat()))) {
continue;
}

$accept = $context->getNavigator()->accept($data, $possibleType);
if ($this->requireAllProperties) {
$visitor->setRequireAllRequiredProperties($previousVisitorRequireSetting);
}

return $accept;
} catch (NonVisitableTypeException $e) {
continue;
} catch (PropertyMissingException $e) {
continue;
} catch (NonStringCastableTypeException $e) {
continue;
} catch (NonIntCastableTypeException $e) {
continue;
} catch (NonFloatCastableTypeException $e) {
continue;
}
}
}

return null;
}

private function determineType(mixed $data, array $type, string $format): ?string
private function matchSimpleType(mixed $data, array $type, Context $context): mixed
{
$alternativeName = null;

foreach ($type['params'] as $possibleType) {
if ($this->testPrimitive($data, $possibleType['name'], $format)) {
return $possibleType['name'];
if ($this->isPrimitiveType($possibleType['name']) && !$this->testPrimitive($data, $possibleType['name'], $context->getFormat())) {
continue;
}

try {
return $context->getNavigator()->accept($data, $possibleType);
} catch (NonVisitableTypeException $e) {
continue;
} catch (PropertyMissingException $e) {
continue;
}
}

return null;
}

private function isPrimitiveType(string $type): bool
{
return in_array($type, ['int', 'integer', 'float', 'double', 'bool', 'boolean', 'string']);
}

private function testPrimitive(mixed $data, string $type, string $format): bool
{
switch ($type) {
Expand Down
17 changes: 17 additions & 0 deletions src/JsonDeserializationStrictVisitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,30 @@ final class JsonDeserializationStrictVisitor extends AbstractVisitor implements
/** @var JsonDeserializationVisitor */
private $wrappedDeserializationVisitor;

/**
* THIS IS ONLY USED FOR UNION DESERIALIZATION WHICH IS NOT SUPPORTED IN XML
*
* @var bool
*/
private $requireAllRequiredProperties = false;

public function __construct(
int $options = 0,
int $depth = 512
) {
$this->wrappedDeserializationVisitor = new JsonDeserializationVisitor($options, $depth);
}

public function setRequireAllRequiredProperties(bool $requireAllRequiredProperties): void
{
$this->requireAllRequiredProperties = $requireAllRequiredProperties;
}

public function getRequireAllRequiredProperties(): bool
{
return $this->requireAllRequiredProperties;
}

public function setNavigator(GraphNavigatorInterface $navigator): void
{
parent::setNavigator($navigator);
Expand Down
19 changes: 18 additions & 1 deletion src/JsonDeserializationVisitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,30 @@ final class JsonDeserializationVisitor extends AbstractVisitor implements Deseri
*/
private $currentObject;

/**
* @var bool
*/
private $requireAllRequiredProperties;

public function __construct(
int $options = 0,
int $depth = 512
int $depth = 512,
bool $requireAllRequiredProperties = false
) {
$this->objectStack = new \SplStack();
$this->options = $options;
$this->depth = $depth;
$this->requireAllRequiredProperties = $requireAllRequiredProperties;
}

public function setRequireAllRequiredProperties(bool $requireAllRequiredProperties): void
{
$this->requireAllRequiredProperties = $requireAllRequiredProperties;
}

public function getRequireAllRequiredProperties(): bool
{
return $this->requireAllRequiredProperties;
}

/**
Expand Down
3 changes: 3 additions & 0 deletions src/Metadata/Driver/AnnotationOrAttributeDriver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
use JMS\Serializer\Annotation\Since;
use JMS\Serializer\Annotation\SkipWhenEmpty;
use JMS\Serializer\Annotation\Type;
use JMS\Serializer\Annotation\UnionDiscriminator;
use JMS\Serializer\Annotation\Until;
use JMS\Serializer\Annotation\VirtualProperty;
use JMS\Serializer\Annotation\XmlAttribute;
Expand Down Expand Up @@ -258,6 +259,8 @@ public function loadMetadataForClass(\ReflectionClass $class): ?BaseClassMetadat
$propertyMetadata->xmlAttributeMap = true;
} elseif ($annot instanceof MaxDepth) {
$propertyMetadata->maxDepth = $annot->depth;
} elseif ($annot instanceof UnionDiscriminator) {
$propertyMetadata->setUnionDiscriminator($annot->field, $annot->map);
}
}

Expand Down
Loading
Loading