Implementation of user name in JSON:API can result in overwriting data

That sounds very bad! I'm not sure yet why this problem doesn't apply to rest.module though, IIRC we just tried to mimic what that exposed in jsonapi.module.

My question in #4 is still relevant as far as I can tell. What does jsonapi.module do different than rest.module?

Not sure, but maybe the display name could be exposed as a separate thing that is explicitly read-only?

+1 — name and display_name could be two separate fields — the latter would be a read-only computed field, which would avoid the problem you've demonstrated.

Thoughts?

@Berdir, the original use case was that the Admin UI needed to show the name of content authors to replicate the content admin listing:

But any decoupled site that wants to show the name of a content author needs access to the user's display name.

I think whatever solution we come up with will need to keep the derived display name under the name attribute in the JSON:API response. To do otherwise would be breaking BC for the read-only use case. That's probably 95% of the usages out there.

I think we could add a new field, perhaps user_name, for mutation purposes. In the interest of resolving this quickly, I think we can hardcode this in JSON:API instead of adding any computed fields to the user entity.

#8: the impact can be seen in the test failure 😀But you already know that of course, because you made the test pass in #10. That change is problematic though, it is a loss of functionality and a BC break.

Slack

This Slack conversation just happened about this issue:

gabesullice  2 hours ago
fwiw, @berdir misunderstood my proposal, so don't let that affect how you read it

berdir  2 hours ago
can you clarify what you meant then? not sure what I misunderstood

gabesullice  2 hours ago
ha, I didn't realize that this was in Drupal slack :smile: I was gonna write a reply on the issue.
I mean that we should make name read-only, and still put the derived label there. But that to make an actual edit to the name field on a user entity, you'd have to send the data under a new field called user_name, which would behave like name really should have from the start. (edited)

gabesullice  2 hours ago
Thus the data loss problem would go away, since if you sent back the value of name, it'd be a no-op

berdir  2 hours ago
but isn't that a bigger BC break than having the display name in a new field?

berdir  2 hours ago
90%+ of the sites probably don't use altered display names (except the anonymous special case)

berdir  2 hours ago
and now making name read-only would break their profile-edit form for example

berdir  2 hours ago
ha, I didn't realize that this was in Drupal slack
good thing you were still polite then :stuck_out_tongue:
:relieved:
1


gabesullice  2 hours ago
My observations are that most JSON:API sites are read-only. A relative minority actually use POST/PATCH and I'd bet an even smaller minority of those allow editing the user.

berdir  2 hours ago
ok, feel free to propose that in the issue then, you'll have to implement that then, don't know the json api code that well. I'm also currently not using it, so personally, I don't really care how we fix it :slightly_smiling_face: Just noticed this while looking into these deprecations and so on

gabesullice  2 hours ago
90%+ of the sites probably don't use altered display names (except the anonymous special case)
Why do you think so?

berdir  2 hours ago
well, the exact number is just a guess, but IMHO it's a feature that's not so commonly used. there are probably quite a few custom alter hooks out there. but https://www.drupal.org/project/usage/realname has just 2-3k installations on D8

gabesullice  2 hours ago
100% of sites using JSON:API and are using the name field are using the label() callback by definition... oh, wait, maybe I misunderstood... Maybe you're saying that 90% have not actually implemented a label callback

berdir  2 hours ago
exactly, not actually implement the alter hook inside the label callback to e.g. show firstname lastname fields instead

berdir  2 hours ago
there are actually quite a few problems with it still in core, lots of places don't use getDisplayName() when they should, autocomplete is a mess (because that still goes against the stored account name but then shows display name)... (edited)

gabesullice  2 hours ago
Gotcha. I hadn't considered that angle...

gabesullice  2 hours ago
it does mitigate my concern about BC a lot. It seems I misunderstood you! :wink:

wimleers (he/him)  2 hours ago
:smile:

berdir  2 hours ago
Yes only sites that actually have different display vs. account names would see a change.. And that annoying user 0 edge case

wimleers (he/him)  2 hours ago
the user 0 edge case is an important one actually

wimleers (he/him)  2 hours ago
IIRC the JS admin UI initiative needed exactly that: the ability to retrieve the anonymous user’s name

Proposal

Based on the above, I would propose this:

1. label: this is whatever $entity->label() returns — this would be read-only, allows the "anonymous" case to still work, and solves the critical data loss bug
2. $label_field, so name on Users, title on Nodes: if there is a label field, this allows it to be PATCHed (and POSTed)

The sad consequence is that for most entity types, we'll see two duplicate values. This would be a huge WTF.

A slight alternative implementation would be to add the necessary metadata to entity types to allow other modules to know whether label() is just returning a field value or whether it's computed. Sort of like label_callback, but different. Then JSON:API could continue to expose only $label_field for most entity types, and for User it could add a read-only label.

