ResourceIdentifier::getVirtualOrMissingResourceIdentifier() ignores field aliases; causes $relatable_resource_types field to be empty and results in an error

Oh, HAH! This seems like a stupid silly mistake! Great find. And thanks for the patch! 👍

How did you run into this? We'll need test coverage to prevent this from ever regressing.

Interesting... What is going on in this project is that when the code is trying to get the relatable resources types, it uses an English field name. However, the definition that is available in the $relatable_resource_types is a Dutch language field name. I suspect this field was renamed at some point in the past, but the rename was skipped at some point. Will investigate further. As to how to reproduce this condition in a test...
Edit: This was not actually the case, see next comment.

OK, no, actually. The Dutch language name is the resource type public name (per \Drupal\jsonapi\ResourceType\ResourceTypeRepository::calculateRelatableResourceTypes()). Which is being used as the field name in \Drupal\jsonapi\ResourceType\ResourceType::getRelatableResourceTypesByField().

So I guess the rabbit hole goes a little deeper than it first appeared.

Fascinating! Thanks for digging deeper.

I suspect this field was renamed at some point in the past, but the rename was skipped at some point.

What does "rename was skipped at some point" mean? And does #7 mean that that "skipped" thing wasn't the case?

What I meant was that the rename might not have been complete somehow (i.e. done in some relevant config objects, but not all). Yes, #7 indeed means the rename/skip thing was a red herring. Sorry about that.

I see. So your config could be in an inconsistent state. I wonder if we have diagnostic tooling for that in core or contrib. Pinging @Gábor Hojtsy about that.

Hmm. You think? AFAICT, it's a fairly simple matter of using one thing in place of another (by the code, that is, trying to address the array of relatable resource types with a field name while the keys are actually resource types - which could work as long as your resources are named after the reference fields you are using). It doesn't appear to have ill effects at this point (at least in our particular situation), so we went ahead with the patch to at least get the troublesome endpoint going again, but it seems like a logic error.

I agree the patch looks like a logic error, but what you describe in #9 does sound like a bug in config handling, either in core or your own process. That's why I wrote #10 :)

I'd love to commit the patch, but I can't without a regression test or at the very very least steps to reproduce the problem.

@gabesullice is now on paternity leave, so he won't deal with this.

I still need steps to reproduce or — ideally — a failing test.

The patch in #2 causes the regression that that #2984647: Dangling entity references in entity reference field with multiple possible target bundles: results in exception/500 response introduced to fail. Because it fixes it incorrectly.

Here's the correct fix :)

Actually, no, what's in HEAD is correct AFAICT, because there is no way for

$relatable_resource_types = $resource_type->getRelatableResourceTypesByField($field->getName());

to return an empty array. If that statement of mine is correct, then the change in #14 is unnecessary.

I've asked for steps to reproduce about a month ago. I still don't know how to reproduce this, even after having spent an hour on the related code.

Meanwhile, here's a patch to verify my assumptions. If it passes tests, my understanding is confirmed.

Green. My assumptions are confirmed. Changing to Support request.

Just so you know, #9 is not what happened, as I said before, it was an (as it turns out) incorrect hypotheses, a red herring as I said before.

As far as I have been able to debug up to now, the crux of the matter is actually in #7, a difference in naming between the resource and the reference field.

I will see if we can get time to do further debugging and preferably come up with a way to reproduce in a more controlled environment.

#18: Thanks for confirming that again — it wasn't 100% clear to me until your comment just now :) Removing the tags that I added based on #9, since they're now certainly inaccurate.

Thanks for helping to move this issue forward :) I'd like to see this properly resolved, just like you.

Sorry, I just edited #18, having missed #19. What I added to #18:

As far as I have been able to debug up to now, the crux of the matter is actually in #7, a difference in naming between the resource and the reference field.

That is, in our particular case. The code works as long as they are named the same, but they shouldn't have to be.

I apologize for lacking the time at the office to provide a full reproduction scenario, but in my case the error message in the original post has appeared after I have deleted some entities which were referenced in another entity's Entity Reference field.

I am also running JSON:API Extras to rename the fields in the API output, if that has any bearing on anything.

Specifically, I had the setup of a node with an Entity Reference field that refers to Media entities, and some Media entities were deleted. I could no longer access the overview of all nodes in the API after that.

The change in #14 fixed the issue for me, so I'm cautiously setting this to RTBC. YMMV if that is the right spot to fix the problem, or if it's rooted somewhere deeper.

This still needs a regression test.

Marked #3055441: Invalid condition for getting $resource_type in ResourceIdentifier :: getVirtualOrMissingResourceIdentifier as a duplicate of this.

Marked #3056596: JSON:API call to a member function getTypeName() on boolean as a duplicate of this.

Increasing priority since more people seem to be running into this, despite us still not having concrete steps to reproduce.

To reproduce the #3056596: JSON:API call to a member function getTypeName() on boolean you can remove the taxonomy term, referenced from a node.

Thanks! Any chance you could add a failing test case to JsonApiRegressionTest? 😊🙏

+++ b/src/JsonApiResource/ResourceIdentifier.php
@@ -417,6 +417,7 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
     $relatable_resource_types = $resource_type->getRelatableResourceTypesByField($field->getName());

Well, I was wrong. The issue here is that $resource_type->getRelatableResourceTypesByField() expects the public name of a field but we pass an internal one.

