#2953207: Can't get the right target type when filtering on relationship with bundle-specific target entity type introduced better expansion of paths when an entity reference field may be able to reference multiple bundle types.

When the field resolver is unable to determine which entity type is intended, it throws an exception in this ambiguous case that reads:

Ambiguous path. Try one of the following: foo:bar, foo:baz, before the given path: qux

Its intention was to suggest that a developer change a filter path like field_ref.qux to either field_ref.foo:bar.qux or field_ref.foo:baz.qux.

However, this cannot work because we never introduced the corresponding logic to actually handle a path specifier like foo:bar.

On the surface this seem similar to #2973681: Regression introduced by #2953207: Deep nested include on multi target entity type field fail, but it is not the same because this is about filters and #2973681 is about includes.

CommentFileSizeAuthor
#16 2973916-16.patch11.05 KBwim leers
#16 interdiff.txt3.58 KBwim leers
#14 2973916-14.patch10.84 KBgabesullice
#14 interdiff-11-14.txt8.14 KBgabesullice
#11 interdiff-8-11.txt6.69 KBgabesullice
#11 2973916-11.patch9.38 KBgabesullice
#8 interdiff-7-8.txt3.25 KBgabesullice
#8 2973916-8.patch9.1 KBgabesullice
#2 2973916-2.patch1.94 KBgabesullice
#3 2973916-3.patch2.56 KBgabesullice
#3 interdiff-2-3.txt636 bytesgabesullice
#4 2973916-4.patch5.41 KBgabesullice
#4 interdiff-3-4.txt3.02 KBgabesullice
#6 2973916-6.patch6.75 KBgabesullice
#6 interdiff-4-6.txt1.58 KBgabesullice
#7 2973916-7.patch7.56 KBgabesullice
#7 interdiff-6-7.txt1.59 KBgabesullice

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

gabesullice
Primary language English
commented
Assigned: Unassigned » gabesullice
Issue summary: View changes
StatusFileSize
new 2973916-2.patch1.94 KB

Failing tests.

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new 2973916-3.patch2.56 KB
new interdiff-2-3.txt636 bytes

Fixed.

gabesullice’s picture

gabesullice
Primary language English
commented
Assigned: gabesullice » Unassigned
Status: Active » Needs review
StatusFileSize
new 2973916-4.patch5.41 KB
new interdiff-3-4.txt3.02 KB

Better DX error message.

The last submitted patch, 2: 2973916-2.patch, failed testing. View results

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new 2973916-6.patch6.75 KB
new interdiff-4-6.txt1.58 KB

Whoops, realized another case that I hadn't considered.

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new interdiff-6-7.txt1.59 KB
new 2973916-7.patch7.56 KB

And... the fix.

EDIT: 

+++ b/tests/src/Kernel/Context/FieldResolverTest.php
@@ -102,7 +102,11 @@ class FieldResolverTest extends JsonapiKernelTestBase {
-      'entity reference then property specifier `entity:entity_test_with_bundle` then a primitive field' => ['field_test_ref1.entity:entity_test_with_bundle.field_test1', 'field_test_ref1.entity:entity_test_with_bundle.field_test1'],

The failure here is actually a bad test, not a bug.

That's because field_test_ref1 doesn't actually reference a bundle which has field_test_1.

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new 2973916-8.patch9.1 KB
new interdiff-7-8.txt3.25 KB

Per #7, I've moved that bad test to the expected exception tests (and added even more). I've added many more of the same type tests that really should work.

The last submitted patch, 6: 2973916-6.patch, failed testing. View results

The last submitted patch, 7: 2973916-7.patch, failed testing. View results

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new 2973916-11.patch9.38 KB
new interdiff-8-11.txt6.69 KB

Finally, CS violations, more commentary and typo fixes.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +DX (Developer Experience)
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work 
  1. +++ b/src/Context/FieldResolver.php
@@ -368,11 +369,13 @@ class FieldResolver {
+   * @param string[] $original_path_parts
+   *   The unresolved path parts.

@@ -453,4 +460,22 @@ class FieldResolver {
+   * Get the property name from a raw path part.

    "original" vs "unresolved" vs "raw" 

  2. +++ b/src/Context/FieldResolver.php
@@ -386,10 +389,12 @@ class FieldResolver {
       // @todo Add test coverage for this in https://www.drupal.org/project/jsonapi/issues/2971281
-      $message = sprintf('Ambiguous path. Try one of the following: %s, before the given path: %s', $choices, $given);
+      $message = sprintf('Ambiguous path. Try one of the following: %s, in place of the given path: %s', implode(', ', $choices), implode('.', $original_path_parts));

+++ b/tests/src/Kernel/Context/FieldResolverTest.php
@@ -145,6 +169,18 @@ class FieldResolverTest extends JsonapiKernelTestBase {
+      [
+        'entity_test_with_bundle', 'bundle1',
+        'field_test_ref1.entity:entity_test_with_bundle.field_test1',
+      ],
+      [
+        'entity_test_with_bundle', 'bundle1',
+        'field_test_ref1.entity:entity_test_with_bundle.field_test2',
+      ],
+      [
+        'entity_test_with_bundle', 'bundle2',
+        'field_test_ref1.entity:entity_test_with_bundle.field_test2',
+      ],

    I think we should add that explicit test coverage with the expected error message here. The necessary infrastructure is already here! 

  3. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
@@ -109,6 +112,27 @@ class FieldResolverTest extends JsonapiKernelTestBase {
+      'entity reference then property specifier `entity:entity_test_with_bundle` then a primitive field; variation A' => [
+        'field_test_ref1.entity:entity_test_with_bundle.field_test3',
+        'field_test_ref1.entity:entity_test_with_bundle.field_test3',
+      ],
+      'entity reference then property specifier `entity:entity_test_with_bundle` then a primitive field; variation B' => [
+        'field_test_ref2.entity:entity_test_with_bundle.field_test1',
+        'field_test_ref2.entity:entity_test_with_bundle.field_test1',
+      ],

    What's the difference? 

  4. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
@@ -109,6 +112,27 @@ class FieldResolverTest extends JsonapiKernelTestBase {
+      'entity reference then property specifier `entity:entity_test_with_bundle` then a primitive field; variation C' => [
+        'field_test_ref3.entity:entity_test_with_bundle.field_test3',
+        'field_test_ref3.entity:entity_test_with_bundle.field_test3',
+        'entity_test_with_bundle', 'bundle2'
+      ],
+      'entity reference then property specifier `entity:entity_test_with_bundle` then a primitive field; variation D' => [
+        'field_test_ref3.entity:entity_test_with_bundle.field_test2',
+        'field_test_ref3.entity:entity_test_with_bundle.field_test2',
+        'entity_test_with_bundle', 'bundle3'
+      ],

    What's the difference?

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new interdiff-11-14.txt8.14 KB
new 2973916-14.patch10.84 KB

#13.1: Fixed.
13.2: You're right, done.
13.3/4: Good call, updated the test case labels

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new interdiff.txt3.58 KB
new 2973916-16.patch11.05 KB

#14: 👌

+++ b/src/Context/FieldResolver.php
@@ -453,4 +460,22 @@ class FieldResolver {
+   * Get the property name from an entity typed or untyped path part.

Übernit: s/Get/Gets/

5 CS violations.

Fixed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

  • Wim Leers committed cf5dba6 on 8.x-1.x authored by gabesullice 
    Issue #2973916 by gabesullice, Wim Leers: Impossible to filter using...
  • Wim Leers committed ecb1ad0 on 8.x-2.x authored by gabesullice
    Issue #2973916 by gabesullice, Wim Leers: Impossible to filter using...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.