Spec Compliance: Inaccessible collection/related resources surface errors: should be 200 with hypermedia + metadata

This is a great observation. This is my 2c

1. I agree that if all items in a collection have the same error we should bubble that error code as the response code.
2. Some fail, some don't. We should respond with a 200 and add errors to the meta
3. All fail, but with different error codes. We should respond with a generic 400

What do you think?

Having a list of entities of which some are 403s should be filtered out and no information about their 403 state should be returned as that is information disclosure.

I have 2 articles of which one is unpublished.

curl --header 'Accept: application/vnd.api+json' --request GET http://drupal.d8/jsonapi/node/article | json_pp                                       8.4.x

{
   "meta" : {
      "errors" : [
         {
            "links" : {
               "info" : "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4"
            },
            "code" : 0,
            "detail" : "The current user is not allowed to GET the selected resource.",
            "title" : "Forbidden",
            "id" : "/node--article/d70893d6-fada-4840-a7a1-c0bac3b66241",
            "source" : {
               "pointer" : "/data"
            },
            "status" : 403
         }
      ]
   },
   "links" : {
      "self" : "http://drupal.d8/jsonapi/node/article"
   },
   "data" : [
      {
         "attributes" : {
            "status" : true,
            "vid" : 1,
            "title" : "Article 1",

Requesting a single entity resulting in a 403 is ok as one knows the UUID somehow.

IMHO the collection never should contain 403 items.

We are now required to add filters to prevent 403 items like

curl --header 'Accept: application/vnd.api+json' --request GET "http://drupal.d8/jsonapi/node/article?filter\[status\]\[value\]=1" | json_pp

I agree that #2 is the right approach.

#3:

[…] no information about their 403 state should be returned as that is information disclosure.

That is not true. See how core's REST module handles this: #2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out.

Related: #2930231: JSON API 403 errors don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out.

I'm not 100% sure whether using a 403/404 is the best idea.

Just imagine the following case: You are a developer not aware of Drupal at all. You are accessing a URL, you get a 200 back. Your code has some condition to handle the 404 case, but it should not happen, given you handle authentication properly. One day someone deletes a bunch of content from the site and suddenly your app shows some error message.

In other words I think there is a semantic difference between a 404/403 of a collection resource and a 404/403 of individual bits embedded in that resource. A 403/404 is a client error, but well, in this case it was certainly not an error the client did, it is simply a state change of the server.

@dawehner isn't that the same that happens with a front-end that requests node/2 after it got deleted or unpublished? But we accept that requesting node/2 after deletion/unpublication will yield 404/403, right?

@e0ipso
Interesting point/question!
Well, if you think about it, if you know that /node/2 is deleted, you no longer have to request it. It would not be true for collections.

Maybe the right approach would be to look out for public APIs (github, some API best practises etc.).

OT:
The guzzlehttp library throws an exception by default when you have a 404/403 result.

The decided approach in #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" was to include only a resource identifier for the forbidden resources, which makes this issue outdated. As there will be values in the data regardless of every entity being inaccessible. Therefore, a 200 is acceptable.

Per #2943176-34: 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", #2943176 actually hasn't reached a conclusion yet. Reopening.

I'm gonna quickly try to summarize some background for my thinking:

• The JSON API spec is RESTful.
• REST makes a distinction between "resources", "representations", "entities" (I put these all in quotes because I am using the terms in the REST, not Drupal or JSON API sense).
• "Resources" are the responses which are returned for a particular URI. They MAY contain more than one "representation of an entity".
• "Entities" are data objects which can be transferred via HTTP.
• "Representations" are the various serializations of entities (e.g. JSON vs. XML, or a particular format)
• The JSON API module has "individual", "collection", "related" and "relationship" resources (exactly one collection per entity type + bundle, and exactly one individual resource for each Drupal-entity)
• The JSON API module maps Drupal-entities (node, terms, etc) to REST entities (which are confusingly called "resources" by the JSON API spec (language :facepalm:)).
• Over time and/or with different query parameters, the entities represented at "the $resource_type collection resource" will change, but it's still the same REST resource.

All of this leads me to believe:

The status code for a "collection resource" should be entirely distinct from any or all of the entities represented at that resource.

Unless there is an access control mechanism that is explicitly a prerequisite for accessing any list of entities of a given type that we can leverage, then it must be assumed that any consumer must have access to any collection resource.

It is completely consistent with REST to return a 200 response to an empty collection, even when all those entities have been elided for access reasons.

The proposed resolution that "[we should return] a 403 Forbidden response instead, without any meta errors" would therefore be inconsistent with the REST idea that resources and representations are distinct.

The original post touches on our problem when it states: "...and all of the entities of that type are not accessible." Unfortunately, we simply cannot know when that is true for "all of the entities" of a type.

Let's refocus on the counterintuitive piece: "a successful HTTP status (200) and a "meta" section filled with an error for each [denied] entity"

Also, let's remember that the meta errors themselves have "status": 403 piece which is itself counterintuitive because status codes are a response-level concept.

I think the ideal solution is to:

1. Always return 200 OK for collection resources
2. Stop putting individual errors for every access denied entity in under meta.errors section of document (removing entities for access reasons is neither a client nor a server error)
3. Put a single non-error message under the meta section when resources have been elided for access reasons.
   • The non-error message should have useful details explaining *why* some items were removed.
   • It *should* include links to the individual resources which were elided.
   • The links should exist so that a request could be made to those individual resources to determine the *reason* each was removed (this could also be raised into the "meta" section of the links object).

I like #13! Well-reasoned. Thank you for taking the time to lay out your rationale 👍❤️

Assigning to @e0ipso for feedback, since he's the one who designed the current approach. He likely has more insight into and opinions about this than I do.

I like #13! Well-reasoned. Thank you for taking the time to lay out your rationale 👍❤️

Ditto.

Only one consideration, let's move these changes to 2.x.

Woot! 🤘

Updated the IS.

Only one consideration, let's move these changes to 2.x.

➕

Wonderful to reach consensus :) Well done, @gabesullice! 👏

Can the consensus here help us get to consensus on #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"?

Idea: whenever some content is inaccessible, we should send a 206 response? https://httpstatuses.com/206

I'm not sure 206 is meant for this particular case, but for range requests.

The server is successfully fulfilling a range request for the target resource by transferring…

Good point. It's just that partial content is an accurate description here. But I think you're right; it's only intended for range requests.

@Wim Leers assigned this to @e0ipso in #14 for his feedback, @e0ipso gave that feedback in #15. Therefore it no longer needs to be assigned to @e0ipso.

Assigning to myself!

First, let's see if any exceptions besides access related ones actually bubble into meta.errors so we know if that needs to be taken into consideration.

We can identify those because all access errors have an id member that looks like: /$resource_type_name/$resource_id.

Interesting! That's good to know :) I've been working on this locally, but don't have something to POST yet (🤣). In the meantime, I think this is a major issue for 2.x.

Interesting! That's good to know :)

Can you briefly explain what's so interesting/what this means exactly?

Can you briefly explain what's so interesting/what this means exactly?

This is the only place in the code that meta.errors are added/surfaced in the document. Meaning that if I handle the all inaccessible entities at a higher level—say by adding disallowed access results as an argument to JsonApiDocumentTopLevel (this is just an off-the-cuff idea)—then all the merging and tracking of meta.errors can just disappear... because the only possible errors were access errors!

Very interesting indeed :)

I think that this will be easier to accomplish after this is fixed.

#2985426: Spec compliance: `related` routes should return 200, not 403, if field access is allowed but the related resources are forbidden landed!

I believe #2949807: Spec Compliance: Error responses are missing the `jsonapi` top-level member should land first, and may make things simpler here.

#2949807: Spec Compliance: Error responses are missing the `jsonapi` top-level member also landed! 🚀

This is now the last remaining major bug!

Since this is the last remaining blocker for 2.x and we are very enthusiastic about releasing a beta ASAP, I recommend that we be pragmatic like we are being w/ revisions. That is, like revisions, let's review the HTTP API aspect of this patch with scrutiny and then stabilize this after.

With that said, this patch does exactly what I would want from the HTTP API perspective and matches points 2+3 of the proposed resolution we came to consensus on after #13. I still need to validate that collections only ever return 200 and then update the rest of the test expectations for it if we are agreed on this approach.

Here is an example of a response in this format.

somehow I thought it would be a JSON Pointer: `item#0` ➡️ `#/data/0`, `#/included/7`, …

somehow I thought it would be a JSON Pointer

Huh, that never occurred to me. Is that the case with error pointers today? They just point at /data don't they? IOW, they don't track position do they?

I chose the item#N because: (a) the spec generally treats links object keys as link-relation types (although it's not an officially in the spec) and (b) the spec disallows lists of links.

So I went with item + an incrementing integer with no specific regard for document position. See https://tools.ietf.org/html/rfc6573#section-2.1.

WRT JSON Pointer

Is that the case with error pointers today?

I think it is the case today. For example:

• \Drupal\jsonapi\Controller\EntityResource::createIndividual()
• \Drupal\jsonapi\Controller\EntityResource::checkPatchFieldAccess()
• …

We just don't have any examples of us pointing to a numerically keyed item in a data structure ("arrays"), we only happen to have string keyed items ("objects").

You even had something like that in #2943176-21: 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" :)

WRT link keys

I chose the item#N because: (a) the spec generally treats links object keys as link-relation types (although it's not an officially in the spec) and (b) the spec disallows lists of links.

Hm, that's tricky :(

General

How could you ever generate an actually accurate/usable index or position? The omitted resources are … omitted, and therefore the subsequent resources take the omitted resource's place. So if there's 3 resources, they'd have indexes/positions 0, 1 and 2. Imagine the one in position 1 is omitted because it's not accessible. Then saying /data/1 or #item1 is omitted is rather futile, because the resource in position 2 has received position 1.

IOW: I'm not sure I understand either approach. It feels like I'm missing something? 🤓

We just don't have any examples of us pointing to a numerically keyed item in a data structure ("arrays"), we only happen to have string keyed items ("objects").

meta.errors objects never have any other pointer than /data. Even when they are for included resources. Remember, meta.errors do not appear in error documents and they only appear for access denied cases in collection/related routes.

My interpretation of the spec has always led me to believe that pointers are for identifying the location of an error in a request document. I.e. for field validation errors. Not for identifying where something came from in a request document. I believe that latter case is a concept unique to our implementation. It's not without merit, it's just not the current case and it's not something standard.

I agree that position would be incredibly difficult to do and we're not doing it today. Perhaps we can leave that feature on the table and use #2993654: Provide JSON Pointer as a link on a resource to ease matching with included items to point to the link object when an include has been removed. This wouldn't work for removed primary data items though.

Remember, meta.errors do not appear in error documents and they only appear for access denied cases in collection/related routes.

That's what I was getting at in my "General" section in #41!

That's why I was confused to see item#0 in your #37 screenshot.

Upon re-reading, I see that I missed So I went with item + an incrementing integer with no specific regard for document position.. That makes more sense. However, I can see how a passerby would interpret item#3 as meaning "the fourth item".

I expected something like:

"meta": {
  "omitted": {
    "detail": "Some resources have been removed from this document because they're not accessible by you.",
    "links": [
      "http://drupal.test/jsonapi/node/article/9c7e8a8b-ca58-491c-b535-70b317e4098f",
      "http://drupal.test/jsonapi/node/article/e45ef8b4-c468-491c-b535-70b317e4098f"
    ]
  }
}

But you're right that the JSON API spec seems to imply that links should be keyed by their (IANA) link relation type. There isn't a clear one. So let's just make them have unique keys:

"meta": {
  "omitted": {
    "detail": "Some resources have been removed from this document because they're not accessible by you.",
    "links": {
      "node--article/9c7e8a8b-ca58-491c-b535-70b317e4098f": "http://drupal.test/jsonapi/node/article/9c7e8a8b-ca58-491c-b535-70b317e4098f",
      "node--article/e45ef8b4-c468-491c-b535-70b317e4098f": "http://drupal.test/jsonapi/node/article/e45ef8b4-c468-491c-b535-70b317e4098f"
    }
  }
}

Actually, we may want to list the reason; that may be relevant or even required in some use cases:

"meta": {
  "omitted": {
    "detail": "Some resources have been removed from this document because they're not accessible by you.",
    "links": {
      "node--article/9c7e8a8b-ca58-491c-b535-70b317e4098f": {
        "href": "http://drupal.test/jsonapi/node/article/9c7e8a8b-ca58-491c-b535-70b317e4098f",
        "reason": "This content is unpublished."
      },
      "node--article/e45ef8b4-c468-491c-b535-70b317e4098f": {
        "href": "http://drupal.test/jsonapi/node/article/e45ef8b4-c468-491c-b535-70b317e4098f"
        "reason": "This content is unpublished."
      } 
    }
  }
}

and actually, having looked at https://www.iana.org/assignments/link-relations/link-relations.xhtml, I … end up finding the item link relation type, so we can make that slightly better:

"meta": {
  "omitted": {
    "detail": "Some resources have been removed from this document because they're not accessible by you.",
    "links": {
      "item#node--article/9c7e8a8b-ca58-491c-b535-70b317e4098f": {
        "href": "http://drupal.test/jsonapi/node/article/9c7e8a8b-ca58-491c-b535-70b317e4098f",
        "reason": "This content is unpublished."
      },
      "item#node--article/e45ef8b4-c468-491c-b535-70b317e4098f": {
        "href": "http://drupal.test/jsonapi/node/article/e45ef8b4-c468-491c-b535-70b317e4098f"
        "reason": "This content is unpublished."
      } 
    }
  }
}

… of course at this point I've reached essentially the same solution as #36/#37. But I think not having sequential numerical IDs is essential here. Something like what I proposed makes it far clearer that the FOO in item#FOO is not a meaningful number, but just something to generate a unique ID.

IOW: I thought I disagreed with you, and started to explain why, and in doing so I ended up agreeing… 😅 😭

I wonder if reading the #37 screenshot also made @e0ipso jump to conclusions like it did for me!?

Ah, yes. Thanks for the extra context. At some point we talked about having the unaccessible resource objects without any fields (just type and id) populated in place. In that scenario it would make sense to have a pointer. I had a mix of realities, I think we are good to go with the current approach.

Let me increment the counter of the times where we moan for the poor hypermedia controls of JSON API. http://gtramontina.com/h-factors

Thanks for pointing to http://gtramontina.com/h-factors/, I hadn't seen that yet!

I had a mix of realities, I think we are good to go with the current approach.

Same here. And me thinking "Okay I'll explain what I mean", trying to do so in #43, and then concluding that Gabe Was Right™ was … funny and humbling 😊😃

In the interest of making this go faster, I took @gabesullice's #36 and pushed it further. I did implement it with item#node--article/bb3c7dfe-16d0-433b-a6f9-cdc83b8bd354, i.e. item#<resource type name>/<ID>, to avoid the potential for misinterpretation that @e0ipso and I both made, independently! 😇

http://gtramontina.com/h-factors

This is awesome ^ Thanks for sharing it :) Hypermedia is a sad, sad story in JSON API indeed. I'm very excited for profiles to land so we can fix it :)

I think we are good to go with the current approach.

Consensus! 🎉

Purely aesthetic things:

• The # looked good when it preceded a numbering. Now I like that the whole thing looks more like an identifier.
• The high-level detail was a little too informal for me because it used "you". Now it's a little bit more broadly applicable to different clients.
• I specifically don't like to use "reason" for link attributes because it already has an HTTP meaning (see "Reason-Phrase" here). "detail" is better.

Next up, fixing tests.

Whoops, forgot a string.

Posting some slow going progress.

Some more. The essence of most of these changes is that some errors don't have an id and many don't have a source.pointer. This makes it hard to generate any kind of link. For that, I added a self link onto all errors. This patch and the previous is mostly finding and updating that as necessary.

Cleaning up a little and hopefully making this a bit greener.

For that, I added a self link onto all errors.

I'm tempted to make this via instead of self. Anyone have thoughts about it?

Made it via. I like this because it avoid some confusion with a resource objects' self link, which generally refers to the route where that resource can be retrieved. For example, a resource object in a collection does not have a self link for the route it was retrieved at, it has the route where it can specifically be found. But an error shouldn't really be "found" anywhere. In essence, it was just "delivered via" its parent page.

Hm, not sure what happened there.

This should clear the last functional test failures 🤞

This should be green.

We still need to clean up all the test code that says it should be deleted in this issue and clean up new CS violations.

Okay, this cleans up all the test code that was supposed to be removed by this issue and also by #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" which this issue subsumed.

I made another change to the link keys that will need a +1 from @e0ipso and @Wim Leers. Instead of the resource type and UUID, I just MD5 the index.

Why? A few reasons. First, because too many errors don't have an ID at all! In that case, I tried using the pointer, but the pointer was often wrong too.

Most importantly though... I don't want these keys to represent some kind of API, they're just a way to make unique keys. Clients will inevitably try to deconstruct the key for some purpose. That won't be good if JSON API 1.1 or some profile tries to improve the linking situation. IOW, if we put some language in JSON API about this, we won't be up a creek having to break BC and we won't be sending a mixed signal.

A resource identifier is in the href everywhere that one applies already and that's what's supposed to be used for parsing.

Now moving on to CS violations.

Easy fixes.

I … was hoping that the patch would remain close to the size that it was in #46… it went from 5K to 40K. Alas.

Most of it is due to

The essence of most of these changes is that some errors don't have an id and many don't have a source.pointer. This makes it hard to generate any kind of link. For that, I added a self link onto all errors.

I'm very concerned that we're making responses more complex and making such a big set of changes only to accommodate our tests. Furthermore, the actual logic changes that we're making do only concern "meta errors" that do have an ID set.

So I dug in and tried to find another way. It's taking longer/is harder than expected/hoped. Stay tuned. (Posted this already so you guys know what the hell I'm doing!)

