Spec Compliance: Error responses are missing the `jsonapi` top-level member

I think #2 is ready for review?

Ah, no, it's blocked on Untangle HttpExceptionNormalizerValue from FieldNormalizerValue. probably. But that means #2 should be tested to prove that it fails (although it obviously will, so little point).

Changing to Needs work then.

Yes, I know. By "being ready for review", I meant it probably should be tested by testbot to prove that it's failing. I explained that it'll obviously fail, so little point in doing so.

The spec says in principle that the top-level jsonapi member is a MAY … but in practice I think it's a MUST in our implementation, because right now it's behaving inconsistently. This violates the spirit of the spec IMHO.

The scope of this issue just got reduced, because thanks to #2948666: Remove JSON API's use of $context['cacheable_metadata'], relationship responses now do have the jsonapi top-level member!

👍

Hi,

I am a new for this module. I have patched #15. I appreciate if you give me steps for testing?

Thanks,
Chandu

Thanks to the work I've been doing yesterday on #2984911: Remove access to the Request object in the normalization process, I had a better sense of how to approach this issue. Attached patch makes tests pass for Node. It's possible (likely) that more test assertions need to be updated.

FYI: the approach could be even simpler if we didn't have the concept of meta.errors: then JsonApiDocumentTopLevel could continue to receive a single set of values, and if they're all exceptions, then it'd result in a JSON API document with only the errors top-level member.

P.S.: I clearly ran into cacheability-related aspects here, in part helped by #2948666: Remove JSON API's use of $context['cacheable_metadata'], in part a next step to #2948666. This will certainly also help #2929428: [>=8.5] Convert "throw new *HttpException" into "throw new Cacheable*HttpException" where possible!

Tests were already taken care of by @gabesullice in #15!

Got one detail wrong 😡

The one detail was that the X-Drupal-Cache-Contexts header is being generated for error responses, but there's an empty set of cache contexts X-Drupal-Cache-Contexts: (which parses to ['']). That is what is causing most of the failures in #18, because our test coverage is creating a "merged" response, and in doing so is trying to create a merged response with $cache_contexts = [''], which of course is not a valid cache context, and hence the assertion fails…

This fixes that.

Note that this problem will cease to exist with #2929428: [>=8.5] Convert "throw new *HttpException" into "throw new Cacheable*HttpException" where possible, because then exceptions will have associated cache contexts, and hence error responses will also have associated cache contexts.

Fix some one-off failures.

The 4xx-response cache tag is now appearing on responses, and hence also on merged responses. But an individual response being 4xx doesn't cause the collection response to also be 4xx — the collection will still remain a 200 response! This fixes that.

Hah, #27 does the same as #2991389-4: Test coverage: relationship response cacheabiliity! We're converging on finding the same problems from different starting points! 😆

This should make most tests pass.

The problem is that 4xx responses used to be not cacheable and now they are. Hence testCollection()'s automatically generated "expected responses" (which are generated from responses to each individual resource expected to be in the collection) are now merging 4xx responses' cacheability too, which is resulting in these test failures.

Note: I'm quite hopeful that #2991389: Test coverage: relationship response cacheabiliity will land before this and will make this patch simpler!

Update UserTest::testQueryInvolvingRoles()'s expectations.

Updated MessageTest::testCollection()'s expectations.

Updated RestJsonApiUnsupported's expectations.

Ah, and here's the last bit! This should be green! 🙏🙏🙏🙏🙏🙏🙏

(Stupidly, I had indented this code many hours ago because it needed to be scrutinized again. It somehow ended up being added to one of the interdiffs … and hence I'd forgotten about it. Gah!)

#26: I hadn't forgotten about you/this comment! I just wanted to finish what I started before going onto nice-to-have things like this :)

With that one could make the case that the single argument that the function receives is its "primary data". That frees us from awkwardly tracking this boolean value and needing these mutually exclusive arguments.

I tried that — I explained this in #18:

FYI: the approach could be even simpler if we didn't have the concept of meta.errors: then JsonApiDocumentTopLevel could continue to receive a single set of values, and if they're all exceptions, then it'd result in a JSON API document with only the errors top-level member.

That attempt failed because it detected an "errors only" document by checking that all values in JsonApiDocumentTopLevel were HttpExceptionInterface instances. But doing that makes it impossible to e.g. have a collection response with empty data and only meta.errors (because all nodes are unpublished for example).

Wouldn't the same happen in your proposal?

It wasn't using an EntityCollection to carry errors, it was just a plain array of HttpExceptionInterface objects.

_{(That's not what the current docs for JsonApiDocumentTopLevel say, I know, but … PHP is not strictly typed and there's no assert(), so … yeah, plain arrays are already being passed in there in HEAD too!)}

I like what you said, so I gave it a try … and it works! :) I do slightly prefer your proposal/this patch because it gives us more strict types, rather than array inspection/manipulation logic. I think that as other parts of the logic become stricter typed/cleaned up, the approach introduced here will become gradually simpler to understand.

So: 👍 from me.

1. 🙂
2. Hah! I had it as an independent variable first. I chose this approach for its explicitness and immutability.
3. Correct, we don't put links on error documents. Plus, the links inside error objects (http://jsonapi.org/format/#error-objects) are all absolute w3.org URLs! So at least for now, this is correct.

Yay!

Since your RTBC though, both #2985426: Spec compliance: `related` routes should return 200, not 403, if field access is allowed but the related resources are forbidden and #2991389: Test coverage: relationship response cacheabiliity have been committed, both of which require changes to be made here.

First, let's rebase. Notable changes:

1. DefaultExceptionSubscriber now has additional deletions; lines that were only added in #2991389!
2. JsonApiDocumentTopLevelNormalizerValue still makes the same set of changes, but the existing code now looks a little different

I got #47.1 wrong! Fixed.

If this works, then one of my main reservations about this patch actually is already fully fixed, thanks to #2985426: Spec compliance: `related` routes should return 200, not 403, if field access is allowed but the related resources are forbidden! 😁 🍻

Yay, #49 passed on 8.5/8.6/8.7!

Fixing CS violations.

@gabesullice, please re-confirm RTBC.

🚢

Created change record: https://www.drupal.org/node/2991990.