Support `include` parameter on relationship routes (turns out it accidentally works on cold caches: fix + test this)

Hi @zhangyb! Thanks for the concise report. You even referenced the spec 👍👍👍(w/ an anchor link no less!)

It's funny that you posted this issue today. I was thinking about some JSON API stuff last night and realized that this might be happening (completely different code paths for related and collection resources). You've just confirmed my hunch for me, so thank you!

It's worth noting two things:

An endpoint MAY also support an include request parameter to allow the client to customize which related resources should be returned.

If an endpoint does not support the include parameter, it MUST respond with 400 Bad Request to any requests that include it.

1. We're not required to support includes on related endpoints because of the MAY (but I think we should).
2. Because we're not supporting this, the "bug" is that we're not returning a 400 which is required.

Personally, I feel we should just add support for it, but it may be a short-term solution to just return a 400.

@Wim Leers, do you have any thoughts?

Thanks for the clarification. I agree that this would be a nice feature to support.

+1 to supporting this in the 8.x-2.x branch.

I don't know if it was a mistake on my part or if we didn't previously support included on related resources, but I've confirmed that we do support them on related routes currently.

This is only about relationship routes then.

👏

This gets us halfway there. For http://d8/jsonapi/node/article/90e5de48-341f-486c-964b-b58ccca22365/relationships/uid?include=roles, this results in:

{
  "data": {
    "type": "user--user",
    "id": "fdd88f09-873c-4a05-aac1-a784804e9dba"
  },
  "jsonapi": {
    "version": "1.0",
    "meta": {
      "links": {
        "self": {
          "href": "http://jsonapi.org/format/1.0/"
        }
      }
    }
  },
  "links": {
    "self": {
      "href": "http://d8/jsonapi/node/article/90e5de48-341f-486c-964b-b58ccca22365/relationships/uid"
    },
    "related": {
      "href": "http://d8/jsonapi/node/article/90e5de48-341f-486c-964b-b58ccca22365/uid"
    }
  },
  "included": [
    {
      "type": "user--user",
      "id": "fdd88f09-873c-4a05-aac1-a784804e9dba",
      "attributes": {
        "drupal_internal__uid": 1,
        "langcode": "en",
        "preferred_langcode": "en",
        "preferred_admin_langcode": null,
        "name": "root",
        "mail": "a@a.com",
        "timezone": "UTC",
        "status": true,
        "created": "2018-08-07T13:37:40+00:00",
        "changed": "2018-08-07T13:38:34+00:00",
        "access": "2018-10-09T13:21:42+00:00",
        "login": "2018-08-07T13:38:34+00:00",
        "init": "a@a.com",
        "default_langcode": true,
        "path": null
      },
      "relationships": {
        "roles": {
          "data": [
            {
              "type": "user_role--user_role",
              "id": "feffe9f0-0174-402d-89d9-b47cd4d59304"
            }
          ],
          "links": {
            "self": {
              "href": "http://d8/jsonapi/user/user/fdd88f09-873c-4a05-aac1-a784804e9dba/relationships/roles"
            },
            "related": {
              "href": "http://d8/jsonapi/user/user/fdd88f09-873c-4a05-aac1-a784804e9dba/roles"
            }
          }
        },
        "user_picture": {
          "data": null,
          "links": {
            "self": {
              "href": "http://d8/jsonapi/user/user/fdd88f09-873c-4a05-aac1-a784804e9dba/relationships/user_picture"
            },
            "related": {
              "href": "http://d8/jsonapi/user/user/fdd88f09-873c-4a05-aac1-a784804e9dba/user_picture"
            }
          }
        }
      },
      "links": {
        "self": {
          "href": "http://d8/jsonapi/user/user/fdd88f09-873c-4a05-aac1-a784804e9dba"
        }
      }
    },
    {
      "type": "user_role--user_role",
      "id": "feffe9f0-0174-402d-89d9-b47cd4d59304",
      "attributes": {
        "langcode": "en",
        "status": true,
        "dependencies": [],
        "drupal_internal__id": "administrator",
        "label": "Administrator",
        "weight": 2,
        "is_admin": true,
        "permissions": []
      },
      "links": {
        "self": {
          "href": "http://d8/jsonapi/user_role/user_role/feffe9f0-0174-402d-89d9-b47cd4d59304"
        }
      }
    }
  ]
}

i.e. it still includes the values of the relationship too; those should be omitted.

i.e. it still includes the values of the relationship too; those should be omitted.

Fixed.

Turns out that instead of all these changes, I can just generalize some of the logic in \Drupal\jsonapi\Controller\EntityResource::getRelated().

RTBC if tests pass.

Tests in #8, #10 and (probably) #12 are failing on

    Json::decode($this->drupalGet('/jsonapi/node/article/' . $uuid . '/relationships/uid', [
      'query' => ['include' => 'uid'],
    ]));

which returns 200 in HEAD, but 400 now. I think the test is wrong. If you're already looking at the resource identifiers for uid how does it make sense to also include them?

If you're already looking at the resource identifiers for uid how does it make sense to also include them?

This is not how includes work.

At the URL you gave, you're viewing resource identifier objects. By including them, you're saying "also give me the resource objects which are identified". RIOs !== ROs.

The spec has this example:

GET /articles/1/relationships/comments?include=comments.author HTTP/1.1
Accept: application/vnd.api+json

Notice that both the path and the include have the comments field name in them.

FWIW, I also found this confusing, but it is the proper behavior.

FWIW, I also found this confusing, but it is the proper behavior.

Alright. It definitely is confusing. My next step was to consult the spec, but I'm glad that you already experienced the same confusion in the past and are hence able to bring clarity based on past experience :)

So then that means I'm breaking existing spec-compliant behavior in the above patches.

As of #3026030: [regression] Includes are no longer respected when POSTing/PATCHing, includes are again supported on every operation. That issue brought it back to createIndividual() and patchIndividual(). It already was supported in getIndividual(), getCollection(), getRelated(), getRelationship(), addToRelationshipData()/replaceRelationshipData()/removeFromRelationshipData() (which all call getRelationship()). That's everything.

The only bug here is that if you first request /jsonapi/node/article/a2c1bfb4-b715-46b3-b196-e56b79f14119/relationships/uid followed by /jsonapi/node/article/a2c1bfb4-b715-46b3-b196-e56b79f14119/relationships/uid?include=uid, then because the url.query_args:include cache context is absent, the original cached response will be served and hence it appears to not work. But this is simply incorrect cacheability metadata!

Even more complex cases like /jsonapi/node/article/a2c1bfb4-b715-46b3-b196-e56b79f14119/relationships/field_tags?include=field_tags.vid work fine.

Attached patch is purely expanding the existing test coverage and proves this is just a bit of missing cacheability. It will fail due to this.

And this fixes it.

DrupalCI is apparently having problems… let's give it some time to recover. 😴

TODO: remove this:

// @todo This should be applied in relationship collections in https://www.drupal.org/project/jsonapi/issues/2965056.
…

Address #25.

#29:

+++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -257,12 +257,8 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
-    // @todo This should be applied in relationship collections in https://www.drupal.org/project/jsonapi/issues/2965056.
     // Make sure that different sparse fieldsets are cached differently.
-    $cache_contexts = array_map(function ($query_parameter_name) {
-      return sprintf('url.query_args:%s', $query_parameter_name);
-    }, ['fields', 'include']);
-    return $normalized->withCacheableDependency((new CacheableMetadata())->addCacheContexts($cache_contexts));
+    return $normalized->withCacheableDependency((new CacheableMetadata())->addCacheContexts(['url.query_args:fields']));

I think that this @todo really was trying to say that we should get rid of both url.query_args:fields and url.query_args:include. Right now it's only removing the latter.

#31:

+++ b/tests/src/Functional/EntryPointTest.php
@@ -35,7 +35,6 @@ class EntryPointTest extends BrowserTestBase {
       // @todo: remove the `url.query_args` cache contexts in https://www.drupal.org/project/jsonapi/issues/2992673.
       'url.query_args:fields',
-      'url.query_args:include',

This is removing one of these thanks to the fix above, but in this case there is no doubt: we want both of these url.query_args:* cache contexts to disappear. This is fixing one but not the other. Fixing my remark about #29 would also fix this properly.

So let's do that too.

For #32.

Well, actually, since this never even was a feature, it can't be broken and thus it can't be a bug, can it?

Rebased #31 (it no longer applied without conflict).

Worked to finish this because it removes two @todos under our control, and per #3028970: Audit all @todos, we need to keep only relevant @todos for JSON:API to go into core.

#3028970: Audit all @todos landed!

Let's land this next.

More accurate title.

Fix CS violation.

Ahhh, just one more fail…

1. +++ b/src/Controller/EntityResource.php
   @@ -855,7 +855,15 @@ class EntityResource {
   +    $response = new ResourceResponse(new JsonApiDocumentTopLevel($data, $includes, $links, $meta), $response_code, $headers);
   +    $cacheability = (new CacheableMetadata())->addCacheContexts([
   +      // Make sure that different sparse fieldsets are cached differently.
   +      'url.query_args:fields',
   +      // Make sure that different sets of includes are cached differently.
   +      'url.query_args:include',
   +    ]);
   +    $response->addCacheableDependency($cacheability);
   
   +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -258,13 +258,7 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   -    $cache_contexts = array_map(function ($query_parameter_name) {
   -      return sprintf('url.query_args:%s', $query_parameter_name);
   -    }, ['fields', 'include']);
   -    return $normalized->withCacheableDependency((new CacheableMetadata())->addCacheContexts($cache_contexts));
   

   ❤️

2. +++ b/src/Controller/EntityResource.php
   @@ -855,7 +855,15 @@ class EntityResource {
   +      'url.query_args:fields',
   

   Uber-technicality: we could not add the fields context to relationship requests that don't have an include query parameter.

   However, I think that for simplicity's sake, we don't need to worry about that.

The last failure is from a bad test expectation. RTBC when tests pass :)

However, I think that for simplicity's sake, we don't need to worry about that

Yep, exactly. That's a far-future optimization if the need ever arises.

Thanks for the review! 🙏

This fixes the one fail we still have, but I think it might break something else.

🎉 🚢