In IRC, @e0ipso suggested we use a true random value, not a hash of the index. This does that.

I'm very concerned that we're making responses more complex and making such a big set of changes only to accommodate our tests.

I don't think I agree that we're making responses more complex.

We're removing the ID from all errors so that they're not on some but not others. In it's place we're adding a consistent and meaningful URL to every error in its place.

The tests are simplified because I was able to remove numerous chunks of code that existed only to work around the inconsistency of an ID. There are many changes to it because I had to add the $via_link everywhere, but the flip side of that is that every error is consistent.

At each stage, I asked myself if I was happy with the API for errors this was setting up for the 2.x and I think it's reasonable. The only thing I would change that is not already done here is that I would also remove the code key from errors.

Can you point to what you think is adding complexity that wasn't already there?

I can certainly think of how this could be simplified as we generate errors. But my intention here, as I said in #36 was to make the HTTP API better and then work on the code to generate that API better, later.

Can you point to what you think is adding complexity that wasn't already there?

+++ b/src/Normalizer/EntityAccessDeniedHttpExceptionNormalizer.php
@@ -37,12 +38,13 @@ class EntityAccessDeniedHttpExceptionNormalizer extends HttpExceptionNormalizer
+        $errors[0]['links']['via'] = $url->getGeneratedUrl();

