Do not try do retrieve object in ItemNormalizer $context['resource_class'] is not defined #2326

lyrixx · 2018-11-13T17:32:16Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

We store some document in Elasticsearch and we hydrate them with the Serializer.
But the ItemNormalizer raise an error because the resource_class is not defined.

teohhanhui · 2018-11-13T22:50:47Z

I'm not sure I understand the problem here. The AbstractItemNormalizer / ItemNormalizer only supports resource classes. So $context['resource_class'] should always be set.

teohhanhui · 2018-11-13T22:55:30Z

It's even explicitly set here:

core/src/Serializer/AbstractItemNormalizer.php

Lines 125 to 127 in 516c9c7

    
           if (!isset($context['resource_class'])) { 
        
               $context['resource_class'] = $class; 
        
           }

lyrixx · 2018-11-13T23:42:03Z

I guess you can simply try to decode some data with an id param and with an empty context to reproduce.

but basically this condition evaluate to true

core/src/Serializer/ItemNormalizer.php

Line 35 in 516c9c7

if (isset($data['id']) && !isset($context[self::OBJECT_TO_POPULATE])) {

so it jumps in updateObjectToPopulate, an InvalidArgumentException exception is thrown, and finally this line breaks everything:

core/src/Serializer/ItemNormalizer.php

Line 52 in 516c9c7

    
           foreach ($this->propertyNameCollectionFactory->create($context['resource_class'], $context) as $propertyName) {

Did I miss something?

soyuka · 2018-11-14T09:12:43Z

Can't you give a context? What's your use case?

lyrixx · 2018-11-14T09:23:41Z

@soyuka sure, it's already explained in the PR description.

dunglas · 2018-11-14T09:25:21Z

Normalizers must work even if no context is provided. The fix looks good to me. Would you mind adding an unit test to prevent regressions @lyrixx?

lyrixx · 2018-11-14T09:26:22Z

@dunglas Sure 👍 I will have to clone the projet locally then 😂

dunglas · 2018-11-14T09:27:47Z

src/Serializer/ItemNormalizer.php

@@ -32,7 +32,7 @@ class ItemNormalizer extends AbstractItemNormalizer
    public function denormalize($data, $class, $format = null, array $context = [])
    {
        // Avoid issues with proxies if we populated the object
-        if (isset($data['id']) && !isset($context[self::OBJECT_TO_POPULATE])) {
+        if (isset($data['id']) && !isset($context[self::OBJECT_TO_POPULATE]) && isset($context['resource_class'])) {


Or maybe that a better fix would be to resolve this value if not provided by calling ResourceClassResolver::getResourceClass()? Something like $context['resource_class'] ?? $this->resourceClassResolver($data) in updateObjectToPopulate()?

I disagree. If it's not provided, it means we are outside the nominal api platform workflow.
We should not guess

We could expect from a denormalizer that it updates relations by default, isn't it?

You lost me. Sorry. If we set $context['resource_class'], the code will still raise an exception. Here I don't want to fetch something via the iriConverter.

dunglas · 2018-11-14T09:28:33Z

Or you can just run composer update --prefer-source on the project you're working on :P

lyrixx · 2018-11-14T09:29:00Z

@dunglas I made the initial patch on github ;) But don't worry, I know how to update a patch

teohhanhui · 2018-11-14T09:47:11Z

As I've already mentioned, the ItemNormalizer is for use with resource classes. If you're using it with a non-resource class, that's not a supported use case.

@dunglas I agree we should set $context['resource_class'] if it's missing.

lyrixx · 2018-11-14T09:49:28Z

@teohhanhui Heuuuuu, I don't get it

1.crearte a new

install api platform
get some data from an API (let's say elasctisearch)
hydrate your object with symfony default serializer
💥

Since API Platform modify the default serialization, you can not say:

If you're using it with a non-resource class, that's not a supported use case.

lyrixx · 2018-11-14T09:51:19Z

I have added an unit test

teohhanhui · 2018-11-14T10:16:51Z

Can you open a PR with a failing Behat test? Because ItemNormalizer::supportsDenormalization should return false for non-resource classes.

get some data from an API (let's say elasctisearch)

hydrate your object with symfony default serializer

Need more context. Is it inside an API Platform operation? Are you doing it through a data provider?

lyrixx · 2018-11-14T11:19:09Z

Sure, it's quite simple: lyrixx/test@085fe66

To make things crystal clear: we use the same object for the "public" API and for Elasticsearch storage

teohhanhui · 2018-11-14T15:04:40Z

If you declare a class as an API Resource, it's normal that our ItemNormalizer would handle it.

We store some document in Elasticsearch and we hydrate them with the Serializer.

In your case, you just want to use Symfony's default ObjectNormalizer, right?

lyrixx · 2018-11-14T15:06:42Z

@teohhanhui Yes. And other Normalizers that could live in my application.
I don't see what you have in mind... is it a big deal to add a safe guard here?

teohhanhui · 2018-11-14T15:19:31Z

is it a big deal to add a safe guard here?

Please bear with us in trying to figure out the best way to go about it.

So far I'm not convinced that it should be done this way. I'm inclined more towards @dunglas' suggestion of setting $context['resource_class'] if it's missing.

As for your specific use case:

If you want it to be an API Platform operation: Have you considered using a data provider? (Sorry for repeating myself.)
If you want it to be a custom operation: Don't use the same class. It's quite honestly nonsensical to use ItemNormalizer for denormalizing your object in your above use case.

lyrixx · 2018-11-14T16:07:51Z

We are already using a DataProvider to retrieve data from ES. We go plain array from ES. But we need to hydrate objects. That's is the blocking part
It's not the case

It's quite honestly nonsensical to use ItemNormalizer for denormalizing your object in your above use case.

I did not choose to use it, it's build in with API Platform.

teohhanhui · 2018-11-14T16:20:59Z

Okay, I think I get it now. And it should be a supported use case, yes. The question is how? 🤔

teohhanhui · 2018-11-14T17:52:01Z

@lyrixx A possible workaround could be to pass a custom $format, and register a denormalizer which supports that custom $format, at a higher priority than ItemNormalizer.

lyrixx · 2018-11-14T17:53:22Z

In my code, or in ApiPlatform ?

teohhanhui · 2018-11-14T17:54:06Z

In your code. 😄

lyrixx · 2018-11-14T17:54:11Z

For now I used a very simple workaround

            $this->serializer->denormalize($logResult['_source'], Log::class, 'json', [
                'object_to_populate' => $log = new Log(),
                'groups' => ['elasticsearch'],
            ]);

dunglas · 2018-11-14T21:17:05Z

The patch is good enough for now, it doesn't break anything, and prevent an error that must be handled anyway. Could you just rebase against 2.3 please? (because it's a bug fix)

teohhanhui · 2018-11-15T12:46:12Z

@dunglas I think it's the wrong fix. As we've discussed, $context['resource_class'] should be set if it's missing.

soyuka · 2018-11-15T17:23:19Z

Maybe something like this?

if (!isset($context['resource_class'])) {
  log(Something about this not being right);
  return parent::normalize(...);
}

This patch also feels wrong to me, on the other hand it doesn't harm so I'm mitigated :p.

dunglas · 2018-11-18T16:42:38Z

Can we merge this until we find a better fix @api-platform/core-team? Having a hard failure is worst...

soyuka · 2018-11-19T16:23:08Z

If we log something because it's not normal as I suggested (#2326 (comment)) I'm 👍 to merge.

…ass'] is not defined We store some document in Elasticsearch and we hydrate them with the Serializer. But the `ItemNormalizer` raise an error because the `resource_class` is not defined.

…Normalizer

lyrixx · 2018-11-20T09:38:12Z

I added a log line

teohhanhui

Still think it's the wrong fix. The correct fix should be to set $context['resource_class'] when it's missing.

lyrixx · 2018-11-22T09:59:08Z

Ok, Let's try what you suggest to prove it does not work:

If I set $context['resource_class'] when it's missing, I got the following error:

Item not found for "/api/foobars/123".

Legit, so I will have to implement a DataProvider. When I will implement it, I will need to deserialize my data because it's stored as JSON (in Elasticsearch, but we don't care). And ... 💥 Infinite loop.

At some point you said:

If you're using it with a non-resource class, that's not a supported use case.

Well:

The item normalizer is installed by default with API platform
The code is not robust: You could either check if the key is defined before using it, or throw an exception if the key is missing. The actual code is based on assumptions. This is wrong too.

You also said:

If you want it to be a custom operation: Don't use the same class. It's quite honestly nonsensical to use ItemNormalizer for denormalizing your object in your above use case.

So your suggestion, in order to not merge this PR, is that I need to duplicate all my model class to get one form API Platform, and another one for ES <-> PHP, and a layer that will my map my two model ?
It sound really overkill. Do you always use this pattern with doctrine?

I'm sorry, but you are making things really difficult. You ask for things, I did them, then you refused it again. It's really not king.

Thus, I will tell you what is the real fix could be:

1/ Update Symfony with:

 interface NormalizerInterface
 {
-     public function supportsNormalization($data, $format = null);
+     public function supportsNormalization($data, $format = null, array $context = array());
 }

2/ update ItemNormalizer::supportsNormalization to return true only if $context['resource_class'] is set.

teohhanhui · 2018-11-22T10:01:18Z

No. I recognize that your use case is a valid one and it's unfortunate that it's not working at the moment (but you have found workarounds).

What I'm saying is that it's bad for us to have such workarounds in our code.

dunglas · 2018-11-22T12:44:50Z

1/ Update Symfony with:

It's already supported in Symfony: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Serializer/Normalizer/ContextAwareNormalizerInterface.php#L26

2/ update ItemNormalizer::supportsNormalization to return true only if $context['resource_class'] is set.

I don't agree with this one, it's totally valid to use API Platform to serialize in JSON-LD outside of an API context (in a worker, to send data to a Kafka, an Elastic...).

Legit, so I will have to implement a DataProvider. When I will implement it, I will need to deserialize my data because it's stored as JSON (in Elasticsearch, but we don't care). And ... 💥 Infinite loop.

This specific problem can be fixed like this: https://api-platform.com/docs/core/data-providers/#injecting-the-serializer-in-an-code-classlanguage-textitemdataprovidercode

It's a hack, and the good solution would be to use a specific Serializer instance in API Platform (not the same one than the default serializer of Symfony). It would be cleaner, but it requires some more work in SF (creating a factory to build properly configured serializer services). I'll do it when I'll have some free time.

In the meantime, I in favor of merging @lyrixx patch. We should never throw an unhandled PHP error, it's not robust code...

teohhanhui · 2018-11-22T13:21:22Z

We should never throw an unhandled PHP error

Agree. But this patch is not doing the right thing. Setting $context['resource_class'] (if missing) would be the right thing to do, and as @lyrixx has noted, it throws a proper exception even though it does not address his use case.

lyrixx · 2018-11-22T15:03:38Z

About the point 3, I'm not talking about a DIC circular reference,
I'm talking about the serializer that call use the ItemNoramlizer, that will use the DataPRovider, that will use the serializer (here is the loop)

joelwurtz · 2018-11-30T11:02:24Z

@teohhanhui I don't understand in your solution how can someone deserialize something that must not use ApiPlatform, our process is very simple:

Fetch Data From Elastisearch -> Deserialize it (we don't want API Platform here, it's just a stupid object) -> Do Some Work -> Serialize It (with api platform)

With what your are proposing the only solution would be to use another Serializer, I would be fine with that if API Platform did not replace the one from Symfony. In our use case we have installed API Platform after doing the deserialization with Elasticsearch, and changing a lot of our code and DIC for that may be painful for some users.

IMO A viable solution would be to change the way API Platform is injected :

Not replacing the default serializer for Symfony
Injecting HAL Normalizer / JSON-LD Normalizer into the one of Symfony
Introduce a new context key : json_format
HAL Normalizer would only work if json_format is 'hal'
JSON-LD Normalizer would only work if json_format is 'json-ld'
Use the json format key in all services of API Platform which need to serialize / deserialize the data

dunglas · 2018-12-20T08:21:25Z

@lyrixx isn't #2399 a better fix (no logs)?

@api-platform/core-team we need to take a decision regarding this bug, it's annoying for end users. I'll merge this one or #2399 today.

lyrixx · 2018-12-20T11:02:18Z

@dunglas #2399 Functionally it works but this is a major waste of resource:

We want to unserialize an object
This normalizer will try to update an existing object
This line $context[self::OBJECT_TO_POPULATE] = $this->iriConverter->getItemFromIri((string) $data['id'], $context + ['fetch_data' => true]); will throw an exception. Exception are costly
The exception will be catched and this line will be executed successfully $context[self::OBJECT_TO_POPULATE] = $this->iriConverter->getItemFromIri(sprintf('%s/%s', $this->iriConverter->getIriFromResourceClass($context['resource_class']), $data[$identifier]), $context + ['fetch_data' => true]);
So in the end, even before the denormalization occurs, the object is already hydrated via an heavy processing (IriConverter is nice, but CPU intensive (router::match)

As I said many time, I really thing this patch is correct, and we should not try to goes through API Platform Normalizer at all cost. There are legitimate case where we don't want IRI stuff. Here is one example.

And again, I know we could create another serializer + normalizers but it's really boring to have to put many lines of services configuration in my project, where a simple if could solve everything.

On some "page" of our application, we retrieve 100 objects. So there will be 100 Exception + 100 Router::match :(

dunglas · 2018-12-20T11:03:29Z

Let's merge

dunglas · 2018-12-20T11:03:37Z

Thanks @lyrixx

lyrixx · 2018-12-20T11:03:39Z

Thanks you

dunglas reviewed Nov 14, 2018

View reviewed changes

lyrixx force-pushed the patch-1 branch from b3ac746 to c46bac0 Compare November 14, 2018 09:42

dunglas approved these changes Nov 14, 2018

View reviewed changes

Do not try do retrieve object in ItemNormalizer $context['resource_cl…

f19c5d9

…ass'] is not defined We store some document in Elasticsearch and we hydrate them with the Serializer. But the `ItemNormalizer` raise an error because the `resource_class` is not defined.

lyrixx changed the base branch from master to 2.3 November 20, 2018 09:20

lyrixx force-pushed the patch-1 branch from 3bd78c9 to f19c5d9 Compare November 20, 2018 09:20

Added some log when the $context['resource_class'] is missing in Item…

fae3f1d

…Normalizer

lyrixx force-pushed the patch-1 branch from 12ce4ba to fae3f1d Compare November 20, 2018 09:30

Simperfit approved these changes Nov 20, 2018

View reviewed changes

soyuka approved these changes Nov 20, 2018

View reviewed changes

teohhanhui suggested changes Nov 21, 2018

View reviewed changes

lyrixx mentioned this pull request Nov 30, 2018

Deactivate ItemNormalizer when not in ApiPlatform context #2006

Closed

soyuka mentioned this pull request Dec 20, 2018

fix #2158 - denormalize items without context and no Iri #2399

Closed

dunglas merged commit e51024c into api-platform:2.3 Dec 20, 2018

lyrixx deleted the patch-1 branch December 20, 2018 11:04

dunglas mentioned this pull request Dec 22, 2018

The denormalize method in the ItemNormalizer does not set the resource_class correctly. #2158

Closed

nicodmf mentioned this pull request Jun 1, 2021

fix(serializer): collection denormalization from object linked to doctrine entities #4301

Closed

Do not try do retrieve object in ItemNormalizer $context['resource_class'] is not defined #2326

Do not try do retrieve object in ItemNormalizer $context['resource_class'] is not defined #2326

Conversation

lyrixx commented Nov 13, 2018

teohhanhui commented Nov 13, 2018

teohhanhui commented Nov 13, 2018

lyrixx commented Nov 13, 2018

soyuka commented Nov 14, 2018

lyrixx commented Nov 14, 2018

dunglas commented Nov 14, 2018

lyrixx commented Nov 14, 2018

dunglas Nov 14, 2018

Choose a reason for hiding this comment

lyrixx Nov 14, 2018

Choose a reason for hiding this comment

dunglas Nov 14, 2018

Choose a reason for hiding this comment

lyrixx Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

dunglas commented Nov 14, 2018

lyrixx commented Nov 14, 2018 • edited Loading

teohhanhui commented Nov 14, 2018

lyrixx commented Nov 14, 2018

lyrixx commented Nov 14, 2018

teohhanhui commented Nov 14, 2018 • edited Loading

lyrixx commented Nov 14, 2018 • edited Loading

teohhanhui commented Nov 14, 2018

lyrixx commented Nov 14, 2018

teohhanhui commented Nov 14, 2018

lyrixx commented Nov 14, 2018

teohhanhui commented Nov 14, 2018

teohhanhui commented Nov 14, 2018

lyrixx commented Nov 14, 2018

teohhanhui commented Nov 14, 2018

lyrixx commented Nov 14, 2018

dunglas commented Nov 14, 2018

teohhanhui commented Nov 15, 2018

soyuka commented Nov 15, 2018

dunglas commented Nov 18, 2018

soyuka commented Nov 19, 2018

lyrixx commented Nov 20, 2018

teohhanhui left a comment

Choose a reason for hiding this comment

lyrixx commented Nov 22, 2018 • edited Loading

teohhanhui commented Nov 22, 2018 • edited Loading

dunglas commented Nov 22, 2018 • edited Loading

teohhanhui commented Nov 22, 2018 • edited Loading

lyrixx commented Nov 22, 2018

joelwurtz commented Nov 30, 2018

dunglas commented Dec 20, 2018

lyrixx commented Dec 20, 2018 • edited Loading

dunglas commented Dec 20, 2018

dunglas commented Dec 20, 2018

lyrixx commented Dec 20, 2018

lyrixx Nov 14, 2018 •

edited

Loading

lyrixx commented Nov 14, 2018 •

edited

Loading

teohhanhui commented Nov 14, 2018 •

edited

Loading

lyrixx commented Nov 14, 2018 •

edited

Loading

lyrixx commented Nov 22, 2018 •

edited

Loading

teohhanhui commented Nov 22, 2018 •

edited

Loading

dunglas commented Nov 22, 2018 •

edited

Loading

teohhanhui commented Nov 22, 2018 •

edited

Loading

lyrixx commented Dec 20, 2018 •

edited

Loading