Spec Compliance: JSON API's schema disallows duplicate resource identifiers. EntityReferenceItems which reference the same entity must have an "arity"

I believe this is a bug, and a serious one. I suspect that the schema validation is complaining about adding duplicate item multiple times, which should not be allowed by the spec.

@Grimreaper can you share the entry in your logs to see the exact error this is throwing?

In JSON API, a relationship is a logical linkage between two different resources: "Resource 'A' is related to Resource 'B' by way of the 'author' relationship," for instance. From that perspective, it doesn't make sense to have Resource 'A' related to Resource 'B' twice; a relationship either exists, or doesn't exist between two resources.

I'm not sure this is a bug, per se. It seems more like a limitation of the standard as it is defined.

Modifying the schema.json file is not a good idea, as that comes from an upstream source: https://github.com/json-api/json-api/blob/gh-pages/schema

It might make sense to submit a PR for the JSON API standard itself, or that schema file at the above link and see what comes of that. Otherwise, the fix may be to disable validation on sites that need to have duplicates in a relationship by removing the validation library. It is meant to be a dev-only requirement, anyway.

@Grimreaper the response validation should only be running if:
1. You've composer required the JSON API module with the --dev option, to install development-only dependencies.
2. You have assertions enabled in PHP.

In other words, validation should only be running in a development environment, not in production, and only if you really want it to be running.

Thanks for your big help with this @Grimreaper and @hampercm.

This is blocked until we can get a response from the JSON API maintainers. Maybe it's worth adding a PR against the schema as well so it is clearer what the issue is about?

Removing the RC blocker.

I updated the PR: https://github.com/json-api/json-api/pull/1156#issuecomment-351113732

Just mentioning that nearly 5 months later, there still is no response from the JSON API spec maintainers: https://github.com/json-api/json-api/pull/1156.

Clarifying issue title, updating issue metadata. This is not a bug in the JSON API module, it's a limitation of the JSON API spec. And it currently only affects DX for "dev" sites, "prod" sites do not (should not!) have JSON API response schema validation enabled, and hence are unhampered by this, which makes this a DX-only issue. (Although perhaps certain JSON API clients do also make this assumption!)

Heh, I've never noticed this issue before. I'm sorry to throttle the the issue metadata, but after digging in, I think I've had some revelations which will give us a path forward.

This is not a bug in the JSON API module, it's a limitation of the JSON API spec.

I disagree. This is another case of us trying to "serialize the entity reference field" rather than represent relationships between resources. Resource identifiers are not meant to tell you about field values. They exist to tell an API consumer about linkage between resources.

Just mentioning that nearly 5 months later, there still is no response from the JSON API spec maintainers

FWIW, a contributor to the spec did respond. The (distilled) answer was that multiple distinct relationships should have differentiating data under the meta member of the resource identifier.

I agree with that assessment.

Two notes from our own documentation:

However, when "Drupalisms" cannot be reconciled with the specification, the module will always choose the implementation most faithful to the specification.

However, inconsistencies with the JSON API specification will be considered bugs.

Therefore, marking this as a bug and updating the title/issue summary (again). Also getting rid of the DX tag, because this really is a spec issue (albeit a very subtle one). Keeping the "minor" priority because it only significantly impacts dev sites, as @Wim Leers pointed out.

Thanks for challenging my assessments. ❤️ That's what helps build better software!

Adding the issue comment you linked to as a related issue.

Your argument makes sense. I'm fine with the approach you propose (i.e. do what a JSON API spec contributor said). I think that makes this a 2.x issue, because it could be considered a BC break for some.

More importantly, I think this actually points to a weakness or at least ambiguity in Drupal's data modeling in general, and entity reference fields in particular. Is this really something Drupal is explicitly designed to do? I searched, and found the following relevant issues:

• (core) #2729953: Clarify handling of multiple references to the same file from entities
• (entityreference_view_widget) #2301131: Allow the same entity to be referenced twice in the same field — this lists an explicit use case
• (entityreference) #1572938: Can Multi Value Entity reference filed be unique?
• … and finally, it seems like this has been a capability of Entity Reference fields since inception, because https://www.drupal.org/project/entityreference_unique exists specifically to achieve the behavior that the JSON API spec mandates.
• n.b.: \Drupal\Core\Entity\Plugin\Validation\Constraint\ValidReferenceConstraint does not have a message about "duplicate reference", confirming that this is not something that core's entity reference field type cares about.

