Server error when accessing non-fieldable entity resources with filters

We had a fix 2 days ago on the FieldResolver. Can you try to update to the latest dev? If that still fails, can you try to see if that fix is the cause of your error?

@dpolant thanks for the research.

+    // Right now we are exposing all the fields with the name they have in
+    // the Drupal backend. But this may change in the future.
+    if (strpos($external_field_name, '.') === FALSE) {
+      return $resource_type->getInternalName($external_field_name);
+    }

These 6 lines code exit before commit da9b3865ca06892f40dc373ccf9c65249db63137

commit aeca73d7a50b1c85edaeeb78ea46c305ffc767eb delete them.

@caseylau, good research.

Indeed, those lines would prevent this error, but it was just hiding a problem before. We could add it back in as a stop gap. But it looks like what we really need to do is not load field definitions for entities that do not implement FieldableEntityInterface.

Can't write a patch now, but we should probably have something on ResourceTypes to help determine that.

@gabesullice
Yeah, the previous #7's patch is totally wrong,

If the entity didn't implement FieldableEntityInterface, we should return $field_name directly.
#12 did this.

@caseylau what's the reasoning behind:

If the entity didn't implement FieldableEntityInterface, we should return $field_name directly.

AFAICT that would imply that those names cannot be overridden. Right?

@e0ipso

> AFAICT that would imply that those names cannot be overridden. Right?

Yes, you are right.
those names should can be overridden.

what's the reasoning?

@e0ipso, I think @caseylau was following my suggestion from #11.

I got that idea from this code, where the exception in the IS originated.

My suggestions should be taken with a grain of salt, it's all from research done from my phone :P

This code needs a comment explaining that reasoning (if we solve it with this fix), tests, and we should move the interface checking into Drupal\jsonapi\ResourceType\ResourceType so we can avoid statically loading the EntityType.

Thanks for your help with this @caseylau!

AFAICT that would imply that those names cannot be overridden. Right?

I'm not sure that's true @e0ipso, why couldn't you override field names for config entities (which don't implement the interface)?

It's possible I misunderstood. Gonna figure this out on Monday.

Also, @caseylau, your patch in #12 was more correct than #15. We want to check that even across nested fields or else this won't work. For example, it would break when you're filtering on the role name of an author's user entity (uid.roles.name).

@gabesullice
Thanks for your sweet review on weekend even just have
a phone:).

I have tested jsonapi/view/view?sort=display.default.position on #12 and #15's patch,
Both works well.

Going to sleep now,It's too late in China,Good night @gabesullice:):):)

Here's a test-only patch which replicates the issue.

In the last test only patch, I had the order of the expected and input values backwards.

I've attached a patch that includes #15 to illustrate my remarks in #17 about nested field paths. I.e. the first new test should pass, but the second one should fail on type.uuid because the "gate" code isn't run for the second $part ("uuid") because we're already in the while loop.

I say #12 is "more correct" because it will be run even for nested paths. However, it will still be broken because the $entity_type_id variable will not have been updated to the nested resource type. I.e. it will still be using the id from the first part of the path, not the referenced entity type.

Something is off about #20's combined patch:

00:01:53.153 ----------------   Starting patch   ----------------
00:01:53.153 Directory created at /var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-19557/ancillary/patch
00:01:53.154 Patch Error
00:01:53.154 The patch file /var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-19557/ancillary/2934730-20+15.patch is invalid.
00:01:53.155 Patch Validation Error
00:01:53.155 Failed to validate the patch.

Alright, let's see what everyone thinks of something like this...

I tried to keep the abstraction of resource types involved, rather than loading the entity type definition directly. This might be wasted effort though, considering that we're relying on entity field definitions later in the code.

WTH? Trying this again.

The number of places I had to change tests is really making me doubt the ResourceType change.

