For quite a while, I'd been wondering what the exact purpose was of id_field: uuid in config/install/jsonapi.resource_info.yml. Then I ran into \Drupal\jsonapi\ParamConverter\EntityConverterField and it all made sense.

My concerns:

  1. Its config schema mysteriously labels it API field, whatever that means
  2. Entities are standardized on id and uuid. So what other field could you possibly have, or want to have? I see this was raised in #2745719: [FEATURE] Allow using alternate fields as ID too, but never addressed.
  3. Finally, the default configuration (uuid) gets in the way for those entity types that don't have UUIDs. Because even though EntityInterface contains a uuid() method, it's not at all guaranteed sadly:
       * @return string|null
       *   The UUID of the entity, or NULL if the entity does not have one.
    

    This means that the default configuration doesn't allow you to access those entities at all.

  4. If we continue to need the _entity_load_key route option, it should become _jsonapi_entity_load_key: all route options and similar things should be prefixed with the module name.
  5. This being configurable makes it too easy to write contrib modules and JS code that breaks when site A configured it to id, but site B configures it to uuid.
  6. JSON API the spec (http://jsonapi.org/) is highly opinionated. JSON API the Drupal 8 module should therefore also be. The fewer knows to play with to configure things, the more portable any code built to talk to Drupal 8 JSON API servers will be. And hence the more productive and plug-and-play the ecosystem will be.
  7. Therefore I propose to remove this setting altogether, and always use uuid when an entity supports UUIDs, and automatically use id otherwise.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Status: Active » Needs review
FileSize
3.29 KB
Wim Leers’s picture

  1. +++ b/config/schema/jsonapi.schema.yml
    @@ -6,4 +6,4 @@ jsonapi.resource_info:
    -      label: 'API field'
    +      label: 'Entity identifier field name'
    

    This can be deleted if we implement point 7, but if we don't do that, we should improve this label.

  2. +++ b/src/ParamConverter/EntityConverterField.php
    @@ -21,10 +15,13 @@ class EntityConverterField extends EntityConverter {
    -      if (!$entities = $storage->loadByProperties([$route->getOption('_entity_load_key') => $value])) {
    +      $entity_type_definition = $this->entityManager->getDefinition($entity_type_id);
    +      $id_field_name = $entity_type_definition->hasKey('uuid')
    +        ? $entity_type_definition->getKey('uuid')
    +        : $entity_type_definition->getKey('id');
    +      if (!$entities = $storage->loadByProperties([$id_field_name => $value])) {
    

    This is how I propose to implement point in the IS.

dawehner’s picture

+++ b/src/ParamConverter/EntityConverterField.php
@@ -21,10 +15,13 @@ class EntityConverterField extends EntityConverter {
+      $id_field_name = $entity_type_definition->hasKey('uuid')
+        ? $entity_type_definition->getKey('uuid')
+        : $entity_type_definition->getKey('id');

It would be kinda nice to point to a place which explains that UUIDs are optional

Wim Leers’s picture

+1. @see \Drupal\Core\Entity\EntityInterface::uuid() is the only place that mentions it. There's no explanation anywhere. Once such an explanation is added, it'd be referenced at \Drupal\Core\Entity\EntityInterface::uuid(), methinks.

e0ipso’s picture

All in favor of this new approach. To be honest, I think that an API consumer should not care about the form of ID they are getting. They have to get IDs from an entry point (collections).

One concerning point is POST and PUT. We should be able to accept as input the same thing we're spitting as output. That means that we need to make sure that the denormalizers are also updated in this patch.

My final thought is about BC. Do you think that if an entity gets a uuid key in the future, that will break BC for the API? Routes for those entities will change, relationships to those entities will change, …

Some comments about the current patch:

  1. +++ b/config/schema/jsonapi.schema.yml
    @@ -6,4 +6,4 @@ jsonapi.resource_info:
         id_field:
    

    We should drop this config property since we will not be using it anymore.

  2. +++ b/src/ParamConverter/EntityConverterField.php
    @@ -32,28 +29,11 @@ class EntityConverterField extends EntityConverter {
    -  public function applies($definition, $name, Route $route) {
    

    Shouldn't we keep this to avoid involuntary upcasting in non-JSONAPI routes?

e0ipso’s picture

Wim Leers’s picture

Do you think that if an entity gets a uuid key in the future, that will break BC for the API?

The only content entity that does not have a UUID currently, is \Drupal\aggregator\Entity\Item (thanks @Berdir for pointing that out!). And that's really an oversight: it has a guid field, which is kind of like a UUID, but RSS-specific. There's an issue to fix that: #2149851: Remove todo about GUID field on the 'aggregator_item' entity and add UUID field.

I think it's fair to assume in practice that every content entity has a UUID. So I think it's definitely fair to consider an entity type that does not have UUIDs will not get it in the future. If that changes, it's a BC break for the entity type, and hence it's unavoidably also a BC break for the JSON API output for that entity type. But, everything indicates this is a super super rare occasion. I don't think JSON API can possibly protect against this.

That means that we need to make sure that the denormalizers are also updated in this patch.

Can you elaborate on this? I don't quite understand what you mean.

Wim Leers’s picture

FileSize
3.39 KB
1.82 KB

Addressed all feedback.

Status: Needs review » Needs work

The last submitted patch, 9: 2831134-9.patch, failed testing.

e0ipso’s picture

Can you elaborate on this? I don't quite understand what you mean.

This patch is only centered on loading entities in a GET. Right now you can POST an article that relates to a author via the entity id (if configured). We'll need to enforce that the relationship is using UUID and not Entity ID, when UUID is available.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
344 bytes

Status: Needs review » Needs work

The last submitted patch, 12: 2831134-12.patch, failed testing.

Wim Leers’s picture

I think this is running into the bug described at #2833850: IDs not converted to UUIDs for POST/PATCH to relationship endpoint?

Wim Leers’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: 2831134-12.patch, failed testing.

Wim Leers’s picture

So to make this pass tests, I need to update \Drupal\Tests\jsonapi\Kernel\Normalizer\DocumentRootNormalizerTest(). Which also requires updating ResourceConfig::getIdKey(), to ensure it always returns 'uuid'.

This unfortunately also causes not this to be returned:

      'data' => [
        'type' => 'node_type--node_type',
        'id' => 'article',
      ],

but:

      'data' => [
        'type' => 'node_type--node_type',
        'id' => '<SOME UUID>',
      ],

Because now also config entities must return UUIDs, not IDs, and so for the config entity node.type.article, whose contents look like this:

langcode: en
status: true
dependencies: {  }
name: Article
type: article
description: 'Use <em>articles</em> for time-sensitive content like news, press releases or blog posts.'
help: ''
new_revision: true
preview_mode: 1
display_submitted: true

and for which the entity type definition (\Drupal\node\Entity\NodeType) includes this:

*   entity_keys = {
 *     "id" = "type",
 *     "label" = "name"
 *   }, 

… this means we're returning its UUID and not its ID (which was stored in the type key).

How do you propose we deal with this, e0ipso?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.1 KB
4.45 KB

Forgot the patch with my changes that caused me to ran into this.

Status: Needs review » Needs work

The last submitted patch, 18: 2831134-17.patch, failed testing.

Wim Leers’s picture

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
9.46 KB
1.94 KB
Wim Leers’s picture

Title: \Drupal\jsonapi\ParamConverter\EntityConverterField and `id_field: uuid` » Remove configurable 'id_field': always use UUID
Issue tags: -Backwards compatible API change

This has shifted course since it started, because now it's about removing the ability to use "ID" at all.

Status: Needs review » Needs work

The last submitted patch, 21: id_uuid-2831134-21.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.25 KB
10.86 KB
  1. removed the ID-related logic from the paramconverter
  2. renamed EntityConverterField to EntityUuidConverter, which is consistent with EntityRevisionConverter.
  3. renamed the service name to be consistent, and documented the reason for its priority.

Not testing this patch, because it's relative to #2838580: Remove configurable 'prefix' + change the non-configurable prefix from '/api' to '/jsonapi'.

hampercm’s picture

Patch in #24 looks good to me!

Wim Leers’s picture

FileSize
34.6 KB
25.97 KB

Now working to remove ResourceConfig::getIdKey(), because with this change it no longer has a reason to exist.

This also forced me to update quite a bit of test coverage. In fact, this required the addition of some very arduous missing test coverage (and especially mocks) to \Drupal\Tests\jsonapi\Unit\Normalizer\JsonApiDocumentTopLevelNormalizerTest. Missing, because the existing test coverage only handled id, not uuid.

Wim Leers’s picture

Title: Remove configurable 'id_field': always use UUID » [PP-1] Remove configurable 'id_field': always use UUID
Status: Needs review » Postponed
hampercm’s picture

The patch in #26 looks good as well. Verified tests pass when applied on top of #2838580: Remove configurable 'prefix' + change the non-configurable prefix from '/api' to '/jsonapi'. Can be RTBC once that lands!

Wim Leers’s picture

Wow, thanks for your thorough testing!

Wim Leers’s picture

Title: [PP-1] Remove configurable 'id_field': always use UUID » Remove configurable 'id_field': always use UUID
Status: Postponed » Needs review
FileSize
34.6 KB
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

RTBC per #28.

e0ipso’s picture

Some comments (none of them require a code change):

  1. +++ /dev/null
    diff --git a/config/schema/jsonapi.schema.yml b/config/schema/jsonapi.schema.yml
    deleted file mode 100644
    

    w00t!

  2. +++ b/jsonapi.services.yml
    @@ -83,9 +83,10 @@ services:
    +    class: Drupal\jsonapi\ParamConverter\EntityUuidConverter
    

    I think that this could be a good converter for Drupal core. Thoughts?

  3. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
    @@ -119,16 +118,13 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
    -        list($entity_type_id,) = explode('--', $value['type']);
    ...
    +      list($entity_type_id,) = explode('--', $value['type']);
    

    This shows that even with the current context class instead, we are exploding the type here. This should be fixed in a follow up.

  4. +++ b/src/Normalizer/RelationshipNormalizer.php
    @@ -83,7 +83,8 @@ class RelationshipNormalizer extends NormalizerBase {
    +      // @todo why have host_entity_id if we already have host_uuid, and they're identical?
    

    We should remove host_uuid in that follow up.

  5. +++ b/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
    @@ -365,7 +365,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
    +    $this->assertSame($normalized['data']['id'], NodeType::load('article')->uuid());
    

    Node types as UUIDs are not ideal, but that's life.

  6. +++ b/tests/src/Unit/Normalizer/EntityReferenceFieldNormalizerTest.php
    @@ -75,6 +78,19 @@ class EntityReferenceFieldNormalizerTest extends UnitTestCase {
    +    $self = $this;
    +    $entity = $self->prophesize(EntityInterface::class);
    

    I'm curious, more than anything else, about the reason of this variable reassign.

  • e0ipso committed 139afa5 on 8.x-1.x authored by Wim Leers
    feat(Maintainability) Remove configurable 'id_field': always use UUID (#...
e0ipso’s picture

Status: Reviewed & tested by the community » Fixed

Merged.

Wim Leers’s picture

Status: Fixed » Needs review
FileSize
1.48 KB

#32.2: yep, I was thinking the same. BUT! One major concern is that this would mean you can access example.com/node/1 and example.com/node/<UUID>, and they'd return exactly the same. But, good news: #2353611: Make it possible to link to an entity by UUID is doing exactly this! Once that's in, we can delete this code. You asking this made me remember that issue. I'll create a follow-up to add that @todo.

#32.6: because we need to access $this in a closure:

+++ b/tests/src/Unit/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
@@ -40,6 +44,30 @@ class JsonApiDocumentTopLevelNormalizerTest extends UnitTestCase {
+    $self = $this;
+    $uuid_to_id = [
+      '76dd5c18-ea1b-4150-9e75-b21958a2b836' => 1,
+      'fcce1b61-258e-4054-ae36-244d25a9e04c' => 2,
+    ];
+    $entity_storage->loadByProperties(Argument::type('array'))
+      ->will(function ($args) use ($self, $uuid_to_id) {
+        $result = [];
+        foreach ($args[0]['uuid'] as $uuid) {
+          $entity = $self->prophesize(EntityInterface::class);
+          $entity->uuid()->willReturn($uuid);
+          $entity->id()->willReturn($uuid_to_id[$uuid]);
+          $result[$uuid] = $entity->reveal();
+        }
+        return $result;
+      });

… but due to copy/pasting this code and refactoring it in one other case, that other case has an unnecessary $self = $this. We should fix that.

e0ipso’s picture

I'll merge this and also remove host_uuid in the commit.

  • e0ipso committed 9529719 on 8.x-1.x authored by Wim Leers
    refactor(Maintainability) Cleanup and refactor UUID properties (#2831134...
e0ipso’s picture

Status: Needs review » Fixed

re-fixed

Status: Fixed » Closed (fixed)

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