Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber [#2855693]

Problem/Motivation

See #2829977: SerializableHttpException and Drupal\jsonapi\Error\* are not necessary, specifically #2829977-25: SerializableHttpException and Drupal\jsonapi\Error\* are not necessary. The most relevant info of that issue:

We had the same problem in the REST module, and we fixed that in #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty. We want the exact same solution here. Then there is no need for a __sleep() method. Yes, that means we're duplicating code from the REST module. But better a duplication than an incorrect abstraction. If it turns out that it's the right abstraction, we can move the code for this over to the Serialization module. So that still leaves open the door for future simplification. For now, what matters is that we follow the exact same pattern.

In other words: we should revert the changes made by #2818221: [BUGFIX] Restore serialization with HttpException inline responses and instead introducing the same solution that is used in core's rest module: #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty. Opened #2855693: Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber for that, because tacking that onto this issue will make this issue/patch very very difficult to review.

Postponed on #2829977: SerializableHttpException and Drupal\jsonapi\Error\* are not necessary, which is postponed on #2841027: JSON API format should only be used on JSON API routes, but can currently also be used for REST resources.

Proposed resolution

Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse()
Add ResourceResponseSubscriber
Add explicit test coverage proving it works correctly with Dynamic Page Cache

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Comment	File	Size	Author
#21	2855693-21.patch	21.7 KB	Wim Leers
#21	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 295 pass
#21	interdiff.txt	2.17 KB	Wim Leers
#18	2855693-18.patch	19.59 KB	Wim Leers
#18	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 293 pass, 4 fail PHP 7 & MySQL 5.5, D8.4 293 pass, 4 fail
#18	interdiff.txt	7.42 KB	Wim Leers
#17	2855693-17.patch	15.37 KB	Wim Leers
#17	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 292 pass, 6 fail PHP 7 & MySQL 5.5, D8.4 292 pass, 6 fail
#17	interdiff.txt	1018 bytes	Wim Leers
#14	2855693-14.patch	15.37 KB	Wim Leers
#14	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 289 pass, 10 fail
#14	interdiff.txt	4.28 KB	Wim Leers
#10	2855693-10.patch	15.46 KB	Wim Leers
#10	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 291 pass, 5 fail
#10	rebase-interdiff.txt	1.83 KB	Wim Leers
#8	2855693-8.patch	15.12 KB	dawehner
#8	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 251 pass, 20 fail
#3	2855693-3.patch	15.5 KB	Wim Leers
#3	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 Patch Failed to Apply

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

24 February 2017 at 13:21

Wim Leers created an issue. See original summary.

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 24 February 2017 at 13:22

Issue summary:	View changes
Related issues:		+#2818221: [BUGFIX] Restore serialization with HttpException inline responses, +#2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty

Comment #3

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 24 February 2017 at 14:05

File	Size
2855693-3.patch	15.5 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 Patch Failed to Apply

\Drupal\Tests\jsonapi\Kernel\Controller\RequestHandlerTest's only purpose is testing the automatic validation of JSON API responses against JSON API's JSON schema. That code now lives in \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber. That still needs to be moved.

Comment #4

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 27 February 2017 at 09:33

Title:

[PP-2] Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber

» [PP-1] Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber

#2841027: JSON API format should only be used on JSON API routes, but can currently also be used for REST resources landed, one less blocker to this now!

Comment #5

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 27 February 2017 at 14:34

Parent issue:

» #2829327: Review JSON API module for core-readiness

Comment #6

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 4 March 2017 at 06:02

Title:	[PP-1] Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber	» Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber
Status:	Postponed	» Needs review

#2829977: SerializableHttpException and Drupal\jsonapi\Error\* are not necessary was committed. This is now unblocked.

Comment #7

4 March 2017 at 06:05

Status:

Needs review

» Needs work

The last submitted patch, 3: 2855693-3.patch, failed testing.

Comment #8

dawehner

German

CreditAttribution: dawehner at Chapter Three commented 5 March 2017 at 13:02

Status:

Needs work

» Needs review

File	Size
2855693-8.patch	15.12 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 251 pass, 20 fail

Here is a reroll.

Comment #9

5 March 2017 at 13:08

Status:

Needs review

» Needs work

The last submitted patch, 8: 2855693-8.patch, failed testing.

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 6 March 2017 at 11:15

Status:

Needs work

» Needs review

File	Size
rebase-interdiff.txt	1.83 KB
2855693-10.patch	15.46 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 291 pass, 5 fail

2 files were hidden/shown/deleted

File	Size
2855693-3.patch	15.5 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 Patch Failed to Apply
2855693-8.patch	15.12 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 251 pass, 20 fail

I'm afraid the rebase in #8 wasn't entirely correct. Thanks though :)

I think this one is.

Comment #11

dawehner

German

CreditAttribution: dawehner at Chapter Three commented 6 March 2017 at 11:25

Thank you wim!

Comment #12

6 March 2017 at 12:36

Status:

Needs review

» Needs work

The last submitted patch, 10: 2855693-10.patch, failed testing.

Comment #13

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 6 March 2017 at 13:32

Apparently not enough yet. On it.

Comment #14

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 6 March 2017 at 13:48

Assigned:	Wim Leers	» Unassigned
Status:	Needs work	» Needs review

File	Size
interdiff.txt	4.28 KB
2855693-14.patch	15.37 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 289 pass, 10 fail

1 file was hidden/shown/deleted

File	Size
2855693-10.patch	15.46 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 291 pass, 5 fail

