Unable to filter on columns of entity reference fields

Thanks for reporting this! This seems at least a major bug.

#2962457: Handle computed fields in entity queries: throwing a helpful exception is better than a PHP fatal error seems related.

This is actually a separate issue from computed fields (although they both have to do with the field resolver), but they don't need to be handled together.

The issue here is that we are blindly saying that any path part after a reference field must be a field on the targeted entity and are ignoring the fact that reference fields can have other properties.

The most obvious case is target_id, which is not a reference property like the (computed) entity property on entity reference fields.

Another issue I discovered while doing this is that it would also be impossible to target references that do not have the entity property. The language property on a language field is an example of this kind of reference (although it's inapplicable to filtering because it's computed).

Tests and patch attached.

This is more complex than I first thought. Or maybe it's my still congested head that's having trouble understanding this.

1. +++ b/src/Context/FieldResolver.php
   @@ -139,14 +141,32 @@ class FieldResolver {
   +      // Determine if the next path part is a sub-property of the field.
   

   Nit: s/is/will/ (because it's for the next iteration of the loop.

2. +++ b/src/Context/FieldResolver.php
   @@ -139,14 +141,32 @@ class FieldResolver {
   +        // If the property is a reference, then we can continue resolving the
   +        // path.
   

   Hm … why is this necessary? Following multiple levels of entity references already was working before. That's why we have the while ($part = array_shift($parts)) { loop. Why are we now skipping ahead one iteration of the loop?

3. +++ b/src/Context/FieldResolver.php
   @@ -139,14 +141,32 @@ class FieldResolver {
   +        // sub-properties, not subsequent fields.
   ...
   +      // sub-properties of the field.
   

   Nit: s/sub-property/property/

   Fields have properties. Subproperties are not a thing.

4. +++ b/src/Context/FieldResolver.php
   @@ -139,14 +141,32 @@ class FieldResolver {
   +      if (!empty($parts)) {
   +        $reference_breadcrumbs[] = 'entity';
   +      }
   
   @@ -174,7 +194,7 @@ class FieldResolver {
   -    $entity_path = implode('.entity.', $references);
   +    $entity_path = implode('.', $references);
   

   Why this move?

5. +++ b/src/Context/FieldResolver.php
   @@ -320,4 +340,49 @@ class FieldResolver {
   +  protected static function isCandidateDefinitionProperty($part, $candidate_definitions) {
   

   Nit: array $candidate_definitions

6. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -75,6 +75,11 @@ class FieldResolverTest extends JsonapiKernelTestBase {
   +      ['field_test_ref1.target_id', 'field_test_ref1.target_id'],
   

   This uses \Drupal\Core\TypedData\DataReferenceTargetDefinition, which is not an instance of \Drupal\Core\TypedData\DataReferenceDefinitionInterface. Why is this now working?

7. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -75,6 +75,11 @@ class FieldResolverTest extends JsonapiKernelTestBase {
   +      ['field_test_ref1.entity.field_test_text.value', 'field_test_ref1.entity.field_test_text.value'],
   

   This is just a normal DataDefinition … I don't understand why this didn't work in HEAD, even though I can see that the test is failing.

I can see that it's fixing certain use cases simply because your newly added tests are failing and are passing after making the changes. But I don't understand yet why the changes make things work. I could step through it with a debugger, but I suspect it might be faster for you to explain?

Ah, yes, this is complicated. The change is relatively simple, but how it all fits together is definitely not.

Just as a reminder before I dig in: The entity query system expects uid.entity.name but JSON API makes it possible to just do uid.name to be mirror the dot-separated syntax of include paths. The current logic blindly assumes entity is the "glue" between path parts, but that's not truly the case.

Okay, so:

- An entity reference field has two properties, target_id and entity
- target_id is an instance of DataReferenceTargetDefinition
- entity is an instance of DataReferenceDefinition (missing "Target")
- Our heuristic for determining if a field is a reference field is: "does the field have a DataReferenceTargetDefinition property?"
That's here:

$main_property_definition = $item_definition->getPropertyDefinition(
  $item_definition->getMainPropertyName()
);

// Check if the field is a flavor of an Entity Reference field.
if (!$main_property_definition instanceof DataReferenceTargetDefinition) {
  return [];
}

- If the current part is a reference based on that heuristic, we added it to the "breadcrumbs"

=== If the field is a reference ===
- Then, we advance to the next part of the path, assuming the next part is a field on the targeted entity type. (e.g. uid.name on a node would say "uid" is a reference, therefore continue on the path and evaluate "name" against the User entity type)
- When we are done resolving the path, we then concatenate all the breadcrumbs with "entity"
- This works because the entity query system looks at the property definitions of a field and checks for DataReferenceDefinitions and "entity" is the property for those on most (perhaps all) entity reference-like fields.
- To say it differently, the entity query system does not use the target_id property to determine what entity table to join against, it uses the DataReferenceDefinition identified in the path (usually "entity" is this property)

=== If the field is not a reference ===
- We assume all remaining parts target deltas or properties of the field (or properties of those properties, etc etc. in the case of nested Map or ComplexData property types, hence "sub-properties"). An example of this might be body.2.value.

Where does this fail:

If the field is a reference, then we assumed (wrongly) that the next part of the path must be a field on the target entity type.
However, if the part is actually a property of the reference field, that possibility is never considered.
It's also possible that the DataReferenceDefinition is not actually named entity, in that case, it could never have worked with JSON API, because we always assume that it is.

How does this fix things:
If the field is a reference, we now "peek" ahead to the next path part to see if it's actually a property of the reference field.

If it's not, we continue with the path described above.

If it is, then we check if it's a reference property or not (instanceof DataReferenceDefinition).
If it is a reference property like entity (or anything other string), then we push that onto the breadcrumbs: $reference_breadcrumbs[] = array_shift($parts);.

If it is not a reference property, but it is a property of the reference field, then we assume that all remaining parts of the string are "sub-properties" of that property.

In explaining this, I realized two errors/unhandled cases in my logic 😅I've written the above as if the logic is correct.

The two flaws I found are:

1. I'm not actually checking for indices, as in the 1 in body.1.value
2. The continue; is incorrect. The loop still needs to do some processing in case the next part is a reference. So:

if (static::isReferenceProperty($parts[0], $candidate_definitions)) {
  $reference_breadcrumbs[] = array_shift($parts);
  continue;
}
return $this->constructInternalPath($reference_breadcrumbs, $parts);

becomes:

if (!static::isReferenceProperty($parts[0], $candidate_definitions)) {
  return $this->constructInternalPath($reference_breadcrumbs, $parts);
}
$reference_breadcrumbs[] = array_shift($parts);
// rest of loop still gets evaluated

More tests and patch to follow.

…
However, if the part is actually a property of the reference field, that possibility is never considered.
…
[…] in that case, it could never have worked with JSON API, because we always assume that it is.

Thanks for making it all click :) I still don't understand the field_test_text.value case though. Text fields aren't referencing anything at all.

I realized two errors/unhandled cases in my logic 😅

Looking forward to the next iteration!

I'm not actually checking for indices

I thought about this too. But I'm not sure you can support this; whether you can depends not on logic in FieldResolver, but of capabilities of the Entity Field Query system?

P.S.: I think this could be made much easier to understand (through refactoring) by having more methods, that each do one thing explicitly.

Okay, I tried to flatten out the logic and more verbose commentary. I added more test cases as well. I'm attaching an interdiff as well, but I doubt it'll be that useful.

I still don't understand the field_test_text.value case though. Text fields aren't referencing anything at all.

I don't understand what you don't understand. If you mean:

+      ['field_test_ref1.entity.field_test_text.value', 'field_test_ref1.entity.field_test_text.value'],

It was the entity part that was failing because it wasn't possible to specify a property of any kind on the entity reference.

I thought about this too. But I'm not sure you can support this; whether you can depends not on logic in FieldResolver, but of capabilities of the Entity Field Query system?

The field resolver is primarily an entity query validator with two JSON API specific transformations:

1. honor field aliases
2. allow the reference property field to be elided after an entity reference field

It is therefore possible to support this because it's possible in the entity query system.

I think this could be made much easier to understand (through refactoring) by having more methods, that each do one thing explicitly.

Indeed, I ended up adding more methods and also repeating some logic rather than trying to cleverly reduce if/else statements.

I could step through it with a debugger, but I suspect it might be faster for you to explain?

I still think stepping through with a debugger would be helpful.

Whoops, that included some other debugging stuff.

And fixing the CS violations.

#12

Okay, I tried to flatten out the logic and more verbose commentary. I added more test cases as well. I'm attaching an interdiff as well, but I doubt it'll be that useful.

The many early returns you added make it much easier to understand this logic — thank you! And this interdiff actually was useful! I applied #12 locally to look at helper methods not being introduced in #12, but #12's interdiff allowed me to see what you were changing and why.

It was the entity part that was failing […]

D'oh, of course! 🙈😅 Thanks for clarifying!

It is therefore possible to support this because it's possible in the entity query system.

I didn't know the entity query system supported this! I just checked \Drupal\Core\Entity\Query\QueryInterface::condition(), and sure enough, it's there. Grep for "delta".
This actually revealed to me that there is a special query condition called %delta, which allows one to find entities which have e.g. 5 items (5 deltas). Does JSON API's querying/filtering already support this? If not, can we add an explicit failing test? Can we create an issue to clarify this?

Interdiff review:

1. +++ b/src/Context/FieldResolver.php
   @@ -117,20 +119,21 @@ class FieldResolver {
   +    // This complex expression is needed to handle the string, "0", which would
   +    // otherwise be evaluated as FALSE.
   +    while (!is_null(($part = array_shift($parts)))) {
   

   This comment is most definitely necessary! This is where PHP the language gets in the way, and such careful conditions (and accompanying components) are an absolute necessity!

2. +++ b/src/Context/FieldResolver.php
   @@ -117,20 +119,21 @@ class FieldResolver {
   -      // remaining path parts are for sub-properties.
   +      // remaining path parts are targeting field deltas and/or field
   +      // properties.
   

   MUCH clearer ❤️

3. +++ b/src/Context/FieldResolver.php
   @@ -117,20 +119,21 @@ class FieldResolver {
   -      $candidate_definitions = $this->getFieldItemDefinitions(
   -        $resource_types,
   -        $field_name
   -      );
   +      // Different resource types have different field definitions.
   +      $candidate_definitions = $this->getFieldItemDefinitions($resource_types, $field_name);
   

   Nit: the comment is fine, but can we revert the code reformatting? It's an unnecessary distraction.

4. +++ b/src/Context/FieldResolver.php
   @@ -141,34 +144,45 @@ class FieldResolver {
   +      // If the next part is a delta, as in "body.0.value", then we add it to
   +      // the breadcrumbs and remove it from the parts that still must be
   +      // processed.
   +      if (static::isDelta($parts[0])) {
   +        $reference_breadcrumbs[] = array_shift($parts);
   +      }
   +
   +      // If there are no remaining path parts, the process is finished.
   +      if (empty($parts)) {
   

   This implies you could have an expression like body.0 or field_tags.3.

   Do we actually support those? This logic suggests we do. If we do, then we should have explicit test coverage for it.

5. +++ b/src/Context/FieldResolver.php
   @@ -141,34 +144,45 @@ class FieldResolver {
   +        // If the property is not a reference property, then all
   +        // remaining parts must be further property specifiers.
   +        if (!static::isReferenceProperty($parts[0], $candidate_definitions)) {
   

   This says "must be", but it doesn't validate this; it only checks the next expression path part. Can we add a @todo to validate this in the future?

6. +++ b/src/Context/FieldResolver.php
   @@ -141,34 +144,45 @@ class FieldResolver {
   +      else {
   +        // The next path part is neither a delta nor a field property, so it
   +        // must be a field on a targeted resource type. We need to guess the
   +        // intermediate reference property since one was not provided.
   +        $reference_breadcrumbs[] = static::getDataReferencePropertyName($candidate_definitions, $parts);
          }
   

   Woah! I think this would benefit from an example in the comment.

   I stepped through this with a debugger. I first thought this was necessary only for the most complex edge cases. But actually it's necessary for even something as simple as type.uuid! This comment sounds more complex than it is: it usually will result in 'entity' being appended to $reference_breadcrumbs.

7. +++ b/src/Context/FieldResolver.php
   @@ -340,6 +354,48 @@ class FieldResolver {
   +   * Determine the reference property name from the given field definitions.
   

   Nit: s/Determine/Determines/

8. +++ b/src/Context/FieldResolver.php
   @@ -340,6 +354,48 @@ class FieldResolver {
   +        if ($property_definition instanceof DataReferenceDefinitionInterface) {
   

   Can't this reuse static::isReferenceProperty()?

9. +++ b/src/Context/FieldResolver.php
   @@ -340,6 +354,48 @@ class FieldResolver {
   +  protected static function isDelta($part) {
   +    return (bool) preg_match('/^[0-9]+$/', $part);
   +  }
   

   👌

And one more review of the entire patch:

1. +++ b/src/Context/FieldResolver.php
   @@ -320,4 +352,93 @@ class FieldResolver {
   +      $message = sprintf('Ambiguous path. Try one of the following: %s, before the given path: %s', $choices, $given);
   +      throw new BadRequestHttpException($message);
   

   There's no explicit test coverage for this yet. Having that would also help address #18.6.

2. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -86,12 +86,38 @@ class FieldResolverTest extends JsonapiKernelTestBase {
   +      // Entity reference with followed by field property, with and without
   +      // multiple deltas.
   ...
   +      // Entity reference with followed by field, with and without reference
   +      // properties.
   

   These comments should be turned into array keys for these test cases; these array keys are considered *test case labels* by PHPUnit.

   So when any of them fail, you'll see "test case #15 ()" rather than just "test case #15").

Once #18 and #19 are addressed (mostly nits, but nits that help improve maintainability), this is RTBC. This is obviously a very important bugfix, comes with explicit test coverage (just see how many more test cases \Drupal\Tests\jsonapi\Kernel\Context\FieldResolverTest::resolveInternalProvider() gets!), and helps make the crucial FieldResolver component of JSON API more maintainable!

Nit: the comment is fine, but can we revert the code reformatting? It's an unnecessary distraction.

Done.

This implies you could have an expression like body.0 or field_tags.3. Do we actually support those? This logic suggests we do. If we do, then we should have explicit test coverage for it.

We do, and there is coverage for it :) See:

+++ b/tests/src/Kernel/Context/FieldResolverTest.php
@@ -86,12 +86,38 @@ class FieldResolverTest extends JsonapiKernelTestBase {
+      ['field_test_ref1.0.target_id', 'field_test_ref1.0.target_id'],
+      ['field_test_ref1.1.target_id', 'field_test_ref1.1.target_id'],

Can we add a @todo to validate this in the future?

Done.

I first thought this was necessary only for the most complex edge cases. But actually it's necessary for even something as simple as type.uuid! This comment sounds more complex than it is: it usually will result in 'entity' being appended to $reference_breadcrumbs.

I flipped the logic so the common case is first, added more commentary and an example.

18.7: Done

Can't this reuse static::isReferenceProperty()?

Not really. I renamed the method to make it more specific to its use case.

There's no explicit test coverage for this yet. Having that would also help address #18.6.

Without a custom field type, this isn't possible to test. I'm happy to remove the logic, make a follow-up issue and hard-code the word entity in the method for now. What do you think? Add the robust logic now, without a test, or have a hardcoded value now with a follow-up and test?

These comments should be turned into array keys for these test cases; these array keys are considered *test case labels* by PHPUnit.

Done!

We do, and there is coverage for it :)

😅 I even saw it after I wrote that remark. Forgot to remove that remark. Sorry!

I flipped the logic so the common case is first, added more commentary and an example.

+

18.7: Done

✔️

Without a custom field type, this isn't possible to test. […] What do you think? Add the robust logic now, without a test, or have a hardcoded value now with a follow-up and test?

Let's add the robust logic now and add a test in a follow-up. (Also: I was gonna say we already have a jsonapi_test_field_type test module, but that doesn't actually allow you to test this particular

… which means I can add a @todo and don't need to wait for you to just add a single comment for this patch to be RTBC'able/committable 😃🎉

This is the issue I created for that explicit test coverage btw: #2971281: Test coverage: ambiguous filter expression paths.

And I of course meant to RTBC in #23 — I said it, but didn't do it. Sorry for the noise.

Green on both 8.5 & 8.6, let's ship this excellent bugfix!