Problem/Motivation

#2864680: Spec Compliance: JSON API's schema disallows duplicate resource identifiers. EntityReferenceItems which reference the same entity must have an "arity" added an arity key to relationships returned by jsonapi to avoid an error when a reference field contains the same entity multiple times.

However, we have a similar problem when returning data about a specific field.
The previous fix only applies to ResourceIdentifier, but when querying a field, jsonapi returns normalized ResourceObject and those don't have an arity key.

Steps to reproduce

  1. Create an entity with a reference field and add the same entity multiple times in this reference field.
  2. Browse to /jsonapi/[entity_type]/[bundle]/[uuid]/[field_name] (real life example: /jsonapi/paragraph/carrousel/2f03a78f-1d0a-43e5-a6ee-6fb0fd8a90f7/field_images).
  3. You get this error:

    A JSON:API response failed validation (see the logs for details). Please report this in the issue queue on drupal.org

(However /jsonapi/[entity_type]/[bundle]/[uuid] does not have the same error because there, the relationship has arity keys.)

Proposed resolution

jsonapi should add an arity key on duplicated ResourceObject objects.

The arity logic from ResourceIdentifier should probably be refactorized to be used by other classes.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#39 3275497-nr-bot_0n5sowp9.txt1.41 KBneeds-review-queue-bot
#37 3275497-nr-bot_95oh4o0a.txt1.41 KBneeds-review-queue-bot
#25 3275497-nr-bot_wgw06b8_.txt90 bytesneeds-review-queue-bot
#23 3275497-nr-bot_q1r1qp6r.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3275497

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

prudloff created an issue. See original summary.

prudloff’s picture

prudloff commented

I pushed a commit that shows how to fix this in a very basic way.
However, a cleaner fix will probably require refactorizing parts of the code to reuse the existing ResourceIdentifier logic.

prudloff opened merge request !2122

bbrala’s picture

bbrala
Location Netherlands
commented

Hmm, that sounds like an oversight and I agree that should be identified as a bug.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

prudloff’s picture

prudloff commented

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

gaëlg made their first commit to this issue’s fork.

gaëlg changed the visibility of the branch 3275497-add-arity-key to hidden.

gaëlg opened merge request !9618

prudloff’s picture

prudloff commented
Issue summary: View changes
Status: Active » Needs work

I need to fix related tests.

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

The MR adds the arity key to every resource even if not necessary (for example on responses containing a single resource).
It keeps the code simple and more predictable, but it means a lot of tests had to be updated.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left comments/questions in MR.

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

I answered the questions.
If this needs fixing, we should probably fix it in JsonApiResource as well?

smustgrave’s picture

smustgrave commented

All the tests having to be updated to pass makes me think we are introducing a breaking change

bbrala’s picture

bbrala
Location Netherlands
commented

Kinda depends, adding a meta to add responses does mean we are doing a change. Is it breaking? If your client support jsonapi it shouldn't be, its part of the spec.

So i lean towards not breaking.

smustgrave’s picture

smustgrave commented

Can we add an example where it’s not passed to make sure both ways are covered?

bbrala’s picture

bbrala
Location Netherlands
commented

It's late. Is this also for creating entities? If so, then it should also work without, otherwise it is indeed a BC break since you might not do that :x

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Not sure correct status but just thinking if we can add an example mentioned in #18

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

The MR only adds the arity key to read responses, it does not change the requirements for creating/editing entities.
I did not change the tests for these operations so if they pass it probably means it still works without an arity key.
(However we could have a followup to allow specifying arity when editing a reference field with duplicates, in order to specify which of the references we want to change.)

I'm not sure it is a breaking change. If your client does not use the arity key, it will continue to work as before.
The main change is that now a response can contain the same entity multiple times.

I agree that changing all these tests does not feel optimal. An alternative solution would be to only add the arity key when the response contains the same entity multiple times but it might make the code more complex and less predictable for a small benefit.

smustgrave’s picture

smustgrave commented

