Problem/Motivation

#3010432: Filtering by referenced entity requires ".uuid" to be specified in filter path expression added a BC break.

    $this->drupalGet('jsonapi/node/article', [
      'query' => [
        'filter[nid]' => $node->id(),
      ],
    ]);

This works but

    $this->drupalGet('jsonapi/entity_test/entity_test', [
      'query' => [
        'filter[id]' => $entity_test->id(),
      ],
    ]);

This doesn't

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

None

API changes

Hopefully, the BC break will be fixed.

Data model changes

None

CommentFileSizeAuthor
#2 screenshot-127.0.0.1-2018.11.30-20-33-31.png12.59 KBjibran

Comments

jibran created an issue. See original summary.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Title: Filtering using serial id doesn't work if the field name is 'id' » Filtering using serial ID doesn't work if the field name is 'id'
Issue summary: View changes
StatusFileSize
new screenshot-127.0.0.1-2018.11.30-20-33-31.png12.59 KB

Posting @Wim Leers reply from #3010432-81: Filtering by referenced entity requires ".uuid" to be specified in filter path expression:

“id” had been the UUID for years. Passing a serial ID instead indeed won’t work.

If you have a need for filtering by serial ID, there’s #3015759: `?filter[drupal_internal__id]=ID` does not work: drupal_internal__id should not be converted to uuid when filtering to make that work again.

First of all thanks for the reply.

Passing a serial ID instead indeed won’t work.

This is not a correct statement. Filtering by node serial ID works. The problem is we have the field with the name of 'id'.

It is a whole other discussion that why we feel the need to replace the id value to uuid at the root level. Let's not get into that.

I get the idea behind the fix as mention in the change notice JSON:API filter paths now closely match JSON:API's output structure

My concern is about the 'id' field in the attributes. Maybe we need to update the doc or maybe we need some fix but this filter works for nodes and not for entity_test. Here are my points to revert the fix.

  1. ?filter[type]=entity_test--entity_test doesn't work why are we supporting ?filter[id].
  2. Filtering solution is drupalism we are not following any standard afaics. We should follow drupal coventaions as close as possible to make it easier to undersand, see the next two points for why.
  3. Filter values are directly associated with EFQ. As per https://www.drupal.org/docs/8/modules/jsonapi/filtering
    ?filter[a-label][condition][path]=field_first_name
&filter[a-label][condition][operator]=%3D  <- encoded "=" symbol
&filter[a-label][condition][value]=Janis

    These are directly translated to ->condition($property, $value = NULL, $operator = NULL, $langcode = NULL) so the fix implemented is a disconnet with that approach.

  4. On https://www.drupal.org/docs/8/modules/jsonapi/filtering ?filter[a-label][condition][path] is describe A 'path' identifies a field on a resource 'id' is a field on a resource so the fix implemented is a disconnet with that approach as well.
  5. One can always use jsonapi/entity_test/entity_test/b2765ac0-122b-4087-9820-f8798a318f62 but if you are using it in a complex query then for the sake of clarity ?filter[uuid] should be used so that we don't relay on JSON:API magic.
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Improved some wording in the above comments.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +API-First Initiative

Thanks for expanding your explanation/use case, I think I understand you now! 🤓

I think you're saying that the EntityTest entity type has always had an id field that contains the serial ID. And you probably have a custom entity type that also has a id field containing a serial ID. And you probably want that to continue to be the ID exposed in JSON:API and to be able to filter by it.

There are a few problems with that:

  1. even in JSON:API 1.x, the id exposed for a resource object (every entity is mapped to the "resource object" JSON:API concept) was already using the UUID of the corresponding entity
  2. but in JSON:API 1.x, your entity's id field would still show up under attributes, meaning you'd have a resource object like:
    {
  id: 12f8a25b-05cb-4109-a48f-478be68ed8a0,
  type: 'entity_test--entity_test',
  attributes: {
    id: 12345,
    foo: "bar",
    …
  },
  …
}
  3. since #2977669: Spec Compliance: some entity types have an "id", "type" or "uuid" field, this violates the spec (CR: https://www.drupal.org/node/2984247), JSON API 2.x has stopped doing this, since having those two occurrences of id violated the spec (the spec says that the keys under attributes plus relationships are all "fields" and share a common namespace plus there are two reserved keys: id and type), so now you'd get:
    {
  id: 12f8a25b-05cb-4109-a48f-478be68ed8a0,
  type: 'entity_test--entity_test',
  attributes: {
    foo: "bar",
    …
  },
  …
}

    (i.e. attributes.id is omitted)

  4. … but actually, some applications really need that serial ID, for example the JS admin UI. So actually, attributes.id is not omitted, using it is just heavily discouraged, by aliasing it with a drupal_internal__ prefix. So, really, it looks like this:
    {
  id: 12f8a25b-05cb-4109-a48f-478be68ed8a0,
  type: 'entity_test--entity_test',
  attributes: {
   drupal_internal__id: 12345,
    foo: "bar",
    …
  },
  …
}
  5. Then #3010432: Filtering by referenced entity requires ".uuid" to be specified in filter path expression was opened, and it pointed out that it really doesn't make sense for JSON:API's URL filter syntax (?filter URL query argument) to not match the structure of the resource object above. It didn't make sense in neither JSON:API 2.x nor 1.x. You should be able to request /jsonapi/entity_test/entity_test?filter[id]=12f8a25b-05cb-4109-a48f-478be68ed8a0, but no, you needed to know the Drupal internals to guess the correct filtering syntax: only /jsonapi/entity_test/entity_test?filter[uuid]=12f8a25b-05cb-4109-a48f-478be68ed8a0 worked! This is terrible DX! So we fixed that. Now the filter syntax matches the document structure exactly. Yay, much better UX!
  6. But we overlooked one thing, which causes /jsonapi/entity_test/entity_test?filter[drupal_internal__id]=12345 to not work, even though it should, since that's what matches the document structure exactly. @dagmar discovered this, and reported it at #3015759: `?filter[drupal_internal__id]=ID` does not work: drupal_internal__id should not be converted to uuid when filtering, where it is currently being fixed.

