Allow union deserialization using discriminator map #1553

idbentley · 2024-07-26T18:02:52Z

Q	A
Bug fix?	no
New feature?	yes
Doc updated	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1549
License	MIT

Hi @goetas here's a simpler version of PR #1549 - this one only adds support for discriminated complex unions.

idbentley · 2024-08-06T14:47:12Z

Hi @goetas - I wonder if you've had a chance to look at these changes?

goetas

hi, sorry for the delay in my review. i have left some comments, could you answer my questions please?

after you answer to #1553 (comment), could you please update the documentation as well? most of what you need is to update /doc/reference/annotations.rst

goetas · 2024-08-08T22:09:06Z

src/Handler/UnionHandler.php

+            if (null !== $finalType && null !== $finalType['name']) {
+                return $context->getNavigator()->accept($data, $finalType);
+            } else {
+                foreach ($type['params'] as $possibleType) {


something is odd with this foreach loops, you have here a nested loop the same variable (line 70 and 102). shouldn't they be independent?

This was an error - removing the inner loop is the correct fix.

goetas · 2024-08-08T22:10:26Z

src/Handler/UnionHandler.php

+                        }
+                    } else {
+                        $finalType = [
+                            'name' => $lkup,


i think that is an anti pattern to leak class name into the JSON representation.... it might be a "quick thing" but it has backfired at me so many times...
It can represent also potential security issues as it allows third parties to instantiate potentially any class.
are you using this approach? (as example in the "inheritance" discriminator map this feature is intentionally requiring the discriminator map to be not empty)

I'm not sure I understand the alternative to putting the className into the type.

This can happen in many ways. The simplest I can think of is:

class SomeClass { private MappedDiscriminatedAuthor $data }

will result in such a type:

$type = ['name' => 'MappedDiscriminatedAuthor', 'params' => []]

I think I must be misunderstanding your question.

goetas · 2024-08-08T22:24:30Z

src/Metadata/Driver/TypedPropertiesDriver.php

@@ -102,6 +102,10 @@ public function loadMetadataForClass(ReflectionClass $class): ?ClassMetadata
        foreach ($classMetadata->propertyMetadata as $propertyMetadata) {
            // If the inner driver provides a type, don't guess anymore.
            if ($propertyMetadata->type) {
+                if ('union' === $propertyMetadata->type['name']) {
+                    $propertyMetadata->setType($this->reorderTypes($propertyMetadata->type));


you could potentially encode in the union type already the info regarding the discriminator field and map.
advantage of it is that it would be done only once at compile cache metadata time vs at runtime as done now.
it should be much faster.

#[UnionDiscriminator( field: 'objectType', map: [ 'author' => 'JMS\Serializer\Tests\Fixtures\MappedDiscriminatedAuthor', 'comment' => 'JMS\Serializer\Tests\Fixtures\MappedDiscriminatedComment' ] )]

can be translated to

[ 'type' => 'union', 'params' => [ 'objectType', [ 'author' => 'JMS\Serializer\Tests\Fixtures\MappedDiscriminatedAuthor', 'comment' => 'JMS\Serializer\Tests\Fixtures\MappedDiscriminatedComment' ] ] ]

in this way you have all the data ready in the UnionHandler and no need to fetch metadata each time

do you see it as doable approach?

I'm not sure I understand what you're asking for, so I put together a commit (34439ca) that contains your ask as I understand it.

Please see the most recent commit for the change in full. Basically, my change does the following.

If a UnionDiscriminator annotation exists, then the params array on the type will be overwritten:

from

[ 'name' => 'union', 'params' => [['name' => 'Path\Type1', 'params' => []], ['name' => 'Path\Type2', 'params' => []] ]

to

[ 'name' => 'union', 'params' => ['type'] ]

If the UnionDiscriminator annotation contains a field and a map, both are included:

[ 'name' => 'union', 'params' => ['type', ['type1' => 'Path\Type1', ...]] ]

The UnionHandler can handle a params list that contains any of these combinations:

A param list of possibleTypes (if they are primitives, they will resolve if the data matches).

A param list with just a field name - the parameter with a matching name will be used to find the resolution type.

A param list with a field name and a lookup map. The parameter with a matching name will be used to lookup the final type from the lookup map.

goetas · 2024-08-12T21:26:39Z

Your latest version is exactly what I had in mind, nice!

The only thing left in my opinion is to address the potential security issue. With your implementation,

class ComplexDiscriminatedUnion
{
    #[UnionDiscriminator(field: 'type')]
    private DiscriminatedAuthor|DiscriminatedComment $data;

If a user sends this JSON

{
 "type":  "DOMDocument"
}

with the current implementation the serializer will attempt to instantiate a DOMDocument class and to set it to $data.

In this case DOMDocument is harmless, but any class can be passed there.

The solution here is to have the map parameter strictly necessary and only the classes in the map should be used (this is the same solution used for the inheritance version)

src/Metadata/Driver/AnnotationOrAttributeDriver.php

idbentley · 2024-08-13T14:54:20Z

@goetas thanks for the feedback. I think I captured all the changes you asked for!

goetas · 2024-08-13T21:12:13Z

src/Handler/UnionHandler.php

+                    } else {
+                        throw new NonVisitableTypeException('Union Discriminator Map does not contain key \'' . $lkup . '\'');
+                    }
+                } else {


this else branch still exposes the ability to pass any class name in the json and later that class will be instantiated. I think that this should be removed

…alizer into idbentley-union-types-discriminated

- use early returns - simplify the reordering of union types

goetas · 2024-08-13T21:55:47Z

@idbentley I took the liberty of pushing the changes my self, could you please have a look?

idbentley · 2024-08-14T13:26:56Z

@goetas Your changes look good to me!

idbentley · 2024-08-15T20:03:04Z

I'm going to be OOTO next week. Feel free to proceed without me!

scyzoryck · 2024-08-17T05:59:20Z

src/GraphNavigator/SerializationGraphNavigator.php

@@ -177,6 +177,16 @@ public function accept($data, ?array $type = null)

                throw new RuntimeException($msg);

+            case 'union':


Just a thought - as the logic in growing and most probably is becoming a first class citizen - IMHO using handler might be redundant here. I guess we might go with direct implementation in GraphNavigator in the future. If we need additional logic here, having handler seems to be redundant.

scyzoryck · 2024-08-17T06:02:26Z

src/Metadata/PropertyMetadata.php

+    /**
+     * @var string|null
+     */
+    public $unionDiscriminatorField;


When we moved the code to the type, those fields seems to be not needed anymore. :)

hmm, can you please clarify? im not sure I understand

idbentley added 6 commits July 26, 2024 10:37

only handle discriminated

e711589

tests

3466c01

iter

411c72c

iter

687d62c

style

9532fb6

unusued imports

b5cc1ce

idbentley mentioned this pull request Jul 26, 2024

Union types complex #1549

Closed

scyzoryck requested a review from goetas July 30, 2024 10:16

goetas reviewed Aug 8, 2024

View reviewed changes

idbentley added 2 commits August 12, 2024 12:58

PR feedback

34439ca

add docs for UnionDiscriminator

dbd19ba

goetas reviewed Aug 12, 2024

View reviewed changes

src/Metadata/Driver/AnnotationOrAttributeDriver.php Show resolved Hide resolved

require map and refactor where the union setType occurs

3f4f69f

goetas reviewed Aug 13, 2024

View reviewed changes

goetas added 2 commits August 13, 2024 23:16

Merge branch 'union-types-discriminated' of github.com:idbentley/seri…

df02675

…alizer into idbentley-union-types-discriminated

- do not allow dynamic class name to be used as union type

1bf60d9

- use early returns - simplify the reordering of union types

goetas added 2 commits August 14, 2024 00:00

fix phpstan

052acb2

no fast fail for static analysis

beba384

goetas requested a review from scyzoryck August 13, 2024 22:08

scyzoryck reviewed Aug 17, 2024

View reviewed changes

goetas added 2 commits August 18, 2024 22:40

add documentation and metadata drivers

568f439

skip union metadata drivers when less than php 8

007d9a3

goetas changed the title ~~Union types discriminated~~ Allow union deserialization using discriminator map Aug 18, 2024

goetas merged commit b71da8c into schmittjoh:master Aug 18, 2024
16 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow union deserialization using discriminator map #1553

Allow union deserialization using discriminator map #1553

idbentley commented Jul 26, 2024

idbentley commented Aug 6, 2024

goetas left a comment •

edited

Loading

goetas Aug 8, 2024

idbentley Aug 12, 2024

goetas Aug 8, 2024 •

edited

Loading

idbentley Aug 12, 2024

goetas Aug 8, 2024

idbentley Aug 12, 2024 •

edited

Loading

goetas commented Aug 12, 2024 •

edited

Loading

idbentley commented Aug 13, 2024

goetas Aug 13, 2024 •

edited

Loading

goetas commented Aug 13, 2024

idbentley commented Aug 14, 2024

idbentley commented Aug 15, 2024

scyzoryck Aug 17, 2024

scyzoryck Aug 17, 2024

goetas Aug 18, 2024

		@@ -177,6 +177,16 @@ public function accept($data, ?array $type = null)

		throw new RuntimeException($msg);

		case 'union':

Allow union deserialization using discriminator map #1553

Allow union deserialization using discriminator map #1553

Conversation

idbentley commented Jul 26, 2024

idbentley commented Aug 6, 2024

goetas left a comment • edited Loading

Choose a reason for hiding this comment

goetas Aug 8, 2024

Choose a reason for hiding this comment

idbentley Aug 12, 2024

Choose a reason for hiding this comment

goetas Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

idbentley Aug 12, 2024

Choose a reason for hiding this comment

goetas Aug 8, 2024

Choose a reason for hiding this comment

idbentley Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

goetas commented Aug 12, 2024 • edited Loading

idbentley commented Aug 13, 2024

goetas Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

goetas commented Aug 13, 2024

idbentley commented Aug 14, 2024

idbentley commented Aug 15, 2024

scyzoryck Aug 17, 2024

Choose a reason for hiding this comment

scyzoryck Aug 17, 2024

Choose a reason for hiding this comment

goetas Aug 18, 2024

Choose a reason for hiding this comment

goetas left a comment •

edited

Loading

goetas Aug 8, 2024 •

edited

Loading

idbentley Aug 12, 2024 •

edited

Loading

goetas commented Aug 12, 2024 •

edited

Loading

goetas Aug 13, 2024 •

edited

Loading