@bbrala that work for you?

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3275497-nr-bot_q1r1qp6r.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

I rebased the MR.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3275497-nr-bot_wgw06b8_.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

I rebased the MR again.

bbrala credited casey.

bbrala’s picture

bbrala
Location Netherlands
commented

Sorry for the late reponse. I think that would be ok to tranparantly do this. Don't see how that would create issues. Clients will be OK, only see possible issues in tests which would do a full compary of the output. Doesn't seem like a problem.

I discussed the fact we are adding 0 to existing also with casey. We do think this will not result in issues since its in the meta. Also looking at how the count is done, that also seems fine.

What I'm kinda wondering, we should probably also have at least some sort of test for the meta data. Seems we can so that easily here? Perhaps even patching with artity also (although maybe not for every entity type) to see if that works?

file: core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php

    // 6. Single relationship item.
    $single_output = Json::decode($this->drupalGet('/jsonapi/node/article/' . $uuid . '/relationships/node_type'));
    $this->assertSession()->statusCodeEquals(200);
    $this->assertArrayHasKey('type', $single_output['data']);
    $this->assertArrayNotHasKey('attributes', $single_output['data']);
    $this->assertArrayHasKey('related', $single_output['links']);
    // 7. Single relationship image.
    $single_output = Json::decode($this->drupalGet('/jsonapi/node/article/' . $uuid . '/relationships/field_image'));
    $this->assertSession()->statusCodeEquals(200);
    $this->assertArrayHasKey('type', $single_output['data']);
    $this->assertArrayNotHasKey('attributes', $single_output['data']);
    $this->assertArrayHasKey('related', $single_output['links']);
    // 8. Multiple relationship item.
    $single_output = Json::decode($this->drupalGet('/jsonapi/node/article/' . $uuid . '/relationships/field_tags'));
    $this->assertSession()->statusCodeEquals(200);
    $this->assertArrayHasKey('type', $single_output['data'][0]);
    $this->assertArrayNotHasKey('attributes', $single_output['data'][0]);
    $this->assertArrayHasKey('related', $single_output['links']);
    // 8b. Single related item, empty.

Added casey because we sat down together to disect the possible implications.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Needs work
Issue tags: +Needs tests
prudloff’s picture

prudloff commented
Status: Needs work » Needs review
Issue tags: -Needs tests

@bbrala The MR already has some tests that check arity keys in JsonApiFunctionalTest:
https://git.drupalcode.org/project/drupal/-/blob/868a9723aea3a084a2f8714...
https://git.drupalcode.org/project/drupal/-/blob/868a9723aea3a084a2f8714...

(The code you are pointing to is about /jsonapi/node/article/[uuid]/relationships/[field_name] but this MR is about/jsonapi/node/article/[uuid]/[field_name] (relationships already have arity keys that are added only when the relationship contains the same entity twice).

I added some arity keys to write tests (POST and PATCH) to make sure it does not break anything to send this key.

bbrala’s picture

bbrala
Location Netherlands
commented
Issue tags: +Needs change record

Think this is all good. I considered if we need to make sure we have tests with and without, but don't think that is an issue. Also considered if we should actually validate the arity key to be >0 and int in withArity, but i think that is a little overkill.

Fine to get artity in meta this way to release that info. We do need a change records that tells developers we are adding this. After that RTBC is fine.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Needs work

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

prudloff closed merge request !2122

prudloff’s picture

prudloff commented
Status: Needs work » Needs review
Issue tags: -Needs change record

I see bbrala added a CR.

bbrala’s picture

bbrala
Location Netherlands
commented

I was waiting for the tests and forget oops, thanks!

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3275497-nr-bot_95oh4o0a.txt1.41 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs work » Needs review
Issue tags: +no-needs-review-bot

LOcally this phpstan error is not present. Also looking at the arguments, i see there might be confusion on the interface vs concrete classes, but they do end up at the same end results. Gonna ignore the bot for now.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3275497-nr-bot_0n5sowp9.txt1.41 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.