That's the whole history. I think the answer to your question is:

Sorry, but JSON:API is opinionated and believes all resources (entities) should be identified by UUIDs; if you insist on using serial IDs, you'll have to use drupal_internal__SOMETHING, and yes, there's one known bug with that, which is being fixed in #3015759: `?filter[drupal_internal__id]=ID` does not work: drupal_internal__id should not be converted to uuid when filtering

To answer your concrete point-by-point criticisms:

  1. Happening in #3017047: Allow filtering by `type`
  2. I strongly disagree. Requiring knowledge of Drupal internals in order to be able to use filtering leads to confusing DX. Allowing the developer to deduce filter syntax by looking at the response document structure yields a far better DX.
  3. It's simply a matter of aliasing the serial ID field's label, it's getting prefixed by drupal_internal__ in all JSON:API situations. Before that EFQ condition is added, we map it back to the internal name. I know that doesn't work right now. That's what #3015759: `?filter[drupal_internal__id]=ID` does not work: drupal_internal__id should not be converted to uuid when filtering is fixing right now.
  4. That's "field" as in a JSON:API field (https://jsonapi.org/format/#document-resource-object-fields), not a Drupal field. Usually they map 1:1, but not always. I'll update the docs to clarify this.
  5. I strongly disagree, for the same reason as #2. That'd be a leaky abstraction. JSON:API 1.x and 2.x before RC2 was leaky in that very way, it no longer is, and it's better for it.

I think it's great you're so vocally questioning our choices. I respect your input a lot. But based on prior discussions in the issues I've linked above, where all three JSON:API maintainers jointly concluded this approach was best, I still feel strongly this was and is the right decision. That's why I've taken considerable time writing this comment, to explain every reason for every change in full detail. Hopefully I've convinced you at this point :)

If not, let's set up a call. I want us to get on the same page.

@gabesullice, @e0ipso, looking forward to your thoughts.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Active » Closed (works as designed)

Thank you for your time and all the work. Let's put it to rest. You(all three of you) clearly have the bigger picture in mind. Maybe I was just bummed that RC2 broke my tests 😛. But I feel a lot better after reading your reply. I understand the reason and you have already made the appropriate doc fixes so it is all good. Thank you again.

Have a very nice day!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I'm very glad to hear that! ❤️

gabesullice’s picture

gabesullice
Primary language English
commented

@jibran++!
@Wim Leers++!

I wholeheartedly echo this: "I think it's great you're so vocally questioning our choices. I respect your input a lot."

@Wim Leers got every point right. We made a conscious decision to add a layer of indirection between filter and EFQ. The corollary is that we removed a layer of indirection between JSON:API's output and filter. I think the latter better fits with JSON:API's philosophy from the beginning.

I think once we finish #3015759: `?filter[drupal_internal__id]=ID` does not work: drupal_internal__id should not be converted to uuid when filtering and [##3017047], the migration path will be fairly straightforward. We won't release 2.0 or another RC until those are resolved.

Thanks again!!!

effulgentsia’s picture

effulgentsia commented

The issue summary says that ?filter[nid]=... works. Is that true? It shouldn't. Is there an existing issue to fix that?

effulgentsia’s picture

effulgentsia commented

To clarify #8, if nid is renamed to drupal_internal__nid in the resource object, then ?filter[nid] should not work and ?filter[drupal_internal__nid] should. I'm not entirely clear on why we prefix nid with drupal_internal__ though, so perhaps an alternate fix is to stop doing that?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

then ?filter[nid] should not work and ?filter[drupal_internal__nid] should.

Correct. We have an open bug report with a working patch for that: #3015759: `?filter[drupal_internal__id]=ID` does not work: drupal_internal__id should not be converted to uuid when filtering. That should land soon.

I'm not entirely clear on why we prefix nid with drupal_internal__ though, so perhaps an alternate fix is to stop doing that?

Because we want to discourage the use of alternative identifiers such as Drupal's classical sequential identifiers in favor of UUIDs.

Rationale at https://www.drupal.org/node/2984247