FieldResolver::resolveInternalIncludePath() does not handle resolving a single public include path to multiple internal include paths

Thanks for digging into this and providing a patch!

Based on what I read in this issue, it should be quite simple to expand the existing FieldResolver test coverage to prove the bug in HEAD and prevent i from ever regressing again :)

Oh, and what do you mean by "local includes"?

Back to needs work for the missing tests.

Now actually back to Needs work :)

Also clarified the title further! And linking to the issue that introduced this code.

Part of the docs for FieldResolver::resolveInternalIncludePath

Each resource type may define its own external names for its internal field names. As a result, a single external include path may target multiple internal paths.

Just wanted to call out that @gabesullice added this comment in #2973681-36: Regression introduced by #2953207: Deep nested include on multi target entity type field fail 👍

Here's the missing test coverage. This patch should fail.

And this then is #12 (test coverage only) with #5 (@ndobromirov's solution) added to it. This should be green

Sorry about that, one extra \n caused the coding standards violations.

And finally, a review of @ndobromirov's solution in #5:

1. +++ b/src/Context/FieldResolver.php
   @@ -197,7 +197,7 @@ class FieldResolver {
            // Each resource type may resolve the path differently and may return
            // multiple possible resolutions.
   -        $resolved += static::resolveInternalIncludePath($relatable_resource_type, $remaining_parts, $depth + 1);
   +        $resolved = array_merge($resolved, static::resolveInternalIncludePath($relatable_resource_type, $remaining_parts, $depth + 1));
   

   👍 This change totally makes sense! This is just clearly wrong, even just based on the comment. We should've spotted this in #2973681: Regression introduced by #2953207: Deep nested include on multi target entity type field fail :(

   (I really wish PHP had stronger typing available for when you want to use it… that'd have prevented this for sure.)

2. +++ b/src/Context/FieldResolver.php
   @@ -216,9 +216,17 @@ class FieldResolver {
   -    return array_map(function ($possibility) use ($internal_field_name) {
   -      return array_merge([$internal_field_name], $possibility);
   -    }, $resolved);
   +    $resolved_unique = array_unique(array_map(function ($possibility) use ($internal_field_name) {
   +      // Convert to strings so we can make uniqueness easier.
   +      return implode('.', array_merge([$internal_field_name], $possibility));
   +    }, $resolved));
   +
   +    // Have the unique list converted back to what it needs to be.
   +    $result = array_map(function ($string) {
   +      return explode('.', $string);
   +    }, $resolved_unique);
   +
   +    return $result;
   

   👎 This can be quite a bit simpler, and we can avoid some unnecessary operations by ensuring uniqueness before prepending the current field name.

The interdiff is quite confusing, but the end result is a patch that is simpler, that makes fewer changes.

Apologies, no dreditor in this laptop. I only have nits. And props for the included tests.

+ $resolved_as_strings = array_map(function ($possibility) use ($internal_field_name) {

I don't see the need of $internal_field_name, can we drop it?

Also, I see that we're hashing the array items using the implode function to assess uniqueness. Instead, I think that a more explicit approach would be better.

+    // Remove duplicates by converting to strings and then using array_unique().
+    $resolved_by_hash = array_reduce($resolved, function ($by_hash, $possibility) {
+      $hash = spl_object_hash((object) $possibility);
+      $by_hash[$hash] = $possibility;
+      return $by_hash;
+    }, []);
+    $resolved_unique = array_values($resolved_by_hash); // <-- Not necessary, but just to make the point.

I don't see the need of $internal_field_name, can we drop it?

Good catch! Done.

Also, I see that we're hashing the array items using the implode function to assess uniqueness. Instead, I think that a more explicit approach would be better.

We want to assert uniqueness in terms of include path, and the received include path from the URL (e.g. ?include=foo.bar.baz) has been mapped to an array (e.g. ['foo', 'bar', 'baz']). So I think using implode() is fine here: it makes it clear what uniqueness we're comparing.
Your proposal uses spl_object_hash() to achieve the same, but I've frankly seen that function be used so rarely that I think the current patch is much easier to understand?

I tried implementing @e0ipso's suggestion, but that didn't work out: https://3v4l.org/uW39Z. @e0ipso said "wfm" in chat, so I'm gonna go ahead and commit this :)

Nice fix for resolveInternalIncludePath(). Does the same bug exist for resolveInternalEntityQueryPath()? If not, why not?

I guess what I'm asking is: do we have test coverage for 2 different fields, one on bundle1 and another on bundle2, both having the same public name. And an ER field that allows targeting either bundle. And then doing a filter query using the common public name? Will such a filter correctly query across both fields?

I'm glad you like what you see here!

And great question.

Will such a filter correctly query across both fields?

Related: #2953207, and specifically in #2953207-21: Can't get the right target type when filtering on relationship with bundle-specific target entity type and later comments shows that today's Entity/Field Query API does not allow for that, and hence JSON:API does not allow for it either. This is why \Drupal\jsonapi\Context\FieldResolver::getDataReferencePropertyName() asks you to be more specific whenever doing filtering, and even that was only made possible relatively recently, in #2424791: Entity query hardcodes entity_reference and entity specifier. #2953207 opened #2971281: Test coverage: ambiguous filter expression paths for testing this more explicitly.

An important difference is of course that JSON:API resolving includes does not rely on the Entity/Field Query API. Hence it is not bound to the same limitations. It's far more common to request includes across a complex set of resources (with different resource types) than it is to filter across a complex set of resources, especially because #2956414: Support mixed-bundle collections (e.g. `/jsonapi/node`) is still open.

You may wonder how that can be common then. Well: lots of entity reference fields allow resources (entities) of different resource types to be referenced (same entity type, different bundles, e.g. "talk" and "article" nodes), and it's typical to need to fetch (and hence ?include) all those related resources (referenced entities). That's what this patch made work even in this fairly uncommon scenario.

Must-read comments related to #23's explanation (and are much more thorough):

• #2973681-15: Regression introduced by #2953207: Deep nested include on multi target entity type field fail
• #2973681-23: Regression introduced by #2953207: Deep nested include on multi target entity type field fail
• #2973681-36: Regression introduced by #2953207: Deep nested include on multi target entity type field fail

All by @gabesullice.