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"

This is much cleaner and makes more sense.

+++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -321,7 +321,7 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
-      throw new EntityAccessDeniedHttpException(NULL, AccessResult::forbidden(), '/data/id', 'IDs should be properly generated and formatted UUIDs as described in RFC 4122.');
+      throw new EntityAccessDeniedHttpException(AccessResult::forbidden(), '/data/id', 'IDs should be properly generated and formatted UUIDs as described in RFC 4122.');

:)

In a collection resource, if you have multiple errored entities, how would you know that a given error applies to a given resource?

@e0ipso, can't we use the links, source and/or meta keys to identify it, where necessary?

http://jsonapi.org/format/#error-objects

Gabe I'm OK with that. Links seem to be the best way to identify a resource entity, but I don't have an opinion on that.

However I do not think we should just lose the functionality. 👍 or 👎?

Wow @Wim Leers, I really like #13.3 and I'm jealous I never thought of it lol

This would makes so much more sense for things like pagination too.

All of those options seem interesting. I'd like 3️⃣ slightly better, but all of them will work.

JSON Pointers are cool. JSON Path is great! :-P However in this case JSON pointer is more appropriate.

I've created a dependent issue to start including resource identifiers instead of simply omitting items when access is denied: #2944348: Provide a resource identifier instead of omitting the resource when access is denied

That will allow us to include a source.pointer in error objects as suggested by #13.3

Is there extra work that needs to happen here when #2944348: Provide a resource identifier instead of omitting the resource when access is denied lands? If not we can simply mark this one as fixed instead of postponed.

@e0ipso, the other issue was meant to only add resource identifiers instead of omitting them from the response. After that was finished, I planned on using this issue for changing the pointers.

However, after actually working on this, these two things (removing resources and generating errors) are pretty inextricably linked, so doing it in two issues doesn't make sense any longer. I'm going to close #2944348 in just a second.

The attached patch is the smallest possible set of changes that I felt I could introduce to make this work, but it does feel a bit hacked in. That said, doing anything else would probably mean a much larger patch and our efforts can probably be better used elsewhere.

Without further ado, "it works on my machine" so let's see what the testbot says :P

One clarification: #27.4 is not just a refactoring. It previously would have failed because the attributes key wouldn't be defined on the resource identifier array. Since array_column just casts out arrays that don't have the column key, it avoids all the isset() checking one would otherwise have to do :)

You can make a really neat (albeit naive) nested value extractor with that idea:

$path = 'some.deep.path.to.value';
$deep_values = array_reduce(explode('.', $path), 'array_column', $nested_array);

kinda nifty, no?

Your comment made me think of lodash. I was terrified to find out about https://lodash-php.com.

#28.1

This change actually has little to do with pointers. This change needed to happen because entity access exceptions replace entities in an entity collection when they are loaded and checked for access. Because they replace the entities, we have to preserve identity information on the exception itself so that we can later normalize a resource identifier in the collection (previously, the exceptions were just filtered out). There's no other case where an exception needs to generate a resource identifier.

#28.2

That's correct.

