Regression introduced by #2953207: Deep nested include on multi target entity type field fail

Thanks for filing a separate issue! 🙏

And even better: you've included a failing test! And reproduced it with just core!

@caseylau++
@caseylau++
@caseylau++

In #2953207-49: Can't get the right target type when filtering on relationship with bundle-specific target entity type, you indicated this was a bug introduced by the commit in #2953207. So I tried to verify that. And I can confirm it: it is a regression.

@caseylau++

Thanks for the excellent report!

+++ b/tests/src/Functional/JsonApiRegressionTest.php
@@ -57,4 +69,58 @@ class JsonApiRegressionTest extends JsonApiFunctionalTestBase {
+        'target_bundles' => [
+          'comment' => 'comment',
+          'tcomment' => 'tcomment',
+        ],

This is the root of the issue.

The given path can reference two different bundles. Unlike most reference fields in Drupal, a comment's entity_id field can reference different entity types per bundle. Typically, a shared reference field between bundles always references the same entity type.

What the error is saying is that the field resolver can expand field_comment.entity_id.uid into either:

field_comment.entity_id.entity:node.uid
or
field_comment.entity_id.entity:taxonomy_term.uid

^{we should probably improve the error message in another issue, I think it could be more clear that this is what it is saying}

This affects which tables the entity query API will JOIN against when the path is used in a filter query param.

However, since this is for an include, not a filter, this doesn't really matter.

The difference between include and filter paths has been a nagging issue in the back of my mind for a while.

In the one case, we're only concerned with path validity, in the other, path expansion (which implies validity).

As long as any one expansion of a path is valid, the include path should be considered valid. IOW, when we hit an "ambiguous" part of the path, instead of throwing an error, the logic should just fork.

For those who may have come here because of the regression:

Until a patch lands, there is an easy workaround that you can do from your consumer with the workaround patch below.

If you are sending a request with an include like:

GET /jsonapi/node/page?include=field_comment.entity_id.uid

And receiving a 400 error that reads something like:

Ambiguous path. Try one of the following: entity:node, entity:taxonomy_term, before the given path: uid

The workaround is to send a request using both suggestions:

GET /jsonapi/node/page?include=field_comment.entity_id.entity:node.uid,field_comment.entity_id.entity:taxonomy_term.uid

Quick patch for the workaround.

This is not intended to resolve this issue.

Wow! Thanks @caseylau. Great work.

1. +++ b/src/Context/FieldResolver.php
   @@ -99,13 +99,15 @@ class FieldResolver {
   +   * @param string $query_parameter_type
   +   *   Query parameter type.
   

   Resolving the internal path currently applies to sort, filter & include.

   sort and filter both get passed directly to the entity query API after resolution.

   include doesn't deal with the entity query API and ends up in a completely different code path. Its format comes directly from the spec, while the filter and sort path format is just a convention established by this module.

   Because they're really such different beasts, I think we should probably separate the logic of validating includes out entirely from sort/filter, rather than switching behavior within the resolver method. IOW, they merely "look" the same, but in reality they're very different.

2. +++ b/tests/src/Functional/JsonApiRegressionTest.php
   @@ -57,4 +68,62 @@ class JsonApiRegressionTest extends JsonApiFunctionalTestBase {
   +   * @see https://www.drupal.org/project/jsonapi/issues/2973681#comment-12621224
   

   Supernit: don't need the comment anchor.

3. +++ b/tests/src/Functional/JsonApiRegressionTest.php
   @@ -57,4 +68,62 @@ class JsonApiRegressionTest extends JsonApiFunctionalTestBase {
   +
   

   Supernit: Extra line.

Whoops, messed up the IS.

I'll make these changes and add a new patch.

Explication...

1. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -72,14 +65,11 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   -   * @param \Drupal\jsonapi\Context\FieldResolver $field_resolver
   -   *   The JSON API field resolver.
       */
   -  public function __construct(LinkManager $link_manager, EntityTypeManagerInterface $entity_type_manager, ResourceTypeRepositoryInterface $resource_type_repository, FieldResolver $field_resolver) {
   +  public function __construct(LinkManager $link_manager, EntityTypeManagerInterface $entity_type_manager, ResourceTypeRepositoryInterface $resource_type_repository) {
   

   Validation is a static method on the field resolver now. No need to inject the dependency. That's because the resource type already has all required information.

2. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -254,14 +244,12 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   +      $path_parts = explode('.', $related ? "{$related}.{$trimmed}" : $trimmed);
   +      FieldResolver::validateIncludePath($resource_type, $path_parts);
   

   These are the operative lines of the patch.

   We need to prepend the related field name to the path because $context['resource_type'] is the resource type of the base individual resource not the targeted resource type(s).

   For example, the resource type at /jsonapi/node/article/{uuid}/field_tags is not taxonomy_term--tags, it is node--article. Therefore, if one wishes to include the vocab entity, the path would be vid. However, we need to validate it as field_tags.vid against the node--article resource type (see JsonApiFunctionalTest:L99-112).

   This only applies to related routes, not relationships routes. That's because relationship routes require the full path in the query string (see JsonApiFunctionalTest:L148-161 and the include examples given by the JSON API specification).

   GET /articles/1/relationships/comments?include=comments.author HTTP/1.1
   Accept: application/vnd.api+json
   

   - JSON API: Inclusion of Related Resources

3. +++ b/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
   @@ -756,14 +756,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
   -    $resource_type = new ResourceType(
   -      $entity_type_id,
   -      $bundle,
   -      $entity_type_manager->getDefinition($entity_type_id)->getClass()
   -    );
   +    $resource_type = $this->container->get('jsonapi.resource_type.repository')->get($entity_type_id, $bundle);
   

   We need the actual resource type, with relationship data set, not a mock resource type because setRelatableResourceTypes must be called on the resource type before that information can be accessed by the validation logic (the repository does this for us automatically).

CS violations.

1. +++ b/src/Context/FieldResolver.php
   @@ -69,6 +69,51 @@ class FieldResolver {
   +      throw new BadRequestHttpException("Empty include path.");
   

   nit: single quotes.

2. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -254,14 +244,12 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   -      $resolved = $this->fieldResolver->resolveInternal(
   

   Did we lose the ability to have field aliases? What is the reason for that?

1. ✔️

2. +++ b/src/Context/FieldResolver.php
   @@ -69,6 +69,51 @@ class FieldResolver {
   +  public static function validateIncludePath(ResourceType $resource_type, array $path_parts) {
   ...
   +    $field_name = $resource_type->getInternalName($path_parts[0]);
   

   Nope, it's still there ^

#5:

The difference between include and filter paths has been a nagging issue in the back of my mind for a while.

In the one case, we're only concerned with path validity, in the other, path expansion (which implies validity).

As long as any one expansion of a path is valid, the include path should be considered valid. IOW, when we hit an "ambiguous" part of the path, instead of throwing an error, the logic should just fork.

This went over my head … I don't think I see sufficient documentation for this yet in the current patch.

#18

1. 👍
2. I think some of this comment belongs in the code comments (just like parts of your comment #5).
3. 👍

#20:

1. Done.
2. Great remark! I think this is indeed a regression. (Which only affects JSON API Extras.) The new validation method still does the resolving,
   but does not return it.

1. +++ b/src/Context/FieldResolver.php
   @@ -69,6 +69,51 @@ class FieldResolver {
   +      throw new BadRequestHttpException("$field_name is not a valid relationship field name. Possible values: $possible");
   

   Nit: missing trailing period. Fixed.

   Also: missing explicit test coverage.

2. +++ b/src/Context/FieldResolver.php
   @@ -69,6 +69,51 @@ class FieldResolver {
   +      throw new BadRequestHttpException(sprintf("$path is not a valid include path. %s", implode(' ', $previous_messages)));
   

   Missing explicit test coverage.

3. +++ b/tests/src/Functional/JsonApiRegressionTest.php
   @@ -22,16 +24,24 @@ class JsonApiRegressionTest extends JsonApiFunctionalTestBase {
   -  public function testBundleSpecificTargetEntityTypeFromIssue2953207() {
   +  protected function setUp() {
   +    parent::setUp();
   

   The point of regression tests is that they each set up their own environment as minimally as possible to reproduce it. Rather than sharing this in a setUp() method that happens to work for the two regression tests we have so far, I'd much rather just repeat this in each.

   Fixed.

The new validation method still does the resolving, but does not return it.

D'oh! 🤦‍♂️

Which ofc invalidates my comment in #5:

In the one case, we're only concerned with path validity, in the other, path expansion (which implies validity).

Because we are concerned with more than validity. We also care about resolution.

Right now, FieldResolver::resolveInternal() does 3 things:

1. resolution: it maps external field names to internal field names if JSON API Extras has overridden the ResourceType (e.g. author -> uid)
2. validation: it ensures that the field names in the path exist on the resource types they target (e.g. if the path is /node/article, the resource type is node--article, which has a field uid, which targets a user--user resource type, which has a name field, therefore uid.name is a "valid" path).
3. expansion: it maps a path expression to an entity query compatible specifier (e.g. uid.name must become uid.entity.name and sometimes uid.entity:node.name).

For sort and filter, all three are required. For include, we do not want expansion and validation has a slightly different set of constraints. We also have an extra constraint: during validation, only entity reference fields are valid (e.g. uid.name would not be valid because name is not an entity reference, it's nonsensical to "include" this).

It's because filter and include only truly share resolution that I'm suggesting we separate out into an entirely new method rather than keep it all in resolveInternal.

As long as any one expansion of a path is valid, the include path should be considered valid. IOW, when we hit an "ambiguous" part of the path, instead of throwing an error, the logic should just fork.

This went over my head

There's an additional validation constraint on filter that does not apply to include: a path relationship cannot reference more than one possible entity type. That's what's happening in this issue: field_comment.entity_id.uid applies both to:

field_comment.entity_id:node.uid
field_comment.entity_id:taxonomy_term.uid

The entity query system can't handle that, therefore it's invalid for filter.

But it's not invalid for include. For include, if any of the paths are valid, the include path is valid. So, when something is ambiguous, we want to "fork" and keep the validation logic for each possibility.

That's because we evaluate include paths at runtime, per entity. So, we effectively do: $entity->get('field_comment')->get('entity')->get('entity_id')->get('entity')->get('uid')->get('entity'); for each resource. In that case, it doesn't matter if entity_id references two different entity types.

Better comments. Restores field de-aliasing. Still needs tests.

1. +++ b/src/Context/FieldResolver.php
   @@ -70,7 +70,18 @@ class FieldResolver {
   +   * can reference two different JSON API resource types, which have the
   

   Unnecessary comma.

2. +++ b/src/Context/FieldResolver.php
   @@ -79,43 +90,60 @@ class FieldResolver {
   +  public static function resolveInternalIncludePath(ResourceType $resource_type, array $path_parts, $depth = 0) {
   

   Undocumented param.

3. +++ b/src/Context/FieldResolver.php
   @@ -79,43 +90,60 @@ class FieldResolver {
   +      throw new BadRequestHttpException($depth === 0
   

   This could use a comment.

4. +++ b/src/Context/FieldResolver.php
   @@ -79,43 +90,60 @@ class FieldResolver {
   +    // resolution happens in a recursive process. Prepend the current field name
   +    // before returning.
   

   Second sentence is superfluous.

Fixes a couple CS violations and fixes the errors in #24.

I think #23 deserves to be added to jsonapi.api.php, because it's essential information about the JSON API module's code architecture! 😯👏

It's because filter and include only truly share resolution that I'm suggesting we separate out into an entirely new method rather than keep it all in resolveInternal.

Given that explanation, this totally makes sense!

The entity query system can't handle that, therefore it's invalid for filter.

But it's not invalid for include.

Again such an eloquent, crystal clear explanation! 👏 Let's document it! Let's not keep it locked up in this one issue!

Thanks @gabesullice!

I'll add test coverage for resolveInternalIncludePath()

🙏 Removed Needs tests tag! :)

I have test many extreme case of include and filter in our site, all of my test requests works.

🎉

Thanks @caseylau!

Review of #24's interdiff:

1. +++ b/src/Context/FieldResolver.php
   @@ -70,7 +70,18 @@ class FieldResolver {
   +   * For example, a path of @code field_manager.user @endcode might resolve to
   

   Übernit: fieldmanager would be a very atypical field name, wouldn't it? That makes this comment less grokkable than it otherwise would be.

2. +++ b/src/Context/FieldResolver.php
   @@ -70,7 +70,18 @@ class FieldResolver {
   +   * @code field_manager.field_user @endcode if @code field_manager @endcode
   +   * can reference two different JSON API resource types, which have the
   +   * external field name @code user @endcode aliased to different internal
   +   * field names.
   

   Wait, is that even allowed? How can the same alias be used for multiple fields?! :O

3. +++ b/src/Context/FieldResolver.php
   @@ -70,7 +70,18 @@ class FieldResolver {
   +   * This may also happen when an entity reference field has
   +   * different allowed entity types or allowed bundles *per* bundle (as is the
   +   * possible with comment entities).
   

   Ahhhhh…

   Can't we make jsonapi_extras smart enough to disallow this? Wouldn't that be the saner solution?

   I guess the idea is that each bundle (and hence resource type) is free to do whatever it wants on bundle-specific fields, and if some of those bundle-specific fields happen to have the same name, then there's nothing wrong with that.

   Can you confirm that this is the rationale?

4. +++ b/src/Context/FieldResolver.php
   @@ -79,43 +90,60 @@ class FieldResolver {
   +  public static function resolveInternalIncludePath(ResourceType $resource_type, array $path_parts, $depth = 0) {
   

   $depth is not yet documented. It is documented in a later iteration ✔️

5. +++ b/src/Context/FieldResolver.php
   @@ -79,43 +90,60 @@ class FieldResolver {
   +      $message = "`$internal_field_name` is not a valid relationship field name.";
   +      if (!empty(($possible = implode(', ', array_keys($resource_type->getRelatableResourceTypes()))))) {
   +        $message .= " Possible values: $possible.";
   

   This means that we'll be exposing internal (actual) rather than external (public) field names.

   OTOH, we have no way to get the external/public/alias for a field. So … this is actually the best we can do. Probably worth calling out in a comment though?

6. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -244,13 +244,21 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   +    // The primary resource type for 'related' routes is different than the
   +    // primary resource type of individual and relationship routes and is
   +    // determined by the relationship field name.
   

   🙏 This comment will save much "WTFF!?" exclamations in the future :)

7. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -244,13 +244,21 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   +    // Flatten the resolved possible include paths.
   +    $public_includes = array_reduce($public_includes, 'array_merge', []);
   

   Wow, neat trick!

Review of #30's interdiff:

1. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -63,6 +63,73 @@
   +    $resource_type = \Drupal::service('jsonapi.resource_type.repository')->get($entity_type_id, $bundle);
   ...
   +    $resource_type = \Drupal::service('jsonapi.resource_type.repository')->get($entity_type, $bundle);
   

   Nit: Let's make this service available as $this->resourceRepository in setUp().

2. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -63,6 +63,73 @@
   +    $this->setExpectedException(BadRequestHttpException::class);
   

   Can we also expect a message?

3. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -63,6 +63,73 @@
   +      ['entity_test_with_bundle', 'bundle1', 'host.fail!!.deep'],
   

   This one should fail because it's an invalid notation, right?

4. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -63,6 +63,73 @@
   +      ['entity_test_with_bundle', 'bundle2', 'field_test_ref2'],
   +      ['entity_test_with_bundle', 'bundle1', 'field_test_ref3'],
   

   What's the difference between these?

5. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -63,6 +63,73 @@
   +      ['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'],
   

   What's the difference between these?

6. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -63,6 +63,73 @@
   +      // Should fail because the nested fields is not a valid relationship
   +      // field name.
   +      ['entity_test_with_bundle', 'bundle1', 'field_test1'],
   +      // Should fail because the nested fields is not a valid include path.
   +      ['entity_test_with_bundle', 'bundle1', 'field_test_ref1.field_test3'],
   

   Nit: The comments should become the array keys.

35.1: Ah, good point. I hadn't even considered it. I was thinking of "manager" as in "product manager". Regardless, I think I made the comments a lot better.

35.2/3:

Can't we make jsonapi_extras smart enough to disallow this? Wouldn't that be the saner solution?

I think it's a key DX part of JSON API Extras to allow aliasing different internal fields with the same external field name. That's important to maintain BC as data models evolve, but also to unify concepts, like if you you added an author field to taxonomy terms, you may want to alias author to field_author for terms, but uid for nodes.

Can you confirm that this is the rationale?

Yes, each resource type can do whatever it likes. That is the rationale for the first part. I think you may have missed some reasoning in the second part though.

It doesn't really have anything to do with "bundle-specific"-ness. That comment is there because it's possible that you end up with resource types for different entity types (that's proved by the existence of this bug report).

How?

Imagine an entity type (like a node) has an entity reference field that can reference the entity type comment and does not have handler_settings limiting it to a single bundle. We know different comment types (bundles) all have an entity_id field. This field is an entity reference field. Unlike the first entity reference field though, entity_id's target_type differs per comment type (per bundle).

This means one comment type's entity_id field might reference nodes while another comment type might reference taxonomy terms. Let's call those ncomment and tcomment respectively (their resource type name would be comment--ncomment and comment--tcomment).

In code, you'd realize that $entity->get('field_comments')->referencedEntities() would return both ncomment tcomment comment entities at the same time. Go one step further and you'd realize that $entity->get('field_comments')->get('entity_id')->referencedEntities() can return either nodes or taxonomy terms at the same time too. That's the ambiguous part that the entity query API can't handle, but include totally can, because the pseudo-code I just showed would work just fine.

35.4: 👍
35.5: I think array_keys($resource_type->getRelatableResourceTypes()) actually returns public field names because setRelatableResourceTypes has this documentation:

@param array $relatable_resource_types ... The array should be a multi-dimensional array keyed by public field name whose values are an array of resource types.

35.6: 👍
35.7: 🤘

Review of #30's interdiff

I'll address that in my next comment/patch.

#35.30.1-6: All done, plus a few CS violation fixes.

This one should fail because it's an invalid notation, right?

That one's been around for longer than I can remember. Technically, it just fails because of host, not because of the anything else (like the !! chars)

#36 😍 👌

This comment is perfection.

The interdiff is too.

👏👏👏

Actually, I found the smallest, stupidest nit possible in #36:

+++ b/src/Context/FieldResolver.php
@@ -16,7 +16,44 @@ use Drupal\jsonapi\ResourceType\ResourceTypeRepositoryInterface;
+ * resolver ensures that the @code uid @endcode (which it resolved from @code

"that the uid" -> "that the uid field" or "that uid"

#37:

1. +++ b/src/Context/FieldResolver.php
   @@ -25,10 +25,11 @@ use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
   + * node--article @encode might "alias" the internal field name @code
   
   @@ -36,12 +37,12 @@ use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
   + * because @code field_first_name @encode is not an entity reference field,
   

   s/@encode/@endcode/

2. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -236,27 +245,38 @@ class FieldResolverTest extends JsonapiKernelTestBase {
   +      'nested fields' => [
   +        'entity_test_with_bundle', 'bundle1', 'host.fail!!.deep',
   

   ❓ s/nested fields/invalid path/

I'd fix it myself and commit right away, but the last point I'm not 100% certain about my last point. So, one last time to NW :)

40.36.1: 🦅👁

40.37.1: whew man, I did that everywhere!

40.37.2: I just updated the input path to (hopefully) get rid of the confusion. There is not, nor has there ever been, character validation of the path (which is what I think you're asking about?).

That's better :)

🚢

Moving to Drupal core's issue queue.

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