What if $label_field == 'label' :) I think if we'd expose that, then would need to be separate, outside of the field structure, to avoid conflicts similar to the URL), I think.

I thought about that too. We could put label on the same level as id, type, attributes, fields and meta. But that'd be odd. I think it'd be better to put it under meta. That'd still be outside the field structure, thus avoiding conflicts!

Well, we already have the label entity key, although nothing prevents a module to define that and still override label(). Which I think is even how media works,

Hm … 🤔Great idea! 👍 I think we can fix Media, because it currently does sort of violate the docs for \Drupal\Core\Entity\EntityTypeInterface::getKeys(), which say:

   *   - label: (optional) The name of the property that contains the entity
   *     label. For example, if the entity's label is located in
   *     $entity->subject, then 'subject' should be specified here. If complex
   *     logic is required to build the label,
   *     \Drupal\Core\Entity\EntityInterface::label() should be used.

because \Drupal\media\Entity\Media::label() indeed does:

  public function label() {
    return $this->getName();
  }

  public function getName() {
    $name = $this->getEntityKey('label');

    if (empty($name)) {
      $media_source = $this->getSource();
      return $media_source->getMetadata($this, $media_source->getPluginDefinition()['default_name_metadata_attribute']);
    }

    return $name;
  }

I like @Berdir's proposal. Let's see what @gabesullice thinks!

I'm not 100% sure that I understand the proposal @Wim Leers. The media part is confusing me, are you proposing we fix that too?

As I understand it, the proposal has only 2 parts:

1. Remove the special handling for the user entity type's name field.
2. Add a label member to every entity's meta member and populate the value with the label callback result.

Per the Slack thread above (thanks for transcribing that Wim!), I'm fine with part 1.

I don't like part 2 because if the label is part of the meta member, it can't be removed as part of a sparse fieldset.

Since the critical bug only applies to the user entity, let's keep the scope of this issue to only the user entity so we don't create a new feature without fully thinking all aspects of it through. And.. if we're scoping this to only the entity, we don't have to worry about conflicting field names, so we can add a display_name attribute that's populated by User::getDisplayName(). I think that's logical and not WTFy.

New Proposal

1. Remove special casing for the user name field so it's treated like any other field
2. Add a read-only display_name attribute to the user--user resource type whose value is the result of User::getDisplayName()

FWIW, I do think it'd be worthwhile to pursue a feature that adds an attribute which shows the entity label on every resource object under a consistent location. This probably isn't the place to do that though.

Good point about sparse fieldsets!

But replacing one piece of special-casing for User in JSON:API with another doesn't really get us very far. Other entity types that have a label() implementation that dynamically computes a name:

• \Drupal\commerce_log\Entity\Log::label()
• \Drupal\commerce_payment\Entity\Payment::label() (also uses data of an actual stored field)
• \Drupal\commerce_payment\Entity\PaymentMethod::label()
• \Drupal\entity_test\Entity\EntityTestNoLabel::label()
• \Drupal\group\Entity\GroupContent::label()
• \Drupal\paragraphs\Entity\Paragraph::label()
• \Drupal\tmgmt\Entity\Job::label()
• \Drupal\tmgmt\Entity\JobItem::label()
• \Drupal\tmgmt_local\Entity\LocalTask::label()
• \Drupal\tmgmt_local\Entity\LocalTaskItem::label()
• \Drupal\webform\Entity\WebformSubmission::label()
• … and that's just from the contrib modules I have checked out in my core development environment!

Especially now that Drupal Commerce is actively adopting JSON:API, I think it's important that we tackle that use case here too. Adding display_name for user--user resources may be an easy way out here, but once we come up with a generic solution that will have to stay around forever.

Well, we already have the label entity key, although nothing prevents a module to define that and still override label(). Which I think is even how media works, there is a field but it can also dynamically generate a value (most of those cases then persist it on the field in the end).

This sounds like a straight up bug that makes it impossible for modules like JSON:API to do this properly. So, let's fix that. There's only 3 entity types in Drupal core that do this wrong.

This does not fix the reported bug, but I think this probably a blocker in even being able to solve this generically. Thanks to @Berdir for pointing this out!

Rather than continue to keep talking about it, I figured that because this is a critical bug, I would just implement how I understood @Berdir's guidance (thanks again, @Berdir!).

Whenever an entity type has a label() callback that does more than just returning the label entity key field (detected using reflection that happens only during resource type repository rebuilds, i.e. not on every request), this adds a read-only label_dynamic field (it is ignored in writes).

This passes most JSON:API tests for at least User and Media. (Only MediaTest::testCollectionFilterAccess() is failing.)

