Can't get the right target type when filtering on relationship with bundle-specific target entity type

I was able to reproduce the first error on simplytest.me. The log shows:

Drupal\Core\Entity\Query\QueryException: 'name' not found in Drupal\Core\Entity\Query\Sql\Tables->ensureEntityTable() (line 348 of /home/d5td/www/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php).

What is entity_id in this context? It's probably something internal to the Comment implementation, but I'm not familiar with it.

Below is a sample output from a sandbox, `entity_id` to client side developer is just another relationship

{
    "data": [
        {
            "type": "group_content--test-group_membership",
            "id": "186644a0-7df6-4719-a3a3-dc945cddca76",
            "attributes": {
                "id": 1,
                "uuid": "186644a0-7df6-4719-a3a3-dc945cddca76",
                "langcode": "en",
                "label": "admin",
                "created": 1521112115,
                "changed": 1521112115,
                "path": {
                    "alias": null,
                    "pid": null,
                    "langcode": "en"
                },
                "default_langcode": true
            },
            "relationships": {
                "type": {
                    "data": {
                        "type": "group_content_type--group_content_type",
                        "id": "b19f252c-bbb5-4b31-b25c-823aa0867b14"
                    },
                    "links": {
                        "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/186644a0-7df6-4719-a3a3-dc945cddca76/relationships/type",
                        "related": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/186644a0-7df6-4719-a3a3-dc945cddca76/type"
                    }
                },
                "gid": {
                    "data": {
                        "type": "group--test",
                        "id": "23c4d3a7-f5e7-4855-9f92-e8d16112a593"
                    },
                    "links": {
                        "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/186644a0-7df6-4719-a3a3-dc945cddca76/relationships/gid",
                        "related": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/186644a0-7df6-4719-a3a3-dc945cddca76/gid"
                    }
                },
                "entity_id": {
                    "data": {
                        "type": "user--user",
                        "id": "99740515-d55a-40cf-a680-19e336225177"
                    },
                    "links": {
                        "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/186644a0-7df6-4719-a3a3-dc945cddca76/relationships/entity_id",
                        "related": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/186644a0-7df6-4719-a3a3-dc945cddca76/entity_id"
                    }
                },
                "uid": {
                    "data": {
                        "type": "user--user",
                        "id": "99740515-d55a-40cf-a680-19e336225177"
                    },
                    "links": {
                        "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/186644a0-7df6-4719-a3a3-dc945cddca76/relationships/uid",
                        "related": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/186644a0-7df6-4719-a3a3-dc945cddca76/uid"
                    }
                },
                "group_roles": {
                    "data": [],
                    "links": {
                        "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/186644a0-7df6-4719-a3a3-dc945cddca76/relationships/group_roles",
                        "related": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/186644a0-7df6-4719-a3a3-dc945cddca76/group_roles"
                    }
                }
            },
            "links": {
                "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/186644a0-7df6-4719-a3a3-dc945cddca76"
            }
        },
        {
            "type": "group_content--test-group_membership",
            "id": "efc3c949-7218-4bb6-8826-df5d0102dc8e",
            "attributes": {
                "id": 2,
                "uuid": "efc3c949-7218-4bb6-8826-df5d0102dc8e",
                "langcode": "en",
                "label": "admin",
                "created": 1521112115,
                "changed": 1521112115,
                "path": {
                    "alias": null,
                    "pid": null,
                    "langcode": "en"
                },
                "default_langcode": true
            },
            "relationships": {
                "type": {
                    "data": {
                        "type": "group_content_type--group_content_type",
                        "id": "b19f252c-bbb5-4b31-b25c-823aa0867b14"
                    },
                    "links": {
                        "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/efc3c949-7218-4bb6-8826-df5d0102dc8e/relationships/type",
                        "related": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/efc3c949-7218-4bb6-8826-df5d0102dc8e/type"
                    }
                },
                "gid": {
                    "data": {
                        "type": "group--test",
                        "id": "23c4d3a7-f5e7-4855-9f92-e8d16112a593"
                    },
                    "links": {
                        "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/efc3c949-7218-4bb6-8826-df5d0102dc8e/relationships/gid",
                        "related": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/efc3c949-7218-4bb6-8826-df5d0102dc8e/gid"
                    }
                },
                "entity_id": {
                    "data": {
                        "type": "user--user",
                        "id": "99740515-d55a-40cf-a680-19e336225177"
                    },
                    "links": {
                        "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/efc3c949-7218-4bb6-8826-df5d0102dc8e/relationships/entity_id",
                        "related": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/efc3c949-7218-4bb6-8826-df5d0102dc8e/entity_id"
                    }
                },
                "uid": {
                    "data": {
                        "type": "user--user",
                        "id": "99740515-d55a-40cf-a680-19e336225177"
                    },
                    "links": {
                        "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/efc3c949-7218-4bb6-8826-df5d0102dc8e/relationships/uid",
                        "related": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/efc3c949-7218-4bb6-8826-df5d0102dc8e/uid"
                    }
                },
                "group_roles": {
                    "data": [],
                    "links": {
                        "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/efc3c949-7218-4bb6-8826-df5d0102dc8e/relationships/group_roles",
                        "related": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/efc3c949-7218-4bb6-8826-df5d0102dc8e/group_roles"
                    }
                }
            },
            "links": {
                "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership/efc3c949-7218-4bb6-8826-df5d0102dc8e"
            }
        }
    ],
    "jsonapi": {
        "version": "1.0",
        "meta": {
            "links": {
                "self": "http://jsonapi.org/format/1.0/"
            }
        }
    },
    "links": {
        "self": "http://d5td.ply.st/jsonapi/group_content/test-group_membership"
    }
}

