Comprehensive JSON API integration test coverage phase 6: POST/PATCH/DELETE of relationships

#2953321: Comprehensive JSON API integration test coverage phase 5: nested includes and sparse field sets just landed, which means this is the only remaining bit! 🎉

1. +++ b/tests/modules/jsonapi_test_field_access/jsonapi_test_field_access.module
   @@ -7,15 +7,20 @@
   +    return (($account->hasPermission($permission))
   

   Übernit: that's a lot of parentheses!

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -247,7 +248,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -        'target_bundles' => [$account_bundle => $account_bundle],
   +        'target_bundles' => NULL,
   

   Hm, I suspect the code in HEAD is incorrect, and you needed to change it? (But without consequences in HEAD, which is why HEAD's passing tests just fine?)

3. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -729,10 +730,18 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -    $this->assertSame(array_keys($expected_document), array_keys($actual_document), 'The documents must share the same top-level members');
   +    $expected_keys = array_keys($expected_document);
   +    $actual_keys = array_keys($actual_document);
   +    $missing_member_names = array_diff($expected_keys, $actual_keys);
   +    $extra_member_names = array_diff($actual_keys, $expected_keys);
   +    if (!empty($missing_member_names) || !empty($extra_member_names)) {
   +      $message_format = "The document members did not match the expected values. Missing: [ %s ]. Unexpected: [ %s ]";
   +      $message = sprintf($message_format, implode(', ', $missing_member_names), implode(', ', $extra_member_names));
   +      $this->assertSame($expected_document, $actual_document, $message);
   +    }
   

   +1 — this improves testing/debugging DX. We have something very similar for cache tags/contexts headers test assertions :)

4. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1228,7 +1237,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -   * Tests GETing relationships of an individual resource.
   +   * Tests CRUD of individual resource relationship data.
   
   @@ -1237,13 +1246,32 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -  public function testGetRelationships() {
   +  public function testRelationships() {
   

   Why?

   Why not create testPostRelationships() etc?

5. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1322,6 +1352,50 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +    // Test POST: empty body.
   +    // Test POST: empty data.
   +    // Test POST: data as resource identifier, not array of identifiers.
   +    // Test POST: missing field.
   +    // Test POST: invalid target.
   +    // Test POST: internal resource type target.
   +    // Test POST: success.
   

   Nice todo list!

1. +++ b/tests/modules/jsonapi_test_field_access/jsonapi_test_field_access.permissions.yml
   @@ -0,0 +1,7 @@
   +field_jsonapi_test_entity_ref update access:
   +  description: 'Gives access to to PATCH, POST or DELETE values of the relationship test field.'
   +  title: 'Update JSON API relationship test field'
   +
   +field_jsonapi_test_entity_ref edit access:
   +  description: 'Gives access to to PATCH, POST or DELETE values of the relationship test field.'
   +  title: 'Update JSON API relationship test field'
   

   These are nearly identical… this is very confusing! 😵 I think one is intended for updating, the other for creating?

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1237,13 +1245,32 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +    // Test GET.
   ...
   +    // Test POST.
   

   PATCHing relationships is not yet tested?

3. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1322,6 +1351,96 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      // Test POST: missing field.
   +      // @todo write this test.
   +      // Test POST: invalid target.
   +      // @todo write this test.
   +      // Test POST: internal resource type target.
   +      // @todo write this test.
   

   These are still intended to happen in this issue, I think?

4. +++ b/tests/src/Functional/ShortcutTest.php
   @@ -56,7 +56,7 @@ class ShortcutTest extends ResourceTestBase {
   -        'uri' => 'internal:/admin/content/comment',
   +        'uri' => 'internal:/user/logout',
   

   Why this change?

5. +++ b/tests/src/Functional/UserTest.php
   @@ -104,6 +104,7 @@ class UserTest extends ResourceTestBase {
   +    $user->setEmail("$key@example.com");
   

   Why this change?

Just FYI: #13's interdiff looks great :)

1. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1397,11 +1397,19 @@ abstract class ResourceTestBase extends BrowserTestBase {
          // Test POST: missing field.
   ...
   +      $request_options[RequestOptions::BODY] = Json::encode(['data' => array_intersect_key($target_identifier, ['id' => 'id'])]);
   

   This is not just missing a field, this is missing essential information. Updating the comment is sufficient.

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1418,14 +1426,20 @@ abstract class ResourceTestBase extends BrowserTestBase {
          //// Test POST: success, relationship already exists, with unique arity.
   

   Übernit: not yet unindented :)

EPIC EPIC EPIC work!

+++ b/tests/src/Functional/ResourceTestBase.php
@@ -1440,12 +1440,12 @@ abstract class ResourceTestBase extends BrowserTestBase {
       // @todo: Uncomment the following two assertions in https://www.drupal.org/project/jsonapi/issues/2977659.
-      //// Test POST: success, relationship already exists, no arity.
-      //$response = $this->request('POST', $url, $request_options);
-      //$this->assertResourceResponse(204, NULL, $response);
+      // Test POST: success, relationship already exists, no arity.
+      $response = $this->request('POST', $url, $request_options);
+      $this->assertResourceResponse(204, NULL, $response);

I'm 99% certain Gabe only meant to remove the extraneous slashes on line 1447, but in doing so, PHPStorm helpfully also uncommented lines 1448 and 1449. All failures are on line 1448.

So, reverting this.

So, review time!

My review in #12:

1. #12.1 was addressed by #17: it removed that.
2. #12.2+#12.3 were implemented by subsequent patches.
3. #12.4+#12.5 were answered/clarified in #13

Issues discovered here (yay for test coverage to find places where things were not quite working the right way!):

1. #2977653: Spec Compliance: Return 204 or 200, not 201 for relationship POST requests.
2. #2977659: Spec Compliance: POST|PATCH|DELETE on relationships should respect arity rules
3. #2976371: Impossible to POST|PATCH relationship to bundle-less entity or entity reference field without target bundles

The last of those three is already fixed, and already shipped with the 1.19 release!

Issues not discovered here, but related: #2956084: Impossible to raise an error when an `include` is requested for an inaccessible relationship field.. In #8, @gabesullice said that issue would solve problems encountered in this issue. I took it as meaning that #2956084 was a blocker for this issue, but obviously it isn't.

Re-review of the patch:

1. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1323,6 +1352,214 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      // Test DELETE: no existing relationships, no op, success.
   ...
   +      $this->assertResourceResponse(201, $expected_document, $response);
   +      /* $this->assertResourceResponse(204, NULL, $response); */
   

   Woah! This makes sense, but I had to think it through. Idempotency means this must indeed be a 204 response.

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1323,6 +1352,214 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      // Test PATCH: success, new value is different than existing value.
   +      $request_options[RequestOptions::BODY] = Json::encode(['data' => [$target_identifier, $target_identifier]]);
   ...
   +      // Test DELETE: two existing relationships, both removed because no arity
   +      // was specified.
   

   This is great, but I have one question/reservation: we're never testing the deleting of a specific arity.

   What if the first quoted line would create three relationships instead of two, we'd then delete the "middle" one by specifying the appropriate arity, verify this in the response, and then delete without specifying an arity, which should result in no relationships being left?

   Perhaps this is something that is up to #2977659: Spec Compliance: POST|PATCH|DELETE on relationships should respect arity rules to add?

That second point is the only remaining question I have! After it's answered/addressed, this is RTBC :)

BUT, there is coverage that just needs to be uncommented:

🙈 OMG! 😳

That's what I looked for, but I only looked further down in the patch rather than up. Which doesn't even make sense. If I'd seen that, I'd have committed it already.

I don't think it's necessary to test with 3 relationships and verify 2 remaining, since arity is not a stored value, it is a calculated one.

Right again; I misremembered arity for its original implementation (synonym for Drupal's delta), but obviously that's not what it is.

That's is in there too :)

Yep, that's the portion that's already being run/tested :)

RTBC confirmed, committing…

🍻