Where did I assume it was this simple? I added an all-bold disclaimer to the end of #19 specifically to make it clear that I didn't think that'd fix it all.

Anyway, reverting #19's last hunk because of #21. That simultaneously fixes that last failing test in MediaTest :)

+++ b/core/modules/jsonapi/tests/src/Functional/MediaTest.php
@@ -46,6 +46,11 @@ class MediaTest extends ResourceTestBase {
+  /**
+   * {@inheritdoc}
+   */
+  protected static $labelFieldName = 'name';

+++ b/core/modules/jsonapi/tests/src/Functional/UserTest.php
@@ -205,7 +206,7 @@ public function testPatchDxForSecuritySensitiveBaseFields() {
     // @todo Remove the array_diff_key() call in https://www.drupal.org/node/2821077.

This addition is no longer needed.

+++ b/core/modules/jsonapi/tests/src/Functional/UserTest.php
@@ -205,7 +206,7 @@ public function testPatchDxForSecuritySensitiveBaseFields() {
     // @todo Remove the array_diff_key() call in https://www.drupal.org/node/2821077.
     $original_normalization['data']['attributes'] = array_diff_key(
       $original_normalization['data']['attributes'],
-      ['created' => TRUE, 'changed' => TRUE, 'name' => TRUE]
+      ['created' => TRUE, 'changed' => TRUE, 'label_dynamic' => TRUE]
     );

We wouldn't have to change this if the @todo had been resolved like it should've been (#2821077: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors was fixed in 2018!).

Opened #3075422: Follow-up for #2821077: address forgotten @todo in UserTest::testPatchDxForSecuritySensitiveBaseFields() for this.

I can make #22's last test failure pass … but I don't understand why it's failing just yet. Rather than investigating, I'm holding off to first get feedback. I think I've proven that this approach can work.

My experience building the JSON:API Explorer with @zrpnr taught me the value of the label() method. It's often very handy to have a human-friendly identifier that is a) at a known location and b) uniform across resource types. This feature of the EntityInterface makes it easy to create lists and more friendly UIs. That's a feature that JSON:API (the spec) lacks.

If we go with @Wim Leers's proposed patch, we're creating a Drupalism instead of a shareable solution. The next version of the JSON:API spec will have the concept of extensions that would permit us to author a "title" extension for the JSON:API spec. I think that'd be a really valuable contribution. Beyond that, by using an extension, we'd be able to put the label into a "reserved" area of the spec. IOW, we could make the label value a sibling of type and id:

{
  "type": "article",
  "id": "some-uuid-here",
  "title:value: "My first article"
}

In fact, that'd be super useful for field_tags if the extension applied to resource identifier objects as well. Users wouldn't need to do the clunky ?include=field_tags&fields[taxonomy_term--taxonomy_term]=name just to support the very simple case of a list of tag names on an article.

It's this kind of thinking that I want to exhaustively explore before we commit to a fix as far-reaching as adding a label_dynamic field to every resource object. I didn't think this critical bug fix was the place to have that conversation though. Maybe seeing the possibilities I'm thinking about will convince you of that @Wim Leers.

In #16, I proposed that we use the display_name attribute to preserve our freedom to choose another attribute with the term "label" in it without causing too much confusion.

The only downside of doing #16 is that, if we decide to put a label attribute on all entity types, the user entity will often have name, display_name and label_dynamic with identical values. However, I think a little risk of future duplication is a small price to pay in order to preserve our freedom to choose a more elegant solution for labels later, like the one I hinted at above.

You've convinced me! 🤩

Let's go with #16 then. If you can roll the patch, I can review. (And ideally @Berdir too.)

Gah, what an aggravating set of tests to update :| just toilsome.

Hopefully I got everything.

You did! 👏🤓

This is very close :)

1. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -242,6 +242,12 @@ public function createIndividual(ResourceType $resource_type, Request $request)
   +          // User resource objects contain a read-only attribute that is not a
   +          // real field on the user entity type.
   +          // @see \Drupal\jsonapi\JsonApiResource\ResourceObject::extractContentEntityFields()
   
   @@ -317,6 +323,13 @@ public function patchIndividual(ResourceType $resource_type, EntityInterface $en
   +    // User resource objects contain a read-only attribute that is not a real
   +    // field on the user entity type.
   +    // @see \Drupal\jsonapi\JsonApiResource\ResourceObject::extractContentEntityFields()
   
   +++ b/core/modules/jsonapi/src/JsonApiResource/ResourceObject.php
   @@ -308,9 +313,12 @@ protected static function extractContentEntityFields(ResourceType $resource_type
   +    // Special handling for user entities that allows a JSON:API user agent to
   +    // access the display name of a user. This is useful when displaying the
   +    // name of a node's author.
   +    // @see \Drupal\jsonapi\JsonApiResource\ResourceObject::extractContentEntityFields()
   
   +++ b/core/modules/jsonapi/src/Normalizer/ContentEntityDenormalizer.php
   @@ -47,6 +47,13 @@ protected function prepareInput(array $data, ResourceType $resource_type, $forma
   +    // User resource objects contain a read-only attribute that is not a real
   +    // field on the user entity type.
   +    // @see \Drupal\jsonapi\JsonApiResource\ResourceObject::extractContentEntityFields()
   
   +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -244,6 +244,14 @@ protected static function getFieldMapping(array $field_names, EntityTypeInterfac
   +    // Special handling for user entities that allows a JSON:API user agent to
   +    // access the display name of a user. This is useful when displaying the
   +    // name of a node's author.
   +    // @see \Drupal\jsonapi\JsonApiResource\ResourceObject::extractContentEntityFields()
   

   I'd like to see these point to a follow-up where we generalize this.

2. +++ b/core/modules/jsonapi/src/JsonApiResource/ResourceObject.php
   @@ -281,11 +282,14 @@ protected static function extractContentEntityFields(ResourceType $resource_type
   +    // name of a node's author.
   

   Let's either add "for example" or not tie this to Node entities.

3. +++ b/core/modules/jsonapi/tests/modules/jsonapi_test_user/jsonapi_test_user.info.yml
   @@ -0,0 +1,4 @@
   +name: 'JSON API user tests'
   
   +++ b/core/modules/jsonapi/tests/modules/jsonapi_test_user/jsonapi_test_user.module
   @@ -0,0 +1,17 @@
   + * Support module for JSON API user hooks testing.
   

   Übernit: s/JSON API/JSON:API/. Sorry 😅

4. +++ b/core/modules/jsonapi/tests/modules/jsonapi_test_user/jsonapi_test_user.module
   @@ -0,0 +1,17 @@
   +function jsonapi_test_user_user_format_name_alter(&$name, AccountInterface $account) {
   +  if ($account->isAnonymous()) {
   +    $name = 'User ' . $account->id();
   +  }
   +}
   
   +++ b/core/modules/jsonapi/tests/src/Functional/UserTest.php
   @@ -441,7 +442,7 @@ public function testCollectionContainsAnonymousUser() {
   -    $this->assertSame('Anonymous', $doc['data'][0]['attributes']['name']);
   +    $this->assertSame('User 0', $doc['data'][0]['attributes']['display_name']);
   

   👏👏👏

5. +++ b/core/modules/jsonapi/tests/src/Functional/UserTest.php
   @@ -551,4 +552,58 @@ public function testCollectionFilterAccess() {
   +    // 200 for well-formed GET request. Page Cache hit because of HEAD request.
   +    // Same for Dynamic Page Cache hit.
   

   This comment is a copy/paste remnant and should be deleted.

6. +++ b/core/modules/jsonapi/tests/src/Functional/UserTest.php
   @@ -551,4 +552,58 @@ public function testCollectionFilterAccess() {
   +    //print_r(Json::decode($response->getBody()));
   

   Oops 😁

7. +++ b/core/modules/jsonapi/tests/src/Functional/UserTest.php
   @@ -551,4 +552,58 @@ public function testCollectionFilterAccess() {
   +    // Load the user entity again, make sure the name was not changed.
   +    $this->entityStorage->resetCache();
   +    $updated_user = $this->entityStorage->load($this->entity->id());
   +    $this->assertEquals($original_name, $updated_user->get('name')->value);
   

   👍 This is the critical bugfix! Let's also upload a test-only patch; this should fail.

Looks like this is also coming up in #2336597, see #2336597-141: Convert path aliases to full featured entities and #2336597-144: Convert path aliases to full featured entities.

1. Created #3079254: API for JSON:API specific "extra" fields, e.g. for entity labels and updated the comments.
2. Done.
3. Done.
4. :)
5. Fixed.
6. Fixed.
7. Will do.

Perfect! 🥳

Thanks for the follow-up to try to generalise this. The special-casing is not pretty but I can't think of anything else immediate to close the critical bug. so.. Committed 5fabcff and pushed to 8.8.x. Thanks!

Whoa, this didn't have a change record or a release notes snippet! Fixed.

Here's the CR: https://www.drupal.org/node/3085275

Thanks @gabesullice! Good catch.

The release note ideally should also describe what it did before so users can understand whether the disruption applies to them. So just a teensy bit more of the CR content. Docs and examples here: https://www.drupal.org/issue-summaries#release-notes

Thanks for the pointers, @xjm! Fixed.

Great, thank you!