Add 'drupal_internal__target_id' to JSON:API representation of entity reference fields, because that can't be retrieved from the target resource for target entity types without corresponding resources

Hi @mstef, I'm pretty sure this report confirms my suspicion in #3034701-5: Filtering users by role.

IOW, I think this is a new regression in the 2.x branch.

I'll let @Wim Leers decide which should be retitled and/or closed as a duplicate.

Thanks for the very detailed bug report!

I'm gonna go ahead and mark the other as a duplicate. This issue summary more closely describes the root cause and doesn't have any back and forth comments from diagnosing the problem.

I think technically we could make this a feature request because the behavior was removed prior to 2.0, but it might be blocking users from updating and it's also forcing users to use a less performant query string.

Thanks for the issue management, @gabesullice :)

I posted a proposed solution in #3034701-6: Filtering users by role.

The proposed solution only deals with targeted config entities, it does not address the performance regression associated with extra JOINS when the referenced entity is a content entity (and should be filterable). It's also unclear how it would "match the JSON:API output structure" too.

I think we need to add drupal_internal__target_id to the meta member of resource identifiers, then we can accept filter[field_entity_reference.meta.drupal_internal__target_id]=42 as a valid filter path, per https://www.drupal.org/node/3015183

The proposed solution only deals with targeted config entities

Correct.

It's also unclear how it would "match the JSON:API output structure" too.

We'd generate a slightly different query so that on the surface that remains true. But I guess you're saying that config entities are identified by UUID in JSON:API, not their unique name, so we'd still not match the JSON:API output structure?

I think we need to […]

I like that. It's not pretty, but it's a consequence of JSON:API identifying everything by UUID. It keeps consistency and it makes it clear that you're coupling this to a name that could change (e.g. if a role gets renamed.) rather than a guaranteed-to-never-change UUID.

@mstef I don't think this is currently being looked at.

Hi e0ipso. I think this feature is very useful.
Are you now any solution to get all user of specific role?

Here's an initial implementation. If someone would be willing to write new tests and update the existing tests for this then this would land much more quickly :)

Setting to Needs Review to get an initial test run. I imagine lots will fail because nothing is expecting the new property to show up under relationship fields.

That last one will fail code standards. This one should pass.

I've tried applying this patch as a workaround to the performance issues we've experienced which are described in https://www.drupal.org/project/jsonapi/issues/3022864#comment-13211445.

