Spec Compliance: JSON API's profile/extention (Fancy Filters, Drupal sorting, Drupal pagination, relationship arity) needs to be explicitly communicated

This is the main reason why we are not requiring the extension key in the Accept request header, nor responding with the extensions in use in the Content-Type response header.

Well, the spec also does not allow us to specify extensions in the request/response headers anyway, because an extension mechanism has not yet been specified!

I'm open to call them something different to extension given that 1) the extension system is in the limbo in JSON API spec and 2) they are cornerstones of our implementation and are non-negotiable.

Why haven't I thought of this? :D 👌

Oops, cross-posted with #6.

+1 to keeping them for core inclusion.

Agreed that they're important to keep for the wonderful JSON API DX.

Let's not tie ourselves to a specification WRT to surfacing error, I think we need to test and solidify that a bit more.

I won't say no to better test coverage for this either. What parts of it do you feel are not solid enough yet? My impression is that the majority of it is already stable?

I do think Fancy Filters are an extension that should be negotiated between client/server.

This is honestly the bit I'm torn on the most. On the one hand I agree, because Partial Success merely uses meta, which is specifically for application-specific implementations, whereas Fancy Filters is much more fundamental.

On the other hand, the JSON API spec — despite being quite great — is utter garbage when it comes to filtering. http://jsonapi.org/format/#fetching-filtering says this:

Filtering

The filter query parameter is reserved for filtering data. Servers and clients
SHOULD use this key for filtering operations.

Note: JSON API is agnostic about the strategies supported by a server. The
filter query parameter can be used as the basis for any number of filtering
strategies.

That's literally all the JSON API spec has to say about it. Obviously this makes it impossible to have multiple interoperable implementations!

This is where I'm torn: every JSON API implementation must come up with its own decisions/design/convention/"extension" for filtering, because otherwise it's impossible to do any filtering.

Given this, the best we can do AFAICT is:

1. Document these two non-negotiable extensions explicitly in jsonapi.api.php and explain the rationale.
2. Explicitly link them in every JSON API response:
   

   "jsonapi": {
     "version": "1.0",
     "meta": {
       "links": {
         "self": "http://jsonapi.org/format/1.0/",
         "partial_success": "https://gist.github.com/e0ipso/732712c3e573a6af1d83b25b9f0269c8",
         "fancy_filters": "https://gist.github.com/e0ipso/efcc4e96ca2aed58e32948e4f70c2460"
       } 
     }
   },
   

Thoughts?

Discussion is still ongoing in https://github.com/json-api/json-api/issues/1207. Hopefully that will reach consensus soon, then we won't be forced to invent our own approach anymore.

Do we want to set some sort of timeout to wait for consensus?

Good idea. Shall we say April 15, i.e. just after DrupalCon?

I still think that we must have fancy filters by default

From a DX/consumer developer POV I wholeheartedly agree. Fancy filters are a super powerful feature.

Just cross-posted this date to https://github.com/json-api/json-api/issues/1207#issuecomment-378654093.

Earlier today, I posted a review of the actual proposal/PR: https://github.com/json-api/json-api/pull/1268#issuecomment-394334144. AFAICT details still need to be refined, but in broad strokes, I think the proposal A) works, B) can already be implemented.

Taking this on.

Per:

• From my perspective, partial success seems like something we can do conventionally without negotiating it as an extension because we're free to put information in the meta member at will.

• This is why I want to be really careful about how we treat partial success. Let's not tie ourselves to a specification WRT to surfacing errors, I think we need to test and solidify that a bit more. I think partial success can be a "convention" for now.

I am reducing the scope to only Fancy Filters.

This was simpler than I thought.

This will still need tests to verify that if a request lists profiles that are not supported result in a 400.

Next:

1. Update the response documents per https://github.com/json-api/json-api/pull/1268.
2. Aforementioned test coverage.

Although, don't we also need to have profiles for our sorting and pagination? This is what @e0ipso raised in #2812923-2: [FEATURE] Implement extension system, 1.5 year ago.