Note: #1801304: Add Entity reference field added the contrib entityreference module to core, in February 2013.

Conclusion: we'll need to expose this through meta, because we will definitely not be able to make core prevent duplicate references without breaking lots of sites.

I think that makes this a 2.x issue, because it could be considered a BC break for some.

Can you elaborate on that? I don't see it.

This would introduce a meta key in a location that today has no meta key. I suspect quite a few applications will have been written without taking this into account.

Of course, per our documentation, we don't consider this a BC break. So you're right.

Adding unit test coverage.

Great, that failed!

And here's the fix, which adds [meta][arity].

Almost forgot: we can now revert the JSON API schema change being made by earlier iterations of this patch!

@Wim Leers, this is great work, but I think it went in a slightly different direction than I was thinking it would. BUT, you were following the suggestion exactly, I just misread it to begin with. I'm hopeful that this can work, but I'm concerned about one thing.

I had thought we would end up with:

data: [
  {type: foo, id: 1, meta: {arity: 0}},
  {type: foo, id: 1, meta: {arity: 1}},
]

^{I think "arity" is the wrong word for this example. I should have realized that before.}

IOW, that we would represent every relationship, not collapse them.

With this approach, is it possible that multiple entity reference items with a shared target_id but different values for their extra properties will lose data?

The use case that I'm thinking of is an image field referencing the same image twice, but with different alt and title text.

The use case that I'm thinking of is an image field referencing the same image twice, but with different alt and title text.

That's a very good point @gabesullice.

multiple entity reference items with a shared target_id but different values for their extra properties will lose data?

Woah, that's even MORE edge-casey! I'd never even thought of that.

The use case that I'm thinking of is an image field referencing the same image twice, but with different alt and title text.

This honestly seems fairly contrived.

Regardless of what I or we think, here's the thing:

data: [
  {type: foo, id: 1, meta: {arity: 0}},
  {type: foo, id: 1, meta: {arity: 1}},
]

… this still violates the JSON API spec/the uniqueItems constraint I just restored in #29… 😢

Here's the other thing:

same image twice, but with different alt and title text.

As long as the pairs of metadata remain intact, we can retain what I'm doing in #28/#29 and hence comply with the JSON API spec:

          { "type": "foobar", "id": "123", "meta": { "arity": 5,  } },

Let me try that.

Expanded test coverage. This should fail.

This is what I set out to achieve, for a very complex example:

         'data' => [
            [
              'type' => 'file--file',
              'id' => $img_id,
              'meta' => [
                'arity' => 3,
                'metas' => [
                  ['alt' => 'Cute llama', 'title' => 'My spirit animal'],
                  ['alt' => 'Adorable llama', 'title' => 'My spirit animal 😍'],
                ],
              ],
            ],
            ['type' => 'user', 'id' => 1, 'meta' => ['arity' => 2]],
            ['type' => 'user', 'id' => 2],
          ],

And logic.

Back to both of ya. 🏓

[Multiple resource identifiers with unique metadata] still violates the JSON API spec/the uniqueItems constraint

I'm not yet convinced this is true. Looking at justinrainbow/json-schema, it seems like uniqueItems is doing a deep equality test.

Looking at the spec itself, there is no preclusion against multiple resource identifiers under a relationship field. In fact, the spec says elsewhere WRT to "full linkage" that "Compound documents require “full linkage”, meaning that every included resource MUST be identified by at least one resource identifier object in the same document." (emphasis mine).

Are we really sure that the schema is invalid when the type and ID are duplicated while the meta information contains unique data? If the answer to this is "yes", #34 is seems like an elegant compromise.

I'm not yet convinced this is true.

But … isn't that exactly what was reported originally in this issue? Isn't that exactly what we've been working towards? It definitely contradicts @hampercm's excellent explanation in #9 and I think it also contradicts your own comment at #20?

The bits you're quoting about full linkage are specifically for compound documents. This is about resource linkage.

Are we really sure that the schema is invalid when the type and ID are duplicated while the meta information contains unique data?

