Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only

As the architect of REST module's test coverage, I'll take this on, but I'll require sign-off from @e0ipso before committing.

#2885372: Can't PATCH User entity's 'mail' field via JSON API will get clarity thanks to this issue.

This will likely end up being blocked on #2882451: Make the response consistent ordered..

#2841851: Audit cacheable metadata generation will also get clarity.

This is now my #1 priority. Starting to actively work on this. Finally! 🎉

I've started with Node, because that's the most commonly used entity type. I'm essentially porting:

1. \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase and its parent class 1:1 to \Drupal\Tests\jsonapi\Functional\ResourceTestBase
2. \Drupal\Tests\rest\Functional\EntityResource\Node\NodeResourceTestBase 1:1 to \Drupal\Tests\jsonapi\Functional\NodeTest

(I started with the test coverage before #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior landed, and that issue added crucial new test coverage, which we should port over.)

I've already made some key simplifications:

1. not testing multiple authentication providers, not even testing the "anonymous" case — only testing using basic_auth right now
2. testing without ?_format=api_json wherever possible, because this is pretty annoying and very much a Drupalism. This works fine, except for 404s, where I'm testing explicitly without ?_format=api_json (HTML response) *and* with (JSON API response), to make the difference abundantly clear. Hopefully we can improve this behavior in the future.

Lots and lots of things aren't yet tested. GET and DELETE are mostly complete. For GET, I already got one crucial bugfix committed: #2933515: JSON API does not support HEAD requests. #2933939: JSON API module must not send cacheable responses to PATCH, POST and DELETE requests is something I encountered while working on the DELETE test coverage. Both have plenty of @todos.

For PATCH and POST, I'm running into so many bugs that block progress (even with significant portions of their test coverage commented out!) … that I decided to just post a failing patch here, and then work on bugfixes that allow more of the test to become green. Creating patches with explicit test coverage for each of those bugfixes is going to be extremely time consuming. So I propose to be pragmatic and let bugfixes get committed if they allow the integration tests being added here to become green (or fail on a later assertion in the test scenario).

Created #2934149: [>=8.5] JSON API routes not specifying _content_type_format route requirement, resulting in bad DX for the next POST test coverage blocker. But turns out there's also a core bug that only appears in the specific way that JSON API has its routes defined. 😩😩😩

If #2934149: [>=8.5] JSON API routes not specifying _content_type_format route requirement, resulting in bad DX landed, we could've done this. But until #2934152: ContentTypeHeaderMatcher should not run for GET, HEAD, OPTIONS or TRACE requests lands, #2934149 won't be able to land.

So, alas, we can't bring back some of the commented test coverage quite yet.

Created #2934164: \Drupal\jsonapi\Controller\RequestHandler::deserializeBody() should not use the format corresponding to the Content-Type request header's value, to fix not the root cause, but only the direct cause. Also committed it.

Reuploading+retesting #9's patch should show a different error now.

#9:

1) Drupal\Tests\jsonapi\Functional\NodeTest::testPostIndividual
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"errors":[{"title":"Unprocessable Entity","status":422,"detail":"Unprocessable Entity: validation failed.\ntitle: Title: this field cannot hold more than 1 values.\n","links":{"info":"http:\/\/www.w3.org\/Protocols\/rfc2616\/rfc2616-sec10.html"},"code":0}]}
+{"errors":[{"title":"Unprocessable Entity","status":422,"detail":"There was an error un-serializing the data. Message: Deserialization for the format  is not supported","links":{"info":"http:\/\/www.w3.org\/Protocols\/rfc2616\/rfc2616-sec10.html"},"code":0}]}

/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:392
/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:470
/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:749

#12:

1) Drupal\Tests\jsonapi\Functional\NodeTest::testPostIndividual
Failed asserting that 400 is identical to 422.

/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:380
/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:470
/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:749

Now the test is failing because JSON API it receives in invalid request body. So let's explicitly test that error response, and fix the request body being sent, so that that failing assertion can finally test what it's supposed to test: the validation error when sending multiple values for a single-value field.

As expected:

1) Drupal\Tests\jsonapi\Functional\NodeTest::testPostIndividual
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"errors":[{"title":"Unprocessable Entity","status":422,"detail":"Unprocessable Entity: validation failed.\ntitle: Title: this field cannot hold more than 1 values.\n","links":{"info":"http:\/\/www.w3.org\/Protocols\/rfc2616\/rfc2616-sec10.html"},"code":0}]}
+{"errors":[{"title":"Unprocessable Entity","status":422,"detail":"title: \u003Cem class=\u0022placeholder\u0022\u003ETitle\u003C\/em\u003E: this field cannot hold more than 1 values.","code":0,"source":{"pointer":"\/data\/attributes\/title"}}]}

Hurray, that is the error we need! It just is formatted in a different way. That should be easy enough to fix. Continuing tomorrow :)