+++ b/src/Normalizer/HttpExceptionNormalizer.php
@@ -83,6 +83,7 @@ class HttpExceptionNormalizer extends NormalizerBase {
+    $error['links']['via'] = \Drupal::request()->getUri();

+++ b/tests/src/Functional/CommentTest.php
@@ -315,7 +315,7 @@ class CommentTest extends ResourceTestBase {
-    $this->assertResourceErrorResponse(500, 'The "" entity type does not exist.', $response);
+    $this->assertResourceErrorResponse(500, 'The "" entity type does not exist.', $response, FALSE, $url->setAbsolute()->toString());

+++ b/tests/src/Functional/ResourceTestBase.php
@@ -1420,10 +1385,10 @@ abstract class ResourceTestBase extends BrowserTestBase {
-      $this->assertResourceErrorResponse(400, 'Empty request body.', $response);
+      $this->assertResourceErrorResponse(400, 'Empty request body.', $response, FALSE, $url->setAbsolute()->toString());
...
-      $this->assertResourceErrorResponse(400, 'Empty request body.', $response);
+      $this->assertResourceErrorResponse(400, 'Empty request body.', $response, FALSE, $url->setAbsolute()->toString());

et cetera

I already addressed those:

There are many changes to it because I had to add the $via_link everywhere, but the flip side of that is that every error is consistent.

I think that's adding assertions about consistency. Not increasing complexity in any meaningful way.

Fixed the sorting that caused the previous errors.

But do we truly need that via link?

D'oh, of course we do … we need it to that the omissions can point to the omitted resource, which is the should be 200 with hypermedia part of the issue title. Sorry for not realizing that sooner.

I tried to address my concerns by stepping through the patch's changes with a debugger. I noticed that it also worked correctly for ::testGetIndividual()'s doTestIncluded(), which ends up in getExpectedIncludedResourceResponse(). And there too, I saw that the 403 response for an inaccessible relationship also resulted in a correct via value. But then I didn't see any meta.omitted appear in the final response. I then realized that's also not happening today. So then why do we need the via link on 403 relationship responses?

(I specifically dug in doTestIncluded() because that's the only test coverage that is failing in #46! If it's not showing up in the end result of a response with includes, then we can also just make it not deal with meta errors, and tests would also pass… right?)

But I'm not sure yet we need it for a 400 response because a query parameter violated the JSON API spec?

IOW: AFAICT we only need this via link for collection responses, but we're adding it both to all normalized HTTP exceptions ("errors" in JSON API context), and we're adding it to all test coverage.

I'm still not fully convinced. I feel like I'm missing something obvious?

CS violations and making the $via_link a non-optional parameter in a more logical position in a couple test methods.

I'm not ignoring you @Wim Leers. I'll respond in a second, I'm just optimistic that I can convince you, I just want to get testbot to run this now.

D'oh, of course we do … we need it to that the omissions can point to the omitted resource, which is the should be 200 with hypermedia part of the issue title. Sorry for not realizing that sooner.

Exactly. But please don't apologize!

I didn't see any meta.omitted appear in the final response. I then realized that's also not happening today.

That's correct. I think what you've spotted is an inconsistency that comes out of this issue: #2956084: Impossible to raise an error when an `include` is requested for an inaccessible relationship field.. Correct me if that's a wrong assumption.

So then why do we need the via link on 403 relationship responses?

Honestly, we don't need a via link in errors under the on error responses anywhere. But I do think its nice to have a consistent API where all objects under either the data or errors key either have a self or via link.

An interesting benefit that one might get from this is for tools like Rollbar which monitor client-side errors. With this, you have an easy way to see what URL was the source of the problem. Without it, error documents themselves contain no self link, so you'd have to either "remember" which URL you sent the request to in your log message or pull from window.location (but that often won't be helpful because apps rarely share show the same URL in the browser that the client is making).

In essence, it makes the error self-contained. The error object itself can answer "where did I get this 403 from?"

But I'm not sure yet we need it for a 400 response because a query parameter violated the JSON API spec?

I think a query parameter violating the spec is actually a great reason to put it there! Error objects have source.parameter to inform the client which parameter caused the violation and the via link can show you that parameter in context (perhaps your URL generator messed something up). With it there, you have everything you need when you do console.log(errorObject); you don't need to cross-reference it with the network tab.

Apparently, manipulating arrays by reference is hard.

I think what you've spotted is an inconsistency that comes out of this issue

Of course! I should've thought of that. _{This is why it's important to fix inconsistencies, because they make maintenance harder, even for somebody who's been working in a codebase for many months straight :D}

In essence, it makes the error self-contained. The error object itself can answer "where did I get this 403 from?"

Another good point.

I think a query parameter violating the spec is actually a great reason to put it there! Error objects have source.parameter to inform the client which parameter caused the violation

And again a good point.

Consider me convinced!

1. +++ b/src/Normalizer/EntityAccessDeniedHttpExceptionNormalizer.php
   @@ -37,12 +38,13 @@ class EntityAccessDeniedHttpExceptionNormalizer extends HttpExceptionNormalizer
   +          sprintf('jsonapi.%s.individual', \Drupal::service('jsonapi.resource_type.repository')->get($entity_type_id, $bundle)->getTypeName()),
   

   Nit: I think using the repository is overkill.

2. +++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
   @@ -216,6 +216,12 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
   +    if (!empty($rasterized['meta']['errors'])) {
   +      $omitted = $this->mapErrorsToOmittedItems($rasterized['meta']['errors']);
   +      unset($rasterized['meta']['errors']);
   +      $rasterized['meta']['omitted'] = $omitted;
   +    }
   
   @@ -261,4 +267,40 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
   +   * @todo: remove this after refactoring internal error handling. This is a temporary measure to stabilize the 2.x HTTP API for errors.
   

   Doesn't this mean that ['meta']['errors'] effectively cannot ever exist anywhere anymore? If so, why not s/meta.errors/meta.omitted/ everywhere?

   Correct me if I'm wrong: this @todo is about exactly that?

3. +++ b/tests/src/Functional/CommentTest.php
   @@ -315,7 +315,7 @@ class CommentTest extends ResourceTestBase {
   -    $this->assertResourceErrorResponse(500, 'The "" entity type does not exist.', $response);
   +    $this->assertResourceErrorResponse(500, 'The "" entity type does not exist.', $url->setAbsolute()->toString(), $response, FALSE);
   

   I think it'd already help if this accepted not just string, but also Url, so that this ->setAbsolute()->toString() stuff is done in the helper, rather than being repeated dozens of times.

4. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -172,10 +171,12 @@ class JsonApiFunctionalTest extends JsonApiFunctionalTestBase {
   +    $this->assertSame('help', reset($link_keys));
   +    $this->assertRegExp('/^item:[a-zA-Z0-9]{7}$/', next($link_keys));
   

   Nice use of next().

5. +++ b/tests/src/Functional/NodeTest.php
   @@ -306,9 +306,9 @@ class NodeTest extends ResourceTestBase {
   +            'via' => $url->setAbsolute()->toString(),
              ],
              'code' => 0,
   -          'id' => '/node--camelids/' . $this->entity->uuid(),
   

   This will of course need a CR. And actually, I think having that CR will make reviewing and committing this simpler :)

6. +++ b/tests/src/Functional/ResourceResponseTestTrait.php
   @@ -119,13 +138,18 @@ trait ResourceResponseTestTrait {
   +          sprintf('jsonapi.%s.%s.related', $entity->getEntityTypeId() . '--' . $entity->bundle(), $public_field_name),
   
   +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1641,7 +1608,11 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +        sprintf('jsonapi.%s.%s.relationship', $entity->getEntityTypeId() . '--' . $entity->bundle(), $relationship_field_name),
   

   Use static::$resourceTypeName. See \Drupal\Tests\jsonapi\Functional\ResourceTestBase::testGetIndividual() for an example.

7. +++ b/tests/src/Functional/ResourceResponseTestTrait.php
   @@ -463,15 +486,13 @@ trait ResourceResponseTestTrait {
   -      // @todo uncomment in https://www.drupal.org/project/jsonapi/issues/2943176
   -      /* 'id' => '/' . $resource_identifier['type'] . '/' . $resource_identifier['id'], */
        ];
   -    if (!is_null($id)) {
   -      $error['id'] = $id;
   -    }
        if ($pointer || $pointer !== FALSE && $relationship_field_name) {
   

   👏

8. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -729,35 +728,14 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -    // @todo remove this in https://www.drupal.org/project/jsonapi/issues/2943176
   -    $strip_error_identifiers = function (&$document) {
   -      if (isset($document['errors'])) {
   -        foreach ($document['errors'] as &$error) {
   -          unset($error['id']);
   -        }
   -      }
   -      if (isset($document['meta']['errors'])) {
   -        foreach ($document['meta']['errors'] as &$error) {
   -          unset($error['id']);
   -        }
   -      }
   -    };
   -    $strip_error_identifiers($expected_document);
   
   @@ -1156,12 +1140,6 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -    // @todo remove this loop in https://www.drupal.org/project/jsonapi/issues/2853066.
   -    if (!empty($expected_document['meta']['errors'])) {
   -      foreach ($expected_document['meta']['errors'] as $index => $error) {
   -        $expected_document['meta']['errors'][$index]['source']['pointer'] = '/data';
   -      }
   -    }
   
   @@ -1188,12 +1166,6 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -    // @todo remove this loop in https://www.drupal.org/project/jsonapi/issues/2853066.
   -    if (!empty($expected_document['meta']['errors'])) {
   -      foreach ($expected_document['meta']['errors'] as $index => $error) {
   -        $expected_document['meta']['errors'][$index]['source']['pointer'] = '/data';
   -      }
   -    }
   
   @@ -1224,12 +1196,6 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -    // @todo remove this loop in https://www.drupal.org/project/jsonapi/issues/2853066.
   -    if (!empty($merged_document['meta']['errors'])) {
   -      foreach ($merged_document['meta']['errors'] as $index => $error) {
   -        $merged_document['meta']['errors'][$index]['source']['pointer'] = '/data';
   -      }
   -    }
   
   @@ -1837,14 +1796,6 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -    if (!empty($resource_document['meta']['errors'])) {
   -      foreach ($resource_document['meta']['errors'] as $error) {
   -        // @todo remove this when inaccessible relationships are able to raise errors in https://www.drupal.org/project/jsonapi/issues/2956084.
   -        if (strpos($error['detail'], 'The current user is not allowed to view this relationship.') !== 0) {
   -          $expected_document['meta']['errors'][] = $error;
   -        }
   -      }
   -    }
   

   👏

9. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1747,24 +1718,12 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -        $related_response = (new ResourceResponse([
   -          'jsonapi' => static::$jsonApiMember,
   -          'errors' => [
   -            [
   -              'status' => 403,
   -              'title' => 'Forbidden',
   -              'detail' => $detail,
   -              'links' => [
   -                'info' => HttpExceptionNormalizer::getInfoUrl(403),
   -              ],
   -              'code' => 0,
   -              'id' => '/' . $base_resource_identifier['type'] . '/' . $base_resource_identifier['id'],
   -            ],
   -          ],
   -        ], 403))->addCacheableDependency($access);
   ...
   +        $related_response = static::getAccessDeniedResponse($entity, $access, $via_link, $relationship_field_name, $detail, FALSE);
   

   👏

10. +++ b/tests/src/Functional/ResourceTestBase.php
    @@ -1747,24 +1718,12 @@ abstract class ResourceTestBase extends BrowserTestBase {
    +        $id = '/' . $base_resource_identifier['type'] . '/' . $base_resource_identifier['id'];
    

    Nit: this is dead code, can be removed 🤘

1. I agree that it is not pretty 😞. But we need to do it or else this will cause a fatal exception if JSON API Extras has overridden the type name. It'll cause the route name to not be found.
2. This is there because I wanted to get the tests right, release a 2.x-beta ASAP, and fix the _actual_ code ASAP. Better to get a beta out now than let the perfect be the enemy of the good, so to speak.
3. OHHHH, you mean I should see the forest through the trees? Excellent remark. Fixed.
4. Thank you!
5. Will do.
6. Done.
7-9. :)
10. Done!

Gah, that's was silly.

#81

1. Ah, I did not think of that. Of course!
2. That's fair. WFM.
3. Yep :)

AFAICT you just missed a single spot:

+++ b/tests/src/Functional/ResourceResponseTestTrait.php
@@ -119,13 +138,18 @@ trait ResourceResponseTestTrait {
+          sprintf('jsonapi.%s.%s.related', $entity->getEntityTypeId() . '--' . $entity->bundle(), $public_field_name),

static::$resourceTypeName

+++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
@@ -261,4 +267,40 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
+   * @todo: remove this after refactoring internal error handling. This is a temporary measure to stabilize the 2.x HTTP API for errors.

Would you mind already creating a follow-up minor/normal task for this? I want to make sure we don't forget.

Done.

👌

+++ b/tests/src/Functional/ResourceResponseTestTrait.php
@@ -144,7 +144,7 @@ trait ResourceResponseTestTrait {
-          sprintf('jsonapi.%s.%s.related', $entity->getEntityTypeId() . '--' . $entity->bundle(), $public_field_name),
+          sprintf('jsonapi.%s.%s.related', static::$resourceTypeName, $public_field_name),

I asked for this change, but it was wrong, because this method (\Drupal\Tests\jsonapi\Functional\ResourceResponseTestTrait::getExpectedIncludedResourceResponse() does:

$entity = $entity->{$field_name}->entity;

i.e. it digs deeper into the include path, following it down to the next level, until there is no next level. After following the first level relationship, using static::$resourceTypeName is therefore using the wrong resource type name!

Fixing on commit.

Given that @e0ipso has given his +1 multiple times in the issue and actively contributed to changes in the patch in chat and in this issue, going ahead and committing this!

This still needs a change record though!

I made many.

Writing them made me feel very good about what was accomplished :)

Writing them made me feel very good about what was accomplished :)

That's exactly why I asked you to write them, because you did the bulk of the work here. I also find that writing CRs is therapeutic — it gives closure and a sense of accomplishment!

5 days after we finished this, somebody filed an issue to complain about how it used to work: #2995522: Omit meta errors from output 🙏👌😁