Spec Compliance: some entity types have an "id", "type" or "uuid" field, this violates the spec

Thank you very much for this crystal-clear bug report! Expanded the issue summary with an example of type.

Initial question: did you select 8.x-2.x-dev because that is the version you're using?

I honestly have no idea how we're going to fix this without massive disruption. If we can find a way, some disruption is going to be unavoidable, so this is definitely 2.x material.

Actually… there is a pretty simple answer to this. We should simply never normalize (expose)

1. the id entity key (which is nid for Node, uid for User, but id for Menu), because JSON API strongly encourages the use of the spec-mandated id, hence it's just useless noise
2. idem dito for bundle entity key (type for Node, comment_type for Comment) — also useless noise

In fact, it's this fact that it's useless noise that has made me wonder about this in the past. Omitting these would also make the JSON API responses easier to understand! In fact, this is probably one of the biggest WTFs for non-Drupalists that use a Drupal-powered JSON API implementation!

I honestly have no idea how we're going to fix this without massive disruption. If we can find a way, some disruption is going to be unavoidable, so this is definitely 2.x material.

I think this is okay to leave as-is in 8.1.x. It's a spec violation, but I don't see how it could be a significant problem in practice.

We should simply never normalize [the id and bundle entity keys].

+1

My only concern is that it we need to double-check that you do not need to know the Drupal ID to POST/PATCH an entity reference field on an individual route and that a UUID is sufficient (I'm 85% sure that this is case).

My only concern is that it we need to double-check that you do not need to know the Drupal ID to POST/PATCH an entity reference field on an individual route and that a UUID is sufficient (I'm 85% sure that this is case).

Agreed.

This makes NodeTest::testGetIndividual() pass. @gargsuchi, would you like to do the same for our other test coverage? :)

Fixing the remaining test failures would be a great novice task!

We should simply never normalize [the id and bundle entity keys].

My concern is that some entities require the id as references. For example, menu_link_content uses menu id for its menu_name attribute. Webform submission use webform id as its bundle.

@gabesullice in #6:

My only concern is that it we need to double-check that you do not need to know the Drupal ID to POST/PATCH an entity reference field on an individual route and that a UUID is sufficient (I'm 85% sure that this is case).

@sonnykt in #14:

My concern is that some entities require the id as references. For example, menu_link_content uses menu id for its menu_name attribute. Webform submission use webform id as its bundle.

Thanks to @sonnykt for digging in and explicitly confirming this is necessary. MenuLinkContent's menu_name field indeed is required to use Menu ids (not UUIDs).

My initial reaction would be well, we can translate UUIDs to IDs, but in the MenuLinkContent example that's not true, because its menu_name field is defined like this:

$fields['menu_name'] = BaseFieldDefinition::create('string')

instead of as

$fields['menu_name'] = BaseFieldDefinition::create('entity_reference')

Unless we fix all those field definitions, there's no hope of fixing this in JSON API. Because the metadata in Entity/Field API is incorrect/incomplete.

Furthermore: we should also stop omitting the uuid field in attributes! Because this is pure duplication. Perhaps that should be done in a separate issue though?

Furthermore: we should also stop omitting the uuid field in attributes!

s/stop/start/, right?

No, stop. id contains the UUID. attributes.uuid also contains it. We should omit the latter.
D'oh.

Stop exposing.

Start omitting.

So: yes! 😅😁

This will definitely be perceived as an unnecessary BC break, so this will only land in 2.x.

Rebased, and now also omitting the UUID.

Update expectation in NodeTest for #20.

Updated test expectations.

Working on fix…

Besides the MenuLinkContent edge case described in #15, I think this is ready.

But … that entity type is already barely usable from any HTTP API: #2915792: MenuLinkContentAccessControlHandler does not allow "view" access without admin permission, making these entities inaccessible via REST, JSON API and GraphQL and entity reference fields. So I created #2984224: MenuLinkContent's "menu_name" base field is a string field rather than an entity reference field for this problem.

Ahhh … the classical "crosspost on d.o results in the node being unpublished" bug. Republished this issue node…

I just realized that so far, the patch only makes content entities comply. Time to do the same for config entities!

Republishing for real.

😅

There are four config entity types that are a bit unfortunate: they have a type field. They are not omittable. The following config entity types have such a field:

1. Action
2. FieldStorageConfig
3. NodeType
4. Workflow

This is absolutely not fixable. It's also not reasonable to demand that those config entity types modify their data model for one particular HTTP API specification that they might be exposed via.

The only work-around I can think of, is to automatically alias them to _type. This does that.

Correction on #40: NodeType labels its id entity key field as type. But we omit that field already anyway.

Fix 2 of the failures.

Green 🤞

The only work-around I can think of, is to automatically alias them to _type. This does that.

Additionally, the [low line character] is allowed in member names, except as the first or last character

:\

Can we prefix them with {$entity_type_id}_ rather than just _ or suffix them with _field?

CR created: https://www.drupal.org/node/2984247.

#48 good call! Done.

Off-by-one error.

The "comment" test is failing because the {$entity_type_id}_ prefix is the actual prefix used for a base field on Comment. This is only causing problems because the current logic is blindly performing these transformations. And in fact, ResourceType is intended to be a pure value object, not something containing logic. So we can fix both at the same time.

Will do this on Monday.

Doing #54 was surprisingly much work, but also makes for a surprisingly nice architectural clean-up, and makes things simpler for JSON API Extras! It makes ResourceType a much more valuable and capable value object, and helps a lot in making JSON API more debuggable and hence maintainable.

It does mean there's less of a net negative diffstat though.

Just want to chime in and say that I'm +100 on this change. I really like the outcome :)

I've done a high-level review and have no concerns. But I'll do a very thorough review again when the patch is green.

Just want to chime in and say that I'm +100 on this change. I really like the outcome :)

🎉🎉🎉🎉🎉🎉🎉

Fixed CS violations and failing tests. In doing so, discovered that #2483407-33: Allow configuration schema fallback in ConfigEntityType::getPropertiesToExport() to work without an ID was the root cause, for which I had to add a work-around until that issue lands.

1. +++ b/src/Normalizer/EntityNormalizer.php
   @@ -270,13 +270,21 @@ class EntityNormalizer extends NormalizerBase implements DenormalizerInterface {
   +      // Skip any disabled field, except the always required bundle key and
   +      // required-in-case-of-PATCHing uuid key.
   +      // @see \Drupal\jsonapi\ResourceType\ResourceTypeRepository::getFieldMapping()
   +      if (!$resource_type->isFieldEnabled($internal_name) && $bundle_key !== $internal_name && $uuid_key !== $internal_name) {
            continue;
          }
   

   I wonder if we should instead assert(!in_array($internal_name, [$bundle_key, $uuid_key]) before the continue. That ensures that the resource type is behaving correctly rather than overriding it.

2. +++ b/src/ResourceType/ResourceType.php
   @@ -64,6 +64,29 @@ class ResourceType {
   +   * The list of disabled fields. Disabled by defualt: uuid, id, type.
   

   s/defualt/default/

3. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -146,6 +150,113 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +          throw new \LogicException('The generated alias conflicts with an existing field. Please report this in the JSON API issue queue!');
   

   Nice!

4. +++ b/tests/src/Functional/NodeTest.php
   @@ -150,20 +149,9 @@ class NodeTest extends ResourceTestBase {
   -          'type' => [
   -            'data' => [
   -              'id' => NodeType::load('camelids')->uuid(),
   -              'type' => 'node_type--node_type',
   -            ],
   

   I either forgot or didn't realize that this was just to be removed altogether. I expected to see this test converted to expect 'node_type'. I think it's still useful to have a relationship to the bundle entity, that's how you can get bundle's label for example.

   I imagine you're shocked I took so long to say that, I'm sorry :\ Looking at the issue it should have been completely obvious to me. You even said:

   We should simply never normalize (expose) ... [the] bundle entity key (type for Node, comment_type for Comment) — also useless noise

   I realize how quirky it is. Is there a way we could communicate that data in a better way? Maybe it's not even typical for a user to have access to view the bundle entity and wouldn't be able to access the bundle label in any event.

1. Unless I'm reading this incorrectly, I think you're misunderstanding what's going on :) This if-test is choosing to not execute continue to the next field in the array if the field is the bundle or UUID entity key field. Because those fields are necessary in PATCH requests. Hence the comment:
   

         // Skip any disabled field, except the always required bundle key and
         // required-in-case-of-PATCHing uuid key.
   

2. ✔️
3. ✔️
4. This is being removed because it's a bundle, and a bundle is implied by its resource type name. But you're of course right that it makes it harder to access the bundle resource type, for example for its label.
   

   Is there a way we could communicate that data in a better way?

   I'd like that, but IDK how. Only thought for now: a bundle link. It'd be a Drupalism, but it's just a link. That's far less intrusive than the relationship we are currently exposing.

   Maybe it's not even typical for a user to have access to view the bundle entity and wouldn't be able to access the bundle label in any event.

   Exactly.

   Before bringing back relationships to bundle entities, I want consensus on how to do that.

   I'm fine with just bringing it back like it was before (and automatically aliasing those fields to avoid violating the spec) and in that sense descoping "handle bundle entities in a more sensible way" from this issue.

#61.1:

Unless I'm reading this incorrectly, I think you're misunderstanding what's going on :)

I think you were misreading my remark. I was asking "why should the $resource_type ever tell us that the uuid or bundle field is enabled? We should just ensure that the resource type disables it."

BUT, I don't agree with myself today and after explaining that 😅

4:

I'm fine with just bringing it back like it was before (and automatically aliasing those fields to avoid violating the spec) and in that sense descoping "handle bundle entities in a more sensible way" from this issue.

I'm not comfortable with this big of a change without more feedback from @e0ipso and actual module users.

1. So … we don't need to change anything there anymore? :P

4. So you mean that we should bring back the bundle to the relationships normalization of JSON API, right?

Rebased #61.

1. Yes 😊
2. Yes, and we should probably file an issue for "[handling] bundle entities in a more sensible way".

Done!

CR updated: https://www.drupal.org/node/2984247.

Conflicted with #2984607: Remove Drupal core <8.5 BC layer code and require Drupal >=8.5. Easy conflict resolution.

Solving those last two failures turned out to be pretty damn hard. 😱 Pretty much all of the existing test coverage needed to be updated to account for public vs internal field names. Which makes sense. And in turn means … that JSON API Extras' aliasing abilities are now pretty much guaranteed to not break anymore. (Since JSON API itself will already be doing aliasing of certain fields to comply with the spec, JSON API Extras can just piggyback on that.)

This took hours rather than the 15 minutes I was expecting 😐

Also: #2953346: Define related/relationship routes per field, not dynamically (with route parameters that need validating) landed this morning, and that resulted in more code needing updates…

Sorry for coming late to this one.

+++ b/src/ResourceType/ResourceTypeRepository.php
@@ -146,6 +150,113 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
+    // JSON API resource identifier objects are sufficient to identify
+    // entities. By exposing all fields as attributes, we expose unwanted,
+    // confusing or duplicate information:
+    // - exposing an entity's ID (which is not a UUID) is bad
+    // - exposing its bundle is useless (it's already part of the resource
+    //   type name)
+    // - exposing its UUID as an attribute is useless (it's already part of
+    //   the mandatory "id" attribute in JSON API)
+    // @see http://jsonapi.org/format/#document-resource-identifier-objects

I don't think this is advisable in a world with more services other than Drupal.

Entity ID:
Projects are sending data all around the interwebs, and (in some cases) they are using the entity ID for it. They could definitely use UUID, but this may require sensible work for existing denormalized data sources (Elastic Search, …). I don't think we should drop it and cause trouble when we can just leave it alone.

Bundle:
Resource type name and bundle are independent. For instance you can change it using JSON API Extras to potato and the bundle info is unnecessarily lost. This would be an unnecessary (and implementation-specific) coupling.

UUID:
I'm OK with this one.

This should be green! 🤞

#71:

• Entity ID: JSON API always favored the UUID, and put that front and center. Drupal itself has been evolving towards UUIDs. It's necessary if you want to do things like sharing data between sites, workflows, workspaces.
  Why would this cause problems for Elastic Search?
  Also: any existing site can continue to use JSON API 1.x for a long time to come — there's more test coverage proving things are working fine than for most contrib modules!
• Bundle: I brought this back in #66, based on @gabesullice's feedback. 👍
• UUID: 👍

So that only leaves entity ID.

JSON API always favored the UUID

Yes. And that was the right call, I'm still convinced of that. We have encouraged the trend to move towards UUID, and I'm proud of that. However that is not necessarily related to dropping any other fields.

Why would this cause problems for Elastic Search?

it wouldn't, as I said They could definitely use UUID. However if a project has an index already running based off entity ID, we are making things hard for them.

I understand that we can drop the entity ID, I don't think we should just because we can. Also, locking people in older versions is probably not a viable solution.

Having the entity ID available is essential if you want to do anything in your API consuming application that links back to an admin path in Drupal e.g. contextual links that allow an editor to change content (this is typically done in clients by annotating sections with data-nid="5"). A very strong -1 from me for removing it.

I know you're both well-intentioned, but so am I. I intend to comply with the JSON API spec, because otherwise we break some JSON API clients. Not to mention that it's super confusing to anybody looking at Drupal's JSON API output: we have multiple identifiers! Worse, if we don't make this change now, and decide to do in a year or two (when JSON API is in core), then this will be impossible because it'd be a BC break. That's why I want to do it now, before 2.0 is tagged, because it gives us the chance to break BC once, for a future with fewer worries.

Having the entity ID available is essential if you want to do anything in your API consuming application that links back to an admin path in Drupal

Once #2353611: Make it possible to link to an entity by UUID lands, this will actually work fine with UUIDs!

Once #2353611: Make it possible to link to an entity by UUID lands, this will actually work fine with UUIDs!

But not the other examples that I provided. It feels that we keep telling the world that they are doing it wrong (the probably are!) and giving them a hard time so they are forced to be corrected in their behavior.

Worse, if we don't make this change now, and decide to do in a year or two (when JSON API is in core), then this will be impossible because it'd be a BC break.

Well, I'm arguing we should not do it. Data is data. We should not be judgemental on what we allow to be exposed.

we have multiple identifiers!

I think that's fine, as long as the nid is only in the attributes then we're fine. There is no confusion in what the ID is. The spec makes that very clear.

I intend to comply with the JSON API spec, because otherwise we break some JSON API clients.

I'm confused about this, isn't the spec vocal only about the name of the property? I think that dropping the entity ID does not ensure that some other field is created that breaks the spec.

I know you're both well-intentioned, but so am I

We don't agree, and that makes better software. I still 💙 you, and I know you know that.

I haven't read #77 yet, my eyes were drawn to the blue heart emoji :D

I just want to confirm AS LOUD AS I CAN that HELL YES, disagreeing and then working towards consensus makes for better software! FLOSS FTW! 💙

I think @Wim Leers is right that fields like nid, bundle and type are big DrupalWTFs.

I can see how certain coupled solutions to a Drupal backend would want that data. And I recognize @e0ipso's point that we shouldn't "lock in" certain implementations to the 1.x (IOW, they should be a migration path, even if things need to be changed).

I think this all comes back to this: "how do we mask Drupalisms in a way that permits tight Drupal integrations?"

I suggested this on Slack to @Wim Leers and @e0ipso independently.

What if we make pseudo-field under attributes like entity_keys?

Then, the response would roughly look like:

{
  internal_entity_keys: {
    id: 42,
    type: node,
    bundle: page
  }
}

This advertises that we don't encourage their use but still makes that data available. What do y'all think? @justafish, I didn't get pinged by you about this issue in Slack, so I haven't floated it by you yet :P

There is no confusion in what the ID is.

Except there is: when to use which? What's the meaning of this extra ID?

I'm confused about this, isn't the spec vocal only about the name of the property? I think that dropping the entity ID does not ensure that some other field is created that breaks the spec.

You're right — but quite a few entity types did not alias their id entity key field (Node aliased it to nid, Term to tid, etc, but not all entity types do this — basically only the "old school" ones).

how do we mask Drupalisms in a way that permits tight Drupal integrations?

👏 Perfect description!

I like your proposal a lot. I'd personally even name it drupal_internals, because that's clear even to a total outsider, whereas internal_entity_keys sounds potentially important yet scary!

drupal_internals

, because that's clear even to a total outsider, whereas

internal_entity_keys

sounds potentially important yet scary!

I'm fine with that. I considered it too, but I felt like it polluted people's API responses a little. That's okay for now and I agree it's clearer to an outsider. It might even more effectively dissuade them from using it.

On the back of my mind is that a lot of our error messages and DX-related features loudly advertise "this API is built with Drupal". Which is okay... for now. As adoption grows, I think users will want a more "white-label" experience which let's them build an API that appears no different from one built from scratch specifically for their app/company.

I feel this solution is not solving the problem stated by the issue title.

I'd love to get a reaction to my previous comment:

I'm confused about this, isn't the spec vocal only about the name of the property? I think that dropping the entity ID does not ensure that some other field is created that breaks the spec.

If I add a field named "type" then the spec is still broken. If I add a relationship named "id" the spec is still broken. I don't see how doing anything to these fields fixes the issue.

I have other thoughts about the proposed solution, but I want to hold them until we've discussed this in depth.

I feel this solution is not solving the problem stated by the issue title.

The spec reads: "[A] resource can not have an attribute and relationship with the same name, nor can it have an attribute or relationship named `type` or `id`"

By making those fields live in a new attribute not named type or id, I think it technically solves the problem.

I'd love to get a reaction to my previous comment

Yes, the spec only talks about the property name. The (most recent) proposed solution does not drop the entity ID.

I think the most interesting part of that remark is: "[It] does not ensure that some other field is created that breaks the spec."

Which is true... and maybe we should fix that, but that's technically not part of the issue scope as written. Do you think it should be?

If I add a field named "type" then the spec is still broken. If I add a relationship named "id" the spec is still broken. I don't see how doing anything to these fields fixes the issue.

Can you add fields named type or id? Aren't all configurable fields always prefixed by field_?

I can only think of base fields on a custom entity type not also part of an entity's keys which seems like a pretty small set. I'm not entirely sure what we could do about that or if we want to do anything about it. Other than that, there's jsonapi_extras, which I'd argue should be validating that any aliases are spec-compliant (if it's not doing so already).

Can you add fields named type or id? Aren't all configurable fields always prefixed by field_?

Base fields should also be considered. Moreover, for configurable fields you can have a different field name prefix or none at all. It's a config setting on Field UI. You can also create configurable fields programmatically (not using field ui) and those don't get any prefix.

By making those fields live in a new attribute not named type or id, I think it technically solves the problem.

In my opinion making 2 fields not break the spec is not a satisfactory solution. A satisfactory solution is to make all fields not break the spec.

I can only think of base fields on a custom entity type not also part of an entity's keys which seems like a pretty small set.

I think its not a pretty small set. Additionally you have to add any configurable field per my comment above.

Other than that, there's jsonapi_extras, which I'd argue should be validating that any aliases are spec-compliant (if it's not doing so already).

This is a very good point. I don't think that's ensured. I created #2985375: Validate field aliases to ensure spec compliance for that.

I'm not saying that moving drupalisms to a well defined scope is an unworthy goal. We should definitely consider it in a separate issue. But it's a different problem from what's being reported.

I can only think of base fields on a custom entity type not also part of an entity's keys which seems like a pretty small set.

for configurable fields you can have a different field name prefix or none at all. It's a config setting on Field UI. You can also create configurable fields programmatically (not using field ui) and those don't get any prefix.

[Because of that] I think its not a pretty small set.

TIL, I had no idea that that was configurable. That does indeed increase the set size.

It seems we need to expand the scope of this issue to handle all attribute and relationship field names.

Ideas?

Ideas?

Brainstorm:

1️⃣ What about checking if there is an id or type field and dropping it while normalizing? We could add partial success error that links to a d.o page explaining what happened and how to alias the field to some different name. The error object could even contain the key/value… or not.

2️⃣ Maybe we can add a default alias for type and id to __type and __id or similar. And then notify the dev that there is an alias applied using the same technique as above.

I'm confused about this, isn't the spec vocal only about the name of the property? I think that dropping the entity ID does not ensure that some other field is created that breaks the spec.

If I add a relationship named "id" the spec is still broken.

But those would already be aliased automatically by the current patch:

  foreach ($field_names as $field_name) {
    …
    if ($field_name === 'id' || $field_name === 'type') {
        $alias = $entity_type->id() . '_' . $field_name;
        if (isset($field_name[$alias])) {
          throw new \LogicException('The generated alias conflicts with an existing field. Please report this in the JSON API issue queue!');
        }
        $mapping[$field_name] = $alias;
        continue;
      }

I verified this manually by changing the field prefix from 'field_' to '', then adding an id field to node--article. Even though it's stored as id in the database, JSON API exposes it as node_id:

(And of course this logic applies to both base fields and configurable fields. This manual test proves it works for configurable fields. The logic quoted above shows it doesn't care about what kind of field it is — base or configurable. And finally, #72's interdiff and the preceding ones already prove that it works for base fields.)

In my opinion making 2 fields not break the spec is not a satisfactory solution. A satisfactory solution is to make all fields not break the spec.

Agreed!

But … that's exactly what this patch does! Drupal already doesn't allow multiple fields to have the same name, for obvious reasons. That means that the only remaining problem is: fields names reserved by the JSON API spec. Those are type and id. And those are being automatically aliased (explained above in detail), which therefore makes all fields not break the spec!

It seems we need to expand the scope of this issue to handle all attribute and relationship field names.

See above — this is already done!

2️⃣ Maybe we can add a default alias for type and id to __type and __id or similar. And then notify the dev that there is an alias applied using the same technique as above.

See above — this is already done!

OMG. I'm so sorry about all the churn. I was thrown off by the (in my opinion out of scope) entity ID and bundle removal and an interrupted review. #84 and some other comments before seemed to verify that misunderstanding.

🙏🏽

That leaves the disagreement on the matter discussed in internal_entity_keys and stuff. Also I think we should notify the dev of what we did with partial success instead of server logs.

Do you think this is a fair summary of the things we don't have consensus yet?

Again, sorry for pestering.

No worries :) I probably didn't communicate clearly enough! I probably should've posted a screenshot like the one in #88 earlier.

Do you think this is a fair summary of the things we don't have consensus yet?

Let's see: let's first verify that we understand each other unambiguously!

1. That leaves the disagreement on the matter discussed in internal_entity_keys and stuff.

   @gabesullice proposed this in #80, I +1'd in #81 but suggested a different name, @gabesullice was open to that in #82, and you brought the conversation back to another discussion point in #83. So unless I'm overlooking it, I don't see a disagreeing opinion from you about this yet?

2. Also I think we should notify the dev of what we did with partial success instead of server logs.

   I don't know what this is referring to. No comments have mentioned "log" before. My best guess is that this is referring to throw new \LogicException('The generated alias conflicts with an existing field. Please report this in the JSON API issue queue!');? Please confirm & clarify!

WRT #81 and #82, I am OK with whatever solution, whatever name (I'm specially open to names containing poop emoji 💩).

My only requirement is that the properties work as expected for sparse fieldsets and (specially) filters. I think that shuffling properties around in the normalizers will have undesired effects on filterability because the json path to the property no longer matches the Entity Query path. But again, as long as we can filter and exclude properties I'm 💯 🆗.

My best guess is that this is referring to throw new \LogicException('The generated alias conflicts with an existing field. Please report this in the JSON API issue queue!');? Please confirm & clarify!

Correct. Typically exceptions will be stored in the server logs, that's why I mentioned them. Sorry for the confusion.

(I'm specially open to names containing poop emoji 💩).

😂

Correct. Typically exceptions will be stored in the server logs, that's why I mentioned them. Sorry for the confusion.

That exception will only occur if both of these are true for a foo entity type + some bundle: A) there's an id or type field, B) there also is a foo_id or foo_type field. Because then the automatic aliasing of id/foo would attempt to alias it to foo_id/foo_type respectively, and that'd conflict with something pre-existing. I just tried to keep the logic super simple, to only address this if the need for this ever arises (I'm not sure that it will/does).

My only requirement is that the properties work as expected for sparse fieldsets and (specially) filters. I think that shuffling properties around in the normalizers will have undesired effects on filterability because the json path to the property no longer matches the Entity Query path. But again, as long as we can filter and exclude properties I'm 💯 🆗.

Agreed!

Before actually implementing this, I'd like @gabesullice's +1 too.

That exception will only occur if both of these are true for a foo entity type + some bundle: A) there's an id or type field, B) there also is a foo_id or foo_type field. Because then the automatic aliasing of id/foo would attempt to alias it to foo_id/foo_type respectively, and that'd conflict with something pre-existing.

Oh. In that case as a developer I'd like to be notified of any unanticipated aliasing my responses suffer. I won't hold this as a blocker, maybe we want to have a follow up and work / discuss this separately?

Oh. In that case as a developer I'd like to be notified of any unanticipated aliasing my responses suffer. I won't hold this as a blocker, maybe we want to have a follow up and work / discuss this separately?

This affects you only if you use id and type as field names. You'd think that if somebody develops an entity type specifically for use with JSON API, that they'd … just observe how it is exposed by JSON API while developing? So I don't think we need anything extra for that? Also, if they really want to violate the spec, then they can just override the alias back to the original using JSON API Extras.

Just saying this so that it doesn't get stalled by accident:

I'm ambivalent WRT to the remaining points. In my opinion, this can be committed as is. I won't be marking this RTBC though, because @e0ipso should be the one to do it.

I took inspiration from (I'm specially open to names containing poop emoji 💩). + KISS by not doing drupal_internals: { nid: 5, type: "article"}, which would require more complex data transformations.

So this patch makes nid: 5 get aliased to 💧nid: 5 — which means that the entity ID (the last point of contention/consensus building) remains there, but is clearly marked as Drupal-y thanks to that drop emoji prefix.

(That's also easy to grep for, so if others feel it should be more "professional", then that's easy to implement. I kinda like this though!)

This is a leftover that should've been removed in #66.

4 fails caused by 3 mistakes — now fixed.

Thanks @Wim Leers! Thanks for sticking with us while we took you on such a frustrating ride.

(That's also easy to grep for, so if others feel it should be more "professional", then that's easy to implement. I kinda like this though!)

I feel like a wet blanket, but I really think this should be drupal_internal__ or similar. While the drop emoji is fun, it's a poor DX.

Not everyone has a mac that makes it easy to type those characters out. When developing on Linux for example, I have to figure out what the emoji is called, google it, hope I find it, then copy/paste it or fight a battle figuring out how to get my keyboard to "type" the character.

Not to mention, it won't be at all clear to a non-Drupal developer that the drop is for Drupal.

Also, from the spec: U+0080 and above (non-ASCII Unicode characters; not recommended, not URL safe)

The emphasis is not mine! :P

Now get off of my lawn!

Now get off of my lawn!

😀

Also, from the spec: U+0080 and above (non-ASCII Unicode characters; not recommended, not URL safe)

🙈💩

Done. Took <1 minute.

:) Thanks.

Still going to let @e0ipso have the final say on this one to RTBC or commit.

The code looks good. I have 4 questions:

1. Can we create a follow up to JSON API Extras to use the new field mapping function so we don't duplicate efforts?
2. Can we create a follow up to OpenApi so the properties are properly documented?
3. Can we create a follow up to Schemata so the schema is accurate?
4. Can we create a follow up to JSON API so we notify the user that we changed their property names under the hood as discussed in #93? We can skip this for the drupal_internal__ but not for the others.
5. Can we create a change record explaining what changed here? (since this is a BC break).

1. Yep! See #55 and #69, I already expressed my enthusiasm for making JSON API Extras more robust 😀 Created #2986408: Use aliasing capabilities provided by JSON API 2.x for this. Patch created.
2. Nothing should have changed for this module; it respects Schemata's output, which should still be accurate, as long as it respects aliases and disabled fields, both of which it already needs to do anyway.
3. See above.
4. Isn't that what the CR is for?
5. That CR already exists! Created nearly two weeks ago: https://www.drupal.org/node/2984247. Updated it though.

Nothing should have changed for this module; it respects Schemata's output, which should still be accurate, as long as it respects aliases and disabled fields, both of which it already needs to do anyway.

Hmm. Did you check that the generated schema reflects the output? I'm pretty sure that if you move things in normalizers, then the schema fails. That's the main driver to move away from normalizers.

I'll check.

Wow, apparently Schemata and OpenAPI simply have never supported JSON API Extras? That's very surprising to me — given Contenta shipping with both, I thought this was fixed a very long time ago… I updated #2882269, see #2882269-4: Support for JSON API's ResourceType::getPublicName() and ResourceType::isFieldEnabled().

I don't think we should block this issue on Schemata. Especially since for example #2967572: Small inaccuracies for JSON API schemas also just landed earlier today. We can iterate there while JSON API 2.x moves forward. I posted an initial patch at #2882269-6: Support for JSON API's ResourceType::getPublicName() and ResourceType::isFieldEnabled() — this makes everything in this patch work, in both Schemata, OpenAPI and OpenAPI ReDoc :)

Thanks everyone!

🍻🍻🍻

Published https://www.drupal.org/node/2984247.