Inclusion of nested relationships fails for bundle-specific fields

the patch make loading field definitions on specific bundle.

It's interesting to notice that D8.5 has more test fails than D8.3

Core's comment test:

1. Create a new entity type
2. Create a new comment type with setting target to above new entity type
3. Add comment field to the new entity type,and set point to the new comment type

jsonapi/comment/hieckcomment?include=entity_id,entity_id.field_juice
return error.
The comment has the problem too.
#11 may be the right direction.

can't agree with #13.

1. make a comment type named "forterm", and Target entity type set to "Taxonomy term".

2. add a comment field to vocabulary "tags", and set Comment type to "forterm".

3. GET jsonapi/comment/forterm?include=entity_id,entity_id.vid work correctly.

From #12

It's interesting to notice that D8.5 has more test fails than D8.3

This is due to #2883680: Force all route filters and route enhancers to be non-lazy where the class /RouteEnhancerInterface is now deprecated and will cause a test failure. Currently if contrib modules fix this then the tests fail at 8.4 and 8.3 because the new class interfaces have not been backported. I have raised #2924899: Backport \Routing\EnhancerInterface to core 8.4 to request this.

@alan_blake
your request is
jsonapi/comment/forterm?include=entity_id,entity_id.vid
multi-level include is
entity_id.vid.
the jsonapi's FieldResolver.php#n124

      $entity_type_id = $definitions[$field_name]->getSetting('target_type');

return node.
FieldResolver.php#n104

$definitions = $this->fieldManager->getFieldStorageDefinitions($entity_type_id)
        // We also need the base field definitions in case there are
        // relationships coming from computed fields.
        + $this->fieldManager->getBaseFieldDefinitions($entity_type_id);

node has the field vid, so you don't run into the error.

So when come to comment about this issue, you must include a relationship whose name don't exist on node's field names, Then it will return the error.

If you according to the steps i write on #13
I ensure you can find the error absolutely.

@alan_blake Oh, I forget one task 3

1. make a comment type named "forterm", and Target entity type set to "Taxonomy term".
2. add a comment field to vocabulary "tags", and set Comment type to "forterm".
3. Add a entity_reference field to tags, set machine name as field_myuser and target to user.

GET jsonapi/comment/forterm?include=entity_id,entity_id.field_myuser will get error

http://groupjson.ap:8080/jsonapi/comment/forterm?include=entity_id

{
    "data": [
        {
            "type": "comment--forterm",
            "id": "651cacbe-ec3c-463f-9d21-726b0cc64335",
            "attributes": {
                "cid": 5,
                "uuid": "651cacbe-ec3c-463f-9d21-726b0cc64335",
                "langcode": "en",
                "status": true,
                "subject": "good",
                "name": null,
                "mail": null,
                "homepage": null,
                "created": 1511185537,
                "changed": 1511185537,
                "thread": "01/",
                "entity_type": "taxonomy_term",
                "field_name": "field_forterm_comment",
                "default_langcode": true,
                "comment_body": {
                    "value": "<p>good haha</p>\r\n",
                    "format": "basic_html"
                }
            },
            "relationships": {
                "comment_type": {
                    "data": {
                        "type": "comment_type--comment_type",
                        "id": "c3844a12-1827-457c-add8-0586eceeeed2"
                    },
                    "links": {
                        "self": "http://groupjson.ap:8080/jsonapi/comment/forterm/651cacbe-ec3c-463f-9d21-726b0cc64335/relationships/comment_type",
                        "related": "http://groupjson.ap:8080/jsonapi/comment/forterm/651cacbe-ec3c-463f-9d21-726b0cc64335/comment_type"
                    }
                },
                "pid": {
                    "data": null
                },
                "entity_id": {
                    "data": {
                        "type": "taxonomy_term--tags",
                        "id": "a84d06e3-dd6e-40f8-8efc-4adfb968c7e9"
                    },
                    "links": {
                        "self": "http://groupjson.ap:8080/jsonapi/comment/forterm/651cacbe-ec3c-463f-9d21-726b0cc64335/relationships/entity_id",
                        "related": "http://groupjson.ap:8080/jsonapi/comment/forterm/651cacbe-ec3c-463f-9d21-726b0cc64335/entity_id"
                    }
                },
                "uid": {
                    "data": {
                        "type": "user--user",
                        "id": "136954c7-c4d6-4b92-a6d4-35f371c8d5d9"
                    },
                    "links": {
                        "self": "http://groupjson.ap:8080/jsonapi/comment/forterm/651cacbe-ec3c-463f-9d21-726b0cc64335/relationships/uid",
                        "related": "http://groupjson.ap:8080/jsonapi/comment/forterm/651cacbe-ec3c-463f-9d21-726b0cc64335/uid"
                    }
                }
            },
            "links": {
                "self": "http://groupjson.ap:8080/jsonapi/comment/forterm/651cacbe-ec3c-463f-9d21-726b0cc64335"
            }
        }
    ],
    "links": {
        "self": "http://groupjson.ap:8080/jsonapi/comment/forterm?include=entity_id"
    },
    "included": [
        {
            "type": "taxonomy_term--tags",
            "id": "a84d06e3-dd6e-40f8-8efc-4adfb968c7e9",
            "attributes": {
                "tid": 4,
                "uuid": "a84d06e3-dd6e-40f8-8efc-4adfb968c7e9",
                "langcode": "en",
                "name": "hellotag",
                "description": {
                    "value": "<p>this is the body</p>\r\n",
                    "format": "basic_html"
                },
                "weight": 0,
                "changed": 1511187816,
                "default_langcode": true,
                "path": {
                    "alias": null,
                    "pid": null,
                    "langcode": "en"
                },
                "field_forterm_comment": {
                    "status": 2,
                    "cid": 5,
                    "last_comment_timestamp": 1511185537,
                    "last_comment_name": "",
                    "last_comment_uid": 1,
                    "comment_count": 1
                }
            },
            "relationships": {
                "vid": {
                    "data": {
                        "type": "taxonomy_vocabulary--taxonomy_vocabulary",
                        "id": "4e7e117e-217b-4900-805d-cc571dafe03c"
                    },
                    "links": {
                        "self": "http://groupjson.ap:8080/jsonapi/taxonomy_term/tags/a84d06e3-dd6e-40f8-8efc-4adfb968c7e9/relationships/vid",
                        "related": "http://groupjson.ap:8080/jsonapi/taxonomy_term/tags/a84d06e3-dd6e-40f8-8efc-4adfb968c7e9/vid"
                    }
                },
                "parent": {
                    "data": []
                },
                "field_myuser": {
                    "data": {
                        "type": "user--user",
                        "id": "7bafc9b8-5adb-467d-8422-32bdbae4eaed"
                    },
                    "links": {
                        "self": "http://groupjson.ap:8080/jsonapi/taxonomy_term/tags/a84d06e3-dd6e-40f8-8efc-4adfb968c7e9/relationships/field_myuser",
                        "related": "http://groupjson.ap:8080/jsonapi/taxonomy_term/tags/a84d06e3-dd6e-40f8-8efc-4adfb968c7e9/field_myuser"
                    }
                }
            },
            "links": {
                "self": "http://groupjson.ap:8080/jsonapi/taxonomy_term/tags/a84d06e3-dd6e-40f8-8efc-4adfb968c7e9"
            }
        }
    ]
}