#15 is exactly why I didn't want to post a patch yet… I thought #9 made that clear? I guess I wasn't explicit enough — my bad! :(

So let's make it clear for now: This patch is not remotely ready for reviews yet!

I will indicate it with a similarly clear message stating the opposite when it is ready to be reviewed.

Since you've already spent some time reading the patch, I'll answer your specific questions, but please don't spend more time reviewing, since it's going to undergo massive changes still.

1. Because static caches massively get in the way when running tests. In this case, the camelids NodeTypeis created in the test's setUp() method, but even manually invoking the route builder wouldn't cause its JSON API routes to be generated, due to this static cache.
2. Yes, I'd like that too, and I've been wanting to do that already. Sadly, it's not nearly as simple as you portray it to be in your comment. There are lots of dynamic bits in lots of entities' expected normalizations: from expected UUIDs to testing different normalizations based on BC flags.
3. Yes. Note this is for testing the PATCHing of the path field which is a special snowflake. But as I already indicated in #9, this has not yet been activated, since I'm still struggling to get the basic PATCH and POST test coverage to work, due to the many bugs I'm already encountering. That's what I've been pushing forward in the comments since #9.
4. I'd love that, and I've considered it too, but no, it's not possible, because it'd make the JSON API module require the REST module. In the future, when JSON API is moved into Drupal core, we should move shared test infrastructure out of the rest module into the serialization module. This also applies to non-test code like the ResourceResponse(Subscriber) classes.

Cool :)

I can't wait for you to review this patch, but until it's in a better place, it's going to be a frustrating waste of time! (And I see how the frustration is showing in #16.)

Working to get to that place :)

While working on this, discovered another problem, this time only a soft bug: #2934362: Specify a "code" for every exception that JSON API throws. Made it a child issue of this issue.

Addressed #14 in #2934370: Entity validation constraint violation messages contain JSON-encoded HTML (a new child issue).

This patch is a straight rebase of #14 (several conflicts had to be resolved). The failing assertion causing #14 is now as simple as it should be, but still had to be updated to account for the ['source'=>['pointers'=>'/data/attributes/title']] expectation.
This test coverage patch now fails on the next assertion: again one step further :)

I'll still need to improve \Drupal\Tests\jsonapi\Functional\ResourceTestBase::assertResourceErrorResponse() to allow 'source' to be specified, and 'links to be omitted for 422 errors (per #2934362-14: Specify a "code" for every exception that JSON API throws.4), but keeping that refactoring for a separate interdiff.

The next failing assertion is

    $parseable_invalid_request_body_2 = $this->serializer->encode($this->getNormalizedPostEntity() + ['uuid' => [$this->randomMachineName(129)]], 'api_json');
…
    $request_options[RequestOptions::BODY] = $parseable_invalid_request_body_2;

    // DX: 422 when invalid entity: UUID field too long.
    // @todo Fix this in https://www.drupal.org/node/2149851.
    if ($this->entity->getEntityType()->hasKey('uuid')) {
      $response = $this->request('POST', $url, $request_options);
      $this->assertResourceErrorResponse(422, '…', "Unprocessable Entity: validation failed.\nuuid.0.value: UUID: may not be longer than 128 characters.\n", $response);
    }

This should fail, but in JSON API it results in a 201 response. Why?

It appears to be because JSON API ignores the UUID it receives. In a particular test run, the (random and invalid because 129 characters) UUID d8cdc218-5d03-4b43-a4bc-4dbbba71aa07 was sent, but the 201 response contained the (generated on the server side)
40fffa92-7406-47f7-bdbd-fcb8e5b70089 UUID. Which is why it was able to result in a 201 response.

I already encountered a similar bug in the GET test coverage, which I worked around by simply commenting the relevant assertion for now:

    // Not only assert the normalization, also assert deserialization of the
    // response results in the expected object.
    $unserialized = $this->serializer->deserialize((string) $response->getBody(), get_class($this->entity), 'api_json', [
      'target_entity' => static::$entityTypeId,
      'resource_type' => $this->container->get('jsonapi.resource_type.repository')->getByTypeName(static::$resourceTypeName),
    ]);
    // @todo ………………………………………… (deserialization works, but the UUID is set to a random one rather than the received one)
//    $this->assertSame($unserialized->uuid(), $this->entity->uuid());
    $get_headers = $response->getHeaders();

We'll have to solve this at some point, might as well solve it now. There may be a valid reason for JSON API disallowing letting clients specify a UUID when creating entities, but there is no valid reason why deserializing on the server side results in a different UUID: that should result in an object where all fields match those in the given normalization.

Created #2934386: Accept client-generated IDs (UUIDs) for investigating that.

So, if I comment the assertions blocked on #2934386: Accept client-generated IDs (UUIDs), and also the assertions that follow it that are blocked on #2934149: [>=8.5] JSON API routes not specifying _content_type_format route requirement, resulting in bad DX (see #10), then we're getting to the point where a successful POST request is being made!

Sadly, that assertion fails because there are cache tags and cache contexts in the response, even though there shouldn't be because it's a response to an unsafe (and hence uncacheable) HTTP request! Which brings us back to #2933939: JSON API module must not send cacheable responses to PATCH, POST and DELETE requests.

I expect this to fail with:

1) Drupal\Tests\jsonapi\Functional\NodeTest::testPostIndividual
Failed asserting that true matches expected false.