Filtering by filter[field_reference.drupal_internal_target_id does not appear to reduce the amount of INNER_JOINS in the resulting query unless I'm missing something.

This is the request URL

/jsonapi/node/event
?filter%5Bstatus%5D%5Bvalue%5D=1
&filter%5Bdata%5D%5Bcondition%5D%5Bpath%5D=field_date_time.value
&filter%5Bdata%5D%5Bcondition%5D%5Bvalue%5D=2019-08-06T03%3A59%3A59.999Z
&filter%5Bdata%5D%5Bcondition%5D%5Boperator%5D=%3E
&filter%5Bnode--location%5D%5Bcondition%5D%5Bpath%5D=field_location.meta.drupal_internal__nid
&filter%5Bnode--location%5D%5Bcondition%5D%5Bvalue%5D%5B%5D=2788
&filter%5Bnode--location%5D%5Bcondition%5D%5Bvalue%5D%5B%5D=2789
&filter%5Bnode--location%5D%5Bcondition%5D%5Bvalue%5D%5B%5D=2790
&filter%5Bnode--location%5D%5Bcondition%5D%5Bvalue%5D%5B%5D=2791
&filter%5Bnode--location%5D%5Bcondition%5D%5Boperator%5D=IN
&filter%5Btaxonomy_term--audience%5D%5Bcondition%5D%5Bpath%5D=field_event_audience.meta.drupal_internal__tid
&filter%5Btaxonomy_term--audience%5D%5Bcondition%5D%5Bvalue%5D%5B%5D=202
&filter%5Btaxonomy_term--audience%5D%5Bcondition%5D%5Bvalue%5D%5B%5D=203
&filter%5Btaxonomy_term--audience%5D%5Bcondition%5D%5Bvalue%5D%5B%5D=204
&filter%5Btaxonomy_term--audience%5D%5Bcondition%5D%5Boperator%5D=IN
&page%5Blimit%5D=10
&sort=field_date_time.value

And this is the resulting slow query log

# Time: 190815 20:37:30
# User@Host: pantheon[pantheon] @  [##.###.#.###]
# Thread_id: 18  Schema: pantheon  QC_hit: No
# Query_time: 181.824897  Lock_time: 0.000803  Rows_sent: 11  Rows_examined: 7176
SET timestamp=1565901450;
SELECT base_table.vid AS vid, base_table.nid AS nid, min(node__field_date_time_2.field_date_time_value) AS expression
FROM
node base_table
INNER JOIN node_field_data node_field_data ON node_field_data.nid = base_table.nid
INNER JOIN node__field_event_designation node__field_event_designation ON node__field_event_designation.entity_id = base_table.nid
INNER JOIN node__field_date_time node__field_date_time ON node__field_date_time.entity_id = base_table.nid
INNER JOIN node__field_location node__field_location ON node__field_location.entity_id = base_table.nid
LEFT OUTER JOIN node node ON node.nid = node__field_location.field_location_target_id
INNER JOIN node_field_data node_field_data_2 ON node_field_data_2.nid = node.nid
INNER JOIN node__field_event_audience node__field_event_audience ON node__field_event_audience.entity_id = base_table.nid
LEFT OUTER JOIN taxonomy_term_data taxonomy_term_data ON taxonomy_term_data.tid = node__field_event_audience.field_event_audience_target_id
INNER JOIN taxonomy_term_field_data taxonomy_term_field_data ON taxonomy_term_field_data.tid = taxonomy_term_data.tid
INNER JOIN node_field_data node_field_data_3 ON node_field_data_3.nid = base_table.nid
INNER JOIN node_field_data node_field_data_4 ON node_field_data_4.nid = base_table.nid
INNER JOIN node_field_data node_field_data_5 ON node_field_data_5.nid = base_table.nid
INNER JOIN node__field_location node__field_location_2 ON node__field_location_2.entity_id = base_table.nid
LEFT OUTER JOIN node node_2 ON node_2.nid = node__field_location_2.field_location_target_id
INNER JOIN node_field_data node_field_data_6 ON node_field_data_6.nid = node_2.nid
INNER JOIN node_field_data node_field_data_7 ON node_field_data_7.nid = base_table.nid
INNER JOIN node__field_event_audience node__field_event_audience_2 ON node__field_event_audience_2.entity_id = base_table.nid
LEFT OUTER JOIN taxonomy_term_data taxonomy_term_data_2 ON taxonomy_term_data_2.tid = node__field_event_audience_2.field_event_audience_target_id
INNER JOIN taxonomy_term_field_data taxonomy_term_field_data_2 ON taxonomy_term_field_data_2.tid = taxonomy_term_data_2.tid
INNER JOIN node_field_data node_field_data_8 ON node_field_data_8.nid = base_table.nid
INNER JOIN node_field_data node_field_data_9 ON node_field_data_9.nid = base_table.nid
LEFT JOIN node__field_date_time node__field_date_time_2 ON node__field_date_time_2.entity_id = base_table.nid
WHERE ((node_field_data.status = '1') and (node__field_event_designation.field_event_designation_value = 'events') and (node__field_date_time.field_date_time_value > '2019-08-06T03:59:59.999Z') and (node_field_data_2.nid IN ('2788', '2789', '2790', '2791')) and (taxonomy_term_field_data.tid IN ('202', '203', '204'))) AND (node_field_data_3.status = '1') AND (node_field_data_4.status = '1') AND (node_field_data_5.status = '1') AND (node_field_data_6.status = '1') AND (node_field_data_7.status = '1') AND (taxonomy_term_field_data_2.status = '1') AND (node_field_data_8.status = '1') AND (node_field_data_9.type = 'event')
GROUP BY base_table.vid, base_table.nid
ORDER BY expression ASC
LIMIT 11 OFFSET 0;

Should I expect the extra joins to be there to support the additional WHERE ...status=1 statements?

Bumping priority because this regression can:

• Render one feature unusable with no workaround.
• Cause a significant admin- or developer-facing bug with no workaround.

IIRC, what this issue needs is for someone to write a regression test and to update the existing test expectations to account for the new field.

Rerolled against 8.9.x and added a regression test for a possible use case. Leaving the "Needs tests" because I would also like to validate the case of filtering by a target content entity ID, in addition to a config machine name (like this interdiff shows).

Here's a refinement of the regression test I added in #15.

That means that all that is left for this issue is to go through the test failures and update their implementations of ResourceTestBase::getExpectedDocument() to include the new drupal_internal__target_id property. For the most part, this will just be copy and paste, so I'm removing the "Needs tests" tag since no new tests need to be written.

This should fix the expectations for Drupal\Tests\jsonapi\Kernel\Context\FieldResolverTest::testResolveInternalEntityQueryPathError.

While we're in FieldResolverTest, we should probably also test for the non-error case.

Adding credit for @bbrala because he helped me with #17 and #18.

EDIT: Comment #24 had the corrected patch

I've started fixing the tests, fixed the kernel tests and started on the functional tests.

One of the things i found is that ResourceTestBase::getExpectedGetRelationshipDocumentData() does not return the correct meta for related resources. This is especially visible in the resources that include user or something similar. I tested some changes to that method in Drupal\Tests\jsonapi\Functional\PathAliasTest::testRelationships to try and reproduce the correct meta. But whatever i did i ended up overwriting the arity meta data for the resulting document.

I was basing the logic for finding the correct values on the code in FieldResolver line 340 and up. But just didn't get it to work. I think i was just looking in the wrong place and couldn't really use my debugger to find out where i needed to be unfortunately.

The last testcode i had in ResourceTestBase::getExpectedGetRelationshipDocumentData() on line 1755 and up is below. As i said, it was not working, but i'm curious if i was even in the right spot :)

    if (!$is_multiple) {
      $target_entity = $field->entity;
      if (is_null($target_entity)) {
        return NULL;
      }
      else {
        $resource_identifier = static::toResourceIdentifier($target_entity);

        return $resource_identifier;
      }
    }
    else {
      return array_filter(array_map(function ($item) use ($internal_meta) {
        /** @var \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem $item */
        $target_entity = $item->entity;

        if (is_null($target_entity)) {
          return NULL;
        }
        else {
          $resource_identifier = static::toResourceIdentifier($target_entity);
          $fieldStorageDefinition = $item->getFieldDefinition()
            ->getFieldStorageDefinition();
          $property_definition = $fieldStorageDefinition->getPropertyDefinition('target_id');
          $is_data_reference_definition = $property_definition instanceof DataReferenceTargetDefinition;
          if($is_data_reference_definition){
            $resource_identifier['meta']['drupal_internal__target_id'] = $item->target_id;
          }
          return $resource_identifier;
        }
      }, iterator_to_array($field)));
    }

Ok, i cannot generate a patch file with PhpStorm appearantly. Used git this time.

Ok, trying one last time hopefully.

This patch is not pretty yet, but should fix a load of missing arity meta data.

So most things have been fixed, but the next problem surfaced. It seems there is an issue where the getPostDocument method is used for generating the document to POST but also for the document in the response. This seems to trigger errors where the FieldItemNormalizer tries to make sense of the drupal_internal__target_id. This breaks because it is not really a field. It seems we need a way to separate those two so that the normalizer doesnt screw itself.

Could you have a look @gabesullice if this is true? Or perhaps it's my changes to the getExpectedGetRelationshipDocumentData method, but it doesn't feel that way.

Gabe supplied a small patch for the FieldItemNormalizer issue. I also deduplicated some of the code to a method.

+++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
@@ -1820,4 +1807,29 @@
+    $property_definition = $fieldStorageDefinition->getPropertyDefinition('target_id');

What if target_id doesn't exist on the field? Will this throw an exception?

Elsewhere, we do $main_property_name = $field_storage_definition->getMainPropertyName(), then $property_definition = $field_storage_definition->getPropertyDefinition($main_property_name).

Good catch! I've updated the implementation to check for existance of the key before using it. Also fixed the last missing target_id in one of the tests.

Yay green! :)

