`?filter[drupal_internal__id]=ID` does not work: drupal_internal__id should not be converted to uuid when filtering

Interesting find! We’ll need an explicit regression test for this one for sure. There is an explicit test class just for testing regressions. Perhaps you’d like to contribute the exact test case that you’ve listed here? If you do, I can guarantee that the fix will land tomorrow :)

We need to make this either work (200 response), or we need to explicitly say that we don't support this (400 response with helpful error message).

Per https://www.drupal.org/node/2984247 and https://www.drupal.org/node/3015183, I'm inclined to say we don't support this. The error message would then say: Please filter by resource object ID instead..

@gabesullice, @e0ipso: thoughts?

We need to make this either work (200 response), or we need to explicitly say that we don't support this (400 response with helpful error message)... [because of two recent CRs] I'm inclined to say we don't support this.

I don't think either of those CRs indicate that this shouldn't be supported. drupal_internal__id is in the JSON:API output. We mapped id to drupal_internal__id (instead of disabling it) specifically in acknowledgement of the fact that there are tightly-coupled yet valid use cases for using Drupal's ID fields.

So, I agree with @dagmar that this is a bug that should be resolved.

Good points! :)

Let's do this then. This then seems like a major bug to me.

Thank you so much, @dagmar, for providing failing test coverage!

Indeed, @dagmar, having that test coverage already written was sooo nice! I tweaked it a little and put it in a place that was more logical to me.

I've attached two patches, once that I think is the "minimal" thing we can do to make this work.

The other is what I'd prefer, because I think it makes things a little bit simpler overall by moving some special cases into a helper method.

The failures before are caused by bad expectations that weren't updated in #3010432: Filtering by referenced entity requires ".uuid" to be specified in filter path expression. Our test cases were erroneously using uuid in the filter path and it wasn't causing test failures, meaning what we did in #3010432 let some things slip through, but this new code correctly caught the bad filter paths.

Those same failures did not occur on the "minimal" patch, meaning the above bugs were still present. So, I'm dropping the minimal patch in favor of the more comprehensive "preferred" patch.