No, but how can we be? The spec is not specific enough about this. The closest information we have is this bit under http://jsonapi.org/format/#document-resource-object-linkage:

Note: The spec does not impart meaning to order of resource identifier objects in linkage arrays of to-many relationships, although implementations may do that. Arrays of resource identifier objects may represent ordered or unordered relationships, and both types can be mixed in one response object.

I think your proposal in #30 is literally contradicting your argument in #20, where you wrote:

This is another case of us trying to "serialize the entity reference field" rather than represent relationships between resources. Resource identifiers are not meant to tell you about field values. They exist to tell an API consumer about linkage between resources.

#30 says arity is the wrong word, which likely means the preferable word is delta. If that's a correct interpretation, then that would mean we'd be literally "serializing the entity reference field".

Really curious to see your reply :D

P.S.: there's a lot of "you" in what I wrote, but this is of course not personal/not about attacking you! It's merely disagreeing with the proposal :) Sorry if that sounded harsh! It's late and I'm tired :)

But … isn't that exactly what was reported originally in this issue? Isn't that exactly what we've been working towards? It definitely contradicts @hampercm's excellent explanation in #9 and I think it also contradicts your own comment at #20?

In #20 I said: "The (distilled) answer was that multiple distinct relationships should have differentiating data under the meta member of the resource identifier."

That's consistent with not believing that "[Multiple resource identifiers with unique metadata] still violates the JSON API spec/the uniqueItems constraint." Because it's the "differentiating data under the meta member" that makes the resource identifier unique. My idea was that adding an "arity" to each duplicated resource identifier would provide that "differentiating data". I hope my examples below will make that more clear.

Later, I said: "I'm thinking of is an image field referencing the same image twice, but with different alt and title text." You thought was MORE edge-casey.

The point I was trying to make is that it's plausible to have duplicate type + IDs representing different relationships. Perhaps my example was a bit contrived, but I do think it's plausible. Think of a blog post that's getting written and prepared for publishing, the author might know that they want to add a few images, but wants to find them later or have illustrations made. The author just references a standard placeholder kitten image with the alt and title text for the image they want to be there. Later, the author may plan to get the rights to some stock photography or have a designer make some illustrations. That could easily happen in a real Drupal site (I've done it this way :P).

Take another example that may illustrate the point:he case I'm trying to make clear is that it is possible to have different relationships between the same two entities that changes over time. Like this:

GET /jsonapi/user/user/gabesullice/relationships/connections
data: [
  {type: "user--user", id: wimleers, meta: {type: "colleague"}},
]

POST /jsonapi/user/user/gabesullice/relationships/connections
data: [
  {type: "user--user", id: wimleers, meta: {type: "friend"}},
]

GET /jsonapi/user/user/gabesullice/relationships/connections
data: [
  {type: "user--user", id: wimleers, meta: {type: "friend"}},
  {type: "user--user", id: wimleers, meta: {type: "colleague"}},
]

It's important that they are not collapsed for that reason. Otherwise, we'll have to really bend over backwards to accommodate POST/PATCH to relationships.

The bits you're quoting about full linkage are specifically for compound documents. This is about resource linkage.

These are two sides of the same coin.

Are we really sure that the schema is invalid when the type and ID are duplicated while the meta information contains unique data?

No, but how can we be? The spec is not specific enough about this.

Let's be clear, there are TWO pieces to this argument. Violating the specification and passing schema validation.

The original issue is about the latter, passing validation. We aren't technically violating the spec here. The json.schema has a uniqueItems constraint that is derived from the spirit of the spec.

The constraint that we're violating is simply that the resource identifier objects must be unique.

When I asked "Are we really sure that the schema is invalid when the type and ID are duplicated while the meta information contains unique data?" I meant "does an example like the one above with "friend" and "colleague" pass or fail schema validation?"

I'm pretty sure that the examples above would pass (because of the deep equality check I referenced). While the example below without differentiating metadata would fail...

GET /jsonapi/user/user/gabesullice/relationships/connections
data: [
  {type: "user--user", id: wimleers},
  {type: "user--user", id: wimleers},
]

#30 says arity is the wrong word, which likely means the preferable word is delta. If that's a correct interpretation, then that would mean we'd be literally "serializing the entity reference field