But when you get
http://groupjson.ap:8080/jsonapi/comment/forterm?include=entity_id,entity_id.field_myuser
You will get error

Following #17, I was able to reproduce a similar error on simplytest.me with Core 8.4.2 and JSONAPI 8.x-1.3:

Symfony\Component\HttpKernel\Exception\BadRequestHttpException: Invalid nested filtering. Invalid entity reference "field_myuser". in Drupal\jsonapi\Context\FieldResolver->resolveInternal() (line 108 of /home/dupgo/www/sites/default/modules/jsonapi/src/Context/FieldResolver.php).

On Core 8.4.2 and JSONAPI 8.x-1.1
Same step with #17, no error

load field definitions from bundle first.

1. Add test-only patch to test core's comment problem(fail on jsonapi:1.x-dev, success on jsonapi:1.1)
2. Add test code to 2920194-21.patch
3. Fix 2920194-21.patch's coding standards

#12

It's interesting to notice that D8.5 has more test fails than D8.3

See #2926413: RouteEnhancerInterface is deprecated in Drupal 8.5.0.

Great work here! 👍 Thanks for helping to fix this :)

1. --- a/src/Context/FieldResolver.php
   +++ b/src/Context/FieldResolver.php
   

   We're modifying this.

   Then we should also be able to expand the test coverage in \Drupal\Tests\jsonapi\Kernel\Context\FieldResolverTest.

