#2948666: Remove JSON API's use of $context['cacheable_metadata'] had the side-effect/benefit of ensuring that ResourceResponses always contain a data value that normalizes to a JsonApiDocumentTopLevelValue object.

It does that with:

// Having just normalized the data, we can associate its cacheability with
// the response object.
assert($jsonapi_doc_object instanceof JsonApiDocumentTopLevelNormalizerValue);

This makes sense. JSON API always returns a JSON API document.

#2933062: [>=8.5.4] Spec Compliance: Return 400 for unrecognized/unsupported query parameters added validation of JSON API query parameters. Correctly, it attempts to respond with an errors JSON API document with a relevant hyperlink to the JSON API spec.

Unfortunately, it does this by returning a ResourceResponse whose $data value is a complete JSON API document in array form.

Why doesn't it return a JsonApiDocumentTopLevel object? That's what it's for right? You'd think! But...

Wah, wahh, it can't because that class can't handle JSON API error documents. It's even on the class comment:

 * @todo Add the missing required members: 'error' and 'meta' or document why not.
 * @todo Add support for the missing optional members: 'jsonapi', 'links' and 'included' or document why not.
 */
class JsonApiDocumentTopLevel {

So, what to do? Could we throw an exception? We could, but we don't actually have a generic exception class that's customizable. IOW, if we threw an BadRequestException, we couldn't add that helpful hyperlink to the JSON API spec.

One way to solve this would be to create that exception class.

OTOH, we could make it possible to create a JsonApiDocumentTopLevel object with errors from scratch and add that directly to the ResourceResponse object that the request validator returns.

I propose we do this second object. We'll need to add a value object to src/Normalizer/Value named ErrorObject to mirror the JSON API terminology. That object should implement CacheableDependencyInterface. From there, we would make it possible to construct a JsonApiDocumentTopLevel object with either an iterator of entities (EntityCollection) or an iterator of errors.... and never the twain shall meet.

I ofc assume @Wim Leers will find a three line solution instead :P so in that case, disregard!

(I really would like to get to a point where we're constructing documents out of value objects, rather than returning a mishmash of jsonapi and non-jsonapi non-value objects that normalize to a nest of value objects, this could be a small first-step in that direction).

CommentFileSizeAuthor
#9 2982478-9.patch684 byteswim leers
#4 2982478-4.patch3.74 KBwim leers

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

Priority: Major » Critical

"Critical" because this will unbreak HEAD.

gabesullice’s picture

Issue summary: View changes
wim leers’s picture

Status: Active » Needs review
StatusFileSize
new3.74 KB

Oops, #2977600-13: Spec Compliance: `_format` is a disallowed query parameter name should have been posted here:

Sooo, I'm not too pleased with how this interdiff looks. Not sure if we want to fix it like this for now and just have follow up to do it "right" later or if we want to make it easy to do "right" first.

Wasn't sure about this either, but I think I managed to find another solution without spending a lot of time on it. It:

  • Changes \Drupal\jsonapi\EventSubscriber\JsonApiRequestValidator::validateQueryParams() to not return a ResourceResponse (this was the only place creating a ResourceResponse with array as its data, probably wrongly so — my bad!), and instead return a HTTP exception.
  • HTTP exceptions are already handled by \Drupal\jsonapi\EventSubscriber\DefaultExceptionSubscriber::onException(), so we're basically done! There is only one caveat: that means we lose the errors.links.info = http://jsonapi.org/format/#query-parameters piece of information. But even that can be solved pretty elegantly.

This is even a net code reduction, and frankly is how JsonApiRequestValidator.php should've worked in the first place. It was only added yesterday (in #2933062: [>=8.5.4] Spec Compliance: Return 400 for unrecognized/unsupported query parameters). So it's definitely fine to refine it.

wim leers’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: +#2949807: Spec Compliance: Error responses are missing the `jsonapi` top-level member

OTOH, we could make it possible to create a JsonApiDocumentTopLevel object with errors from scratch and add that directly to the ResourceResponse object that the request validator returns.

I've proven that to be unnecessary above. Long-term, I agree that's what probably should happen. That's what we'll need for #2949807: Spec Compliance: Error responses are missing the `jsonapi` top-level member most likely.

I propose we do this second object. We'll need to add a value object to src/Normalizer/Value named ErrorObject to mirror the JSON API terminology. That object should implement CacheableDependencyInterface. From there, we would make it possible to construct a JsonApiDocumentTopLevel object with either an iterator of entities (EntityCollection) or an iterator of errors.... and never the twain shall meet.

Proven unnecessary.

I ofc assume @Wim Leers will find a three line solution instead :P so in that case, disregard!

Not quite 3 lines, but I did manage to make it a net negative diff :D

Patch is green, so self-RTBC'ing and committing to get HEAD to be green again!

wim leers’s picture

Title: JsonApiDocumentTopLevel cannot contain errors. » JsonApiRequestValidator creates ResourceResponse object with array instead of JsonApiDocumentTopLevelNormalizerValue

More accurate title, reflecting the root cause & solution,.

  • Wim Leers committed b34a449 on 8.x-2.x
    Issue #2982478 by Wim Leers, gabesullice: JsonApiRequestValidator...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed
wim leers’s picture

StatusFileSize
new684 bytes

This introduced 2 CS violations.

Status: Fixed » Closed (fixed)

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