/Users/wim.leers/Work/d8/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:397
/Users/wim.leers/Work/d8/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:819
/Users/wim.leers/Work/d8/modules/jsonapi/tests/src/Functional/NodeTest.php:278

Line 397 does $this->assertSame($expected_cache_tags !== FALSE, $response->hasHeader('X-Drupal-Cache-Tags')); — there shouldn't be a cache tags header, but there is. Which will be fixed by #2933939.

BTW, we're almost at the end of testPostIndividual(). The patches so far contained a full copy-pasted version of REST's EntityResourceTestBase::testPost(). But most of the remaining LoC (the ones after the line we're currently failing on) are actually testing REST-specific BC stuff. Which we don't need to test here! :) So let's remove those.

This updates the assertions at the very end of testPostIndividual(), so that they make sense for JSON API. In doing so, I started using the @jsonapi.entity.to_jsonapi service. And that's how I ran into a known bug with that again: #2925043: Server error when using the jsonapi.entity.to_jsonapi service. But I can work around that for now!

testPostIndividual() is now passing tests! 🎉

Alright, so now that the work on the Drupal core 8.5.0-alpha release mostly complete, shifting attention back to JSON API.

Next up: testPatchIndividual(). Let's start with commenting the assertions that are similar to the ones in testPostIndividual() that are also commented. Those edge cases didn't work in the latter (POST), they also don't work in the former (PATCH).

To my great relief, at least the most crucial security-related test coverage that ensures that fields that cannot be PATCHed (because the fields are read-only) is passing!

Well, it's not quite passing just yet: JSON API helpfully indicates the reason why you are not allowed to PATCH a field. This is actually a great DX enhancement, that is missing in core's REST module! I created an issue to add it there: #2938035: When PATCHing a field is disallowed, no reason is given for *why* this happens 🤘. I will have to change the tests a bit to accommodate this, see the next comment/patch.

That's why this patch should fail with:

1) Drupal\Tests\jsonapi\Functional\NodeTest::testPatchIndividual
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"errors":[{"title":"Forbidden","status":403,"detail":"The current user is not allowed to PATCH the selected field (created).","links":{"info":"http:\/\/www.w3.org\/Protocols\/rfc2616\/rfc2616-sec10.html#sec10.4.4"},"code":0,"id":"\/node--camelids\/09d436e1-ff10-4b24-a7d9-9a46b536f317","source":{"pointer":"\/data\/attributes\/created"}}]}
+{"errors":[{"title":"Forbidden","status":403,"detail":"The current user is not allowed to PATCH the selected field (created). The \u0027administer nodes\u0027 permission is required.","links":{"info":"http:\/\/www.w3.org\/Protocols\/rfc2616\/rfc2616-sec10.html#sec10.4.4"},"code":0,"id":"\/node--camelids\/09d436e1-ff10-4b24-a7d9-9a46b536f317","source":{"pointer":"\/data\/attributes\/created"}}]}

/Users/wim.leers/Work/d8/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:406
/Users/wim.leers/Work/d8/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:993

Great, now we're testing that those 403 responses for PATCHing fields do contain helpful information explaining *why*!

We're now getting stuck on the thing we're testing just after that, which was fixed in core's REST module the day before I started working on this issue (see #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior): when the fields sent in a PATCH request match the currently stored values, we should allow those to be sent, and not complain that the user is not allowed to modify the field … because they're not trying to modify the field!

EDIT: note that the core equivalent of this exact interdiff was posted at #2938035-9: When PATCHing a field is disallowed, no reason is given for *why* this happens!

Note that in working on a patch for #2938035: When PATCHing a field is disallowed, no reason is given for *why* this happens, I discovered a core bug that also affects JSON API and will cause us to get blocked pretty soon: #2938053: AccessResult::orIf() fails to retain the reason if both operands are neutral or forbidden, but the first contains a reason and the second one does not.