But http://jsonapi.org/format/#fetching-sorting is actually sufficiently prescriptive for basic usage, and hence is actually usable. The same is true for http://jsonapi.org/format/#fetching-pagination. But http://jsonapi.org/format/#fetching-filtering is empty; it doesn't say anything except that ?filter should be used for sorting; that's it!

So … I'm a bit torn.

I think the right thing to do here is to indeed have a profile for sorting and pagination too.

Turning this Plan into a Task. This is where all the discussion happened. Closing #2812923: [FEATURE] Implement extension system as a duplicate.

#19 had some unexpected failures, because I didn't think to think certain tests locally. Fixing.

Test coverage:

This will still need tests to verify that if a request lists profiles that are not supported result in a 400.

Note that this is a stricter reading of the spec. But that's fine. It makes things simpler for future maintenance/BC.

This turned out to be extraordinarily difficult because \Symfony\Component\HttpFoundation\AcceptHeader::fromString() is broken; it parses Accept: text/html;charset="UTF-8" as two separate media types… And the same for $acceptable_profiles = AcceptHeader::fromString('application/vnd.api+json;profile="https://www.drupal.org/jsonapi/profile/fancy_filters"'): this results in a application/vnd.api+json and a https://www.drupal.org/jsonapi/profile/fancy_filters item … 😭

https://github.com/symfony/symfony/commit/6b601bd9a639ca141ebfcc3b6b91c2... created the current logic, and its test coverage leaves much to be desired. It looks like Symfony recently improved this in Symfony 4: https://github.com/symfony/symfony/commit/b435e80cae24e85d6e0e923ae932b2... (for https://github.com/symfony/symfony/pull/24699).

So, either way, it looks very unlikely that this will get fixed in the Symfony version that Drupal depends upon. This sadly means we'll need our own parsing. Fortunately, we can keep this to a minimum :)

#23:

I think I disagree about a profile for sort and I certainly disagree about a profile for page.

[…]

This is exactly the doubt I was having.

As you mentioned, the spec is already sufficient to describe our use of sort, the only place where we've extended beyond the normative language of the spec is to follow the recommendation of using full-stop delimited field names for relationships (this is the only thing that is extension-worthy).

That's what I thought, until I looked at https://www.drupal.org/docs/8/modules/json-api/collections-and-sorting … we apparently support not only a path/pointer to sort and a direction, but also a language (langcode, see \Drupal\jsonapi\Query\Sort::LANGUAGE_KEY).

As for pagination, I think the pagination language of the spec is very intentionally silent on filtering strategies and enforces the use of HATEOAS links for communicating pagination links.

Mostly agreed.
I disagree that it's only HATEOAS. Because our pagination strategy assigns a particular meaning to page[offset] and page[limit]. That's something a client may very well rely upon. Arguably, page[offset] is not an API, but page[limit] is. Even though page[offset] is only mentioned in the sample response document, that is still the only way a client can ensure they get 2 or 100 responses. Of course, if we say that the client is not allowed to specify a limit, aka that's not a supported API, then sure … but if that were the case, we should also have hardcoded page[limit].

#25:

I'm not sure that this is the right application of the profile media type parameter in this context.

You may be correct in a purely theoretical perspective. But there's only one explicit mention of that in https://github.com/json-api/json-api/pull/1268's current PR:

+* `profile`: an array of [links][link], each giving the URI of a
+  [profile][profiles] in use in the document.

Fancy filtering changes nothing about the response document.

1. Correct, because the structure of response documents stays identical.
2. Incorrect, because the contents of response documents changes.

For example: /jsonapi/user/user vs /jsonapi/user/user?filter[name]=Wim have vastly different content.

So, if I'm reading the PR correctly, we should be advertising the fancy filter extension via the jsonapi.profiles document member and requiring a client to specify the profile in the query parameter when filtering, but not providing it in every response header.

