Support union/intersect types by vincentchalamon · Pull Request #5470 · api-platform/core

vincentchalamon · 2023-03-17T09:41:32Z

Q	A
Branch?	main
Tickets	#5452
License	MIT
Doc PR	N/A

soyuka

Good start!

To resume my thoughts I think that we should compute most of the type-inflected schema inside a JsonSchema\Metadata\TypePropertyMetadataFactory to reduce the work inside the SchemaFactory. Also move the ReflectionType checks there.
Specifications like JSON-LD, HAL and JSON:API support mutliple types but we stop at the first one.

Deprecating XML, CSV support from our normalizer is another task but reading these parts of the code reminded me of that so don't bother here.

I want to suggest that our codebase should not handle types that are scalars and objects but we probably need to discuss this with more @api-platform/core-team members.

soyuka · 2023-03-27T08:38:49Z

-            $hasAssociation = $totalProperties === $index && $isResourceClass;
        }

        return [$type, $hasAssociation, $currentResourceClass, $currentProperty];


I have a feeling that having a property that is a scalar and a resource class is such a bad practice that we should just not support it.

soyuka · 2023-03-27T08:54:54Z

+        foreach ($types as $type) {
+            if (null !== $type->getClassName()) {
+                return "data/relationships/$fieldName";
+            }


it's harder to find informations about json-api but it looks like it supports polymorphism inside collections, not sure how to handle this case.

soyuka · 2023-03-27T08:55:39Z

+            }

-            $components['relationships'][] = $relation;
+            // if all types are not relationships, declare it as an attribute


I'm not a huge fan, this means that the type is something like scalar|object?

Better something like Foo|Bar|Baz where only Baz is a resource

soyuka · 2023-03-27T09:17:34Z

+            /* From @see AbstractObjectNormalizer::validateAndDenormalize() */
+            // Fix a collection that contains the only one element
+            // This is special to xml format only
+            if ('xml' === $format && null !== $collectionValueType && (!\is_array($value) || !\is_int(key($value)))) {


@dunglas should we really support this? We discussed XML is not supporte-able by our normalizers at the symfony live. Maybe that deprecating the support of XML (w/ JSON by default) for 3.2 should be our target?

soyuka · 2023-03-27T09:19:01Z

+            // In XML and CSV all basic datatypes are represented as strings, it is e.g. not possible to determine,
+            // if a value is meant to be a string, float, int or a boolean value from the serialized representation.
+            // That's why we have to transform the values, if one of these non-string basic datatypes is expected.
+            if (\is_string($value) && (XmlEncoder::FORMAT === $format || CsvEncoder::FORMAT === $format)) {


same for this, wondering if it's not the time to get rid of that and use symfony's normalizers without ours for these.

soyuka · 2023-04-07T21:14:54Z

+                foreach ($propertyTypes as $propertyType) {
+                    if ($fieldConfiguration = $this->getResourceFieldConfiguration($property, $propertyMetadata->getDescription(), $propertyMetadata->getDeprecationReason(), $propertyType, $resourceClass, $input, $operation, $depth, null !== $propertyMetadata->getSecurity())) {
+                        $fields['id' === $property ? '_id' : $this->normalizePropertyName($property, $resourceClass)] = $fieldConfiguration;
+                        // stop at the first valid type


@alanpoulain doesn't graphql support multiple types ?

Apparently, it seems possible, but it requires a special trick:
https://stackoverflow.com/questions/49897319/graphql-union-scalar-type
https://graphql.org/learn/schema/#union-types

If it's not an input type, yes. Otherwise no (see graphql/graphql-spec#825).

soyuka

nice job on the metadata and schema factory part! few things left on the hydra support and I haven't read the part about validators / elasticsearch yet. We need to find out what to do about graphql thoug.

Co-authored-by: Antoine Bluchet <soyuka@users.noreply.github.com>

soyuka · 2023-07-05T22:04:59Z

    public const OPENAPI_DEFINITION_NAME = 'openapi_definition_name';

-    public function __construct(private readonly TypeFactoryInterface $typeFactory, ResourceMetadataCollectionFactoryInterface $resourceMetadataFactory, private readonly PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, private readonly PropertyMetadataFactoryInterface $propertyMetadataFactory, private readonly ?NameConverterInterface $nameConverter = null, ResourceClassResolverInterface $resourceClassResolver = null)
+    public function __construct(ResourceMetadataCollectionFactoryInterface $resourceMetadataFactory, private readonly PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, private readonly PropertyMetadataFactoryInterface $propertyMetadataFactory, private readonly ?NameConverterInterface $nameConverter = null, ResourceClassResolverInterface $resourceClassResolver = null)


This is a really good improvement imo but we still need to cover BC on that constructor. can we trigger a deprecation on using it and we'll remove it in 4.x

soyuka

one last thing awesome work!

chalasr · 2023-08-06T12:13:10Z

-    public function __construct(private readonly TypeFactoryInterface $typeFactory, ResourceMetadataCollectionFactoryInterface $resourceMetadataFactory, private readonly PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, private readonly PropertyMetadataFactoryInterface $propertyMetadataFactory, private readonly ?NameConverterInterface $nameConverter = null, ResourceClassResolverInterface $resourceClassResolver = null)
+    public function __construct(?TypeFactoryInterface $typeFactory, ResourceMetadataCollectionFactoryInterface $resourceMetadataFactory, private readonly PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, private readonly PropertyMetadataFactoryInterface $propertyMetadataFactory, private readonly ?NameConverterInterface $nameConverter = null, ResourceClassResolverInterface $resourceClassResolver = null)
    {
+        if ($typeFactory) {


Don't you want to remove this parameter from the signature at some point e.g. in 4.0?
This looks like a silent BC break. The layer in place does not allow calling code to adapt before the actual removal happens, given there is no way to make such code ready for the 4.0 constructor (unless using named arguments, not a majority)

vincentchalamon self-assigned this Mar 17, 2023

vincentchalamon force-pushed the fix/issues/5452 branch 9 times, most recently from eeba480 to 6659839 Compare March 20, 2023 12:21

vincentchalamon requested review from alanpoulain and soyuka March 20, 2023 12:37

vincentchalamon marked this pull request as ready for review March 20, 2023 12:40

soyuka requested changes Mar 27, 2023

View reviewed changes

vincentchalamon force-pushed the fix/issues/5452 branch 6 times, most recently from 421046d to efb4a00 Compare April 6, 2023 14:13