Specify a "code" for every exception that JSON API throws [#2934362]

Problem/Motivation

Discovered while working on #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only.

http://jsonapi.org/format/#errors contains this:

code: an application-specific error code, expressed as a string value.

The JSON API module does this in \Drupal\jsonapi\Normalizer\HttpExceptionNormalizer::buildErrorObjects():

'code' => $exception->getCode(),

If we look that up — http://php.net/manual/en/exception.getcode.php — then we see:

Returns the exception code as integer in Exception but possibly as other type in Exception descendants (for example as string in PDOException).

Sadly, in 99% of cases, this is 0 (an integer) for the JSON API module's errors. Which is misleading.

Proposed resolution

In \Drupal\jsonapi\Normalizer\HttpExceptionNormalizer::buildErrorObjects(), omit 'code' if it is 0, because in that case, no helpful code was specified, so there's no point in exposing it
Update all existing throw SomeException code in the JSON API module to contain a helpful code
Add a page in the documentation guide where all the codes are documented. Documentation example of an introduced code: 23: The user is trying to access a restricted field in a related request.
Result: helpful codes!

Remove 'code' from \Drupal\jsonapi\Normalizer\HttpExceptionNormalizer::buildErrorObjects() altogether
Result: no helpful codes, but also no more misleading.

Remaining tasks

Decide on an approach and execute it.

Comment	File	Size	Author
#15	2934362-15.patch	1.83 KB	Wim Leers
#15	8.x-1.x: PHP 7 & MySQL 5.5, D8.4 334 pass
#15	interdiff-12-15.txt	939 bytes	Wim Leers
#12	2934362-12.patch	961 bytes	Wim Leers
#12	8.x-1.x: PHP 7 & MySQL 5.5, D8.4 332 pass, 2 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

4 January 2018 at 13:24

Wim Leers created an issue. See original summary.

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 4 January 2018 at 13:25

Issue summary:

View changes

Comment #3

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 4 January 2018 at 13:25

Comment #4

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 5 January 2018 at 10:11

Do we need to gate all the bubbled exceptions to provide meaningful errors? I think @gabesullice and I talked about that in some other issue. Maybe we want to conflate the two.

Paging Gabe to know if he remembers the issue where we agreed on that follow up.

Comment #5

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 5 January 2018 at 10:30

Do we need to gate all the bubbled exceptions to provide meaningful errors?

I'm not sure I understand what this means — can you elaborate?

Comment #6

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 5 January 2018 at 10:37

This is the issue were we decided to have a follow-up #2918155: fix(EntityResource|Query\Filter): path validation of filters needs to be aware of computed fields. Note we never did create that issue.

I'm not sure I understand what this means — can you elaborate?

Basically we have some operations in our code that can throw exceptions (for instance saving or deleting an entity or making entity queries). Some of those are bubbling through the exception subscriber. We should catch those exceptions in place and re-throw them as HttpExceptions.

Is it more clear now?

Comment #7

gabesullice

English

CreditAttribution: gabesullice at Acquia commented 5 January 2018 at 10:45

@e0ipso @Wim Leers, here is another place where something like this was discussed: #2933615-10: Fix related and relationships endpoints for disabled resources

Basically, there are lower level exceptions that might be thrown (SQL constraint violations, for example). @e0ipso is proposing that we use this issue to more thoroughly map these to low level exceptions to HttpExceptions.

Comment #8

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 5 January 2018 at 11:39

#6+#7: it's clear now, thanks!

What isn't clear yet to me is whether you're both suggesting that each of those

catch (SomeException $e) {
  throw new SomeHttpException($some_message, 12345);
}

that you're proposing would include that 12345, where that is a unique code for each throw … occurrence. That's the only way that "code" makes sense.

IOW: are you proposing to go with the first or second proposed resolution in the IS?

Comment #9

Ivan Berezhnov CreditAttribution: Ivan Berezhnov as a volunteer and at Drupal Ukraine Community for Levi9 commented 5 January 2018 at 18:47

Issue tags:

+CSKyiv18

Comment #10

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 6 January 2018 at 07:14

that you're proposing would include that 12345, where that is a unique code for each throw … occurrence. That's the only way that "code" makes sense.

@Wim Leers yes. I'm proposing to catch all kind of exceptions that may happen (plugin not found, etc.) and re-throw them as the appropriate SomeHttpException with a 5xx status code. As part of the same effort we could also add a unique code number to every place we throw an exception… I think that's option 1.

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 8 January 2018 at 13:43

👍

Comment #12

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 8 February 2018 at 10:57

Status:	Active	» Needs review
Issue tags:	-Novice, -php-novice, -CSKyiv18

File	Size
2934362-12.patch	961 bytes
8.x-1.x: PHP 7 & MySQL 5.5, D8.4 332 pass, 2 fail

Until we do #10, shouldn't we remove code altogether? Right now it's pure noise: code is always 0. It's information provided to the developer that isn't any information…

So I think that rather than waiting for some hypothetical future in which we've done #10 for all relevant JSON API and core code … I think the attached patch is probably a more sensible approach that improves DX today, yet still allows code throwing exceptions to associate a "code" with an exception.

Comment #13

8 February 2018 at 11:03

Status:

Needs review

» Needs work

The last submitted patch, 12: 2934362-12.patch, failed testing. View results

Comment #14

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 9 February 2018 at 05:43

I'm OK having a default of 0. Removing it may cause unexpected behaviors in clients parsing it. On top of that I think that having a default error code is industry standard. I don't think that conditionally removing it helps DX.

In any case I think this is not terribly important either way, so if you feel strongly about this I won't be in the way.

Comment #15

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 9 February 2018 at 13:25

Status:

Needs work

» Needs review

File	Size
interdiff-12-15.txt	939 bytes
2934362-15.patch	1.83 KB
8.x-1.x: PHP 7 & MySQL 5.5, D8.4 334 pass

1 file was hidden/shown/deleted

File	Size
2934362-12.patch	961 bytes
8.x-1.x: PHP 7 & MySQL 5.5, D8.4 332 pass, 2 fail

I do feel strongly about it.

(Here's a reroll to update test expectations.)

Comment #16

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 9 February 2018 at 14:12

I do feel strongly about it.

That's decided then. Although you need to know that you get a fixed quota on that.

$strongFeelingsVetos--;

Comment #17

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 9 February 2018 at 16:24

Hah :D

I'd much rather have you agreeing with me, so let me try to convince you one more time! :)

Removing it may cause unexpected behaviors in clients parsing it.

Any client that is actively parsing it would not be actively using it, because it is always zero and hence does not help with debugging.
Any client that is built for the JSON API spec and not Drupal knows that this member MAY be present, so is guaranteed to not break.

On top of that I think that having a default error code is industry standard.

It may be an industry standard, but always sending zero is equally helpful as sending a "number-of-llamas-hurt-while-building-this-response": 0 member.
The point of these error codes is that you can look them up in software's knowledgebase and then read a detailed explanation that helps you fix the problem. Drupal doesn't ever set these error codes. It definitely doesn't have a list of such error codes plus accompanying explanations.
For example: https://dev.mysql.com/doc/refman/5.7/en/error-messages-server.html

I don't think that conditionally removing it helps DX.

I don't understand why you think it doesn't help. It suggests it's a signal, that it's information, but it isn't! Removing pointless information helps developers understand this faster. I would totally have been debugging this, wondering why it's zero, and the answer is … because only exceptions that specify a particular number would be helpful and literally nothing in Drupal does this, but you'd only know this if you read https://secure.php.net/manual/en/exception.getcode.php, and scanned the entire Drupal code base.

My conclusion is: only sending information that is actually relevant helps developers understand errors faster, and prevents them from diving into rabbit holes that may end with them debugging Drupal and hence cursing Drupal/JSON API.

Comment #18

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 11 February 2018 at 08:24

Status:

Needs review

» Reviewed & tested by the community

I am good with the change. This is such a minor thing that we should not be typing more into this issue.

Comment #19

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 11 February 2018 at 08:25

Please move back to Needs Work after merging the initial removal so we can have actually documented codes added in a later patch.

Comment #20

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 11 February 2018 at 08:28

Issue summary:

View changes

Comment #21

24 February 2018 at 08:20

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

Issue #2934362 by Wim Leers, e0ipso, gabesullice, Ivan Berezhnov:...

Comment #22

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 24 February 2018 at 08:21

Status:

Reviewed & tested by the community

» Needs work

Moving back to Needs Work after the merge.

Comment #23

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 7 May 2018 at 15:08

Status:

Needs work

» Closed (outdated)

There have been zero complaints from JSON API users about this change in 2.5 months. IMHO there's nothing left to be done here.

Furthermore, it was stated multiple times by @e0ipso that this is "not terribly important", so rather than keeping this open, I'm closing it. Otherwise this issue will effectively be open forever.