I'm having a similar issue w/ trying to create a comment of a comment of a comment of a node. This last comment won't create giving me a "This entity (comment: 35) cannot be referenced." error, but I can create then directly on the web and the comments above all create fine. Does jsonapi only support a depth of 2 comments types right now?

Thanks for the detailed bug report, and the discussion in the comments! This has been most helpful :)

Based on the IS, this sounds related to #2958587: Unable to filter on columns of entity reference fields, which just got fixed.

But based on #13, this is a bug in \Drupal\Core\Entity\Query\Sql\Tables::addField()? Digging in more…

Reproduced with the steps to reproduce. Debugging now. Also making a slight clarification to the STR in the IS.

(Note this also means #2958587: Unable to filter on columns of entity reference fields did not fix it.)

To be 100% clear: I can reproduce the failure when testing with /jsonapi/comment/tcomment?include=entity_id&filter[entity_id.name][value]=big.

I stepped through the code with a debugger. JSON API correctly expands entity_id.name to entity_id.entity.name. It's deep in the entity query system that things fail.

Now, \Drupal\Core\Entity\Query\QueryInterface::condition()'s docs say this:

   * @param $field
   *   Name of the field being queried. It must contain a field name, optionally
   *   followed by a column name. The column can be the reference property,
   *   usually "entity", for reference fields and that can be followed
   *   similarly by a field name and so on. Additionally, the target entity type
   *   can be specified by appending the ":target_entity_type_id" to "entity".
   *   Some examples:
   *   - nid
   *   - tags.value
   *   - tags
   *   - tags.entity.name
   *   - tags.entity:taxonomy_term.name

Specifically the Additionally, the target entity type can be specified by appending the ":target_entity_type_id" to "entity". caught my attention. Why do we support this? But first let's try my hunch: I added this at the top of \Drupal\Core\Entity\Query\Sql\Tables::addField():

    if ($field === 'entity_id.entity.name') {
      $field = 'entity_id.entity:taxonomy_term.name';
    }

This is equivalent to "hinting" to the entity query system what the target entity type will be. And guess what? Suddenly things worked! I'm very very much suspecting that this is caused by the two referencable entity types from tcomment being user and taxonomy_term, and both have a name field! Still, from the entity_id field on tcomment entities, only the taxonomy_term entity type can be referenced, so it shouldn't be confused by this.

Digging deeper…

Aha, that particular line that stood out to me was added in #2424791: Entity query hardcodes entity_reference and entity specifier!

The problem definitely lies in \Drupal\Core\Entity\Query\Sql\Tables::addField(), but it's extremely complex.

The first call to ::addField(), all goes well. But the second time around, $entity_type_id === 'node', which of course does not make sense; we're only dealing with comment, taxonomy_term and user here! So how could we POSSIBLY end up with node?!

Due to the recursion used this was extremely hard to pinpoint.

@caseylau already alluded to the root cause though: the $field_storage_definitions = $this->entityManager->getFieldStorageDefinitions($entity_type_id); call. It returns seemingly correct data … but actually that's not the case.

So, the first call to ::addField(), all seems well. But what's crucial is that at the end of the call to ::addField(), we set up things for the second call:

      // If there are more specifiers to come, it's a relationship.
      if ($field_storage && $key < $count) {
        // Computed fields have prepared their property definition already, do
        // it for properties as well.
        if (!$propertyDefinitions) {
          $propertyDefinitions = $field_storage->getPropertyDefinitions();
…
        }
        $entity_type_id = NULL;
…
        if (isset($propertyDefinitions[$relationship_specifier]) && $propertyDefinitions[$relationship_specifier] instanceof DataReferenceDefinitionInterface) {
          // If it is, use the entity type if specified already, otherwise use
          // the definition.
          $target_definition = $propertyDefinitions[$relationship_specifier]->getTargetDefinition();
          if (!$entity_type_id && $target_definition instanceof EntityDataDefinitionInterface) {
            $entity_type_id = $target_definition->getEntityTypeId();
          }
          $entity_type = $this->entityManager->getDefinition($entity_type_id);

And this seems fine too, and actually would be fine. If it weren't for the fact that it's setting $entity_type based on data in $field_storage. And the data in $field_storage is very wrong:

So I'd love to say: "the entity query system is BROKEN! Not up to JSON API to fix this!". But … AFAICT the entity query system is designed to work at the entity type level, not the bundle level! Zero mentions of "bundle" in \Drupal\Core\Entity\Query\QueryInterface.

Basically, AFAICT, #2424791: Entity query hardcodes entity_reference and entity specifier was necessary for https://www.drupal.org/project/dynamic_entity_reference fields in general, but a bundle/configurable field (non-base field) on an entity type that is an entity reference and configures different target entity reference types is equivalent to a https://www.drupal.org/project/dynamic_entity_reference field from a querying POV!

It'd be great to have this confirmed by an Entity API/Entity Query API expert.

We can fix this by having JSON API automatically generate the syntax introduced by #2424791: Entity query hardcodes entity_reference and entity specifier. JSON API's FieldResolver can reliably determine the target entity type thanks to

      ->setTargetDefinition(EntityDataDefinition::create($settings['target_type']))
      // We can add a constraint for the target entity type. The list of
      // referenceable bundles is a field setting, so the corresponding
      // constraint is added dynamically in ::getConstraints().
      ->addConstraint('EntityType', $settings['target_type']);

in \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::propertyDefinitions().

(Many thanks to #2064191: Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled for adding that comment. Also see #2064191-48: Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled for prior complaints about bundles + Tables::addField(), which already was infamous back then.)

All of that writing and research eventually results in this … one-liner 😆 Well, tests are of course still necessary :)

@caseylau, please test this patch!

Updated FieldResolverTest to now expect the more specific entity query expressions!

Note that based on the expectations I had to update, I don't have confidence in my patch being 100% correct. Because only the uid.name case is getting the target entity type ID set; any filter expression that already includes entity (e.g. uid.entity.name) will NOT get it in the current patch. I think that's something best left to the person who knows FieldResolver best: @gabesullice.

Down to one fail, yay! Let's get it down to zero.

#27: that problem should be fixed by #30 — the failure in \Drupal\Tests\jsonapi\Functional\JsonApiFunctionalTest::testRead() caught that :)

#2953321: Comprehensive JSON API integration test coverage phase 5: nested includes and sparse field sets will massively expand the test coverage for this.

This looks good except for one thing, let's use the data definitions instead of the "more likely to change" constraint.

I'm happy to see that the "ambiguous" code is already going to prove more useful. Perhaps this is a real use case that we can test, rather than making our own field (as per #2971281: Test coverage: ambiguous filter expression paths)

+++ b/src/Context/FieldResolver.php
@@ -376,7 +376,9 @@
         if ($property_definition instanceof DataReferenceDefinitionInterface) {
-          $reference_property_names[] = $property_name . ':' . $property_definition->getConstraint('EntityType');
+          /* @var \Drupal\Core\Entity\TypedData\EntityDataDefinitionInterface $target_definition */
+          $target_definition = $property_definition->getTargetDefinition();
+          $reference_property_names[] = $property_name . ':' . $target_definition->getEntityTypeId();

instanceof DataReferenceDefinitionInterface … but we're calling getEntityTypeId().

Shouldn't we do:

if ($property_definition instanceof EntityDataDefinitionInterface) {
  $reference_property_names[] = $property_names . ':' . …;
}
else {
  $reference_property_names[] = $property_names;
}

?

Hm, it's kind of an impossibility given that entity references should only reference entities. So, I was sort of counting on the code to blow up if non-entity data was every encountered in the hope that we would one day be alerted to a real world example, but that's bad DX.

What do you think of throwing an exception like: "Filtering or including data in the path $path is not supported. Please report this incident in the JSON API issue queue."

That makes sense (entity references should only reference entities). I tried to do what you suggested, but didn't like the end result (see attached interdiff-nope.txt). But then I realized that this is exactly what assert() is for! What do you think of interdiff.txt?

+++ b/src/Context/FieldResolver.php
@@ -376,8 +377,8 @@ class FieldResolver {
+          assert($target_definition instanceof EntityDataDefinitionInterface, 'Entity reference fields should only be able to reference entities.');

❤️

Let me try to get test coverage going first :)

The failing test is also the interdiff. Regression test coverage modeled after \Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegressionTest(). That makes it very easy to add regression tests for complex scenarios that require particular setup.

+++ b/tests/src/Functional/JsonApiRegressionTest.php
@@ -0,0 +1,62 @@
+class JsonApiRegressionTest extends JsonApiFunctionalTestBase {
...
+   * @see https://www.drupal.org/node/2698811
...
+  public function testBundleSpecificTargetEntityType_2953207() {

This is a really nice pattern.

❤️

I found some more nits that I'll fix, but that means I can get this committed today, yay!

Fixed nits.

Crediting the people who materially helped shape this patch.

Still green, so … committed!

@caseylau found a bug to this patch, he needs to supply additional test and patch to fix it.

#48/#49: This was committed in #46 and released. Please create a new issue for this newly discovered problem! Otherwise it's impossible to find the discussion associated with a given commit.

#49: I think you never got around to filing that, @caseylau? If you did, it was never referenced here.

It just happens to be that @effulgentsia asked a great question in #3040280-22: FieldResolver::resolveInternalIncludePath() does not handle resolving a single public include path to multiple internal include paths that required some issue queue digging. I ended up on this issue again and … happened to see #49. I'm 95% certain that the problem you're reporting is the same problem we just fixed in #3040280: FieldResolver::resolveInternalIncludePath() does not handle resolving a single public include path to multiple internal include paths :)

Moving to Drupal core's issue queue.

I'm working on https://www.drupal.org/project/drupal/issues/3122113