This led me back deep into computed field hell, where we recently fixed bugs in core (see #2916300: Use ComputedFieldItemListTrait for the path field type and related issues) to even do #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior in core in the first place … which after figuring out #2939800: FieldNormalizerValue::rasterizeValue() respects cardinality === 1, but doesn't verify that this is true (making JSON API more robust) led me to the root cause: a bug in core's test coverage that I'd copied over to the test coverage here: #2939802: Follow-up for #2824851: EntityResourceTestBase::getModifiedEntityForPatchTesting() handles @FieldType=path incorrectly. The entire reason that #2939802 was even able to happen is because of Field API's absence of triggering exceptions when doing invalid things… It's really turtles all the way down. Building on weak foundations has ripple effects throughout, even after having done all the work of making REST have very very extensive integration test coverage, we still manage to find edge cases 😩

IOW: this made me:

1. land a one-line hardening patch in JSON API: #2939800: FieldNormalizerValue::rasterizeValue() respects cardinality === 1, but doesn't verify that this is true
2. discover yet another bug in core: #2939802: Follow-up for #2824851: EntityResourceTestBase::getModifiedEntityForPatchTesting() handles @FieldType=path incorrectly
3. import that bugfix in core over to this patch

Finally, as explained in #30, we still need to port over the changes from #2824851 in core to JSON API, but for that we need test coverage … which is what this issue is adding. So rather than modifying JSON API's logic, I split that out to a separate issue: #2939810: EntityResource::patchIndividual() should allow fields that the user is not allowed to change, as long as they match the current value, so that this issue can stay focused on adding test coverage with the absolute minimum of changes made to JSON API's behavior/code.

This tiny interdiff took ~4 hours…

Now that this done, the next challenge is:

1) Drupal\Tests\jsonapi\Functional\NodeTest::testPatchIndividual
Failed asserting that true matches expected false.

/Users/wim.leers/Work/d8/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:414
/Users/wim.leers/Work/d8/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:1012
/Users/wim.leers/Work/d8/modules/jsonapi/tests/src/Functional/NodeTest.php:281

because the response contains a cache tag header: X-Drupal-Cache-Tags: http_response node:1. In other words: JSON API returns responses that implement CacheableResponseInterface, which indicates that respones to the unsafe PATCH method are cacheable … which of course does not make sense. Should not be too hard to fix.

Should not be too hard to fix.

Actually, I already ran into this problem in #9. It's why I created #2933939: JSON API module must not send cacheable responses to PATCH, POST and DELETE requests. Let's not fix that now, let's work around it, just like I did before.

This then also:

1. does for PATCH what #22 did for POST wrt #2934149: [>=8.5] JSON API routes not specifying _content_type_format route requirement, resulting in bad DX
2. comments the field_rest_test and field_rest_test_multivalue test coverage, which we want/need, but let's get to green first
3. removes REST-specific BC layer tests
4. … fixes a bug in the jsonapi.entity.to_jsonapi service, which cannot really be moved to a separate issue because the test coverage that #2874509: Provide service to simplify generating a JSON API representation of an entity in PHP code introduced specifically mocks away the bit that is broken, which is why this easily goes unnoticed…

testPatchIndividual() is now passing tests! 🎉

HAH, the tests are failing on 8.6 because of #2891215: Add a way to track whether a revision was default when originally created, which I asked to be reverted… and so now it's impacting JSON API as well. Working on a fix.

Fixed that. This patch will now only pass on 8.5.x and 8.6.x core, until that's fixed in core, which will happen in #2937850: Mark revision_default as internal for REST consumers.

And the remaining fail for GET is due to #2923779: Normalizers violate NormalizerInterface: one must no longer use the @serializer service to denormalize JSON API normalizations!

I decided to first expand to the Term test because that's the other test that has explicit test coverage for the path field, which the Node test coverage we've been working on so far also contains, but it's currently still commented.

This:

1. enables the \Drupal\Tests\jsonapi\Functional\NodeTest::testPatchPath() test coverage that was previously commented, making the Node test coverage more complete (all test methods that core REST also has!)
2. introduces \Drupal\Tests\jsonapi\Functional\TermTest, including the testPatchPath() and testGetIndividualTermWithParent() methods that test edge cases that are unique to the Term entity, just like core REST
3. uncovered a whole new slew of bugs:

#2940336: JSON API incorrectly requires the "access content" permission for *all* routes
#2940339: Port reference field support for non-empty entity reference fields not pointing to an entity from #2543726
#2940342: Cacheability metadata on an entity fields' properties is lost

FFS d.o … #42 did succesfully attach both a patch and issue relationships, but they don't show up in the comment. 😡

Thanks to #42, also adding test coverage for the EntityTest was fortunately easy, and did not uncover more unknown bugs.

Later this week hopefully :)

I said "later this week hopefully", but sadly that did not happen. I was asked to help out with critical Workflow Translation issues for much of last week. But I did work on this a lot, specifically on Comment entity's test coverage (and User). I ran into lots of problems for both. Comment test coverage is actively being worked on.

1. But since then, #2940336: JSON API incorrectly requires the "access content" permission for *all* routes landed. So this should be rerolled: a few work-arounds can be removed (also from Comment!).
2. #2934386: Accept client-generated IDs (UUIDs) also landed, which allows us to uncomment some commented test code, and hence increase our test coverage!
3. Sadly, the JSON API code base also changed for the worse in one area: the simple change that this patch made to ResourceTypeRepository had to be updated (which is fine), but now causes infinite recursion due to changes elsewhere. All due that annoying static cache. That's because ::all() ends up calling ::get() which ends up calling ::all() (since #2937961: ResourceType should provide related ResourceTypes). See interdiff-static-cache.txt for that, which shows the new work-around relative to HEAD.