Wouldn't it be a good idea to add $field_name = $this->getPublicName($field_name); to the getRelatableResourceTypesByField()?

#29 here is the failing test case - https://www.drupal.org/project/jsonapi_extras/issues/3035544#comment-131....

https://www.drupal.org/project/jsonapi_extras/issues/3035544#comment-131... - another patch, showing that applying #31 resolves the problem (I don't know why DrupalCI say it's failed, whilst tests were passed - https://www.drupal.org/pift-ci-job/1304876).

A combination of #16 + #31 because it's always good to assert.

Thanks everyone for your help on this!

@BR0kEN, I think you nailed it with the internal name vs. public name. Good job.

@Wim Leers, I agree that we should add that assert. I also think we should throw a \LogicException in getRelatableResourceTypesByField when the field is not recognized, instead of returning an empty array. That would help with future debugging and it might help in #2996114: Argument 2 passed to Drupal\jsonapi\Routing\Routes::Drupal\jsonapi\Routing\{closure}() must be an instance of Drupal\jsonapi\ResourceType\ResourceType, NULL given as well (I have to confirm this).

Finally, while mentally stepping through the code, I found another bug that #36 doesn't address. getRelatableResourceTypesFromFieldDefinition returns the result of:

return array_map(function ($target_bundle) use ($entity_type_id, $resource_types) {
  $type_name = "$entity_type_id--$target_bundle";
  return isset($resource_types[$type_name]) ? $resource_types[$type_name] : NULL;
}, $target_bundles);

Notice that $type_name is constructed by concatenating the entity type ID with the bundle ID. However, $resource_types is created with:

return array_merge($resource_types, [
  $resource_type->getTypeName() => $resource_type,
]);

Meaning if the type name has been aliased, we'll have NULL values in the relatable resource type array and nothing to filter those out.

Per #37, this patch needs additional work.

This was reported again, this time at #3089393: Exception in JSON:API fetching content..

I think the main problem is that the method returns a TYPE or NULL. Why not change it to always return TYPE, even if it is a NULL-type (null object pattern). This way all the client code for the method will not need to do the IF check every time and prevent anyone from forgetting the IF code.

@ndobromirov Could you turn that proposal into a patch? 😇

@Wim do you think it is feasible?

After initial review of the classes and how they are used, it seems that there are no interfaces on resource types, so this can not be done easily (on high level review), so I do not think it is feasible now.

Depending on the direction we decide to go, it might turn into multiple issues.

This issue has gotten off track.

What this issue needs:

1. Tests or steps to reproduce the error so a test can be written

That is all.

@BR0kEN's patch looks good to me. We just need to prove that it actually fixes something. #2996114: Argument 2 passed to Drupal\jsonapi\Routing\Routes::Drupal\jsonapi\Routing\{closure}() must be an instance of Drupal\jsonapi\ResourceType\ResourceType, NULL given is the issue where we should be finding and fixing more probable causes of a similar error.

Here is the proof + tests https://www.drupal.org/project/jsonapi_extras/issues/3035544

Ah, thanks @BR0kEN! I did not see that you said the tests were on another issue. Let me try to port that coverage to this issue so the tests can live in Drupal core.

Thanks again @BR0kEN! Can you review this patch? I haven't made any changes to your fix patch in #36.

Test only patch is the interdiff.

This would be a good issue to review at DrupalCon Amsterdam '19.

I’m also coming to DCon and was thinking about trying to finish with this issue in there.

Patch looks cool! There was no ability in jsonapi to rename fields when I was writing the tests for this issue.

It was nice to discover https://www.drupal.org/node/3079797 after being out of APIs world for a couple of months.

I'd say this is ready to be RTBC.

I've spoke with Wim and our case for the issue is not a missing field, but a missing bundle, so it might be different....

We've managed to get on that when existing references to menu_link_item--menu_link_item were changed by menu items extra to menu_link_item--main_navigation, menu_link_item--footer, etc. At that point the old bundle that was referenced in the fields IS fully missing in the system and it's replaced with as many menus you have on the site.

Result is that JSON:API is looking for the bundle on menu link item that is with the same name, but it is missing and a NULL is returned and all follows from there.

I will test, whether this patch fixes the other issue and report back.

@ndobromirov, I'm pretty sure you're describing #2996114: Argument 2 passed to Drupal\jsonapi\Routing\Routes::Drupal\jsonapi\Routing\{closure}() must be an instance of Drupal\jsonapi\ResourceType\ResourceType, NULL given.

I'm trying to keep the scope of this issue very small.

This could use a clearer issue summary.

+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -1187,4 +1187,51 @@ public function testLeakCacheMetadataInOmitted() {
+    // This should throw an error unless the issue is fixed.

is this comment accurate now? i.e. we don't expect this to throw an error anymore, suggest 'Make sure that accessing a node that references a deleted term does not cause an error' or similar?

#57: ✅

Committed 1368057 and pushed to 9.0.x. Thanks!

c/p to 8.9.x

This can be c/p to 8.8.x after the beta.

This looks like a good fix for 8.8.x too. +1 from me.

At Drupal 8.7 is the same issue. Here is a patch for it. /// Oh noo ... File is empty. Sorry, correct one is in commend below

At Drupal 8.7 is same issue. Here is patch for it.

@siva01, the #36 is just the same and can be used for 8.7.