+++ b/src/Context/FieldResolver.php
@@ -258,22 +258,27 @@ class FieldResolver {
-      $candidate_definitions = $this->getFieldItemDefinitions(
-        $resource_types,
-        $field_name
-      );
+      $candidate_definitions = $this->getFieldItemDefinitions($resource_types, $field_name);

Yes, this is just a reflow. But I've ignored it for too damn long! :P

1. +++ b/src/Context/FieldResolver.php
   @@ -258,22 +258,27 @@ class FieldResolver {
   +      if (!$this->isFieldEnabled($field_name, $resource_types, $part)) {
   +        throw new CacheableBadRequestHttpException($cacheability, sprintf(
   
   @@ -428,9 +433,6 @@ class FieldResolver {
   -      if (!$resource_type->isFieldEnabled($field_name)) {
   -        return $result;
   -      }
   
   @@ -473,10 +475,32 @@ class FieldResolver {
   -      return $resource_type->getInternalName($field_name);
   +      return $field_name === 'id'
   +       ? $this->getIdFieldName($resource_type)
   +       : $resource_type->getInternalName($field_name);
   ...
   +  protected function isFieldEnabled($internal_field_name, array $resource_types, $external_field_name) {
   +    return array_reduce($resource_types, function ($carry, ResourceType $resource_type) use ($internal_field_name, $external_field_name) {
   +      return $carry ?: $external_field_name === 'id' || $resource_type->isFieldEnabled($internal_field_name);
   +    }, FALSE);
   +  }
   

   This … is a bit strange. We've now got yet another case of throw new CacheableBadRequestHttpException(…, 'Invalid nested filtering. The field `%s`, given in the path `%s`, does not exist.', …).

   Furthermore, this new isFieldEnabled() helper method seems a bit of a misnomer.

   I'm hoping you can solve this more elegantly. This method is already very complex, this makes it even more complex (it removes some complexity for sure, but then adds even more IMHO).

2. +++ b/src/Context/FieldResolver.php
   @@ -258,22 +258,27 @@ class FieldResolver {
   -        $reference_breadcrumbs[] = $field_name === 'id' ? $this->getIdFieldName(reset($resource_types)) : $field_name;
   +        $reference_breadcrumbs[] = $field_name;
   ...
   -      if (empty($candidate_definitions) && $field_name !== 'id') {
   
   @@ -282,7 +287,7 @@ class FieldResolver {
   -      $reference_breadcrumbs[] = $field_name === 'id' ? $this->getIdFieldName(reset($resource_types)) : $field_name;
   

   I'm very glad to see this disappear. I bit my tongue to not delay the issue that introduced it. But it feels good to see it disappear 👌

3. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -192,8 +192,11 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   -    $mapping[$entity_type->getKey('uuid')] = FALSE;
        $id_field_name = $entity_type->getKey('id');
   +    $uuid_field_name = $entity_type->getKey('uuid');
   +    if ($uuid_field_name !== 'id') {
   +      $mapping[$uuid_field_name] = FALSE;
   +    }
   

   Oh, I see! Rather than always disabling the uuid field, we first check if its name doesn't happen to be id; if it is, then we don't disable it.

   Two questions:
   A) let's update the comment above it
   B) is there a case of this in core?

Hmm, what do you think of this? It renames isFieldEnabled and gets rid of the second exception. It still has the $field_name === 'id' ... ternarys, but I'm okay with that type and id are spec defined member names.

a) done!
b) No, I thought there was one that came up in tests locally, but I couldn't find one now that I look specifically.

1. +++ b/src/Context/FieldResolver.php
   @@ -256,33 +256,30 @@ class FieldResolver {
   +      if (!$this->isMemberFilterable($part, $resource_types)) {
   +        throw new CacheableBadRequestHttpException($cacheability, sprintf(
   ...
   +      assert(!empty($candidate_definitions));
   

   So thanks to this new earlier if-test, we know for a fact that $candidate_definitions will be non-empty. But we just add an assert() to formally verify that understanding, to catch future surprises early.

   Looks good!

2. +++ b/src/ResourceType/ResourceType.php
   @@ -64,13 +64,13 @@ class ResourceType {
   -  protected $disabledFields;
   +  protected $enabledFields;
   

   What do we gain by this inversion?

   (All fields are enabled by default, a minority are disabled, so I think what's in HEAD is clearer. Curious about your reasoning.)

#17.1 is enabled by #17.2.

There's no way to know if a field actually exists on the resource type without #2 because we only know which fields are mapped and if a field is disabled. But if a field is not mapped and not disabled, then it may or may not exist on the underlying entity type. Now, only fields that exist and are not disabled are "enabled". Hence this comment change:

+++ b/src/ResourceType/ResourceType.php
@@ -173,11 +173,11 @@ class ResourceType {
-   *   TRUE if the field is enabled and should be considered as part of the data
-   *   model. FALSE otherwise.
+   *   TRUE if the field exists and is enabled and should be considered as part
+   *   of the data model. FALSE otherwise.

This should fix most, but not all of the previous failures. While looking into this, I realized that there's a lot to be desired in this denormalization code. I opened #3017295: Refactor/clean up JsonApiDocumentTopLevelNormalizer::denormalize() and EntityNormalizer::denormalize() for that.

#19: makes sense, thanks!
#20: from 19 to 6 fails, almost there :)

Config entities--

5 of the 6 failures are caused by config entities for which no properties can be determined without an instance of that config entity in hand and since we don't know about them, they never get put into a field mapping. Now because the above patch changed the "enabled by default" strategy, these config entities started to break because we were asking if unknown fields were enabled or not.

This interdiff now first ensures that we know about the field before we check if it is enabled or not. I reverted the enabledFields property back to disabledFields, but I kept the behavior of isFieldEnabled that ensures the field also exists. I think that is a more robust/secure behavior.

+   * Note: a minority of config entity types which do not define a
+   * `config_mapping` in their entity type annotation will not have their fields
+   * represented here because it is impossible to determine them without an
+   * instance of config available.

Will this still be true in D9, or will that become required? I think that this at least being considered. Linking to that issue would be good, because that’d allow this to be simplified in the future.

Let's land this in the next few days.

Added an explicit regression test that matches the originally reported issue. That was the most important thing that was missing.

Now the only thing that remains is addressing #27.

And for #27, I think that config_export is meant, not config_mapping. I searched, and found the relevant issue: #2949021: Deprecate schema fallback in ConfigEntityType::getPropertiesToExport.

Fixed the CS violation I introduced in #31. Once it comes back green, IMHO this is committable.

Green!

@gabesullice still wrote the majority of this patch; I just reviewed his patches and added an explicit regression test. Committing…

Clarifying issue title, this also matches the original issue report/steps to reproduce and the regression test I added in #29.

Also linked this from the change record at https://www.drupal.org/node/3015183.

Moving to Drupal core's issue queue.

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