This makes for fairly large variations in Content-Type response header. When for example requesting individual resources, there is indeed little point in listing the Fancy Filters profile in the media type's profile parameter. But there's also no harm. And listing it always is simpler. Both in our implementation and our test coverage. But perhaps most importantly, it's also simpler when reverse proxies are involved; rather than needing to know the many possible profiles, they can just hardcode a particular Content-Type, because the Drupal JSON API module will always respond in the same way.
And when thinking of the future, when the JSON API module gains support for additional profiles, old clients would continue to request the current set of profiles. Therefore caching the different variations becomes simpler.

Additionally, it's totally feasible for JSON API response documents to start to contain more links (cfr. HATEOAS); when Fancy Filters is supported, there are various links one could imagine (e.g. on a user--user resource, links to filtered collections of all other resources for which the current user is the owner, e.g. {"links": {"owned-articles": "https://example.com/node/article?filter[uid]=18a503f7-5900-457b-a40c-49d4a3136e09).

+++ b/src/StackMiddleware/FormatSetter.php
@@ -36,6 +42,30 @@ class FormatSetter implements HttpKernelInterface {
+        $unsupported_profiles = static::getUnsupportedProfiles($request);
+        if (!empty($unsupported_profiles)) {

@@ -62,11 +92,74 @@ class FormatSetter implements HttpKernelInterface {
+  protected static function getUnsupportedProfiles(Request $request) {

+++ b/tests/src/Functional/ResourceTestBase.php
@@ -879,6 +879,25 @@ abstract class ResourceTestBase extends BrowserTestBase {
+    // DX: 400 when requesting unsupported profiles.
+    $request_options[RequestOptions::HEADERS]['Accept'] = 'application/vnd.api+json;profile="https://example.com http://foobar.com';

This is only testing the simple case of accepting only a media type with unsupported profiles. We should also test two other cases:

1. some unsupported profiles
2. two Accept values; one with unsupported profiles, one without profiles — which should then result in a 200

Addressed #29.

And finally, the updates to the jsonapi top-level document member! Lots of test expectations had to be updated, hence the size of this interdiff.

Unfortunately there is something super weird going on with:

• CommentTest
• EntityTestTest
• MediaTest
• TermTest

For some reason, they get all the jsonapi.links.profile links twice! I have not been able to figure out how this happens. It'd be great if somebody else could take a stab at this.

Discussed with @gabesullice, and we agreed this belongs in 2.x.

I'll continue working on this on Monday; this will be my top priority.

😅

I think #2864680: Spec Compliance: JSON API's schema disallows duplicate resource identifiers. EntityReferenceItems which reference the same entity must have an "arity" also amounts to a profile: relationship_arity.

This patch should fix the failures in tests other than the 4 mentioned in #31.

#33:

1. I think (1) is the proper understanding of how a profile alters the document. A profile is about "assigning meaning" to the structure/syntax of the document and allowed query params, not about 0, 1 or 2 items being returned.

   I totally get where your'e coming from. But

   That's not quite true either, because https://github.com/json-api/json-api/pull/1268 says

   Profiles might describe how the filter or page params are used.

   and its commit says:

    +A profile **MAY** assign meaning to particular elements of the document
    +structure, such as resource attributes or members of meta objects, and even
    +to query parameters whose behavior is left up to each implementation, such 
    +as `filter` and all those that contain a non a-z character.
   

   So a profile can cover an aspect of a JSON API implementation that results in no alterations to JSON API documents whatsoever.

2. Consider this language:
   […]
   It's the "to indicate that the profile has been applied" language that makes me think that we just want to add that media type profile when ?profile=https://www.drupal.org/jsonapi/profile/fancy_filters is in the request URL.

   A. I think it's good that you're reading it as strictly as possible.
   B. See above; some profiles don't have anything in the document, so following this logic they'd never appear in the Content-Type response hader!
   C. The spec says MUST in case the document is being altered compared to the base spec. The spec does not say MUST NOT when the document is not being altered. And this is IMHO consciously: it allows a JSON API implementation to KISS, by always sending the same Content-Type response header if it does not have any optional/negotiable JSON API profiles.
   D. Certainly having the profile query parameter there is not a hard requirement to list profiles in the Content-Type response header. That query parameter can be used to require a profile to be applied. But a profile can also be requested using Accept. It's even allowed to respond to an Accept: application/vnd.api+json request with a Content-Type: application/vnd.api+json; profile="https://www.drupal.org/jsonapi/profile/fancy_filters https://www.drupal.org/jsonapi/profile/page_limit https://www.drupal.org/jsonapi/profile/sort_language" response; it's not required that a JSON API server makes every profile optional.

   That's why I added the following to jsonapi.api.php:

    * @section profiles Profiles
    *
    * The JSON API module for Drupal implements 3 profiles that extend the base
    * spec, without altering the basic semantics. All 3 are required — this means
    * none of them are optional or negotionable. We may add negotionable profiles
    * in the future.
   

   All of this is supported by the spec:

    +Servers **MAY** add profiles to a JSON API document even if the client has not
    +requested them. The recipient of a document **MUST** ignore any profile extensions 
    +in that document that it does not understand. The only exception to this is profiles
    +whose support is required using the `profile` query parameter, as described below.
   

   The combination of these choices allows our implementation to remain simple and maintainable. If everything becomes negotiable, then that is an enormous burden on us maintainers. We can still choose to support negotiable profiles in the future.

3. Fancy filtering changes nothing about the response document. So, if I'm reading the PR correctly, we should be advertising the fancy filter extension via the jsonapi.profiles document member and requiring a client to specify the profile in the query parameter when filtering, but not providing it in every response header.

   But perhaps most importantly, it's also simpler when reverse proxies are involved; rather than needing to know the many possible profiles, they can just hardcode a particular Content-Type, because the Drupal JSON API module will always respond in the same way.

   I'm not sure I fully understand this use case, can you expand on that point? (I'll admit I'm having a little bit of a pavlovian response to this line of reasoning, it's eerily similar to the ?_format reasoning that's been a big pain point for us. I'll try to overcome that! :P)

   I interpreted your original remark as also meaning that when ?profile=https%3A%2F%2Fwww.drupal.org%2Fjsonapi%2Fprofile%2Ffancy_filters is missing, we also should omit that profile from the Content-Type response header. And I thought you meant that this should be true for every profile. Add to that the fact that you can also request a profile to be applied using the Accept header, and then you reach the point where there can be several variations of responses to the same URL, each with different combinations of profiles applied. Not only is this more complex, it also makes caching in a reverse proxy super hard, because that requires reverse proxies to normalize the values of the Accept request header. To be able to normalize them, the reverse proxy needs to know about all the values that the application cares about. This is brittle and a nightmare to support.

   But now I suspect you were not at all suggesting to respect Accept header based JSON API profile negotiation. You're only suggestion profile query parameter based JSON API profile negotiation. Is that correct?

   And I evensuspect that you meant this should only be applied to the Fancy Filters profile, because that is what defines our interpretation of the filter query parameter, and if that profile is not specified, then theoretically speaking our JSON API implementation is not supposed to know how to interpret it. Is that correct? (To this I say: it's fine to not specify this in the query parameter; it's fine for clients to know that this is the default filtering mechanism. That'd also prevent a pretty significant BC nightmare when updating to 2.x.)

The last one was the trickiest; JsonApiDocumentTopLevelNormalizer had to be modified in just the right spot.

Fixed CS violations.

Still to be done:

1. validating request document's link profiles (if any):
   

    +Except for the `profile` key, each key present in a links object **MUST** have
    +a single link as its value. The `profile` key, if present, **MUST** hold an
    +array of links.
   

2. validating request document's profiles (if any):
   

    +* `profiles` - an object with profile URIs as keys and corresponding
    +  [profile descriptors](profile-descriptors) as values.

3. validating the profile query parameter:
   

    +A client **MAY** use the `profile` query parameter to _require_ the server to
    +apply one or more profiles when processing the request. The value of the `profile` 
    +query parameter **MUST** equal a URI-encoded whitespace-separated list of profile URIs.
    +
    +If a server receives a request requiring the application of a profile or
    +combination of profiles that it can not apply, it **MUST** respond with a `400
    +Bad Request` status code.

Implemented all three, but test coverage is still necessary. I'd first like feedback on the general approach.

I believe #44 implements 100% of https://github.com/json-api/json-api/pull/1268 😀 🎉 🤞

Looking forward to feedback!

@gabesullice and I discussed this in detail last week, together with core committers @xjm, @webchick, @effulgentsia and @lauriii.

Until https://github.com/json-api/json-api/pull/1268 lands and JSON API spec version 1.1 is released, the JSON API module for Drupal would not possibly be able to implement this according to the spec, since the spec hasn't been finalized yet. This might be okay for a contrib module, but imagine what the consequences are for when JSON API is added to Drupal core: we'd end up with potentially our own fork of the spec. This is too big a risk.

It's better for now to let JSON API to work as it does: its primary addition is "fancy filters", and the spec already gives us the leeway to implement this as we see fit: http://jsonapi.org/format/#fetching-filtering.

Therefore, postponing this. Also removing this from #2931785: The path for JSON API to core.

JSON API maintainers just committed profile support to v1.1
https://github.com/json-api/json-api/commit/55cd721e55d4facec6e3242f2724...! See it at http://jsonapi.org/format/upcoming/#profiles.

The 1.1 version of the JSON API spec has not yet been released though. But this does mean this is up for debate again.

What do people think? Should we do this in 2.0? I'm inclined to say "yes" — I think the probability of this still changing is very low. Of course, we can only be 100% certain if this actually ships in a new JSON API spec version.

Discussed with @gabesullice. He pointed out that we already postponed this to a 2.x minor release because it wouldn't break BC anyway. That's still true today.

Plus:

Until https://github.com/json-api/json-api/pull/1268 lands and JSON API spec version 1.1 is released, the JSON API module for Drupal would not possibly be able to implement this according to the spec, since the spec hasn't been finalized yet.

1.1 is still not released, so there's still a risk.

Hence, marking this postponed again, until there's an actual JSON API spec update released.

As a parallel, we began the HTML5 initiative in Drupal 8 core in 2011, even though the spec wasn't finalized as a W3C Recommendation until late 2014.

The difference is that HTML is declarative, and browsers interpret it. It won't require code changes on specific projects. Or, if browsers change, then everybody has to.
With JSON API, clients would need to be modified.

That, plus, Drupal 8 was still very far from being released, so any changes that would still need to happen would not impact actual production sites!

Quoting https://github.com/json-api/json-api/issues/600#issuecomment-429380085:

@wimleers It's gonna happen very soon. I'm just working on a PR for profile registration, and I think #1304, #1302 and #1199 should get in too. The way I think of beta, though, is that it's like TC39's Stage 3. I.e., people should be trying to implement the new features once they get to beta stage (even if not shipping them yet), just to make sure that implementation isn't unexpectedly difficult before we finalize the designs for good.

Agreed. Now to find the time.

My upstream comment I linked to in #56 triggered a public announcement: https://github.com/json-api/json-api/issues/600#issuecomment-430763592 + https://twitter.com/jsonapi/status/1052647059982077953 :)

Time to work on this, to help validate the spec.

#3020136: Author and register a JSON:API profile for resource versioning is doing this for #2992833: Add a version negotiation to revisionable resource types! 🎉

I'd like to see us move this forward again. Let's get a patch here that is green and does add the necessary profile metadata to our responses.

Drupal will not adopt 1.1 until 1.1 is finalized; it's too big a risk for Drupal otherwise.

But we could and should have green patches proving that it's working; that should give sufficient confirmation to the spec maintainers?

On the other hand: since we're now very early in the Drupal 8.8 cycle, we could commit this and revert before 8.8.0 if version 1.1 of the spec is not finalized before the 8.8.0 beta (per https://www.drupal.org/core/roadmap, the week of October 14, 2019). Would that work better for the spec maintainers? Nonetheless, that would still mean no significant real-world use, so it wouldn't be very different from just having a green patch.

Sounds good! Thanks for chiming in @effulgentsia 🙏