Oh hah. #2857658: Get rid of ResourceResponseInterface is what's causing these 5 failures :) A very silly reroll. Removed all 6 occurrences.

Comment #15

dawehner

German

CreditAttribution: dawehner at Chapter Three commented 6 March 2017 at 14:01

Status:

Needs review

» Reviewed & tested by the community

Ha, so we have test coverage for that. Good to know

Comment #16

6 March 2017 at 17:18

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 14: 2855693-14.patch, failed testing.

Comment #17

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 6 March 2017 at 17:33

Assigned:	Unassigned	» Wim Leers
Status:	Needs work	» Needs review

File	Size
interdiff.txt	1018 bytes
2855693-17.patch	15.37 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 292 pass, 6 fail PHP 7 & MySQL 5.5, D8.4 292 pass, 6 fail

1 file was hidden/shown/deleted

File	Size
2855693-14.patch	15.37 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 289 pass, 10 fail

I can't reproduce the failure in RestJsonApiUnsupported (and its duplicate, RestJsonApiFormatUnsupported).
Fixed the failures in JsonApiFunctionalTest and JsonApiFunctionalMultilingualTest.
I totally forgot about the failures in \Drupal\Tests\jsonapi\Kernel\Controller\RequestHandlerTest — despite me writing in #3 that that test coverage also still needed to be moved. Working on that now.

Comment #18

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 6 March 2017 at 18:16

Assigned:

Wim Leers

» Unassigned

File	Size
interdiff.txt	7.42 KB
2855693-18.patch	19.59 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 293 pass, 4 fail PHP 7 & MySQL 5.5, D8.4 293 pass, 4 fail

1 file was hidden/shown/deleted

File	Size
2855693-17.patch	15.37 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 292 pass, 6 fail PHP 7 & MySQL 5.5, D8.4 292 pass, 6 fail

Fixed #17.3:

Converted the kernel test to a unit test.
This required me to refactor away the use of DRUPAL_ROOT.
This also required me to refactor away the reliance on the container (by using \Drupal::logger()).
#2840677: Validate responses against the JSON API specification uses the logger.channel.jsonapi service yet does not define it. IOW: test coverage for the logging itself is missing. Not fixing that here, just adding the service, because I need it.

Comment #19

6 March 2017 at 22:45

The last submitted patch, 17: 2855693-17.patch, failed testing.

Comment #20

6 March 2017 at 23:36

Status:

Needs review

» Needs work

The last submitted patch, 18: 2855693-18.patch, failed testing.

Comment #21

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 7 March 2017 at 10:37

Status:	Needs work	» Needs review
Related issues:		+#2844046: REST Resource config entities do not respect the status (enabled/disabled)

File	Size
interdiff.txt	2.17 KB
2855693-21.patch	21.7 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 295 pass

1 file was hidden/shown/deleted

File	Size
2855693-18.patch	19.59 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.3 293 pass, 4 fail PHP 7 & MySQL 5.5, D8.4 293 pass, 4 fail

100% of the remaining failures are now:

I can't reproduce the failure in RestJsonApiUnsupported (and its duplicate, RestJsonApiFormatUnsupported).

I was finally able to reproduce this! #2844046: REST Resource config entities do not respect the status (enabled/disabled) is causing this. It changed some test internals. This fixes it.

Comment #22

dawehner

German

CreditAttribution: dawehner at Chapter Three commented 7 March 2017 at 12:04

+++ b/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -0,0 +1,214 @@
+
+    assert('$this->validateResponse($event->getResponse())', 'A JSON API response failed validation (see the logs for details). Please report this in the issue queue on drupal.org');
...
+   */
+  protected function validateResponse(Response $response) {
+    if (!class_exists("\\JsonSchema\\Validator")) {

As an additional follow up this could be moved into its own subscriber

+++ b/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -0,0 +1,214 @@
+    $schema_path = dirname(dirname(__DIR__)) .'/schema.json';

+1 for that. ALternative you could have injected the app root

Comment #23

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 7 March 2017 at 12:21

I'd like that. But it'll be harder to detect whether this is a JSON API response in a separate response subscriber. But yes, matter for a separate issue.
That would not have worked, because the JSON API module can be installed in many locations. And drupal_get_path() also calls out to the container. So then a unit test would have to mock the container. This is simpler.

Do we have a RTBC? :)

Comment #24

dawehner

German

CreditAttribution: dawehner at Chapter Three commented 7 March 2017 at 12:41

Status:

Needs review

» Reviewed & tested by the community

I have one :)

Comment #25

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 9 March 2017 at 12:01

But it'll be harder to detect whether this is a JSON API response

We have the request object in the response object. In the request object we have the route object. In the route object we have an attribute saying if the route is JSON API or not. More complex to describe than the actual code.

Responses to JSON API routes can be assumed to need schema validation (if assertions are enabled).

Can we create a follow up ticket to move the assertion to an onResponse subscriber?

Comment #26

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 9 March 2017 at 13:36

Follow-up created: #2859207: Move Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber::validateResponse() to its own subscriber.

Comment #27

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 10 March 2017 at 21:19

Status:

Reviewed & tested by the community

» Fixed

Comment #28

10 March 2017 at 21:19

e0ipso committed aeeaefe on 8.x-1.x authored by Wim Leers

fix(Maintainability): Remove \Drupal\jsonapi\Controller\RequestHandler::...

Comment #29

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 13 March 2017 at 10:22

Thanks!

Comment #30

27 March 2017 at 10:24

Status:

Fixed

» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

Parent issue

Related issues

Referenced by