Those were easy to fix. However, they came with consequences:

1. #2940336 caused:
   1. the expectations for 403 responses are no longer correct: rather than simple "Drupal 403 responses", they're now "refined JSON API 403 responses". Also easy enough.
   2. a different set of cache contexts is present
   3. and the 403 messages are now far more detailed and helpful … and because of that, some of the expectations had to be expanded
   4. somehow the POST normalizations were succeeding even though they were invalid (Node contained a resource ID, Term contained an invalid resource type)
2. #2934386 (plus #2940336) caused me to notice that the 403 POST responses are incorrect (JSON API still auto-generates UUIDs for POSTed resources, which is fine, but it also uses those auto-generated UUIDs in 403 responses, which is not fine). Instead of adding work-arounds in this patch, I figured I'd just create an issue to fix it separately: #2942243: Follow-up for #2934386: server-generated IDs for newly POSTed entities should not be exposed in 403 responses

Plus, I forgot to mention this in #44, but that in fact did uncover another unknown bug, I just hadn't created an issue for it yet:

+++ b/tests/src/Functional/EntityTestTest.php
@@ -0,0 +1,177 @@
+    // @todo Remove this in …
+    if ($method === 'PATCH' || $method === 'POST') {
+      $this->setUpAuthorization('GET');
+    }

… and I now had to duplicate this work-around to TermTest too. Created #2942255: Spec Compliance: Even when successfully POSTing or PATCHing an entity, you can still get a 403 response when you cannot GET it! for that.

Retesting, testbot had database problems …

Whew 😓, here's Comment, that was … hard. I lost count of how many hours I spent on this one. The better part of a week.

1. I ran into trickiness wrt testing PATCH-protected fields that happen to be relationships in JSON API (entity reference fields in Drupal) — we already have #2939810: EntityResource::patchIndividual() should allow fields that the user is not allowed to change, as long as they match the current value for that.
2. I again ran into #2942255: Spec Compliance: Even when successfully POSTing or PATCHing an entity, you can still get a 403 response when you cannot GET it!, but in a harder-blocking way.
3. I also ran into a hitherto undiscovered spec compliance bug: #2942549: Spec Compliance: JSON API allows POSTing relationship fields in 'attributes' rather than in 'relationships'. That took a while to figure out, because I didn't realize I was doing something wrong, having copy/pasted the normalization to POST from core's REST test coverage. A helpful error would've helped me find the problem much faster, and that's what that issue is about :)
4. \Drupal\Tests\jsonapi\Functional\CommentTest::testPostSkipCommentApproval() made me discover #2942561: Assert denormalizing the JSON API response results in the expected object, which fortunately need not be a blocker. Still, that's a pretty flabbergasting find.
5. … and many more problems that didn't turn out to be show-stopping but slowed me down a lot

There are two bits of wonderful news here:

1. there's only one small change to the base class here, which further strengthens the test coverage's comprehensiveness :)
2. this was by far the most complex entity type, with the most special cases/extra test coverage

So I'm hopeful progress will be faster from here on out :)

The attached patch should fail because #2942255: Spec Compliance: Even when successfully POSTing or PATCHing an entity, you can still get a 403 response when you cannot GET it! is not yet committed.

There's the expected failure:

There were 2 failures:

1) Drupal\Tests\jsonapi\Functional\CommentTest::testPostIndividualSkipCommentApproval
Failed asserting that 403 is identical to 201.

/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:402
/var/www/html/modules/contrib/jsonapi/tests/src/Functional/CommentTest.php:342

2) Drupal\Tests\jsonapi\Functional\CommentTest::testPostIndividual
Failed asserting that 403 is identical to 201.

/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:402
/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:885

I just committed #2942255: Spec Compliance: Even when successfully POSTing or PATCHing an entity, you can still get a 403 response when you cannot GET it!, now re-testing #50, that should now pass!

Hurray, #50 now passes as predicted!

And #2942255 having landed also allows me to remove the work-arounds that did work for other entity types.

#2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method added test coverage for the following content entity types:

1. Node
2. User
3. Term
4. EntityTest
5. Comment

All other content entity types were deferred to follow-ups. I propose we do the same here. So that only leaves User! Which is the last content entity type with a number of very special edge cases… let's dive right in.

Testing GET, PATCH, POST and DELETE for the User content entity type was easy. There was only one change that I had to make to ResourceTestBase, to ensure that its uses of the @jsonapi.entity.to_jsonapi service were using the intended account, and hence respected field access correctly. (The UserAccessControlHandler does some fairly special things.)