1. +++ b/src/Context/FieldResolver.php
   @@ -133,6 +134,13 @@ class FieldResolver {
   +      if (!$this->resourceTypesAreTraversable($resource_types)) {
   
   +++ b/src/ResourceType/ResourceType.php
   @@ -77,6 +84,21 @@ class ResourceType {
   +  public function isTraversable() {
   

   So this is the key thing that this is adding, just to allow

   FieldResolver</code > to detect lack of traversability.
   </li>
   
   <li>
   <code>
   +++ b/src/Context/FieldResolver.php
   @@ -156,21 +164,36 @@ class FieldResolver {
   -        // Reconstruct the path parts that are referencing sub-properties.
   -        $field_path = implode('.', $parts);
   -
   -        // This rebuilds the path from the real, internal field names that have
   -        // been traversed so far. It joins them with the "entity" keyword as
   -        // required by the entity query system.
   -        $entity_path = implode('.entity.', $reference_breadcrumbs);
   -
   -        // Reconstruct the full path to the final reference field.
   -        return (empty($field_path)) ? $entity_path : $entity_path . '.' . $field_path;
   +        return $this->constructInternalPath($reference_breadcrumbs, $parts);
          }
        }
    
        // Reconstruct the full path to the final reference field.
   -    return implode('.entity.', $reference_breadcrumbs);
   +    return $this->constructInternalPath($reference_breadcrumbs);
   +  }
   +
   +  /**
   +   * Expand the internal path with the "entity" keyword.
   +   *
   +   * @param string[] $references
   +   *   The resolved internal field names of all entity references.
   +   * @param string[] $property_path
   +   *   (optional) A sub-property path for the last field in the path.
   +   *
   +   * @return string
   +   *   The expanded and imploded path.
   +   */
   +  protected function constructInternalPath(array $references, array $property_path = []) {
   +    // Reconstruct the path parts that are referencing sub-properties.
   +    $field_path = implode('.', $property_path);
   +
   +    // This rebuilds the path from the real, internal field names that have
   +    // been traversed so far. It joins them with the "entity" keyword as
   +    // required by the entity query system.
   +    $entity_path = implode('.entity.', $references);
   +
   +    // Reconstruct the full path to the final reference field.
   +    return (empty($field_path)) ? $entity_path : $entity_path . '.' . $field_path;
   

   This refactoring is exactly what I wanted to suggest in #2920194: Inclusion of nested relationships fails for bundle-specific fields but eventually didn't.

   Needless to say, ❤️ :)

   If you'd split this off to a separate issue/patch, I'd commit this immediately.

2. +++ b/src/ResourceType/ResourceType.php
   @@ -77,6 +84,21 @@ class ResourceType {
   +   * related resources. For example, config entities, which are stored in YAML,
   +   * cannot be filtered by related resources because a SQL join can't be
   +   * constructed.
   

   Entities have storage decoupled from SQL. Config entities can also be stored in various storage back ends.

   So I think this comment is incorrect.

   What is correct, is to say that config entities are never able to express relationships.

   … however … it is theoretically possible to expose config dependencies as relationships to other config entities. Maybe we'll want to add that in the future. What then?

3. Overall, I understand the preference to not "hardcode" certain knowledge in FieldResolver, but to instead enrich ResourceType with additional metadata. But then we need to make sure that this metadata remains valuable in the foreseeable future. In this case, I'm not entirely certain (see my previous review point).

   It feels that the "real" solution here is to lift \Drupal\Core\Entity\EntityInterface::referencedEntities() into an interface of its own, and then have ConfigEntityBase NOT implement that anymore (right now it hardcodes return []…). But that is a pipe dream, because that requires a significant BC break.

Thoughts?

Thoughts?

Yeah, I think you're picking up on a lot of the same uneasiness I had about this change (see #22 and #24). So, given your valid concerns and the fact that in FieldResolver "we're relying on entity field definitions" already. Let's just hardcode it into FieldResolver and address the rest of that class's shortcomings later and fix this regression with minimal changes.

+1

I think your patch was a worthwhile effort though: it explored a solution that was likely going to end up cleaner. The reason it doesn't quite work is that the root cause lies in the incompleteness of the (Config) Entity API. Committing #24 would effectively encode that incompleteness in JSON API.

When you reroll this, please do include a comment with a @todo that explains at a high level what we should do in the future.

Greatly simplified patch.

Yep, patch is simpler because A) no more changes to ResourceType, B) #2935430: Refactor path expansion in FieldResolver landed the bits cited in #26.2.

+++ b/src/Context/FieldResolver.php
@@ -287,6 +306,30 @@ class FieldResolver {
+   * @todo This class really shouldn't need to be aware of entity types and
+   * their definitions. Whether a resource can have relationships to other
+   * resources is information we ought to be able to discover on the
+   * ResourceType.

This is correct, but incomplete: it doesn't mention that we can't reliably implement this in ResourceType because it is due to incomplete architecture in Drupal core.

Okay. Added a bit more detail. I didn't specifically point out anything about potential implementations that would allow this because there are a number of viable solutions.

An interface for the referencedEntities method is just one piece of the puzzle. While that will tell us that an entity type might be referencing another entity, it still isn't enough to tell us if we can actually sort and filter based on the paths we're resolving.

For now, sorting and filtering by the entity query system relies on SQL backed entities to sort and filter across references, but in future filtering could be done using a mix of both SQL and in-PHP processing.

Some of the "validation" we're doing here is actually somewhat related to discussions elsewhere: #2926089-12: Filter operators not working with multiple values

We're clumsily trying to make sure that the entity query system isn't going to choke on something like ?filter[roles.label]=Administrator, which it unfortunately does...

>>> $query = \Drupal::entityQuery('user');
>>> $query->condition('roles.entity.label', 'Administrator');
>>> $query->execute();
LogicException with message 'Getting the base fields is not supported for entity type Role.'

Edit: I say "clumsily" because there aren't APIs that I'm aware of to actually determine this conclusively in advance.

All issues that are similar in nature to #32.

#31++

#32 + #33: That's a very valuable insight! I wonder if we need a general Plan issue to address all that?
Also, can I summarize that to: The Typed Data API gives us metadata about the structure of our data models, but we have no API that gives us metadata about the queryability of our data models?

In any case, #31 is RTBC.

Also, can I summarize that to: The Typed Data API gives us metadata about the structure of our data models, but we have no API that gives us metadata about the queryability of our data models?

s/Typed Data API/Typed Data and Entity APIs/

Merged. I forgot to tick boxes and didn't give proper attribution so I created a second commit with the missing people.