+++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
@@ -114,11 +114,14 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
+      if ($normalizer_value instanceof EntityAccessDeniedHttpExceptionNormalizerValue) {
...
-        $rasterized['meta']['errors'] = array_merge($previous_errors, $normalizer_value->rasterizeValue());
+        $rasterized_value = $normalizer_value->rasterizeValue();
...
+        $rasterized['meta']['errors'] = array_merge($previous_errors, $rasterized_value);
+        $rasterized['data'][] = $normalizer_value->rasterizeResourceIdentifier();

Re: 28.1, you can see here that we previously didn't add a value to data when we encountered an exception.

Now, we need to rasterize the exception into a resource identifier, in addition to rasterizing the error.

Soo... I was initially very gung-ho about this in #14. However, after working on this and writing extensive pagination and filtering docs, I'm no longer as excited about this as I once was. I am no longer am convinced that we should include a resource identifier for inaccessible resources.

Why? It relies on meta.errors to indicate that a resource is not accessible by the current user. meta.errors is a non-standard feature of our implementation. We've tried to formalize it somewhat, but the fact remains that it is not part of the base spec. By including a resource identifier, there is no Drupal-agnostic way for a client to differentiate between a resource which was "forbidden" and a resource which simply doesn't have attributes/relationships.

I was also excited about this making pagination simpler, because page[limit] would then effectively operate as page[size] (meaning it would never have fewer than the limit number of resources when additional pages exist. I thought this would simplify client implementations, but I no longer thing that's true. While this would make it easier to reason about pagination, it wouldn't simplify client implementations because we're pushing the burden of removing resources from the UI into the client's domain... and their only way to do that will be to inspect meta.errors (which would mean they have to be "Drupal-aware") or do some heuristic based on the presence/absence of fields.

Finally, while I'm not sure this is technically a BC break, I think it will certainly cause unexpected behavior for many clients which have been relying on the backend to remove inaccessible resources from the data member of the document. This will be likely to cause JS errors wherever clients have made reasonable (yet optimistic) assumptions, like "every resource under data will have a attributes.title key".

So, what next?

I think we need to reevaluate adding individual errors for inaccessible resources altogether. If we're concerned about DX, then what we should be most concerned with is reducing the signal to noise ratio and providing actionable information. Individual errors do not satisfy that need. Instead, I propose that we provide a single error when any resource is omitted from the response:

{
  "meta": {
    "errors": {
      "title": "Insufficient Privileges",
      "detail": "One or more resources have been omitted from the primary data of this document due to insufficient privileges.",
      "links": {
        "about": "https://www.drupal.org/docs/8/modules/json-api/filtering#filters-access-control"
      },
      "source": {
        "pointer": "/data"
      }
    }
  }
}

I think this (1) reduces the signal to noise ratio, (2) is actionable, (3) is useful even when inactionable (e.g. providing a UI indication that items have been removed, even when they can't be filtered out).

I'm completely open to changing the langauge/contents of the error object. I'll point out that I chose "Insufficient Privileges" to distinguish it from "403 Access Denied" and I would be (reluctantly) open to including a meta object in the error object itself with links to the omitted resources. Like so:

{
  "title": "Insufficient Privileges",
  "...",
  "meta": {
    links: {
      "/user--user/550e8400-e29b-41d4-a716-446655440000": "http://example.com/jsonapi/user--user/550e8400-e29b-41d4-a716-446655440000",
    }
  }
}

^{Why not include those links in the parent error object? Because the spec isn't clear about whether we can or can't add additional members to the error object's link object."}

However, I think it really doesn't serve a very useful purpose. I'm open to being convinced otherwise though.

My strong feelings regarding the (brilliant) argumentation @gabesullice made are:

1. the spec may not have an opnion / contract on this matter, but Partial Success does. All consumers interacting with the Drupal-flavored JSON API servers have to be aware of its extensions, and respect them as they respect the base spec. With that, we have successfully extended the spec to cover this gap that @Wim Leers has pointed. The only missing part is that we should be forcing our clients to use Accept: application/vnd.api+json; extensions="fancy-filters,partial-success", instead of Accept: application/vnd.api+json, to formalize that awareness.
2. I don't believe that we should hide available (useful) information from the consumer's developer. The digested error message doesn't work for me. I would really rather revert to the wonky, but somewhat clear behavior we had before. If "pointer" doesn't work for us, we can find a way to add a JSON Pointer to the appropriate entity. Maybe: meta.externalPointer -> https://example.org/jsonapi/node/article/1234-abcd#/data/attributes/title. Whatever we do, we add it to the Partial Success extension and it becomes as formal as if it was in the base spec.

If we want to have the digested error added on top of that, and then detailed errors hidden from the the average user via a permission; I don't feel strongly about that.

To re-iterate, this is the only wording preventing us from having a pointer to an external JSON document in the source.pointer:

I wouldn't oppose being practical and having a JSON pointer that is not restricted to the requested document. Even if that is not 100% in line with the base spec.

I think I see where the mental leap is that I failed to state explicitly. I think the single greatest distinction in my mind vs. your minds is now this:

An entity removed from a collection for access reasons is not an error, nor is it exceptional. It is expected behavior.

Everything else follows from that.

How do we know this is true? Let me show it by example:

If a node is not present in a query result because of node grants, Drupal doesn't throw an exception, it silently removes the result. It's not an error. It's expected behavior.

In the PHP API you may override that behavior, but it's still the default. Since this is a public API, we cannot override it, but that doesn't change the principle.

I appreciate that, but the JSON API spec version 1.0 does not deal with partial success at all.

If you join me in my thinking above, then "partial success" is not relevant because nothing failed. There's no need to even concern ourselves with extensions and other whatnot in that regard.

I don't believe that we should hide available (useful) information from the consumer's developer.

Neither do I! That is why I explicitly said:

"I would be (reluctantly) open to including a meta object in the [digested] error object itself with links to the omitted resources."

And I even included an example of how we might do that :) If clients wish to know the reason that access was denied, they may follow the link, where they'll receive a status, reason, and detailed information.

An entity removed from a collection for access reasons is not an error, nor is it exceptional. It is expected behavior.

Everything else follows from that.

We don't treat entities differently depending on if they are in a collection or individual resources. This suggests that we do here. If an individual request to an entity fails with an error, then I think that the same operation as part of a collection should also be seen as one. I believe (by the accumulation of reports in that sense) that many assume the same, and it would be very confusing to treat it as success or error depending if it's a collection or not.

I do believe that things have partially failed if you request a collection where some entities fail.

Also, I remember when researching this in the past that all the questions and approaches in this matter (in the forums) acknowledged the existence of an error of an error.

We don't treat entities differently depending on if they are in a collection or individual resources... If an individual request to an entity fails with an error, then I think that the same operation as part of a collection should also be seen as one.

We respond with a 403 error for an inaccessible individual resource. 400-level errors are client errors. The error is accessing the individual resource (resource as in route, not as in entity) with insufficient authorization. IOW, the entity did not fail, the authorization did.

In #2853066-13: Spec Compliance: Inaccessible collection/related resources surface errors: should be 200 with hypermedia + metadata, we agreed that even a collection with 0 results (for access reasons) is a 200. Access to the "collection resource" is a success (200).

In a collection with omitted entities, I think it's correct to return a single "Insufficient Privileges" error that mirrors the client error at the individual resource level, with extra metadata about which resources would be viewable if those additional privileges were attached to the request. That's consistent with the consensus we reached (reflected in the IS update at #2853066-18) for the 2.x branch. This complements the idea that you might want to "correct" your request by adding a ?filter[status]=1 filter to your query, since it was a client error, not an entity error.

Given that #2853066, deals with a superset of the issue here and that we're trying to freeze the 1.x branch and release 2.x very soon, I propose we simply close this issue as "works as designed" for the 1.x branch, ignoring any spec violation WRT to the error ID in 1.x. It'll be a non-issue in 2.x.

Moving to Drupal core's issue queue.

I'm working on https://www.drupal.org/project/drupal/issues/3122113

Hmm, if this works as designed, we might need to move along and try and execute: /Users/bjorn/projects/drupal/core/modules/jsonapi/tests/src/Functional/NodeTest.php:341. Which is a todo to remove some code when this issue is closed. Opening a child issue.

#3449891: Move to new test path in NodeTest as per todo.