But then there were also the two additional pieces of test coverage:

1. testPatchDxForSecuritySensitiveBaseFields(), added back in Q4 2016, in the original REST test coverage issue (#2737719). Getting this to work was easy — no new bugs discovered!
2. testPatchSecurityOtherUser(), added only recently, in #2885469: Regression: manually setting the ID field for newly-created content entities is not possible anymore (public follow-up to SA-2017-002). Thanks to fixes in Entity API, JSON API is secure, but it is not able to return the appropriate error response just yet. Due to a bug in JSON API's handling of denormalizing fields. Fortunately, it's a consequence of an existing problem: #2939810: EntityResource::patchIndividual() should allow fields that the user is not allowed to change, as long as they match the current value. See #2939810-6: EntityResource::patchIndividual() should allow fields that the user is not allowed to change, as long as they match the current value for details. Once that is fixed, this will behave entirely as it should, and we have the test coverage to prove it!

Now on to the config entity types (#2737719 did Block, ConfigTest, Role and Vocabulary), and then on getting the commented assertions uncommented, then this will be ready!

While working on the Block test coverage, I found a significant bug in the normalization of config entities. #2733097: [FEATURE] Add GET support for configuration entities added config entity GET support, but it did not add any integration test coverage. The only test coverage is \Drupal\Tests\jsonapi\Kernel\Normalizer\JsonApiDocumentTopLevelNormalizerTest::testNormalizeConfig(), but that didn't verify that a config entity's dependencies key is exposed correctly.

This is what we'd expect:

…
          'dependencies' => [
            'theme' => [
              'classy',
            ],
          ],
…

but in reality we get:

…
          'dependencies' => [
            'classy',
          ],
…

Obviously that's losing very important information!

Added a work-around for that. To fix this bug, I created #2942979: JSON API's normalization of config entities' "dependencies" key-value pair omits essential information when there's only a single dependency.

The test coverage for ConfigTest, Role and Vocabulary did not suffer from this because none of them have dependencies!

Hurray, testbot agreed! Now down to the last stretch: then on getting the commented assertions uncommented, then this will be ready! — this means going through this patch's \Drupal\Tests\jsonapi\Functional\ResourceTestBase class and looking at every thing that is commented out: it should no longer be commented out, otherwise the test coverage is incomplete!

That's for tomorrow :)

A first round of analyzing commented code, and attempting to fix it, remove it, or otherwise, create an issue for it.

1. #2934386: Accept client-generated IDs (UUIDs) has been fixed, but it only fixed a subset of the problem reported in #21, it did not fix this:

   // Not only assert the normalization, also assert deserialization of the
       // response results in the expected object.
       $unserialized = $this->serializer->deserialize((string) $response->getBody(), get_class($this->entity), 'api_json', [
         'target_entity' => static::$entityTypeId,
         'resource_type' => $this->container->get('jsonapi.resource_type.repository')->getByTypeName(static::$resourceTypeName),
       ]);
       // @todo ………………………………………… (deserialization works, but the UUID is set to a random one rather than the received one)
   //    $this->assertSame($unserialized->uuid(), $this->entity->uuid());
       $get_headers = $response->getHeaders();
   

   We have #2942561: Assert denormalizing the JSON API response results in the expected object for that, created in #50.

   Updated patch to point to that.

2. In the initial test coverage port, I commented all occurrences of:
   

       if (static::$auth) {
         // DX: forgetting authentication: authentication provider-specific error
         // response.
         $response = $this->request('POST', $url, $request_options);
         $this->assertResponseWhenMissingAuthentication('POST', $response);
       }
   
       $request_options = NestedArray::mergeDeep($request_options, $this->getAuthenticationRequestOptions('POST'));
   

   … and now it's clear we're not going to be testing that at all. Because we're only testing basic_auth. So, removed that.

3. Similarly, in the initial test coverage, I commented all occurrences of:

       // @todo JSON API doesn't handle any of these edge cases correctly.
   /*
       // DX: 415 when no Content-Type request header. HTML response because
       // missing ?_format query string.
       $response = $this->request('POST', $url, $request_options);
       $this->assertSame(415, $response->getStatusCode());
       $this->assertSame(['text/html; charset=UTF-8'], $response->getHeader('Content-Type'));
       $this->assertContains('A client error happened', (string) $response->getBody());
       return;
       $url->setOption('query', ['_format' => 'api_json']);
   
       // DX: 415 when no Content-Type request header.
       $response = $this->request('POST', $url, $request_options);
       $this->assertResourceErrorResponse(415, '…', 'No "Content-Type" request header specified', $response);
   
       $request_options[RequestOptions::HEADERS]['Content-Type'] = '';
   
       // DX: 400 when no request body.
       $response = $this->request('POST', $url, $request_options);
       $this->assertResourceErrorResponse(400, 'No entity content received.', $response);
   
       $request_options[RequestOptions::BODY] = $unparseable_request_body;
   
       // DX: 400 when unparseable request body.
       $response = $this->request('POST', $url, $request_options);
       $this->assertResourceErrorResponse(400, 'df', 'Syntax error', $response);
   */
   

   because:

   1) Drupal\Tests\jsonapi\Functional\NodeTest::testPatchIndividual
   Exception: TypeError: Argument 2 passed to Drupal\jsonapi\Controller\EntityResource::patchIndividual() must implement interface Drupal\Core\Entity\EntityInterface, instance of Symfony\Component\HttpFoundation\Request given
   Drupal\jsonapi\Controller\EntityResource->patchIndividual()() (Line: 242)
   …
   2) ) Drupal\Tests\jsonapi\Functional\NodeTest::testPostIndividual
   Exception: TypeError: Argument 1 passed to Drupal\jsonapi\Controller\EntityResource::createIndividual() must implement interface Drupal\Core\Entity\EntityInterface, instance of Symfony\Component\HttpFoundation\Request given
   Drupal\jsonapi\Controller\EntityResource->createIndividual()() (Line: 193)
   

   In other words: when there's no request body for a POST or PATCH request, JSON API has a fatal error instead of sending the appropriate error response.

   For the latter portion (400 when unparseable request body), I created #2943165: Failing to decode should result in a 400 response, failing to denormalize should result in a 422 response. For everything before it, I created #2943170: JSON API's RequestHandler causes fatal PHP error when a PATCH or POST request has no body.