2. +++ b/src/Context/FieldResolver.php
   @@ -101,7 +102,11 @@ class FieldResolver {
   +      $definitions = $bundle_field_definitions
   +        + $this->fieldManager->getFieldStorageDefinitions($entity_type_id)
            // We also need the base field definitions in case there are
            // relationships coming from computed fields.
            + $this->fieldManager->getBaseFieldDefinitions($entity_type_id);
   

   Is this the correct order?

3. +++ b/tests/src/Functional/comment/JsonApiFunctionalCommentTest.php
   @@ -0,0 +1,30 @@
   + * Test multi-level includes of jsonapi on comment's entity_id.
   ...
   + * to test if multi-level includes of jsonapi work well on comment when comment
   

   The JSON API spec (http://jsonapi.org/format/) calls this "nested relationship" vs "direct relationships".

   So let's s/multi-level/nested/

   The JSON API module already is using the term "nested" in several place (for example "nested filtering"), which further confirms this.

Gonna pick this one up since it's been stalled for a month now.

Hey everyone! I'm so thankful for all your work on this! Sorry it's taken so long for us to get back around to it.

Unfortunately, this code has been a focus of some attention in various other issues. The solutions that were committed elsewhere fixed the problem by coincidence without addressing the root problem leading to a lot of confusion here.

Anyway, I'm gonna do a little "review" of the current patch even though I'm about to work on it. I hope that will clarify some of the changes I'll be making for later reviewers.

1. +++ b/src/Context/FieldResolver.php
   @@ -101,7 +102,11 @@ class FieldResolver {
   +      $bundle_field_definitions = array_filter($this->fieldManager->getFieldDefinitions($entity_type_id, $bundle), function ($element) {
   +        return ($element instanceof BaseFieldDefinition) ? $element : FALSE;
   +      });
   

   This is only getting base fields, but we're naming the variable $bundle_field_definitions?

2. +++ b/src/Context/FieldResolver.php
   @@ -101,7 +102,11 @@ class FieldResolver {
   +      $definitions = $bundle_field_definitions
   +        + $this->fieldManager->getFieldStorageDefinitions($entity_type_id)
            // We also need the base field definitions in case there are
            // relationships coming from computed fields.
            + $this->fieldManager->getBaseFieldDefinitions($entity_type_id);
   

   There are a few strange things in here. First, why did we filter out base fields above, just to add them right back? Second, this is broken because by grabbing storage definitions by $entity_type_id we're going to get fields which don't belong to the bundle we're dealing with (the tests for this are incorrect). Finally, there's just no practical reason to grab storage definitions (if we need them, it means we can't rely on computed fields...).

3. +++ b/tests/src/Functional/comment/JsonApiFunctionalCommentTest.php
   @@ -0,0 +1,30 @@
   +namespace Drupal\Tests\jsonapi\Functional\comment;
   

   I don't think we need a namespace just for comments.

4. +++ b/tests/src/Functional/comment/JsonApiFunctionalCommentTestBase.php
   @@ -0,0 +1,177 @@
   +namespace Drupal\Tests\jsonapi\Functional\comment;
   

   same.

5. +++ b/tests/src/Functional/comment/JsonApiFunctionalCommentTestBase.php
   @@ -0,0 +1,177 @@
   +abstract class JsonApiFunctionalCommentTestBase extends BrowserTestBase {
   

   I think we can get away without a base class just for comments.

Okay, this is still a work in progress (I have to simplify the tests), but I think I've gotten to the root problem.

    $definitions = $this->fieldManager->getFieldDefinitions($entity_type_id, $bundle)

The original problem can be seen in the line above. $bundle was never updated as the path was traversed. I.e. it was always the bundle ID of the root entity of the request. This meant that once the code traversed an entity reference this would never return any field definitions unless the bundle was coincidentally the same as the bundle of the root request.

This explains why the other code seemed to fix the problem in most cases. By getting field storage definitions and base field definitions, we were ignoring the bundle and adding all fields for whatever entity type we had; it's possible this caused fields to be seen as valid even when they weren't (I don't think anyone would have ever noticed this in the wild).

Anyway, this is almost done. We need to craft a test case which illustrates the problem in a purer way and exercise this all more thoroughly. Then remove the commented out tests from FieldResolverTest.php (which actually show how this was working incorrectly before) that need to be turned into tests for thrown exceptions.

Alright, cleaned up and added more tests.

This is almost ready, except for one error in another test because we're not passing the correct entity type and bundle into resolveInternal. The FieldResolver code is correct though.

I've removed the tests under tests/src/Kernel/comments because I think the FieldResolverTests now cover the problem, but I've included them in one of the attached patches to confirm that they do in fact pass.

Explaining myself:

1. +++ b/src/Context/FieldResolver.php
   @@ -90,11 +90,6 @@
   -    // Right now we are exposing all the fields with the name they have in
   -    // the Drupal backend. But this may change in the future.
   -    if (strpos($external_field_name, '.') === FALSE) {
   -      return $resource_type->getInternalName($external_field_name);
   -    }
   

   I removed this because I think we should be throwing an exception if the field doesn't exist, even if the given path isn't "nested".

2. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -29,118 +29,26 @@
   +    $this->makeField('entity_reference', 'field_test_ref1', 'entity_test_with_bundle', ['bundle1'], $settings, [
   +      'handler_settings' => [
   +        'target_bundles' => ['bundle2', 'bundle3'],
          ],
   

   Adding 'handler_settings' is the only thing that's actually different with the fields created in setUp, even though it looks drastically different.

3. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -150,39 +58,79 @@
   -    $this->assertEquals('field_test_ref1.entity.field_test1', $this->sut->resolveInternal($entity_type_id, $bundle, 'field_test_ref1.field_test1'));
   -    $this->assertEquals('field_test_ref1.entity.field_test2', $this->sut->resolveInternal($entity_type_id, $bundle, 'field_test_ref1.field_test2'));
   

   These should never have passed because field_test_ref1 only references bundle2 and bundle3, neither of which have field_test1 or field_test2.

Okay, I've tracked down the problem causing the test failure and there's already an issue for it that just needs tests!

I've marked the failing test in this issue as incomplete so that this fix can be committed independently. We can finish the test and verify the solution in #2922121: When fetching related resources, getResourceType() in CurrentContext gets the wrong resource type. in one sweep then.

Whoops, leftover change from another patch removed.

#32: excellent review :)

#34: Thanks for explaining the root cause! I've been bitten by similar bugs in the past while working on Entity API bugs and on core's REST module. This also reminds me how confusing it is that for entity types without bundles, the "bundle" is the same as the entity type ID. I hope that weirdness gets fixed in a future version of Entity API.

1. +++ b/jsonapi.services.yml
   --- a/src/Context/FieldResolver.php
   +++ b/src/Context/FieldResolver.php
   

   Now that I've got a better grasp on this bit of code, I think resolveInternal() is a pretty vague name.

   Its docblock also doesn't help:

   Maps a public field name to a Drupal field name.
   

   It does far more than that: when dealing with "nested abbreviated field expressions", which follow entity references, it resolves those reference fields, resulting in the "nested fully resolved field expressions".

      * Example:
      *   'author.firstName' -> 'field_author.entity.field_first_name'.
   

   This example misleads more than it clarifies, because it includes both "nested abbreviated field expression" resolving *and* JSON API Extras field alias name resolving.

2. +++ b/src/Context/FieldResolver.php
   @@ -98,44 +100,41 @@ class FieldResolver {
   +      // Get all possible referenced field definitions.
   +      $definitions = $this->getFieldDefinitions($resource_types, $field_name);
   

   This comment should not be necessary. If the method call is not sufficiently clear, then we should rename it to incorporate that extra nuance.

   In this case, I think getCandidateFieldDefinitions() might be clearer?

3. +++ b/src/Context/FieldResolver.php
   @@ -98,44 +100,41 @@ class FieldResolver {
   +          'Invalid nested filtering. The field `%s`,  given in the path `%s`, does not exist.',
   

   Nit: s/, given/, given/

   Also: I'd expect the breadcrumb to be used here, otherwise the error message still could be hard to understand/less than helpful.

4. +++ b/src/Context/FieldResolver.php
   @@ -98,44 +100,41 @@ class FieldResolver {
   -        // This is the path from the initial entity type to the entity type that
   -        // contains $field_name. This path is a set of entity references.
   ...
   +        // This is the remaining path that must be for a complex field.
   

   The previous comment contained a bit more detail. I think a mix of both the old and new comment would be clearer.

   Because AFAIK "complex field" is a concept that doesn't exist in Drupal core: it's a JSON API-invented concept?

5. +++ b/src/Context/FieldResolver.php
   @@ -98,44 +100,41 @@ class FieldResolver {
   +        // Reconstruct the full patch to the final reference field.
   

   Nit: s/patch/path/

6. +++ b/src/Context/FieldResolver.php
   @@ -158,6 +157,43 @@ class FieldResolver {
   +   * Get all the definitions from the given resources by their field name.
   

   s/resources/resource types/

7. +++ b/src/Context/FieldResolver.php
   @@ -158,6 +157,43 @@ class FieldResolver {
   +    return array_reduce($resource_types, function ($all, $resource_type) use ($field_name) {
   ...
   +    return array_reduce($definitions, function ($all, $definition) {
   

   "all" really isn't "all" here, but "all results so far".

   I think $result would therefore be a less ambiguous variable name.

8. +++ b/src/Context/FieldResolver.php
   @@ -158,6 +157,43 @@ class FieldResolver {
   +   * Get the possibly referenced ResourceTypes with list of field definitions.
   

   I think "referenceable" is clearer than "possibly referenced".

   Also: s/with list of field definitions/for a set of field definitions/

9. +++ b/src/Context/FieldResolver.php
   @@ -158,6 +157,43 @@ class FieldResolver {
   +   * @param \Drupal\Core\Field\FieldDefinitionInterface $definitions
   

   Missing trailing [] in typehint.

10. +++ b/src/Context/FieldResolver.php
    @@ -158,6 +157,43 @@ class FieldResolver {
    +   *   The possible target resources.
    

    The referenceable target resource types.

11. +++ b/src/Context/FieldResolver.php
    @@ -158,6 +157,43 @@ class FieldResolver {
    +      $resource_types = $this->collectResourceTypesForReference($definition);
    +      return array_merge($all, $resource_types);
    

    These lines can be merged.

    Also: AFAICT the return value of that method call returns an indexed array. Which means that this code allows for duplicates.

    Is that okay?

    (IOW: this logically behaves like merging sets, but in reality it doesn't.)

12. +++ b/src/Context/FieldResolver.php
    @@ -167,23 +198,27 @@ class FieldResolver {
    -    if (!$main_property_definition instanceof DataReferenceTargetDefinition) {
    ...
    +    // Does the type end with '_reference'? Probably an entity reference.
    +    if (strpos($field_data_type, '_reference') !== strlen($field_data_type) - strlen('_reference')) {
    

    Why this change? This seems less robust?

13. +++ b/src/Context/FieldResolver.php
    @@ -167,23 +198,27 @@ class FieldResolver {
    +  protected function collectResourceTypesForReference(FieldDefinitionInterface $field_definition) {
    +    if ($field_definition->isList()) {
    +      $field_definition = $field_definition->getItemDefinition();
    +    }
    

    Rather than doing this dance, why not typehint to \Drupal\Core\Field\TypedData\FieldItemDataDefinitionInterface, so we can remove this strange "auto resolve" logic? It'd make this less errorprone.

14. +++ b/src/Context/FieldResolver.php
    @@ -167,23 +198,27 @@ class FieldResolver {
    -    if (empty($handler_settings['target_bundles'])) {
    +    if (!isset($handler_settings['target_bundles']) || empty($handler_settings['target_bundles'])) {
           // If target bundles is NULL it means ALL of the bundles in the entity ID
    -      // are referenceable.
    +      // can be referenced.
           $bundle_info = $this->entityTypeBundleInfo
             ->getBundleInfo($entity_type_id);
           $target_bundles = array_keys($bundle_info);
         }
    +    else {
    +      $target_bundles = $handler_settings['target_bundles'];
    +    }
    

    I think this can become much clearer:

    $target_bundles = $handler_settings['target_bundles']
     ?: $this->getAllBundlesForEntityType($entity_type_id);

    That'd be much easier to understand, I think?

15. +++ b/src/Context/FieldResolver.php
    @@ -167,23 +198,27 @@ class FieldResolver {
    -      // are referenceable.
    +      // can be referenced.
    

    Why this change?

excellent review :)

Phew, and yours!

Responding to a few questions and comments below. If I didn't respond to something, you can assume that I agree and will change it.

Now that I've got a better grasp on this bit of code, I think resolveInternal() is a pretty vague name.

+1 The complexity of this has far surpassed the original method.

Also: I'd expect the breadcrumb to be used here, otherwise the error message still could be hard to understand/less than helpful.

I went back and forth on this. The breadcrumbs contain the real Drupal field names, not the original query fields. Since we're calling getInternalName, then these would be different from the fields used by the developer in their request. I felt that would be more confusing to show field names that the developer didn't actually put in the request query.

Why this change? This seems less robust?

Yeah... it's ugly :( I had to change this because the previous method of detection relied on FieldStorageDefinitionInterface and methods we can no longer access.

I couldn't figure out how to type check for DataReferenceInterface from this (which I would have preferred), so I followed the footsteps of the documentation, "Note that this definition class assumes that the data type for referencing a certain target type is named "{TARGET_TYPE}_reference".

If there is a better way, I would love to use that instead :)

Its docblock also doesn't help

I've written a lot more documentation here to better reflect the behavior of this method.

Because AFAIK "complex field" is a concept that doesn't exist in Drupal core: it's a JSON API-invented concept?

I wouldn't say we "invented" the concept, but it's not based on anything explicit in core. It's a reference to two things:

1. "columns" as referenced by the entity query system docs (see the $field parameter).
2. ComplexDataInterface and the corresponding fact that a field might have sub-properties (like the text field)

I've tried to clarify those comments.

AFAICT the return value of that method call returns an indexed array. Which means that this code allows for duplicates.

Is that okay?

Technically, yes. It would just mean we'd do extra work. I've de-duped this anyway.

why not typehint to \Drupal\Core\Field\TypedData\FieldItemDataDefinitionInterface

This was a great suggestion! It let me get rid of that nasty check for the "_reference" string and go back to the more stable method.

@Wim Leers++

I think this can become much clearer:

$target_bundles = $handler_settings['target_bundles']
 ?: $this->getAllBundlesForEntityType($entity_type_id);

That'd be much easier to understand, I think?

Couldn't do exactly that, but it's much cleaner now using the helper method you suggested.

It looks like there is an issue with D8.4.

Same test with #17

1. make a comment type named "forterm", and Target entity type set to "Taxonomy term".
2. add a comment field to vocabulary "tags", and set Comment type to "forterm".
3. Add a entity_reference field to tags, set machine name as field_tagsuser and target to user.

GET jsonapi/comment/forterm?include=entity_id,entity_id.field_tagsuser

Drupal8.5.x:
• 1. Before apply 2920194-44.patch:
     http://jsonapi85.dn8/jsonapi/comment/forterm?include=entity_id,entity_id.field_tagsuser get error.
  2. After apply 2920194-44.patch:
     http://jsonapi85.dn8/jsonapi/comment/forterm?include=entity_id,entity_id.field_tagsuser get right result.

Drupal8.4.x:

• 1. Before apply 2920194-44.patch:
     /jsonapi/comment/forterm?include=entity_id get right result
     /jsonapi/comment/forterm?include=entity_id,entity_id.field_tagsuser get error
  2. After apply 2920194-44.patch:
     /jsonapi/comment/forterm?include=entity_id get error.
     Because include=entity_id get error, can't test include=entity_id,entity_id.field_tagsuser.

  TypeError: Argument 1 passed to Drupal\jsonapi\Context\FieldResolver::collectResourceTypesForReference() must be an instance of Drupal\Core\Field\TypedData\FieldItemDataDefinitionInterface, instance of Drupal\Core\Field\TypedData\FieldItemDataDefinition given, called in /app/www/jsonapi/modules/contrib/jsonapi/src/Context/FieldResolver.php on line 209 in Drupal\jsonapi\Context\FieldResolver->collectResourceTypesForReference() (line 227 of /app/www/jsonapi/modules/contrib/jsonapi/src/Context/FieldResolver.php) #0 /app/www/jsonapi/modules/contrib/jsonapi/src/Context/FieldResolver.php(209): Drupal\jsonapi\Context\FieldResolver->collectResourceTypesForReference(Object(Drupal\Core\Field\TypedData\FieldItemDataDefinition)) #1......

FieldItemDataDefinitionInterface is D8.5 only. We'll need to figure out a different way if we want to keep support for existing D8 versions.

It's sad that this happens more and more often. Supporting old and new versions of core is more and more difficult, while "minor" upgrades for existing sites are more and more challenging. People are not updating core "just like that", on top of that contribs are implicitly (and explicitly) dropping support for those versions. I see this as one of the biggest risks to Drupal's future.

I think this should work with 8.4. Tests will find out :)

Fixed a doc typo too.

I have tested 2920194-50.patch both on drupal8.5.x and drupal8.4.x
Functional works well.

Great patch, I have test it. Works well. But It has some coding standards problem:

1. +   * be compatible with the entity query system.
   +   *
   +   * @see http://jsonapi.org/recommendations/#urls-reference-document
       *
       * Example:
   -   *   'author.firstName' -> 'field_author.entity.field_first_name'.
   +   *   'uid.field_first_name' -> 'uid.entity.field_first_name'.
       *
       * @param string $entity_type_id
       *   The type of the entity for which to resolve the field name.

   Parameter tags must be defined first in a doc comment

2. +
   +  public function resolveInternalErrorProvider() {
   +    return [

   Missing function doc comment

3. +      ['entity_test_with_bundle', 'bundle1', 'field_test_ref3'],
   +      // Should fail because the nested field don't exist on the targeted resources.
   +      ['entity_test_with_bundle', 'bundle1', 'field_test_ref1.field_test1'],

   Line exceeds 80 characters.

4. +  protected function makeField($type, $name, $entity_type, $bundles, $storage_settings = [], $config_settings = []) {
   +    $storage_config = [
   +      'field_name' => $name,

   Missing function doc comment

Thanks for reviewing this with your real use-case @caseylau, that's super helpful!

And thank you for keeping me honest :P You got me to finally set up a code sniffer on my new laptop.

I've fixed all the violations and some others. FWIW, "Parameter tags must be defined first in a doc comment" wasn't the real issue. The description should come before @params. I just had to make my @see into a @link.

#43: (reviewing just the changes introduced there)

1. +++ b/src/Context/FieldResolver.php
   @@ -70,10 +68,27 @@
   +   * elide the word "entity" from them (this word is used by the entity query
   

   Nit: s/word/keyword/

2. +++ b/src/Context/FieldResolver.php
   @@ -70,10 +68,27 @@
   +   * This method takes this external field expression and and attempts to
   +   * resolve any aliases and/or abbreviations into a field expression that will
   +   * be compatible with the entity query system.
   

   This entire docblock is now 10x better, but this paragraph in particular is GOLD.

3. +++ b/src/Context/FieldResolver.php
   @@ -70,10 +68,27 @@
   -   *   'author.firstName' -> 'field_author.entity.field_first_name'.
   +   *   'uid.field_first_name' -> 'uid.entity.field_first_name'.
   

   Why not keep both? Was the original wrong after all?

4. +++ b/src/Context/FieldResolver.php
   @@ -155,73 +174,85 @@
   +        array_push($result, $definitions[$field_name]->getItemDefinition());
   

   I don't think I've seen array_push() be used in years.

   Why not the slightly simpler:

   $result[] = $definitions[$field_name]->getItemDefinition();
   

   We do this all over Drupal core already.

5. +++ b/src/Context/FieldResolver.php
   @@ -155,73 +174,85 @@
   +   * Get the reference-able ResourceTypes for a set of field definitions.
   ...
   +   *   The reference-able target resource types.
   

   Nit s/-//

6. +++ b/src/Context/FieldResolver.php
   @@ -155,73 +174,85 @@
   +   * Gets all bundle ids for a given entity type.
   ...
   +   *   The bundle ids.
   

   Nit: s/ids/IDs/

#47+#48+#49+#50: darn! I didn't realize that this interface was 8.5-only! Was apparently added in #2734345: Comments: entity_id base field not overridden correctly in comment bundle field definitions for comment bundles attached to different entity target_type ~3 months ago. Turns out this is just the formalizing in an interface what \Drupal\Core\Field\TypedData\FieldItemDataDefinition has been providing already. So, it's actually safe to typehint to that instead, and then have a @todo + an issue to change the hint to the interface when JSON API becomes 8.5-only (at the release of 8.6 at the earliest). Then we ensure the follow-up doesn't end up being forgotten :)
The work-around that #50 added (typehinting to \Drupal\Core\TypedData\ComplexDataDefinitionInterface instead) is dangerous: it doesn't guarantee that a reference definition is being passed in.

#53 (reviewing the entire patch):

1. +++ b/src/Context/FieldResolver.php
   @@ -31,6 +31,20 @@ class FieldResolver {
   +   * The entity bundle information service.
   

   Nit: The entity type bundle info.

2. +++ b/src/Context/FieldResolver.php
   @@ -31,6 +31,20 @@ class FieldResolver {
   +   * The resource type repository service.
   

   Nit: The JSON API resource type repository.

3. +++ b/src/Context/FieldResolver.php
   @@ -68,10 +82,27 @@ class FieldResolver {
   -   * Maps a public field name to a Drupal field name.
   +   * Resolves external field expressions into internal field expressions.
   +   *
   +   * It is often required to reference data which may exist across a
   +   * relationship. For example, you may want to sort a list of articles by
   +   * a field on the article author's representative entity. Or you may wish
   +   * to filter a list of content by the name of referenced taxonomy terms.
   +   *
   +   * In an effort to simplify the referenced paths and align them with the
   +   * structure of JSON API responses and the structure of the hypothetical
   +   * "reference document" (see link), it is possible to alias field names and
   +   * elide the word "entity" from them (this word is used by the entity query
   +   * system to traverse entity references).
   +   *
   +   * This method takes this external field expression and and attempts to
   +   * resolve any aliases and/or abbreviations into a field expression that will
   +   * be compatible with the entity query system.
   +   *
   +   * @link http://jsonapi.org/recommendations/#urls-reference-document
       *
       * Example:
   -   *   'author.firstName' -> 'field_author.entity.field_first_name'.
   +   *   'uid.field_first_name' -> 'uid.entity.field_first_name'.
   
   @@ -88,54 +119,50 @@ class FieldResolver {
   -    // Right now we are exposing all the fields with the name they have in
   -    // the Drupal backend. But this may change in the future.
   -    if (strpos($external_field_name, '.') === FALSE) {
   -      return $resource_type->getInternalName($external_field_name);
   -    }
   
   @@ -158,35 +187,88 @@ class FieldResolver {
   -    if (empty($handler_settings['target_bundles'])) {
   -      // If target bundles is NULL it means ALL of the bundles in the entity ID
   -      // are referenceable.
   -      $bundle_info = $this->entityTypeBundleInfo
   -        ->getBundleInfo($entity_type_id);
   -      $target_bundles = array_keys($bundle_info);
   -    }
   ...
   +    $target_bundles = $has_target_bundles ?
   +      $handler_settings['target_bundles']
   
   +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -17,6 +17,8 @@ class FieldResolverTest extends JsonapiKernelTestBase {
   +   * The subject under test.
   
   @@ -29,118 +31,26 @@ class FieldResolverTest extends JsonapiKernelTestBase {
   -    EntityTestBundle::create([
   -      'id' => 'bundle1',
   -    ])->save();
   -    EntityTestBundle::create([
   -      'id' => 'bundle2',
   -    ])->save();
   -    EntityTestBundle::create([
   -      'id' => 'bundle3',
   -    ])->save();
   +    $this->makeBundle('bundle1');
   +    $this->makeBundle('bundle2');
   +    $this->makeBundle('bundle3');
   

   This refactoring could easily be moved to a separate issue, titled Refactor FieldResolver and its tests, to make it simpler and more maintainable.

   Then this patch would become SIGNIFICANTLY smaller, and MUCH easier to understand.

   I'd instantly commit such a zero-functional-change patch.

4. +++ b/src/Context/FieldResolver.php
   @@ -88,54 +119,50 @@ class FieldResolver {
   +      $definitions = $this->getCandidateFieldItemDefinitions($resource_types, $field_name);
   

   Would $candidate_fields make the code clearer?

5. +++ b/src/Context/FieldResolver.php
   @@ -88,54 +119,50 @@ class FieldResolver {
   +      $resource_types = $this->getReferencedResourceTypes($definitions);
   

   And $candidate_resource_types here?

6. +++ b/src/Context/FieldResolver.php
   @@ -158,35 +187,88 @@ class FieldResolver {
   +  protected function getCandidateFieldItemDefinitions(array $resource_types, $field_name) {
   

   I suggested this method name, but it's probably better to omit "Candidate" from this, and instead just make clear in the variable name that it's about candidates. The method itself has no logic at all to restrict to potential candidates, it just returns all of them, because they're all candidates.

   My bad!

7. +++ b/src/Context/FieldResolver.php
   @@ -158,35 +187,88 @@ class FieldResolver {
   -    if (!$main_property_definition instanceof DataReferenceTargetDefinition) {
   +    if (!($main_property_definition instanceof DataReferenceTargetDefinition)) {
   

   Unnecessary change.

@Wim Leers here's a refactor and clean-up issue as suggested: #2933892: Refactor FieldResolver and tests

The attached patch depends on #2933892-9: Refactor FieldResolver and tests.

Still needs review. When the other patch is committed, this can be retested.

#2920194: Inclusion of nested relationships fails for bundle-specific fields is in!

1. +++ b/src/Context/FieldResolver.php
   @@ -114,17 +113,15 @@ class FieldResolver {
   -    // Right now we are exposing all the fields with the name they have in
   -    // the Drupal backend. But this may change in the future.
   -    if (strpos($external_field_name, '.') === FALSE) {
   -      return $resource_type->getInternalName($external_field_name);
   -    }
   +
   

   Why do we need this change?

2. +++ b/src/Context/FieldResolver.php
   @@ -216,35 +217,71 @@ class FieldResolver {
   +   * Get the referenceable ResourceTypes for a set of field definitions.
   ...
   +  protected function getReferencedResourceTypes(array $definitions) {
   

   referenced vs referenceable

   I think the latter is the more accurate adjective?

3. +++ b/src/Context/FieldResolver.php
   @@ -216,35 +217,71 @@ class FieldResolver {
   +   * @todo Add PHP type hint, see
   +   *   https://www.drupal.org/project/jsonapi/issues/2933895
   ...
   +  protected function collectResourceTypesForReference($item_definition) {
   

   This should typehint to FieldItemDataDefinition, and in the follow-up issue that should be changed to \Drupal\Core\Field\TypedData\FieldItemDataDefinitionInterface.

   Also clarified that in #2933895-5: [>=8.5] Update type hint to interface instead of concrete class in FieldResolver.

4. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -64,14 +68,9 @@ class FieldResolverTest extends JsonapiKernelTestBase {
        return [
          ['field_test1', 'field_test1'],
          ['field_test2', 'field_test2'],
   -      ['field_test3', 'field_test3'],
    
   -      ['field_test_ref1.entity.field_test1', 'field_test_ref1.field_test1'],
   -      ['field_test_ref1.entity.field_test2', 'field_test_ref1.field_test2'],
          ['field_test_ref2.entity.field_test1', 'field_test_ref2.field_test1'],
          ['field_test_ref2.entity.field_test2', 'field_test_ref2.field_test2'],
   -      ['field_test_ref3.entity.field_test1', 'field_test_ref3.field_test1'],
   -      ['field_test_ref3.entity.field_test2', 'field_test_ref3.field_test2'],
   

   Wait, so these suddenly are no longer valid?

5. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -108,6 +107,14 @@ class FieldResolverTest extends JsonapiKernelTestBase {
        return [
          // Should fail because none of these bundles have these fields.
          ['entity_test_with_bundle', 'bundle1', 'host.fail!!.deep'],
   +      ['entity_test_with_bundle', 'bundle2', 'field_test_ref2'],
   +      ['entity_test_with_bundle', 'bundle1', 'field_test_ref3'],
   +      // Should fail because the nested fields don't exist on the targeted
   +      // resource types.
   +      ['entity_test_with_bundle', 'bundle1', 'field_test_ref1.field_test1'],
   +      ['entity_test_with_bundle', 'bundle1', 'field_test_ref1.field_test2'],
   +      ['entity_test_with_bundle', 'bundle1', 'field_test_ref1.field_test_ref1'],
   +      ['entity_test_with_bundle', 'bundle1', 'field_test_ref1.field_test_ref2'],
   

   These new "should fail" cases do NOT fail without the non-test changes in this patch, right?

   Let's prove that with a test-only patch that fails.

@Wim Leers, most of these answers have more detail in my comment #2920194-36: Inclusion of nested relationships fails for bundle-specific fields, but to recap:

1. I removed this because we're not performing a check to ensure that the field actually exists in ResourceType::getInternalName and I felt we should consistently be checking that the field exists (nested or not).
2. Agreed.
3. Ah, that makes more sense.
4. Yes. they shouldn't have passed because they're not "referenceable"
5. Yes, they should fail because an exception will not be thrown.

1. Ah, this is a top-level-only check, but we're performing this already elsewhere for all levels anyway. Okay.
2. 👍
3. 👍
4. Nice! I didn't realize that you also added tests in #2933892: Refactor FieldResolver and tests that HEAD considers valid, but incorrectly so. That means this patch can explicitly remove those "valid but not really" cases here, which is why I reacted so surprised in #60.4 :)
5. 👍

Tests-only patch failed. Proper patch passed. All concerns addressed. RTBC :)

This was a tricky review, but it's committed now (with a tiny change).

This patch caused a regression as noticed in #2934730: Server error when accessing non-fieldable entity resources with filters.