The point of adding some unique data under the meta key was simply to pass validation on the edge case where there is not already unique information under the meta property.

I intentionally chose arity to avoid a naming collision with Drupal's use of delta. In my head, I was thinking that the result would look something like this:

data: [{                 >>>   data: [{
    type: "user--user",  >>>       type: "user--user",
    id: jane             >>>       id: jane,
  }, {                   >>>       meta: {
    type: "user--user",  >>>         arity: 0
    id: jane             >>>       }
  }, {                   >>>     }, {
    type: "user--user",  >>>       type: "user--user",
    id: wimleers,        >>>       id: jane,
    meta: {              >>>       meta: {
      type: "friend"     >>>         arity: 1
    }                    >>>       }
  }, {                   >>>     }, {
    type: "user--user",  >>>       type: "user--user",
    id: wimleers,        >>>       id: wimleers,
    meta: {              >>>       meta: {
      type: "colleague"  >>>         type: "friend",
    }                    >>>         arity: 0
}]                       >>>       }
                         >>>     }, {
                         >>>       type: "user--user",
                         >>>       id: wimleers,
                         >>>       meta: {
                         >>>         type: "colleague",
                         >>>         arity: 1
                         >>>       }
                         >>>   }]

Notice that "arity" is not the same as the Drupal's delta. The arity doesn't have to do with field item delta because 0 and 1 both appear twice.

It's a simple trick to make the first two items pass schema validation while only adding a very little bit of irrelevant data to the second two items that don't actually need it.

I prefer this trick to "collapsing" the items because it avoids adding a new structural convention under the meta property à la partial-success. IOW, a client that needs to know that there is both a "friend" and "colleague" connection would not have to write special code just for our implementation of JSON API to "uncollapse" the meta.metas property it would just work as expected.

That could easily happen in a real Drupal site (I've done it this way :P).

😂 👏

I'm trying to make clear is that it is possible to have different relationships between the same two entities that changes over time.

What an EXCELLENT example! 👌 I wish I'd thought of that! That's indeed far less contrived, more realistic!

It's important that they are not collapsed for that reason. Otherwise, we'll have to really bend over backwards to accommodate POST/PATCH to relationships.

And this is an EXCELLENT remark too! That would indeed be an important consequence!

Let's be clear, there are TWO pieces to this argument. Violating the specification and passing schema validation. […] The original issue is about the latter, passing validation.

Thanks for disambiguating this! That's indeed very important to keep in mind while discussing this :)

The point of adding some unique data under the meta key was simply to pass validation on the edge case where there is not already unique information under the meta property.

Right, I realize that now. But I did not realize this before!

I intentionally chose arity to avoid a naming collision with Drupal's use of delta.

Right. And I interpreted arity as count. Which is wrong, it's the position or order. Consequence of a non-native speaker interpreting terms he hasn't heard before based on the context, and reaching the wrong conclusion. Sorry about that! 😊 At least, if I'm understanding https://en.wikipedia.org/wiki/Arity correctly.
Also: you didn't choose arity, @beauby did at https://github.com/json-api/json-api/pull/1156#issuecomment-325377995, so don't apologize!

I intentionally chose arity to avoid a naming collision with Drupal's use of delta.

+1

I prefer this trick to "collapsing" the items because it avoids adding a new structural convention under the meta property à la partial-success.

+1

IOW, a client that needs to know that there is both a "friend" and "colleague" connection would not have to write special code just for our implementation of JSON API to "uncollapse" the meta.metas property it would just work as expected.

+1

I was thinking that the result would look something like this:

Works for me!

First, modifying the test coverage to implement #38. (@gabesullice, please review this carefully to ensure this is indeed what you intended!)

The good news is that thanks to the previous test coverage iterations, I only have to modify the existing test coverage to test AFAICT all edge cases :)

And logic changes. This should be green!

Yay for shared understanding! This looks really good and the test coverage is awesome. I didn't think about not assigning arity to single occurrences.

1. +++ b/src/Normalizer/Value/RelationshipNormalizerValue.php
   @@ -74,9 +74,75 @@ class RelationshipNormalizerValue extends FieldNormalizerValue {
   +   * Ensures unique resource linkage.
   

   Let's be really verbose in the comments.

   Ensures each resource identifier object is unique.

   The official JSON API JSON-Schema document requires that no two resource identifier objects are duplicated.

   This adds an `arity` member to each object's `meta` member. The value of this member is an integer that is incremented by 1 (starting from 0) for each repeated resource identifier sharing a common `type` and `id`.

2. +++ b/src/Normalizer/Value/RelationshipNormalizerValue.php
   @@ -74,9 +74,75 @@ class RelationshipNormalizerValue extends FieldNormalizerValue {
   +   *   A list of JSON API resource identifier objects.
   +   * @return array
   

   Nit: Missing newline.

3. +++ b/src/Normalizer/Value/RelationshipNormalizerValue.php
   @@ -74,9 +74,75 @@ class RelationshipNormalizerValue extends FieldNormalizerValue {
   +   * @see https://github.com/json-api/json-api/pull/1156#issuecomment-325377995
   

   Let's also link to this issue.

4. +++ b/src/Normalizer/Value/RelationshipNormalizerValue.php
   @@ -74,9 +74,75 @@ class RelationshipNormalizerValue extends FieldNormalizerValue {
   +  protected static function ensureUniqueResourceLinkage(array $resource_identifier_objects) {
   

   s/Linkage/IdentifierObjects/

5. +++ b/src/Normalizer/Value/RelationshipNormalizerValue.php
   @@ -74,9 +74,75 @@ class RelationshipNormalizerValue extends FieldNormalizerValue {
   +    $resource_identifier_arity = static::computeResourceLinkageArity($resource_identifier_objects);
   ...
   +    foreach ($resource_identifier_objects as $index => $resource_identifier_object) {
   

   I think this can be simplified. What about something like:

   $arities = [];
   
   // Count each repeated resource identifier and track their array indices.
   foreach ($rios as $index => $rio) {
     $composite_key = $rio['type'] . ':' . $rio['id'];
   
     $arities[$composite_key]['arity'] = isset($arities[$composite_key])
       ? $arities[$composite_key]['arity'] + 1
       : 0;
   
     // The index will later be used to assign an arity to repeated resource identifier
     // objects. Doing this in two phases prevents adding an arity to objects which
     // only occur once.
     $arities[$composite_key]['indices'][] = $index;
   }
   
   // Assign an arity to objects whose type + ID pair occurred more than once.
   foreach ($arities as $computed) {
     if ($computed['arity'] > 0) {
       foreach ($computed['indices'] as $arity => $index) {
         $rios[$index]['meta']['arity'] = $arity;
       }
     }
   }
   

6. +++ b/src/Normalizer/Value/RelationshipNormalizerValue.php
   @@ -74,9 +74,75 @@ class RelationshipNormalizerValue extends FieldNormalizerValue {
   +      if (!isset($carry[$id])) {
   

   Alluded to this above, but even though we know these are UUIDs, let's key this by type + ID. Since the spec reads:

   A compound document MUST NOT include more than one resource object for each type and id pair.

   Note: In a single document, you can think of the type and id as a composite key that uniquely references resource objects in another part of the document.

1. ✔️ Agreed!
2. ✔️
3. ✔️
4. ✔️
5. ✔️ — I first wasn't sure about this change, but given that the helper method to compute the arity can then go away, +1, took your approach instead :) In chatting with Gabe, we chose to make one working change.
6. ✔️ Good call!

Self-review:

1. +++ b/tests/src/Unit/Normalizer/Value/RelationshipNormalizerValueTest.php
   @@ -86,12 +86,26 @@ class RelationshipNormalizerValueTest extends UnitTestCase {
   +    $img1->getCacheTags()->willReturn(['ccimg1']);
   ...
   +    $img2->getCacheTags()->willReturn(['ccimg2']);
   

   Should be ctimg1 and ctimg2. Fixed.

2. Also: we should fix the remaining CS violations, all in the test coverage. Keeping that for the next patch iteration to keep the patch understandable.

And fixing remaining CS violations.

🎉

One more edge case that works as expected!

Moving to Drupal core's issue queue.

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

I believe the fix is not ported to core, as I'm experiencing this issue on local dev (with composer require --dev drupal/core-dev), as some comments said, PRD sites don't have dev package can run ok.

Can we reopen this issue?