Time for some long-overdue simplification of the error response assertions. Those assertions are rather verbose and complex, because JSON API error objects (http://jsonapi.org/format/#error-objects) are very rich.

In trying to come up with one simpler assertion helper method, I discovered a few bugs that made it unnecessarily difficult to assert those error objects:

1. #2943176: Spec Compliance: when error object 'id' key exists, it contains the individual resource URI, instead of "a unique identifier for this particular occurrence of the problem"
2. and of course #2934362: Specify a "code" for every exception that JSON API throws, which I created in #19 — for which I just posted a patch to get rid of the unwanted noise that is currently generated, while still allowing code to take advantage of it in the future: #2934362-12: Specify a "code" for every exception that JSON API throws.
3. the special casing introduced in #2832211-14: Add source pointer element to "422 Unprocessable Entity" responses for 422 errors, changing that here because it is just changing implementation details for code this patch is already touching anyway

Attached is a patch that:

1. fixes point 3
2. is not blocked on point 2 (#2934362)
3. is blocked on point 1 (#2943176), at least to the extent that everything is as ugly and complex as before, but with ready-to-uncomment much simplified assertions awaiting us for when #2943176 lands!

I missed one spot :(

Correction, I missed one test method, in which there were two spots. Gah!

Also fixing two nits at the same time.

Uncommented all the field_rest_test test coverage, to test field access edge cases! Happy to say that this was mostly without problems. I stumbled upon #2938053: AccessResult::orIf() fails to retain the reason if both operands are neutral or forbidden, but the first contains a reason and the second one does not as predicted more than 3 weeks ago in #32 and #2938035-6: When PATCHing a field is disallowed, no reason is given for *why* this happens, but fortunately I can work around that for now (yet another @todo that we can remove in the future!).

Also, a minor clean-up issue for the REST module so that the JSON API module can reuse core's rest_test module without awkward consequences (i.e. the rest module being installed for jsonapi tests … 🤐): #2943338: rest_test module for testing REST module could be reused by contrib JSON API module.

Next up: two types of simplifications/nits that are occurring frequently throughout the patch:

1. $this->serializer->encode(…, 'api_json') should really become Json::encode(…) everywhere to KISS
2. figuring out the cacheability weirdness — lots of assertions have had to been modified to match the results, but the results are wrong. I wasn't diligent enough in adding @todos everywhere.

I had tested the individual CRUD operations on NodeTest, but didn't run the entire test suite. Hence #61 failed in some cases. Here are the fixes.

$this->serializer->encode(…, 'api_json') should really become Json::encode(…) everywhere to KISS

Fixed

This patch has >300 coding standards violations. Fixed them all.

Replacing all json_encode() with Json::encode().

figuring out the cacheability weirdness — lots of assertions have had to been modified to match the results, but the results are wrong. I wasn't diligent enough in adding @todos everywhere.

We actually already have issues for them and it won't make sense analyzing those further. Fixing the issues the existing @todos are referring to will automatically ensure we need to update existing assertions.

So that means we're ready for final review! Let's get this done, because it blocks many issues!

…

Except #66 is now apparently failing on InternalEntitiesTest, which was added earlier today. Gah! Looking into that…

Turns out #2939705: Do not include internal resource types under `include` broke JSON API on Drupal 8.6. Reverted it: #2939705-38: Do not include internal resource types under `include`.

Resting #66, should be green!

@e0ipso teased me on Twitter:

Wait to see @wimleers face when he learns that https://www.drupal.org/project/jsonapi/issues/2930028 was committed already. cc/ @GabeSullice.

— Mateu A. B. (@e0ipso) February 12, 2018

To which I replied:

🤨
😲
🤯
😭
🤩

I'm still not sure which I'm more: surprised or grateful :P Definitely thank you to both of you!

I will address #70, which contains lots of great remarks. But first, because this is already committed, I want to make the 8.x-1.x branch pass on 8.4.x again. This patch was only passing against Drupal core 8.6.x!

The committed test coverage doesn't specify the Accept: application/vnd.api+json request header anywhere, yet it does result in 4xx responses in that format. This works in 8.5.x/8.6.x thanks to #2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent fixing significant bugs in the routing system (it changes incorrect priorities in route filters, to correct ones).
It's better to always send that Accept request header — in fact, the JSON API spec demands us to do so. http://jsonapi.org/format/#content-negotiation-clients says: Clients that include the JSON API media type in their Accept header MUST specify the media type there at least once without any media type parameters.

TermTest::testGetIndividualTermWithParent cannot possibly pass on 8.4.x because it was added in #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration, which was committed only to 8.6.x.

For the same reasons as #75, we need to change the 404 test coverage that was added in the initial commit.

The failure in VocabularyTest::testPostIndividual() is because 8.4 doesn't have #2808217: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission, that was committed to 8.5.

And finally, the failures wrt field_rest_test_multivalue occur because #2928702: Make EntityResourceTestBase's field_rest_test_multivalue test field less invasive: omit it from normalizations only landed in 8.5.

3 remaining failures, all due to things that aren't fixed in 8.4 yet, but are in 8.5:

1. EntityTestTest: #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts added a internal_string_field field to test internal vs non-internal — this will come in handy in #2921257: On Drupal 8.5, JSON API should respect Typed Data's new ::isInternal(), and JSON API Extras should use ::setInternal() (we already have a todo pointing to that)
2. NodeTest: #2891215: Add a way to track whether a revision was default when originally created added this field.
3. TermTest: #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration fixed the parent field, in 8.4 we need a different expectation.

Forgot one: TermTest::testPostIndividual(), for similar reasons as #81.

I posted #87 too quickly.

Clean-up.

#75 made me realize I could simplify ResourceTestBase::getAuthenticationRequestOptions(): it doesn't actually need that parameter!

I'm committing #90, because the commit in #72 broke testing on 8.4, and committing #90 now minimizes the duration of JSON API not passing tests on 8.4.

#89 and #90 both passed tests on 8.4. Now testing #90 against 8.6. If that also passes tests, I'm committing it :)

Back to NW for addressing #70.

Haha, no worries. Sometimes this weird stuff happens :)

#70:

1. ✔️
2. Because that allows ResourceTestBase to call this, instead of having to duplicate this 100%.
3. Great question! The short answer is: "because core's tests do this everywhere, I just followed that example both in REST tests and here". Tried to do what you suggested, it did not work, it appears to be related to the hooks and events that that helper function is triggering. Even doing this did not work:
   

       EntityTestBundle::create([
         'id' => 'bar',
         'label' => 'bar',
       ])->save();
       \Drupal::entityManager()->onBundleCreate($bundle, 'entity_test');
   

   Is this too much magic? Yes. Do I understand it? No. Is it out of scope to make this better? Yes.

4. ✔️ Woah, great point! Fixed.
5. ✔️
6. I considered the same, but decided against this. It'd mean 100% duplication. Reusing those same fields means we can reuse the existing test infrastructure. However, I do think that once this is in Drupal core, we should rename the fields to field_httpapi_test and field_httpapi_test_multivalue. That also why I created #2943338: rest_test module for testing REST module could be reused by contrib JSON API module, which already performs a first baby step (reviews welcome!).
7. Hah! You have @tedbow to thank for that line :D
8. ✔️ — the dead horse is now a fine unicorn. 🦄
9. ✔️
10. Sorting alphabetically does not necessarily make sense. Nor does the current ordering of attributes necessarily make sense. But I can see the value in listing more important attributes first. Enforcing a sorting is something we could choose to do, but we shouldn't do it just to make tests slightly simpler.
11. Great question!
    Because target_id could also be a string, and you can't increment a string. The point is that we pick a non-existing target ID, and hardcoding a value that is never going to exist and can be used as both a string and an ID adequately addresses that. Full history in #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.

I know it was a tedious change though, so thank you :)

Thanks to PHPStorm's refactoring feature, it wasn't tedious at all actually :)

It makes a simple string/hash comparison from a CURL impossible.

As long as the order is consistent, that's not true. You can totally diff. In fact, that's what I do all the time while working on these tests: when the expected normalization document doesn't match the actual one, I diff.

And YAY! Up to you or @e0ipso now to commit this :)

Wonderful! See you in #2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases!