Filtering by referenced entity requires ".uuid" to be specified in filter path expression

Welcome to Drupal.org! 😄

I think you make an excellent point. I think it's indeed reasonable to expect that doing /jsonapi/node/article?filter[uid]=<UUID> works, that you shouldn't have to specify /jsonapi/node/article?filter[uid.uuid]=<UUID>.

So I do think there's a case to be made for this:

1. /jsonapi/node/article?filter[uid.uuid]=<UUID of user>
2. /jsonapi/node/article?filter[uid.name]=<name of user>
3. /jsonapi/node/article?filter[uid]=<UUID of user> (alias of #1)

What do my fellow maintainers think? If they don't think we should support this, then I think it's worth calling this out in https://www.drupal.org/docs/8/modules/json-api/filtering#shortcuts.

If we're going to add this DX enhancement, it'll happen only in the 2.x branch. In any case, I encourage you to update to the 2.x branch, we released 2.0-RC1 a week ago: https://www.drupal.org/project/jsonapi/releases/8.x-2.0-rc1.

I think that the correct behavior should be:

1. /jsonapi/node/article?filter[uid.id]=UUID of user
2. /jsonapi/node/article?filter[uid.name]=name of user

The other cases should result in a cacheable 4XX response:

• /jsonapi/node/article?filter[uid.uuid]=UUID of user: 4XX
• /jsonapi/node/article?filter[uid]=UUID of user: 4XX

I agree with #6, in as far as that is what matches the Fancy Filters spec as currently written, per:

The last path element of the property accessor MUST be an attribute followed by an optional group attribute's property names.

From which follows that the last element may not be a relationship name.

And I don't think we should change the spec to allow the last element to be solely the relationship name (and interpret that as meaning the id of the related resource), because the json:api spec does not consider the id alone to be the resource identifier; rather, the combination of type and id is. So I don't think json:api extensions should ever reduce the concept of a resource identifier to less than that, even if Drupal is choosing to use ER fields that are constrained to single entity types and UUIDs that are unique across entity types.

I agree with #6 also.

I think that we then will also agree this is at least a major bug in the current RC that should be fixed in a next RC?

I mostly agree with #6.

There are two cases in #6 I'm not convinced of:

• /jsonapi/node/article?filter[uid.id]=UUID of user: 2XX
• /jsonapi/node/article?filter[uid]=UUID of user: 4XX

Citing the same sentence as #7:

The last path element of the property accessor MUST be an attribute followed by an optional group attribute's property names.

id is not an attribute. id and type are top-level members of a resource object. And attributes is also a top-level member.

Therefore, I think it should be the exact opposite:

• /jsonapi/node/article?filter[uid.id]=UUID of user: 4XX
• /jsonapi/node/article?filter[uid]=UUID of user: 2XX

Looking forward to your thoughts :)

I still favor #6.

id is not an attribute. id and type are top-level members of a resource object. And attributes is also a top-level member.

The spec just seems to be wrong IMHO. I imagine that as we make the FF spec ready to contribute on jsonapi.org, we'll find many inconsistencies and places where it can be refined and generalized. IOW, at this point I think the spec should match our implementation rather than use trying to match it.

The last path element of the property accessor MUST be an attribute followed by an optional group attribute's property names.

For example, taken on its own, the above statement actually precludes chained relationships! Take uid.role.name, according the above, it's forbidden because role is not an attribute field, it's a relationship field.

To me, uid specifies the relationship object. The addition of a dot (.) then "traverses" to filter on value of the target, which could be any of: id, type or an attribute or relationship field name (which can't be in conflict thanks to #2977669: Spec Compliance: some entity types have an "id", "type" or "uuid" field, this violates the spec).

Finally, /jsonapi/node/article?filter[uid]={value} already has a meaningful behavior! It's the same as /jsonapi/node/article?filter[uid.target_id]={value}. That's because that's how Entity Query API handles this—it uses the main property name of the field item when one is not specified in the field path.

For example, taken on its own, the above statement actually precludes chained relationships!

But that'd at least still be a field in JSON API terminology. id is not a field.

Finally, /jsonapi/node/article?filter[uid]={value} already has a meaningful behavior!

Right :( So even if my proposal were better (I'm not convinced it is!), then it'd still constitute a BC break.

Conclusion: #6++ 🙂

Thanks for your patience, @e0ipso and @gabesullice, in exploring all possible solutions.

Finally, /jsonapi/node/article?filter[uid]={value} already has a meaningful behavior! It's the same as /jsonapi/node/article?filter[uid.target_id]={value}. That's because that's how Entity Query API handles this—it uses the main property name of the field item when one is not specified in the field path.

This might be getting a bit off topic, and maybe I should open a new issue for it, but I must say that I don't like these kinds of hidden aliases. For what it's worth, my preference would be that our filters follow the resource's data structure as it is modeled in the json:api output. What I mean by this is that how Drupal internally models entity fields should not leak out to the json:api.

Examples:

• Certain fields, like node title, when output as a json:api resource, appear in the attributes as primitives. In other words, in the resource document, it appears within attributes as title: "My Article Title", not as title: {value: "My Article Title"}. Therefore, when filtering by title, filter[title]=My Article Title should work (which it does), and filter[title.value]=My Article Title should 4xx (as opposed to its current behavior of working). In other words, if we output the title in the resource document as a string, then the API of that resource type is that the title attribute is a string, and we should not support a filter query that bypasses that API contract with internal knowledge about how Drupal stores it.
• Other fields, like node body, when output as a json:api resource, appear in the attributes as objects. In other words, in the resource document, it appears within attributes as body: {value: "My Article Body", ...}, not as body: "My Article Body". Therefore, when filtering by body, filter[body.value]=My Article Body should work (which it does), and filter[body]=My Article Body should 4xx (as opposed to its current behavior of working). In other words, if we output the body in the resource document as an object, then the API of that resource type is that the body attribute is an object, and we should not support a filter query that bypasses that API contract with internal knowledge about which key within that object is "the main one".
• If target_id and uuid do not appear as members/fields of a json:api resource, then including them in a filter path should 4xx (as opposed to its current behavior of working). Again, they're not part of the API of the resource, so we should not support a filter query that bypasses that API contract with internal knowledge about how Drupal stores entities and entity references.
• All of the above is also why I support #6. Because per json:api spec, what is output to the data of a relationship is always an object with both type and id. Therefore, the filter path should explicitly target down to the primitive.

Like I said, I should probably open a new issue to discuss the above (since others could certainly disagree), but just wanted to share my thinking on it since it relates to #6.

Very good points, @effulgentsia. Makes perfect sense.

(I don't think it belongs in a separate issue, because this too is about confusingness/mismatches between how the filter system syntax works, what the JSON API resource objects look like and the correspondingly clearly leaky abstraction.)

Your last bullet is also the perfect justification for #6, and makes perfect logical sense. I wish I'd seen that, and made that argument :) 👌

Ugh, @effulgentsia you're totally right about the leakiness of fancy filters. But... can I please put my head back in the sand now?

That sounds super hard to do and ripe with pitfalls. Despite its correctness.

Things we'd need to consider:

1. Under this construction, shouldn't the filter on a related body be relationships.uid.attributes.body.value? Or, is it okay to elide relationships and attributes because the field names are globally unique? One could argue that since they're unique, they're implied, so one would still write uid.body.value?
2. One can write uid.entity:node.body today in order to ensure that your filter only applies to nodes when it could apply to other entity types (take it as a matter of faith that this is necessary :P). We'll need to have some kind of special syntax to use the resource type name instead of the entity type name (which would open up a can of worms because resource types are bundle specific).
3. Some entity reference fields have properties on them, like the alt text on an image field. One can filter by those values with field_image.alt, but under your premise it would need to be field_image.meta.alt I think.

Truly getting rid of these leaky filter paths would seem to me to be a pretty extensive undertaking and very disruptive. I don't know if we should tackle that prior to 2.0 stable.

Maybe we could postpone the complete reckoning of this realization and later use a profile to negotiate the correct vs legacy behavior. We could then use a BC flag to determine the default.

Or, is it okay to elide relationships and attributes because the field names are globally unique? One could argue that since they're unique, they're implied, so one would still write uid.body.value?

Exactly.

One can write uid.entity:node.body today in order to ensure that your filter only applies to nodes

I need to think about that more before I have a suggestion on how to handle that in terms of a Drupal-independent fancy filters spec.

One can filter by those values with field_image.alt, but under your premise it would need to be field_image.meta.alt I think.

Yes, I think that's correct, because at least in theory, the resource referenced by field_image could have an alt attribute that is different than what is in the meta of the relationship. In fact, it gets even more complicated since the resource referenced by field_image could have metadata with overlapping keys as the metadata of the relationship. In other words, should field_image.meta.alt filter on the "alt" metadata of the field_image relationship or on the "alt" metadata of the referenced resource? We'll need to think about how to disambiguate those.

Maybe we could postpone the complete reckoning of this realization and later use a profile to negotiate the correct vs legacy behavior. We could then use a BC flag to determine the default.

+1.

For field_node_reference.entity:node.body, could we express that as ?filter[field_node_reference.meta.entity_type]=node&filter[field_node_reference.body]=...? And actually include entity_type in the meta of ER relationships and/or the meta of entity resources?

Or is there a reason why this needs to be expressed in a single filter rather than as a conjunction of two filters?

😭 I had a nice reply all written, then my Mac restarted to install Mojave...

@effulgentsia, I'm very much on board with using a complementary filter to narrow down the entity type. That's very clean and intuitive, as opposed to inventing some in-path syntax or copying EFQ's.

However, I'm not very warm about the idea of adding entity_type to the meta object. That seems to just patching the leak in one spot and letting it leak out somewhere else.

Instead, I think we should just have a smarter resolver. We could expand/compile this:

?filter[field_ref.type]=node--article
&filter[field_ref.field_int=42

into something like:

$query->('field_ref.entity:node.bundle', 'article');
$query->('field_ref.entity:node.field_int', 42);

That would mean FieldResolver will need to get smarter (yet again). That's fine though, it has to for all the other cases we've discussed already. Since getting to know all the ins and outs of this space, I'd like to take another stab at FieldResolver anyhow.

I like #18, but how would you limit the filter to 'node' without limiting it to 'node--article'? Or do we not need to support that use-case, since with respect to json:api, we either want to limit to a resource type or we don't, but there's no concept of "resource type groups".

I guess alternatively, a json:api client knows what all the resource types are via /jsonapi, so could construct an IN filter with ['node--article', 'node--page', ...]?

alternatively, a json:api client knows what all the resource types are via /jsonapi, so could construct an IN filter

Exactly. I think an IN filter is what one would do.

This patch does the smallest possible thing:

?filter[uid.uuid] === 400
?filter[uid.id] === 200

It does not address anything else, such as failing on uid, title.value anything else mentioned since #13.

Let's move all that to a followup(s).

There are three patches:

• One showing the code update only, which proves that uid.uuid === 400.
• One showing only updates tot the tests, which proves that the current behavior is uid.id === 400
• And one which shows both patches combined, which proves that is now uid.id === 200 as it should be.

This looks good! (except for red tests) It is not pretty that we have to do this, but there's no way around it.

+++ b/src/Context/FieldResolver.php
@@ -281,7 +281,7 @@ class FieldResolver {
+      $reference_breadcrumbs[] = $field_name === 'id' ? $this->getIdFieldName(reset($resource_types)) : $field_name;

Are we concerned about the use of reset(…)? What if multiple resource types yield a different result? What if $resource_types is []?

I get that a relationship is limited to an entity type (via entity reference), hence the UUID key will have a consistent property. However, maybe it's worth to add a check for future-proofing.

This should fix the test errors.

@Wim Leers and I agreed that this patch should also solve the case of body === 400, body.value === 200, title.value === 400 and title === 200 since value is elided in the resource object because the field only has one property.

We will not try to solve entity:node and field_image.meta.alt in this issue, but we'll open a followup for it.

@Wim Leers and I agreed that this patch should also solve the case of body === 400, body.value === 200, title.value === 400 and title === 200 since value is elided in the resource object because the field only has one property.

We will not try to solve entity:node and field_image.meta.alt in this issue, but we'll open a followup for it.

+1!

The 32 failures in the last patch can all be attributed to the fact that config entities are not fieldable, which caused the field resolution to escape early without mapping id => uuid. This should fix it.

@Wim Leers and I agreed that this patch should also solve the case of body === 400, body.value === 200, title.value === 400 and title === 200 since value is elided in the resource object because the field only has one property.

The attached patch should achieve the above requirement. I've tested this manually on a few examples, but I have not yet added test coverage.

This fixes the failures in FilterTest.

Strengthen the test coverage: verify the executed SQL query is exactly the same as if we'd have constructed the EFQ without the properties being specified.

(This more strictly proves that results will not change even in more complex circumstances.)

Add regression test and improve DX: rather than just saying must be followed by a property name., we now actually list which are possible property names to filter by.

(This is essentially bringing back some of the test coverage from before #31's fixes; to explicitly test that this is now no longer allowed.)

CS violations fixed.

The regression test added in #33 is insufficient; that only covers the "non reference field" case. We need to test both the "reference field without (meta) properties" and the "reference field with (meta) properties" cases too.

This adds regression test coverage for the "reference field with (meta) properties" case.

+++ b/tests/src/Kernel/Query/FilterTest.php
@@ -93,6 +98,20 @@ class FilterTest extends JsonapiKernelTestBase {
+    $this->setExpectedException(CacheableBadRequestHttpException::class, 'Ambiguous filter path. The field `photo`, given in the path `photo`, must be followed by one of the following property names: `target_id`, `alt`, `title`, `width`, `height`.');

This lists "target_id" because that actually is the underlying property name. But … we really shouldn't expose this.

Fixed.

Now the DX for this edge case works as desired! 🎉

This adds regression test coverage for the "reference field without (meta) properties" case.

+++ b/src/Context/FieldResolver.php
@@ -281,13 +282,35 @@ class FieldResolver {
+      // Determine if the specified field has one property or many.
+      $single_property_field = !array_reduce($candidate_definitions, function ($bool, ComplexDataDefinitionInterface $definition) {
+        return $bool ?: count($definition->getPropertyDefinitions()) > 1;
+      }, FALSE);

Note that this is technically inaccurate for entity reference fields. Entity reference fields always have at least two properties: target_id and entity. But only one of them is non-internal: target_id. So this means that technically, entity reference fields should also be treated as single-property fields.

But then they ought to be special-cased, since a filter on an entity reference field still is required to specify the id of the referenced entity.

Fixed.

+++ b/src/Context/FieldResolver.php
@@ -292,9 +292,17 @@ class FieldResolver {
+        // Entity reference fields are special: their `target_id` attribute is
+        // never exposed in the JSON:API representation. Hence it must also not
+        // be exposed in 400 responses' error messages.
+        if (in_array('target_id', $property_names, TRUE)) {
+          $property_names = array_diff($property_names, ['target_id']);
+          array_unshift($property_names, 'id');
+        }

I think the more robust behavior here is to check for DataReferenceDefinitionInterface not string match target_id specifically (even though all EntityReference fields use target_id).

Note that this is technically inaccurate for entity reference fields. Entity reference fields always have at least two properties: target_id and entity. But only one of them is non-internal: target_id. So this means that technically, entity reference fields should also be treated as single-property fields.

But then they ought to be special-cased, since a filter on an entity reference field still is required to specify the id of the referenced entity.

entity is a computed property. Can we do something with that rather than special case entity references?

While starting to address the remaining failures (in FieldResolverTest), I realized that there are two more regression tests that we want:

1. Filtering by a non-existent property.
2. Filtering by an elided sole property.

Having excellent error responses is a huge help for the update path, and is crucial for DX in general. I don't want this to be an afterthought like #2971281: Test coverage: ambiguous filter expression paths. So let's just deal with it now.

This makes those new tests pass.

Let me explain one especially tricky hunk in the interdiff:

+++ b/src/Context/FieldResolver.php
@@ -331,7 +331,7 @@ class FieldResolver {
+      if (!static::isCandidateDefinitionProperty($parts[0], $candidate_definitions) && !empty(static::getAllDataReferencePropertyNames($candidate_definitions))) {

Without this additional condition, we'd enter this branch and static::getDataReferencePropertyName() would fail because it assumes that it's guaranteed that the current field is an entity reference field, which is not actually true.

+++ b/tests/src/Kernel/Query/FilterTest.php
@@ -126,6 +126,34 @@ class FilterTest extends JsonapiKernelTestBase {
+    $this->setExpectedException(CacheableBadRequestHttpException::class, 'Invalid nested filtering. The property `value`, given in the path `promote.value`, does not exist. Just filter by `promote` (there is no property to filter by; this field has only a single value).');

Nit: Can this be Filter by `promote`, not `promote.value` (the JSON:API module elides property names from single-property fields).?

And this then finally makes FieldResolverTest pass. It moves all the failing test cases to expected error cases. Only the test cases involving target_id were salvageable: if we'd change the now failing test cases to instead filter by id, they pass. All other failing tests are related to the new requirement to specify a concrete property.

#47:

I think the more robust behavior here is to check for DataReferenceDefinitionInterface not string match target_id specifically (even though all EntityReference fields use target_id).

+1 — I intended to leave a self-review about exactly that, but you beat me to it :)

entity is a computed property. Can we do something with that rather than special case entity references?

Maybe. But entity is not only a computed property, it's also internal. Besides, that's the same problem as the one you pointed out above: it's still hardcoding a particular property name. I think your initial remark was spot-on; we should be reusing the same logic as \Drupal\jsonapi\Context\FieldResolver::getAllDataReferencePropertyNames()/\Drupal\jsonapi\Context\FieldResolver::getDataReferencePropertyName() is using.

#50:

Nit: Can this be Filter by `promote`, not `promote.value` (the JSON:API module elides property names from single-property fields).?

Absolutely!

In fact, I think there's another such string that can be improved:

+++ b/tests/src/Kernel/Context/FieldResolverTest.php
@@ -281,6 +276,46 @@ class FieldResolverTest extends JsonapiKernelTestBase {
+        'Ambiguous filter path. The field `field_test_text`, given in the path `field_test_ref1.field_test_text`, must be followed by one of the following property names: `value`, `format`, `processed`.',

In fact, I think this error message also could be improved. This filter path/expression is not really ambiguous … it's incomplete. So I think that's what it should say instead.

If anything, this is just another case of Invalid nested filtering., not a case of ambiguity.

I've wrapped up what I was doing, assigning back to @gabesullice.

Fixes the CS violation.

This addresses #50.

In fact, I think this error message also could be improved. This filter path/expression is not really ambiguous … it's incomplete. So I think that's what it should say instead.

If anything, this is just another case of Invalid nested filtering., not a case of ambiguity.

This addresses the above.

And this fixes the error introduced in #58.

Whoops, looks like I missed some things in #61.

This gets rid of the string match on target_id.

+++ b/tests/src/Kernel/Query/FilterTest.php
@@ -133,7 +232,7 @@ class FilterTest extends JsonapiKernelTestBase {
-              'path' => 'colors',
+              'path' => 'colors.value',
               'value' => 'red',
               'operator' => 'CONTAINS',
               'memberOf' => 'nested-or-group',
@@ -141,7 +240,7 @@ class FilterTest extends JsonapiKernelTestBase {

@@ -141,7 +240,7 @@ class FilterTest extends JsonapiKernelTestBase {
           ],
           'condition-1' => [
             'condition' => [
-              'path' => 'shapes',
+              'path' => 'shapes.value',
               'value' => 'circle',
               'operator' => 'CONTAINS',
               'memberOf' => 'nested-or-group',
@@ -149,7 +248,7 @@ class FilterTest extends JsonapiKernelTestBase {

@@ -149,7 +248,7 @@ class FilterTest extends JsonapiKernelTestBase {
           ],
           'condition-2' => [
             'condition' => [
-              'path' => 'colors',
+              'path' => 'colors.value',
               'value' => 'yellow',
               'operator' =>
               'CONTAINS',
@@ -158,7 +257,7 @@ class FilterTest extends JsonapiKernelTestBase {

@@ -158,7 +257,7 @@ class FilterTest extends JsonapiKernelTestBase {
           ],
           'condition-3' => [
             'condition' => [
-              'path' => 'shapes',
+              'path' => 'shapes.value',
               'value' => 'square',

These changes are not necessary.

I first though they were too. It's why I ended up writing code to assert the SQL queries are identical in #32.

These changes are not necessary.

I disagree ;) They're obviously invalid because text fields also have a format property. So, by our rules, there are >1 properties so the path requires a property specifier.

Hm … the failing test is very convincing :) I'm confused by my earlier conclusion then.
Ohhhhh! My eyes were misleading me! I was referring to the hunk above it:

    $nested_or_group = $query->orConditionGroup();
    $nested_or_group->condition('colors', 'red', 'CONTAINS');
    $nested_or_group->condition('shapes', 'circle', 'CONTAINS');
    $or_group->condition($nested_or_group);

    $nested_and_group = $query->andConditionGroup();
    $nested_and_group->condition('colors', 'yellow', 'CONTAINS');
    $nested_and_group->condition('shapes', 'square', 'CONTAINS');
    $or_group->condition($nested_and_group);

    $query->condition($or_group);

That's still unchanged. All good then! But that confuses me on a different level: I'd already made those changes a long time ago. Why is this showing up in an interdiff again? Did you just lose it along the way?

@Wim Leers and I chatted about what was needed to take this patch to completion. We suspected that the meta bit we decided to postpone to a followup in #25 could actually be accomplished now.

This does that.

Why is this showing up in an interdiff again? Did you just lose it along the way?

🙈 I applied your interdiffs 1 by 1 and completely missed #31!

#71: OMG! So few LoC! It passes! #71's interdiff is not actually part of the patch…

#72: no worries 😄 I just thought I was going insane there 😵 😂

Mega: 🤦‍♂️

That's what I git for getting fancy in #68 and then copy/pasting.

GREEN!

Just one nit:

+++ b/src/Context/FieldResolver.php
@@ -334,6 +337,15 @@ class FieldResolver {
+      if ($at_least_one_entity_reference_field && $parts[0] !== 'id') {
+        if ($parts[0] === 'meta') {
+          array_shift($parts);
+        }
+        elseif (in_array($parts[0], $candidate_property_names) && !static::isCandidateDefinitionReferenceProperty($parts[0], $candidate_definitions)) {
+          throw new CacheableBadRequestHttpException($cacheability, sprintf('Invalid nested filtering. The property `%s`, given in the path `%s` belongs to the meta object of a relationship and must be preceded by `meta`.', $parts[0], $external_field_name));
+        }
+      }
+

This could use a comment for at least the outer if-statement.

Other than that, #71's interdiff is surprisingly simple, and clearly performs the job!

WOOOT!

🎉🎉🎉🎉🎉

On to RC2!

Change record published: https://www.drupal.org/node/3015183

hmmm, this is a BC break.
It works.

    $this->drupalGet('jsonapi/node/article', [
      'query' => [
        'filter[nid]' => $node->id(),
      ],
    ]);

It doesn't work.

    $this->drupalGet('jsonapi/entity_test/entity_test', [
      'query' => [
        'filter[id]' => $entity_test->id(),
      ],
    ]);

imo, jsonapi can't assume id a shorthand for uuid.

“id” had been the UUID for years. Passing a serial ID instead indeed won’t work.

If you have a need for filtering by serial ID, there’s #3015759: `?filter[drupal_internal__id]=ID` does not work: drupal_internal__id should not be converted to uuid when filtering to make that work again.

This commit shipped with RC2. If my answer didn’t address your problem, please open a new issue. We cannot revert this anyway, since it alreadh shipped. Thanks!

Created #3017161: Filtering using serial ID doesn't work if the field name is 'id' to keep the discussion going.

Thank you very much! 🙏