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.

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

IBX-8190: Add Symfony's Serializer support #121

Open
wants to merge 103 commits into
base: main
Choose a base branch
from
Open

Conversation

barw4
Copy link
Contributor

@barw4 barw4 commented Aug 12, 2024

🎫 Issue IBX-8190

Related PRs:

https://github.com/ibexa/scheduler/pull/111

Description:

AdapterNormalizer

A new normalizer with low priority is implemented called AdapterNormalizer that handles data normalization or visiting data using the existing visitors. Currently, the visitors are prioritized over incoming (new) normalizers.

Unfortunately, the problem is that in order to visit value objects the Visitor and Generator have to be created and passed as arguments to ValueObjectVisitor instance.

        $valueObjectVisitor->visit($visitor, $generator, $object);

Ibexa\Contracts\Rest\Output\ValueObjectVisitorDispatcher

It is deleted now and the responsibility for finding a proper visitor is moved to the newly introduced ValueObjectVisitorResolverInterface.

Ibexa\Contracts\Rest\OutputVisitor

The visitValueObject method has to behave exactly the same as before as it's used throughout the app so we should keep BC here. I don't think we can change it to use normalizer as it uses the original Visitor's generator.

Concept made by Paweł.

Documentation:

From now on the responses generated by Ibexa will be normalized and encoded by Symfony's Serializer https://symfony.com/doc/5.x/serializer.html. This is a change from the old way of doing things when the only responsibility laid on Json\Generator or Xml\Generator to handle things, so normalizing and encoding depending on the generator used. Currently, the generators are responsible for generating JsonObject which is a representation of data one wants to normalize and they allow to prepare a context for Symfony's encoder and their purpose is to keep BC for older Ibexa data structures.

