Handle entities with no bundle in ResourceTypeRepository

Thanks for filing an issue, providing steps to reproduce and even posting a patch! 🤘

This seems related to #2960766: Support ResourceType which may have a null bundle defined..

I worked on understanding this issue. Because apparently I actually did not fully comprehend this, even though I thought I did in #5 😅

I think you're saying that the problem is that if for example no NodeType exists, that no Node-based resource type is returned. Is that indeed the problem you're pointing out?

If so, the very very rough patch below adds a tests proving this (well, not even test coverage, but debug output in tests 😳).

For my own understanding:

This is about handling bundleable entity types, which have no bundles defined. It does not have to do with non-bundleable entity types, like User.

I don't understand why this is a problem though. It seems the bug report only mentions JSON API Extras and Schemata. If there are no bundles for a bundleable entity type, there can't be any content either, so why should there be a resource type for it (and a URL to retrieve resources of that type)?

If there are no bundles for a bundleable entity type, there can't be any content either, so why should there be a resource type for it (and a URL to retrieve resources of that type)?

Exactly. I didn't want to say that before I had confirmation that that was indeed what's going on.

>>> \Drupal\file\Entity\File::load(1)->bundle()
=> "file"
>>> \Drupal\user\Entity\User::load(1)->bundle();
=> "user"

File and User entities have no bundles. There's special code in the entity system to use the entity type name as the bundle name - see above.

Right. And that is already handled. That already works. Because /jsonapi/file/file and /jsonapi/user/user work just fine.

Oh … I think this issue is perhaps not about making JSON API support it!

It's about updating \Drupal\jsonapi\ResourceType\ResourceTypeRepository::get() be aware about it too! But all of ResourceTypeRepository is an @internal API. If contrib modules want to call this internal API, that's fine, but then it's their responsibility to provide a non-NULL bundle value.

Not sure what to do here. If we commit this, we're publicly supporting an internal API. Which sends the wrong message. But OTOH, it's such a simple change. The alternative change would be to add assert(is_string($bundle) && !empty($bundle)).

Thoughts, @gabesullice?

(IOW: should we ask JSON API Extras/Schemata to fix their code, or should JSON API go out of its way to let contrib modules call an internal API?)

@Wim Leers yep. Exactly - the OpenAPI module does

  /**
   * {@inheritdoc}
   */
  protected function includeEntityTypeBundle($entity_type_id, $bundle_name = NULL) {
    /** @var \Drupal\jsonapi\ResourceType\ResourceTypeRepository $resource_type_repository */
    $resource_type_repository = \Drupal::service('jsonapi.resource_type.repository');
    $resource_type = $resource_type_repository->get($entity_type_id, $bundle_name);
    $resource_config = $resource_type->getJsonapiResourceConfig();
    if ($resource_config->id() && $resource_config->get('disabled')) {
      return FALSE;
    }

    return parent::includeEntityTypeBundle($entity_type_id, $bundle_name);
  }

I think this is okay to handle in the JSON API module because it makes ->get() easier to use in the case of bundle-less configuration entities. Yes you can argue that they not adhering to your API but it's okay to be robust by being more liberal with your inputs but strict about the outputs.

I've tried to make the solution have less logic and therefore be easier to maintain.

Here's a test.

+1 for robustness. But in JSON API, 100% of calls (actually, in absolute numbers: only 2 calls!) to ResourceTypeRepository::get() are based on ResourceType value objects, which is why this doesn't really make the JSON API module more robust.

That's why I'd like input from fellow maintainer @gabesullice before RTBC'ing + committing.

Each time we've changed the resource type repository in a way that caused breaking changes for JSON API Extras, we've fixed it in JSON API Extras rather than writing exceptions into this code.

I know of a few cases where modules are overriding the service with their own extension of it and it's truly risky business. Doing this kind of thing signals that it's safe because there's an unwritten guarantee that we'll try to take their implementations into consideration.

The resource type repository is internal and lightly used, as @Wim Leers points out. We've broken this "API" multiple times and fixed the downstream overrides synchronously instead of adding BC layers and conditional logic to our code. I think that should apply here too.

We expect "bundleless" entities to pass the entity type ID for the bundle parameter, that's a pretty simple rule. The solution for JSON API Extras and OpenAPI could be as simple as $repository->get($entity->getEntityTypeId(), $entity->getBundleId() ?: $entity->getEntityTypeId()). Then there'd be nothing to discuss :)

We expect "bundleless" entities to pass the entity type ID for the bundle parameter, that's a pretty simple rule.

I think this is probably why this issue got filed in the first place. That rule wasn't clear, so it was assumed that we just didn't support the "edge" case for file and user entities. In fact, we're just mirroring the logic of EntityInterface::bundle().

Quoting #14:

The alternative change would be to add assert(is_string($bundle) && !empty($bundle)).

That'd help custom/contrib modules comply, and just improves our code quality. I think adding that is reasonable. It also means codifying what #21 did.

Done.

@gabesullice and @Wim Leers - whilst I get the wish to have thing obey your @internal etc. The thing is we have code in the wild that will be improved and work with #17. The non-bundleable entity is easy to forget about and not that simple to code for. There's also another case that is super-rare but possible - using something other than configuration to determine bundles. Core has a test entity that uses state for example. The addition of the asserts improves robustness but in my opinion misses the first half of the robustness principle - which is about being liberal in what you accept. For me this boils down to why not be helpful to others - the formulation in #17 does not increase complexity that much. In an eco-system as large and as complex as ours, the pragmatic approach should be favoured.

For #25.

The thing that has cause this issue is the OpenApi project. In alpha1 they used this internal service - now in beta1 they don't so this problem has somewhat gone away. But I think the principle of making things as simple as possible for other still stands. Also, I should be clear that #23 is definitely an improvement given the complexities of working with bundles and some entity types.

Another win for #17 is when working with the repository you can now do:

$repo->get('date_format');

And not even have to consider what a bundle for a date format could possibly mean.

Thanks for sharing that positive news WRT Open API :) We've frequently been in touch with Open API maintainer @richgerdes. We've been helping him choose the most maintainable path forward, in #2887868: Use JSON API ResourceTypeRepository to determine which resources are available . I'm glad to hear it's made a difference in this additional way :)

Another win for #17 is when working with the repository you can now do:

$repo->get('date_format');

And not even have to consider what a bundle for a date format could possibly mean.

I see that, but the same concerns as above still stand: this is not intended to be a public API. I don't mind the patch at all, but I don't want to set the expectation that JSON API's internal APIs are A) supported, B) are guaranteed to continue to exist as they do today. I don't know how to reconcile those things 😐

Discussed with fellow maintainer @gabesullice.

This issue would never have been opened if both modules (jsonapi and openapi) had been updated to their latest releases.

Committing #17 wouldn't fix any problem, and adds test coverage that asserts a return value that doesn't make sense:

+++ b/tests/src/Kernel/ResourceType/ResourceTypeRepositoryTest.php
@@ -92,8 +102,11 @@ class ResourceTypeRepositoryTest extends KernelTestBase {
+      ['node', NULL, NULL],

Committing #23 makes the current code more explicit about its expectations. To get a ResourceType, pass in the two parts that make up the resource type name: entity type ID and bundle — which makes sense in the context of JSON API and in the context of how Drupal core's EntityInterface::bundle() operates.

So, committing #23. If this is blocking a real-world use case, let's revisit.