Thanks for putting this together. I wanted to help out, but ran out of time to get to this. The patch looks good and works for me.

A few thoughts on review.

1. 'drupal_internal__' is used throughout the project as a string. Would it make sense to use a constant for this so we use a centralized string?
2. Do you think we should try and include other properties of reference fields (or other fields as well?) For example, Entity Reference Revisions provides a target_revision_id in a addition to the target_id. I'm sure there are other places (the media world for example) where properties are added to references. Should we be iterating over all properties of the fields instead of just the main property? We should be able to do this using getPropertyDefinitions() in place of getMainPropertyName() to get all the definitions. Otherwise $item->propertyDefinitions() may get us what we need.

Otherwise I think everything looks good.

We discussed #33 a bit during the API-first meeting in Drupal Slack. @richgerdes, I like your suggestion for #33.1, but I think that should be a follow-up. Also, maybe you can open another issue to discuss #33.2? The reason no other properties get the prefix is because the target_id field is a weak duplicate of a resource identifier object's id member. The only reason you'd want to use it is to exploit some knowledge of internal implementation details to optimize your query or work around the difference between content and config entities. All other properties already exist on under the meta member, without a prefix and convey unique information.

Wow, thank you @bbrala! This issue took a lot more grit than I anticipated! Thank you! 🙏

Overall, this looks really good. I only had a few things that I'd like to see fixed. None of them were major, so I went ahead and made the changes myself :)

Leaving as NR for tests, but if they're green, I think this is RTBC.

Here are all the things I addressed:

1. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -1240,4 +1243,68 @@ public function testAliasedFieldsWithVirtualRelationships() {
   +   * Tests that content entities can be filtered by a target config entity.
   +   *
   +   * This example uses user roles, another example could be filtering nodes by
   +   * bundle (although it would be better just to filter by resource type).
   

   This is out of date. We filter both by a target config entity and a target content entity.

   I also think that this points out that this issue needs an issue summary and title update. We're not really ensuring that JOINs are not added, but only that it's possible to filter by the target_id property, which is useful under certain circumstances.

2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -1240,4 +1243,68 @@ public function testAliasedFieldsWithVirtualRelationships() {
   +    // Ensure that an entity can be filtered by an integer target entity ID.
   

   Nit: "a target entity integer ID"

3. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -1240,4 +1243,68 @@ public function testAliasedFieldsWithVirtualRelationships() {
   +    // Only one node should be returned, authored by the filtered user.
   

   Nit: "—the one authored by..."

4. +++ b/core/modules/jsonapi/tests/src/Functional/MediaTest.php
   @@ -360,7 +372,7 @@ protected function getExpectedGetRelationshipDocumentData($relationship_field_na
   -        $data['meta'] = [
   +        $data['meta'] = $data['meta'] + [
   
   @@ -369,7 +381,7 @@ protected function getExpectedGetRelationshipDocumentData($relationship_field_na
   -        $data['meta'] = [
   +        $data['meta'] = $data['meta'] + [
   

   I think $data['meta'] should come after the addition operator because I think we want values in the overriding method to take precedence.

5. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
   @@ -1754,14 +1754,83 @@ protected function getExpectedGetRelationshipDocumentData($relationship_field_na
   +          if(!isset($arity_counter[$resource_identifier['type']])){
   +            $arity_counter[$resource_identifier['type']] = [];
   +            if(!isset($arity_counter[$resource_identifier['type']][$resource_identifier['id']])){
   +              $arity_counter[$resource_identifier['type']][$resource_identifier['id']] = 0;
   +            }
   +          }
   +          $arity_counter[$resource_identifier['type']][$resource_identifier['id']]++;
   

   I think this can be made simpler by null coalesce (yay, PHP 7!)

6. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
   @@ -1754,14 +1754,83 @@ protected function getExpectedGetRelationshipDocumentData($relationship_field_na
   +        if(isset($arity_counter[$identifier['type']]) && isset($arity_counter[$identifier['type']][$identifier['id']]) && $arity_counter[$identifier['type']][$identifier['id']] > 1){
   

   null coalesce could help here too.

   Also needs spaces after the if's so it's not if( (no space).

7. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
   @@ -1754,14 +1754,83 @@ protected function getExpectedGetRelationshipDocumentData($relationship_field_na
   +          $identifier['meta']['arity'] = $arity_map[$identifier['type']][$identifier['id']]++;
   

   I believe this will make arity values begin at 1, but arity is indexed from 0.

   Finally, in general, I think this section could use some more commentary.

   P.S. I'm very impressed by your implementation here. Arity is a weird concept Drupal-ism we invented. There are a ton of ways to naïvely get this implementation wrong. For example, it would have been really easy to misunderstand it as the field item delta or to forget that identifiers without a pair do not receive an arity member. Good job!

8. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
   @@ -1754,14 +1754,83 @@ protected function getExpectedGetRelationshipDocumentData($relationship_field_na
   +      $resource_identifier['meta']['drupal_internal__target_id'] = is_numeric($field->target_id) ? (int) $field->target_id : $field->target_id;
   

   I think this could use a comment. The first case is for content entity targets, the second is for config entity targets (which are identified by a machine-name).

+++ b/core/modules/jsonapi/tests/src/Functional/MediaTest.php
@@ -372,25 +372,28 @@ protected function getExpectedGetRelationshipDocumentData($relationship_field_na
+  public function testRelationships() {
+    parent::testRelationships(); // TODO: Change the autogenerated stub
+  }

Whoops! This shouldn't have been in #35.

Thanks for the review, compliment and fixes. A few small things.

4. Yeah you're absolutely right. It should come after.

5. and 6. Good catch about the null coalescing operator, i knew there was a way to get it shorter, but didn't think of that. And yeah, i was relying on PhpStorm for the codestyle. At work we just run fixers for stuff like that, but i couldn't get that to run on core.

7. I think you are incorrect. The ++ applies after assigning the value. So it will start at 0 and assign, then increase the value by 1. (And thanks for the compliment ;))

8. I didn't know exactly what this represented, so i kept it just a type hint.

Thanks for the help on this, it was fun to do :)

Updating the IS.

I'm not sure how to phrase the release notes snippet, given that this was introduced before JSON:API was added to Drupal core and prior to the release of 2.x in contrib. Whether it's a bug fix for a regression or a new feature really depends on the lens that you use to evaluate it.

This issue gets Drupal core's JSON:API back to feature-parity with JSON:API 1.x, but it does so in a way that still requires a client to be updated (this is already true when upgrading from 1.x to Drupal core). However, if you're already on Drupal core's JSON:API, this is purely a new feature. Since the release note is for Drupal core, do we consider it a regression if it was was never possible in Drupal core itself; especially if it was unintentionally removed during the 1.x -> 2.x changes?

Per my last comment, I'm not sure if this is really a regression.

7. I think you are incorrect. The ++ applies after assigning the value. So it will start at 0 and assign, then increase the value by 1.

I had to write a little test script to understand what you said:

<?php

$list = [];
for ($i = 0; $i < 5; $i++) {
  if (!isset($foo['bar'])) {
    $foo['bar'] = 0;
  }
  $list[$i] = $foo['bar']++;
}

print_r($list);

// Prints:

Array
(
    [0] => 0
    [1] => 1
    [2] => 2
    [3] => 3
    [4] => 4
)

What's happening is the the ++ operator doesn't increment $arity_map[$type][$id] until after the value of $arity_map[$type][$id] is assigned to $identifier['meta']['arity']! Which is, ofc, exactly what you said :)

I was incorrect. 🤒

Let's do away with the cleverness altogether, whether it's null coalesce or ++ after an assignment :)

@bbrala and I discussed and agreed to this interdiff in Slack. RTBC.

https://www.drupal.org/project/drupal/issues/303659 // 404 - Page not found
https://www.drupal.org/project/drupal/issues/3036593 // Here

+  /**
+   * Tests that collections can be filtered by an entity reference target_id.
+   *
+   * @see https://www.drupal.org/project/drupal/issues/303659
+   */
+  public function testFilteringEntitiesByEntityReferenceTargetId() {

@bbrala spotted an encoding error:

^{this is @bbrala's screenshot. He posted it in Slack}

Fixing.

+++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
@@ -1777,7 +1777,12 @@ protected function getExpectedGetRelationshipDocumentData($relationship_field_na
-          $arity_counter[$type][$id] = 1 + ($arity_counter[$type][$id] ?? 0);
+          if (!isset($arity_counter[$type][$id])) {
+            $arity_map[$type][$id] = 1;
+          }
+          else {
+            $arity_counter[$type][$id] += 1;
+          }

s/$arity_map/$arity_counter

Re-read this entire issue. Also re-read #3034701: Filtering users by role, which was marked as a duplicate of this.

Issue review

1. 👍In #6, I already +1'd the solution that was proposed in #5.
2. 👍The test coverage added in #15 + #16 makes sense, and tests that #5 is indeed working.
3. 🤔 I did not understand #17. I expected all existing test cases in the data provider to continue to work as-is, and only for new cases to be added.
4. 🤔🙏 #21 is such a huge interdiff that it would take at least an hour to understand why this set of changes is being made. So I hope that subsequent comments reduce patch size and/or justify the individual changes. Comments 22 through 32 did not help unfortunately.
5. 👉 #33.2: for this we have #3057545: ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type" :)
6. #38: Given https://www.drupal.org/project/usage/jsonapi shows only ~1500 sites still on JSON:API 8.x-1.x, meaning a multiple of that are on 8.x-2.x and only a handful of people have reported running into this, I think it's best to communicate it as a new feature first and foremost. I'd then end with a parenthetical that this did exist by accident in an earlier iteration of the contrib module, and that this should ease the update to core JSON:API.

Patch review

1. +++ b/core/modules/jsonapi/src/Context/FieldResolver.php
   @@ -677,7 +679,14 @@ protected static function isDelta($part) {
   +        $property_name = $property_definitions[$property_name] instanceof DataReferenceTargetDefinition
   

   🤔👍 It's so confusing that DataReferenceTargetDefinition does not implement \Drupal\Core\TypedData\DataReferenceDefinitionInterface 🙃 But that's not this code's fault obviously.

   I just expected this to live in isCandidateDefinitionReferenceProperty(), not in isCandidateDefinitionProperty(). But like I said, that's Typed Data API forcing us to do this.

2. +++ b/core/modules/jsonapi/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -150,7 +150,9 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
   +          }, ARRAY_FILTER_USE_KEY));
   

   🥳 AHHHHH! We can finally use this! Because Drupal 8.8 doesn't support PHP 5.5 anymore!

3. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -1240,4 +1243,67 @@ public function testAliasedFieldsWithVirtualRelationships() {
   +    $role_a = $this->drupalCreateRole([]);
   +    $role_b = $this->drupalCreateRole([]);
   ...
   +    $response = $this->request('GET', Url::fromUri('internal:/jsonapi/user/user?filter[roles.meta.drupal_internal__target_id]=' . $role_a), $request_options);
   ...
   +    $response = $this->request('GET', Url::fromUri('internal:/jsonapi/user/user?sort=drupal_internal__uid&filter[roles.meta.drupal_internal__target_id]=' . $role_b), $request_options);
   

   I think this test will become notably easier to understand by specifying a specific role ID, such as llamallovers and catcuddlers.

   Right now, the test is still fairly abstract, because it requires knowing that $role_a and $role_b contain meaningful names that drupalCreateRole() happens to generate.

   The whole point is that we show that it's okay for applications to hardcode certain configuration identifiers, because they are meaningful, and are guaranteed to not change.

   Seeing ?filter[roles.meta.drupal_internal__target_id]=llamalovers will be much easier to understand.

4. +++ b/core/modules/jsonapi/tests/src/Functional/MediaTest.php
   @@ -233,6 +235,9 @@ protected function getExpectedDocument() {
              'bundle' => [
                'data' => [
                  'id' => MediaType::load('camelids')->uuid(),
   +              'meta' => [
   +                'drupal_internal__target_id' => 'camelids'
   +              ],
                  'type' => 'media_type--media_type',
                ],
   

   👍 This is clearly valuable. Because you would only see a UUID before, and not the meaningful manually chosen ID.

5. +++ b/core/modules/jsonapi/tests/src/Functional/TermTest.php
   @@ -155,6 +155,9 @@ protected function getExpectedDocument() {
              'data' => [
                [
                  'id' => Term::load(2)->uuid(),
   +              'meta' => [
   +                'drupal_internal__target_id' => 2,
   +              ],
                  'type' => 'taxonomy_term--camelids',
   
   @@ -184,6 +187,9 @@ protected function getExpectedDocument() {
                  'id' => Term::load(2)->uuid(),
   +              'meta' => [
   +                'drupal_internal__target_id' => 2,
   +              ],
                  'type' => 'taxonomy_term--camelids',
   
   @@ -199,10 +205,16 @@ protected function getExpectedDocument() {
                  'id' => Term::load(3)->uuid(),
   +              'meta' => [
   +                'drupal_internal__target_id' => 3,
   +              ],
                  'type' => 'taxonomy_term--camelids',
                ],
   

   🤔🤔🤔 This feels like we're adding noise in that data. Two kinds of opaque IDs.

   I wonder if … we want to only do this for string IDs?

   At least initially? We could still add these in the future.

   That way, we get to keep our elegance, our minimalist response documents. It seems like a potentially better balance?

6. +++ b/core/modules/jsonapi/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -285,40 +287,34 @@ public function resolveInternalEntityQueryPathErrorProvider() {
          ],
          'message correctly identifies missing field' => [
            'entity_test_with_bundle', 'bundle1',
   -        'field_test_ref1.entity:entity_test_with_bundle.field_test1',
   -        'Invalid nested filtering. The field `field_test1`, given in the path `field_test_ref1.entity:entity_test_with_bundle.field_test1`, does not exist.',
   +        'field_test1.value',
   +        'Invalid nested filtering. The property `value`, given in the path `field_test1.value`, does not exist. Filter by `field_test1`, not `field_test1.value` (the JSON:API module elides property names from single-property fields).',
          ],
   

   🤔 I still don't understand why we're modifying so many test cases instead of keeping them and only adding new ones.

   Right now this just screams "I'm proving that this is a BC break", while we want to prove the opposite?

Thanks @wimleers

4. 🤔🙏 #21 is such a huge interdiff that it would take at least an hour to understand why this set of changes is being made. So I hope that subsequent comments reduce patch size and/or justify the individual changes. Comments 22 through 32 did not help unfortunately.

Yeah, i messed up making that patch. You should look at comment #24 for the correct interdiff and patch. PhpStorm was being unhelpfull.

In a Similar issue when paragraph items such as links are included as part of the include section, JSONAPI validation fails because links is a reserved word, here is a proposed solution for that.

@BryanRice, that seems like a related issue. Maybe you should submit your patch to a new issue to solve that problem.

I've gone through your feedback and think i've fixed everything.

1. Ok, not much i can do there :)
2. Yay indeed! ;)
3. I've changed the patch so it uses your names. I agree making it readable make things a bit more clear.
4. :)
5. I understand your point in this, that it might seem like noise. One of the things i really like about JSON:API and what i feel is its strength is consistency. I feel this is more consistent and therefor better.
6. You are right. Since only the messages have changed, i've updated the patch so it only changes the exception message. Not sure what i tried to do there.

The testbot returns 10 coding standards violations for the current patch. See: https://www.drupal.org/pift-ci-job/1735081

Sorry about that, forgot to run phpcs.

Thank you Björn Brala, phpcs looks good now.

The only failure is one in a functional JS test for CKEditor. So: a fail that's most definitely unrelated.

I'm still not sure about #48.5 and #56's response to it. I want to get the thoughts from either @e0ipso or @gabesullice on it, and preferably both. This is going to have a significant impact on the JSON:API DX.

Yeah i totally understand @wimleers, I tried to get this moving again by awnsering your review :)

Need a patch for 9.1

#63:

I do not like the proposed solution because it is a perfect or elegant solution. I like it because it is a practical one. There are two reasons to use the drupal_internal__target_id field:

1. You do not know, or cannot use, the target UUID. Such as when you want to filter by a role ID or from a content entity by a config entity.
2. You want to optimize away unnecessary JOINS in your query. Filtering by UUID forces Drupal to JOIN against the targeted entity table, but filtering by target ID does not.

@Wim Leers, I understand your desire to limit the scope of this change so that our responses contain fewer drupal_intenal__* members. They are ugly. But, IMO, that ship sailed when we put in attributes like drupal_internal__nid. Limiting to string IDs would be inconsistent not just between different relationship fields, but with attributes like drupal_internal__nid. One would wonder: "why is an integer okay here, but not there?"

And, less importantly, it would make that optimization I mentioned above impossible.

What would the perfect solution be, if this one isn't the perfect solution? I think we would add to or change the entity reference field's properties to include the target's UUID. There is a related existing issue for something like that: #2874548: Add a choice to use UUID in entity reference values instead of the entity's ID

I'm not confident that issue will ever land the way it is and the opt-in nature would force us to keep this inelegant work-around for some time.

We could create a better issue, titled: Track a target entity's UUID alongside its target ID column. which would be something that wouldn't require opt-in and wouldn't break BC. Doing so would allow JSON:API to filter by that UUID without JOINing against the target entity table.

Should we create that issue and postpone this one? IMO, no.

1. I think we should land this issue.
2. Create that issue.
3. Then create a follow-up to it to make use of the new UUID column and hide drupal_internal__target_id on newly created sites.

The reason I think we should do it in this order is because I think it will be years before all that work actually gets done, if ever. Our users shouldn't have to wait that long to filter a user by a role.

Back to NR, looks like a testing blip set this issue back to NW.

This is a really important issue for my organization. We use config entities extensively via entity reference fields, and not being able filter our entities by the id of the related config entity means that we can't use jsonapi, we have to build views with REST displays instead, which is not a great solution for us.

What can we do to help move this issue along?

We are running exactly in to this bug when we are trying to filter nodes based on an entity reference to a config entity. This is an important fix, since it's impossible to filter on the values of a referenced config entity normally, which is frustrating since we just need to filter on the target_id stored in the field.

We are trying to apply this patch to 8.9.x and will verify.

We also use (and help maintain) the entity_reference_uuid module, so I'll be interested to see if the target_uuid field value works with this patch.

In terms of the code, I think the change in \Drupal\jsonapi\Context\FieldResolver::isCandidateDefinitionProperty() could be written in a much more readable way with a foreach() loop.

For the change in \Drupal\jsonapi\Normalizer\JsonApiDocumentTopLevelNormalizer::denormalize() it seems like you could just use array_filter() without also needing the array_diff_key()?

For the change in \Drupal\jsonapi\Normalizer\JsonApiDocumentTopLevelNormalizer::denormalize() it seems like you could just use array_filter() without also needing the array_diff_key()?

Hmm, i've looked into this and i think:

$canonical_ids[] = array_diff_key($reference_item, array_filter($reference_item, function ($key) {
    return strpos($key, 'drupal_internal__') === 0;
}, ARRAY_FILTER_USE_KEY));

Is equal to:

$canonical_ids[] = array_filter($reference_item, function ($key) {
    return strpos($key, 'drupal_internal__') !== 0;
}, ARRAY_FILTER_USE_KEY);

The question is though, what is faster. Small piece of test code:

$array = array( "Italy"=>"Rome", "Luxembourg"=>"Luxembourg", "Belgium"=> "Brussels", "Denmark"=>"Copenhagen", "Finland"=>"Helsinki", "France" => "Paris", "Slovakia"=>"Bratislava", "Slovenia"=>"Ljubljana", "Germany" => "Berlin", "Greece" => "Athens", "Ireland"=>"Dublin", "Netherlands"=>"Amsterdam", "Portugal"=>"Lisbon", "Spain"=>"Madrid", "Sweden"=>"Stockholm", "United Kingdom"=>"London", "Cyprus"=>"Nicosia", "Lithuania"=>"Vilnius", "Czech Republic"=>"Prague", "Estonia"=>"Tallin", "Hungary"=>"Budapest", "Latvia"=>"Riga", "Malta"=>"Valetta", "Austria" => "Vienna", "Poland"=>"Warsaw") ;


function testOld($reference_item){
  $canonical_ids[] = array_diff_key($reference_item, array_filter($reference_item, function ($key) {
    return strpos($key, 'drupal_internal__') === 0;
  }, ARRAY_FILTER_USE_KEY));
  return $canonical_ids;
}


function testNew($reference_item){
  $canonical_ids[] = array_filter($reference_item, function ($key) {
    return strpos($key, 'drupal_internal__') !== 0;
  }, ARRAY_FILTER_USE_KEY);
  return $canonical_ids;
}

$start_time = microtime(TRUE);
$i=100000;
while($i-- > 0){
  testNew($array);
}
$end_time = microtime(TRUE);
echo $end_time - $start_time;

echo "<hr>";

$start_time = microtime(TRUE);
$i=100000;
while($i-- > 0){
  testNew($array);
}
$end_time = microtime(TRUE);
echo $end_time - $start_time;

Output:
2.668869972229
2.6212129592896

Doesn't matter much, so i guess a function less should be better since it's more readable.

Yes, I think just inverting the condition to avoid the extra function is what I was thinking. Not really a big deal for performance, just more readable.

Ok i got around to change the 2 parts to something little more readable. I also changed strpos to substr since that is quite a lot faster (see http://maettig.com/code/php/php-performance-benchmarks.php).

I like these readability improvements. Good call @pwolanin.

+++ b/core/modules/jsonapi/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -150,9 +150,9 @@
-          $canonical_ids[] = array_diff_key($reference_item, array_filter($reference_item, function ($key) {
-            return strpos($key, 'drupal_internal__') === 0;
-          }, ARRAY_FILTER_USE_KEY));
+          $canonical_ids[] = array_filter($reference_item, function ($key) {
+            return substr($key, 0, strlen('drupal_internal__')) === 'drupal_internal__';
+          }, ARRAY_FILTER_USE_KEY);

Tests are failing because this change inverted the logic. $canonical_ids[] is supposed to be all IDs whose keys do not begin with drupal_internal__. N.b.

$in = ['aa' => 'foo', 'ba' => 'foo'];
$out = array_diff_key($in, ['aa' => 'bar']);
var_export($out); // ['ba' => 'foo'];

Yeah noticed it when things started failing. Inverted the change :) Thanks!

It purring contently to the end. The test that were failing are not failing anymore, putting it back on needs review :)

Minor, but the foreach loop can just iterate over the keys and values?

+
+    foreach ($definition->getPropertyDefinitions() as $property_name => $definition) {
+      $name = $definition instanceof DataReferenceTargetDefinition
+        ? "drupal_internal__$property_name"
+        : $property_name;
+      if ($part === $name) {
+        return TRUE;
+      }
     }

Also minor, even if the substr() is trivially faster, the strpos() is more readable I think, so I'd favor that.

Ah, good call, overlooked that loop.

Regarding the substr i dont really agree. Comparing this way is about 4.5 times faster and not really less readable so i rahter not change that.

Failing test is unrelated, but again a falky Javascript test... Ckeditor.Drupal\Tests\ckeditor\FunctionalJavascript\CKEditorIntegrationTest

Sent for retest.

I don't feel strongly about the substr() vs strpos. It would be a little faster/more readable to take the strlen() once outside the array_filter and use the value in the lambda, but as much a style issue as anything.

Also, php micro benchmarks are pretty unreliable and version dependent, so e.g. preferring to use a string function instead of a regex like preg_match() is always true, or making one function call outside a loop instead of inside, but beyond that it's probably in the wash.

In the tests, I don't see why it's using hard-coded numeric IDs.

+                'drupal_internal__target_id' => 2,

Should almost certainly be

+                'drupal_internal__target_id' => $author->id(),

The absolute value of any numeric IDs in the test setup can differ if the underlying DB has a different autoincrement increment or offset.

You are right, there are a few places where we have an actual object to reference.

There is one left that could probably be a real piece of info, but i'm not sure how to get this one:

JsonApiDocumentTopLevelNormalizerTest.php::270

Arg, stupid types.

Fixed it the wrong way around. Oops.

Thanks, looks better but it looks like there are still some more places with a hard-code ID, such as

core/modules/jsonapi/tests/src/Kernel/Normalizer/RelationshipNormalizerTest.php

In that one, the fix is not obvious since it's using a data provider. Likely the best quick fix is to actually set the values of the IDs in the setUp() method where the entities are created so that you know they will always have a known, e.g.

    $this->user1 = User::create([
      'name' => $this->randomMachineName(),
      'mail' => $this->randomMachineName() . '@example.com',
      'uuid' => static::$userIds[0],
      'uid' => 7,
    ]);

(I'm avoiding uid 1 as a best practice since user 1 bypasses all permission checks normally).

Or better, perhaps, create a parallel static variable so the setUp() could look like:

<code>
    $this->user1 = User::create([
      'name' => $this->randomMachineName(),
      'mail' => $this->randomMachineName() . '@example.com',
      'uuid' => static::$userIds[0],
      'uid' => static::$userUids[0],
    ]);

So the test code would be like:

+          'data' => [
+            'type' => 'user--user',
+            'id' => static::$userIds[0],
+            'meta' => [
+              'drupal_internal__target_id' => static::$userUids[0],
+            ],
+          ],

The ID's are also hard-coded in these other test, but that seems to be an existing problem with the way the tests are written, so if you don't feel like fixing those, I think it would be reasonable to create a follow-up issue to address those.

core/modules/jsonapi/tests/src/Functional/TermTest.php
core/modules/jsonapi/tests/src/Functional/CommentTest.php

Yeah, there is a lot of hardcoded stuff around in the tests. I've updated a few files that were changed to something more sane, good pointer to reuse the "logic" for the user uuids. Attached a new patch and interdiff.

Created spinoff issue for hardcoded references.

Types again.

Maybe still need to update RelationshipNormalizerTest.php ti have a static list of expected uids?

Eh, didn't i just do that in my patch?

+++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/RelationshipNormalizerTest.php
@@ -56,6 +56,16 @@ class RelationshipNormalizerTest extends JsonapiKernelTestBase {
+  /**
+   * Static UIDs for use in tests.
+   *
+   * @var string[]
+   */
+  protected static $userUids = [
+    10,
+    11,
+  ];
+
   /**
    * Static UUIDs for use in tests.
    *
@@ -65,6 +75,15 @@ class RelationshipNormalizerTest extends JsonapiKernelTestBase {

@@ -65,6 +75,15 @@ class RelationshipNormalizerTest extends JsonapiKernelTestBase {
     '71e67249-df4a-4616-9065-4cc2e812235b',
     'ce5093fc-417f-477d-932d-888407d5cbd5',
   ];
+  /**
+   * Static UUIDs for use in tests.
+   *
+   * @var string[]
+   */
+  protected static $imageUids = [
+    1,
+    2,
+  ];
 

@@ -209,12 +250,18 @@ public function normalizeProvider() {
               'type' => 'user--user',
               'id' => static::$userIds[0],
-              'meta' => ['arity' => 1],
+              'meta' => [
+                'arity' => 1,
+                'drupal_internal__target_id' => static::$userUids[0],
+              ],
             ],
           ],

maybe I clicked the wrong patch file, sorry. Let me re-review.

Checking on a local site with entity_reference_uuid module enabled, looks like filtering works as expected with latest patch applied to Drupal 8.9.x:

/jsonapi/node/product_container?filter[field_product_type.meta.drupal_internal__target_uuid]=2a86c165-bca3-402a-a451-883de0ec1170

That module is also exposing the entity ID in meta, but I think that's a bug. #2934458: Mark target_id as a computed field

Trying to filter on that gives a 500 error:

/jsonapi/node/product_container?filter[field_product_type.meta.drupal_internal__target_id]=20000001

the error:

"Internal Server Error","status":"500","detail":"Invalid specifier \u0027target_id\u0027"

I don't see any outstanding issues in the patch, so I think it's RTBC

Thanks for the review @pwolanin!

Thanks @pwolanin! :)

This looks good to me, just some uber-nits.

1. +++ b/core/modules/jsonapi/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -150,7 +150,9 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
   +            return substr($key, 0, strlen('drupal_internal__')) !== 'drupal_internal__';
   

   any reason not to use strpos here?

   return strpos($key, 'drupal_internal__') !== 0;

   slightly easier to read?

   feel free to ignore

2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -1362,4 +1365,67 @@ public function testNonCacheableMethods() {
   +   * @see https://www.drupal.org/project/drupal/issues/3036593
   

   We don't need an @see to the issue a test was added for, we have git history for those kind of forensics - can you remove this?

3. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
   @@ -1755,14 +1755,93 @@ protected function getExpectedGetRelationshipDocumentData($relationship_field_na
   +      }
   +      else {
   ...
   +        else {
   

   the else here is superfluous, we returned early - feel free to ignore

We still need a release note and a change record here, but other than that and the removal of the @see and the take-it-or-leave-it observations above, this looks ready to go

Thanks Lorowlan.

1. Both you and @pwolanin gave this feedback. Guess i'll conform with you guys even though I feel it being suboptimal.
2. That line is not part of this patch, so we could change it, but that should not belong here.
3. You are right, less is more, simplified the control structure there.

It's still using the substr() to compare the leading string? Again, doesn't matter much, just a harder to read construction.

The other changes look good to further streamline the code.

Media_library.Drupal\Tests\media_library\FunctionalJavascript\WidgetUploadTest failed, unrelated to this issue. Resetting the status to RTBC

Looks like again unrelated test failed: core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php

That CK editor test is not fun T_T

Adding patch for 9.2.x
Fixed Cspell failed cases and one CS issues.

It's not clear to me why the property has to be drupal_internal_target_id and not target_id. If we were ommitting target_id before, why not add it back as-is? Would be useful if this was in the issue summary and change record, but also an inline comment in the patch somewhere.

Hi @catch!

tl;dr: the reason is to be consistent with the field name represented by JSON:API's output.

Longer answer:

We made an early decision in the development of JSON:API to use and standardize on entity UUIDs wherever possible. We found that including entities' serial IDs caused confusion because developers were not sure which ID to use. Therefore we prefixed the serial ID with drupal_internal to encourage developers to use the UUID (https://www.drupal.org/node/2984247). Later, we made filtering align with what JSON:API was serializing as well: https://www.drupal.org/node/3015183

For example, if one GETs a role config entity, the serial ID will be serialized using the drupal_internal__id field name. Therefore, it would be strange to have to filter by target_id when the field is externally represented to a consumer as drupal_internal__id.

IMO, it makes sense to stick with this convention of adding the drupal_internal__ prefix to any identifiers that are different from the the JSON:API id member wherever they appear so that A) filtering remains consistent with output and B) to encourage UUID usage as much as possible.

Changing to RTBC to get a new review with the information provided by gabe. (asked catch on slack about this).

Just a reroll. No actual changes.

Whether it's a bug fix for a regression or a new feature really depends on the lens that you use to evaluate it.

I'm evaluating this as a feature request, because it's not a regression of anything that was ever in core, and because in HEAD drupal_internal__target_id is fully absent in JSON:API output rather than being present and not filterable (correct me if I'm wrong on that).

Which in my mind makes this ineligible for 9.2.x. However, it would still be great to get it early into 9.3.x and people who need it in 9.2.x can apply the patch on their site.

Retitling to my understanding of the issue, but please correct it if I got it wrong.

Seems reasonable, but changing it to feature is wrong imo. This is a solution to an unintended sideeffect of an earlier change which broken filtering on target_id. This is also mentioned in the initial issue description.

Quick little reroll. Nothing of note changed so changing back to RTBC

Thanks for the re-roll. This still needs a change record and release note before it can be committed - to explain the new property available to developers.

I'll have a go at the change record.

Change record available at: https://www.drupal.org/node/3218910.

Updated documentation for when it lands in 9.3

Edit: fixed broken link

Committed/pushed to 9.3.x, thanks!

Awesome!