How a developer can take advantage of the new implementation?
Instead of writing a dedicated ValueObjectVisitor that handles generation of JsonObject inside Generator\Xml or Generator\Json (for example https://github.com/ibexa/rest/blob/main/src/lib/Server/Output/ValueObjectVisitor/Author.php) one can write a dedicated normalizer: https://symfony.com/doc/5.x/components/serializer.html#component-serializer-normalizers. Example:

Imagine we have a new data structure: TestDataObject with the following definition:

use Ibexa\Contracts\Core\Repository\Values\Content\Location;

final readonly class TestDataObject
{
    public function __construct(
        public string $string,
        public int $int,
        public ?Location $apiLocation = null,
    ) {
    }
}

One can notice the Location object which in fact is an instance of Ibexa's Location, it hints that Symfony's Serializer will be able to work with such data. Instead of writing a dedicated ValueObjectVisitor one can introduce a normalizer:

final class TestDataObjectNormalizer implements NormalizerInterface, NormalizerAwareInterface
{
    use NormalizerAwareTrait;

    /**
     * @return array<string, mixed>
     */
    public function normalize(mixed $object, ?string $format = null, array $context = []): array
    {
        assert($object instanceof TestDataObject);

        $data = [
            'string' => $object->string,
            'int' => $object->int,
            'location' => null,
        ];

        if ($object->apiLocation instanceof Location) {
            $normalizedLocation = $this->normalizer->normalize($object->apiLocation, $format);
            $data['location'] = $normalizedLocation['#'] ?? $normalizedLocation['Location'] ?? null;
        }

        return $data;
    }

    public function supportsNormalization(mixed $data, ?string $format = null): bool
    {
        return $data instanceof TestDataObject;
    }
}

We can notice how easy it is to normalize Ibexa's Location object, one simply does:

$normalizedLocation = $this->normalizer->normalize($object->apiLocation, $format);

and because of how internal Ibexa's AdapterNormalizer works it will provide already normalized data that will be normalized using backwards-compatible value object visitors. This is really important and powerful mechanism as we are allowed to normalize both old data that use value object visitors and new data that don't have value object visitors but have dedicated normalizers instead together.

Finally, to obtain the final encoded data in json/xml format from PHP object representation it really comes down to:

        $dataObject = new TestDataObject(
            'some_string',
            1,
            $this->locationService->loadLocation(2),
        );

        $serializedData = $this->serializer->serialize($dataObject, 'json');

Symfony's Serializer will work internally with Ibexa's Generators and Value Object Visitors to normalize and encode data for the specific format (json in case of the above example) - the final result can be previewed here: https://github.com/ibexa/rest/blob/23bb684b4dd9d68ec5537d5fd125ebdaf26a983b/tests/integration/Serializer/_snapshot/TestDataObject.json.

One must remember that normalizers that normalize Ibexa's data objects has to be registered with the following tag:

services:
    App\Normalizer\TestDataObjectNormalizer:
        tags:
            - { name: ibexa.rest.serializer.normalizer }

It is important for the app safety and performance to NOT autoconfigure these normalizers. If autoconfigured those will receive serializer.normalizer tags and will be injected into the main application serializer, which is ill-advised.

QA:

Technically speaking nothing for the end-user should change, app should behave the same as before. It's definitely worth checking how REST endpoints behave with this implementation (including Scheduler ones) + testing some test data using normalizer as shown in the description above.

@barw4 barw4 self-assigned this Aug 12, 2024
@barw4 barw4 changed the title [POC] [Work in progress] Add Symfony's Serializer support [POC] Add Symfony's Serializer support Aug 26, 2024
@barw4 barw4 requested a review from a team August 26, 2024 12:59
@Steveb-p
Copy link
Contributor

Since I consulted this approach I'll give some explanations too.

Whole concept relies on using Serializer as primary entry point for data normalization.

While analyzing how Visitors worked with Generator, it became apparent that Generator basically serves as a Data Container that changes it's internal "pointer" as instructed by Visitor. This is "shared" between XML and JSON Generators, with the difference being that XML outputs it's state during this process.

So, since the primary difference between Symfony normalization and our Visitor's processing is that Symfony returns data and Visitors pass around Generator (as data storage), we can give Visitors a "fake" Generator - which whole purpose is to store data - and use it's state at the end as normalization result. This results in both processes behaving in a similar way.

@barw4 barw4 force-pushed the serializer-support branch from b39b477 to 0ddbb6d Compare August 27, 2024 14:48
@barw4 barw4 changed the title [POC] Add Symfony's Serializer support [POC] IBX-8190: Add Symfony's Serializer support Aug 29, 2024
@barw4 barw4 force-pushed the serializer-support branch from e71209e to ab72638 Compare September 4, 2024 14:35
@barw4 barw4 requested a review from Steveb-p November 4, 2024 09:27
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

A lot of solid work 💪

Some remarks & questions:

src/bundle/Resources/config/services.yml Outdated Show resolved Hide resolved
src/contracts/Output/Generator.php Show resolved Hide resolved
src/contracts/Output/Generator.php Outdated Show resolved Hide resolved
src/contracts/Output/Generator.php Outdated Show resolved Hide resolved
Comment on lines 134 to 135
$this->json->$name = $object;
$this->json = $object;
Copy link
Member

Choose a reason for hiding this comment

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

The first line doesn't make sense here, as it's immediately overridden by the next line.

tests/integration/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
use NormalizerAwareTrait;

/**
* @return array<string, mixed>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return array<string, mixed>
* @return array<string, mixed>
*
* @throws \Symfony\Component\Serializer\Exception\ExceptionInterface

tests/integration/bootstrap.php Outdated Show resolved Hide resolved
@alongosz alongosz requested a review from a team November 4, 2024 13:50
@barw4 barw4 requested review from alongosz and Steveb-p November 4, 2024 23:18
@barw4 barw4 added the Doc needed The changes require some documentation label Nov 26, 2024
@barw4 barw4 requested a review from alongosz November 26, 2024 11:47
@alongosz alongosz requested a review from a team November 26, 2024 13:46
@Steveb-p
Copy link
Contributor

@barw4

  services:
    App\Normalizer\TestDataObjectNormalizer:
        tags:
            - { name: ibexa.rest.serializer.normalizer }

I'd like to remind once more that it is important for the app safety and performance to NOT autoconfigure these. If autoconfigured those will receive serializer.normalizer tags and will be injected into the main application serializer, which is ill-advised.

@barw4
Copy link
Contributor Author

barw4 commented Nov 26, 2024

@barw4

  services:
    App\Normalizer\TestDataObjectNormalizer:
        tags:
            - { name: ibexa.rest.serializer.normalizer }

I'd like to remind once more that it is important for the app safety and performance to NOT autoconfigure these. If autoconfigured those will receive serializer.normalizer tags and will be injected into the main application serializer, which is ill-advised.

@Steveb-p thanks Paweł, I'll make a note in the description.

src/contracts/Output/Generator.php Outdated Show resolved Hide resolved
src/contracts/Output/Generator.php Outdated Show resolved Hide resolved
src/contracts/Output/ValueObjectVisitorResolver.php Outdated Show resolved Hide resolved
src/contracts/Output/VisitorAdapterNormalizer.php Outdated Show resolved Hide resolved
src/contracts/Output/VisitorAdapterNormalizer.php Outdated Show resolved Hide resolved
src/lib/Output/Generator/Data/ArrayList.php Outdated Show resolved Hide resolved
src/lib/Output/Generator/Json/JsonObject.php Outdated Show resolved Hide resolved
@barw4 barw4 requested a review from konradoboza November 26, 2024 15:07
Copy link

sonarcloud bot commented Nov 26, 2024

Copy link
Contributor

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

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

Great work!

Copy link

@katarzynazawada katarzynazawada left a comment

Choose a reason for hiding this comment

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

  1. Some endpoints return 500 error
  • GET localhost/api/ibexa/v2/user/groups?roleId=1

<!-- Could not normalize object of type &quot;TypeError&quot;, no supporting normalizer found. (500 Internal Server Error) -->

  • PATCH localhost/api/ibexa/v2/user/groups/1/5/12

<!-- Could not normalize object of type &quot;TypeError&quot;, no supporting normalizer found. (500 Internal Server Error) -->

  • POST localhost/api/ibexa/v2/user/groups/1/5/13/users

<!-- Could not normalize object of type &quot;TypeError&quot;, no supporting normalizer found. (500 Internal Server Error) -->

  • GET/POST localhostapi/ibexa/v2/orders/orders
    /orders/order/{identifier}
    <errorCode>500</errorCode>
    <errorMessage>Internal Server Error</errorMessage>
    <errorDescription>Could not normalize object of type "DateTimeImmutable", no supporting normalizer found.</errorDescription>
    <trace>
        <![CDATA[#0 /Users/kzawada/Projects/5.0.0-dev2/vendor/ibexa/rest/src/lib/Output/Normalizer/JsonObjectNormalizer.php(46): Symfony\Component\Serializer\Serializer->normalize(Object(DateTimeImmutable), 'xml', Array)

  • POST localhost/api/ibexa/v2/orders/order/716a317a-305e-46b1-b87c-e37626efe9c3/shipments
    <errorCode>500</errorCode>
    <errorMessage>Internal Server Error</errorMessage>
    <errorDescription>Could not normalize object of type "Money\Currency", no supporting normalizer found.</errorDescription>
  • GET/PATCH localhost/api/ibexa/v2/shipments/abc
    <errorCode>500</errorCode>
    <errorMessage>Internal Server Error</errorMessage>
    <errorDescription>Could not normalize object of type "Money\Currency", no supporting normalizer found.</errorDescription>
    <trace>
        <![CDATA[#0 /Users/kzawada/Projects/5.0.0-dev2/vendor/ibexa/rest/src/lib/Output/Normalizer/JsonObjectNormalizer.php(46): Symfony\Component\Serializer\Serializer->normalize(Object(Money\Currency), 'xml', Array)

  • GET/POST localhost/api/ibexa/v2/corporate/companies
    <errorCode>500</errorCode>
    <errorMessage>Internal Server Error</errorMessage>
    <errorDescription>Could not normalize object of type "Ibexa\FieldTypeAddress\FieldType\Value", no supporting normalizer found.</errorDescription>
    <trace>

  • POST localhost/api/ibexa/v2/product/catalog/catalogs
    <errorCode>500</errorCode>
    <errorMessage>Internal Server Error</errorMessage>
    <errorDescription>Could not normalize object of type "DateTime", no supporting normalizer found.</errorDescription>
    <trace>
        <![CDATA[#0 /Users/kzawada/Projects/5.0.0-dev2/vendor/ibexa/rest/src/lib/Output/Normalizer/JsonObjectNormalizer.php(46): Symfony\Component\Serializer\Serializer->normalize(Object(DateTime), 'xml', Array)


  1. Using serializer in Controller and languages list returns data with ENCODER_CONTEXT that should not be there. In both json and xml format. Example json

^ "{"string":"dsadadasdsa","id":2131,"languages":[{"Language":{"_media-type":"application\/vnd.ibexa.api.Language+json","_href":"\/api\/ibexa\/v2\/languages\/eng-GB","languageId":2,"languageCode":"eng-GB","name":"English (United Kingdom)"},"ENCODER_CONTEXT":[]},{"Language":{"_media-type":"application\/vnd.ibexa.api.Language+json","_href":"\/api\/ibexa\/v2\/languages\/eng-US","languageId":2048,"languageCode":"eng-US","name":"English (United States)"},"ENCODER_CONTEXT":[]},{"Language":{"_media-type":"application\/vnd.ibexa.api.Language+json","_href":"\/api\/ibexa\/v2\/languages\/fre-FR","languageId":524288,"languageCode":"fre-FR","name":"French (France)"},"ENCODER_CONTEXT":[]},{"Language":{"_media-type":"application\/vnd.ibexa.api.Language+json","_href":"\/api\/ibexa\/v2\/languages\/ger-DE","languageId":33554432,"languageCode":"ger-DE","name":"German (Germany)"},"ENCODER_CONTEXT":[]},{"Language":{"_media-type":"application\/vnd.ibexa.api.Language+json","_href":"\/api\/ibexa\/v2\/languages\/pol-PL","languageId":2147483648,"languageCode":"pol-PL","name":"Polish"},"ENCODER_CONTEXT":[]}]} ◀"

  1. Actions in the content tree do not work properly. Example for trashing content
Screenshot 2024-12-11 at 14 55 15
  1. Error when opening image picker or using images in the system
Screenshot 2024-12-13 at 11 22 56
  1. Error in admin-ui integration tests that could be related
Ibexa\Tests\Integration\AdminUi\REST\GetContentTreeExtendedInfoTest::testEndpoint with data set "GET /api/ibexa/v2/location/tree/2/extended-info accepting xml format without payload" (Ibexa\Contracts\Test\Rest\Request\Value\EndpointRequestDefinition Object (...))
Failed asserting that two DOM documents are equal.
--- Expected
+++ Actual
@@ @@
   <function name="create">
     <hasAccess>true</hasAccess>
     <restrictedContentTypeIdsList>
-      <value>1</value>
-      <value>16</value>
+      <restrictedContentTypeIds>1</restrictedContentTypeIds>
+      <restrictedContentTypeIds>16</restrictedContentTypeIds>
     </restrictedContentTypeIdsList>
     <restrictedLanguageCodesList>
-      <value>eng-GB</value>
-      <value>eng-US</value>
+      <restrictedLanguageCodes>eng-GB</restrictedLanguageCodes>
+      <restrictedLanguageCodes>eng-US</restrictedLanguageCodes>
     </restrictedLanguageCodesList>
   </function>
   <function name="edit">
     <hasAccess>true</hasAccess>
     <restrictedLanguageCodesList>
-      <value>eng-GB</value>
+      <restrictedLanguageCodes>eng-GB</restrictedLanguageCodes>
     </restrictedLanguageCodesList>
   </function>
   <function name="delete">
@@ @@
     <hasAccess>false</hasAccess>
   </function>
   <previewableTranslations>
-    <value>eng-GB</value>
+    <values>eng-GB</values>
   </previewableTranslations>
 </ContentTreeNodeExtendedInfo>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc needed The changes require some documentation Feature New feature request Ready for QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants