Problem/Motivation

Discovered in #2932031-22: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases:

Here, contact_message is another special case, because those entities are never stored. That means they can only be POSTed. All other verbs are not allowed. And no routes should be created for them. We have no way to signal this just yet. Core's REST module achieves this via contact_rest_resource_alter() overriding the default REST resource plugin class.
I was contemplating making JSON API respect this automatically for any entity type using the Drupal\Core\Entity\ContentEntityNullStorage storage handler. But because JSON API has a single route for multiple methods, that's less easy to achieve, and merits more debate. So, created a new issue for that: [this issue]

See #2843755: EntityResource: Provide comprehensive test coverage for Message entity for the solution that core's REST module chose.

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#8 2944977-8.patch13.99 KBwim leers
#8 interdiff.txt945 byteswim leers
#5 2944977-5.patch14.05 KBwim leers

Comments

Wim Leers created an issue. See original summary.

gabesullice’s picture

But because JSON API has a single route for multiple methods, that's less easy to achieve, and merits more debate.

Can you elaborate the difficulties?

wim leers’s picture

Note that I also had to add a work-around to allow the POST test for Message to pass, because JSON API blindly generates a Location response header, even if that created resource never is actually stored, and is therefore never even accessible via JSON API:

     // 201 for well-formed request.
     $response = $this->request('POST', $url, $request_options);
-    $this->assertResourceResponse(201, FALSE, $response, Cache::mergeTags(['http_response', static::$entityTypeId . ':' . static::$firstCreatedEntityId], []), $this->getExpectedCacheContexts());
+    $this->assertResourceResponse(201, FALSE, $response, Cache::mergeTags(['http_response'], static::$entityTypeId !== 'contact_message' ? [static::$entityTypeId . ':' . static::$firstCreatedEntityId] : []), $this->getExpectedCacheContexts());
+    // @todo Remove this logic to extract a UUID from the response in https://www.drupal.org/project/jsonapi/issues/2944977
+    $uuid = $this->entityStorage->load(static::$firstCreatedEntityId)->uuid();
+    if (get_class($this->entityStorage) === ContentEntityNullStorage::class) {
+      $r = Json::decode((string)$response->getBody());
+      $uuid = NestedArray::getValue($r, ['data', 'id']);
+    }
     // @todo Remove line below in favor of commented line in https://www.drupal.org/project/jsonapi/issues/2878463.
-    $location = Url::fromRoute(sprintf('jsonapi.%s.individual', static::$resourceTypeName), [static::$entityTypeId => $this->entityStorage->load(static::$firstCreatedEntityId)->uuid()])->setAbsolute(TRUE)->toString();
+    $location = Url::fromRoute(sprintf('jsonapi.%s.individual', static::$resourceTypeName), [static::$entityTypeId => $uuid])->setAbsolute(TRUE)->toString();
     /* $location = $this->entityStorage->load(static::$firstCreatedEntityId)->toUrl('jsonapi')->setAbsolute(TRUE)->toString(); */
     $this->assertSame([$location], $response->getHeader('Location'));
     $this->assertFalse($response->hasHeader('X-Drupal-Cache'));

Therefore the self and relationships URLs in the JSON API response document are all really just URIs, not URLs: they only identify something, they're unable to locate something, because it is not stored, therefore unlocatable.

wim leers’s picture

Can you elaborate the difficulties?

Sure!

In \Drupal\jsonapi\Routing\Routes::routes():

      // Collection endpoint, like /jsonapi/file/photo.
      $route_collection = (new Route($route_base_path, $defaults))
…
        ->setMethods(['GET', 'POST']);
…
      // Individual endpoint, like /jsonapi/file/photo/123.
      $parameters = [$resource_type->getEntityTypeId() => ['type' => 'entity:' . $resource_type->getEntityTypeId()]];
      $route_individual = (new Route(sprintf('%s/{%s}', $route_base_path, $resource_type->getEntityTypeId())))
…
        ->setMethods(['GET', 'PATCH', 'DELETE']);
…
      // Related resource, like /jsonapi/file/photo/123/comments.
      $route_related = (new Route(sprintf('%s/{%s}/{related}', $route_base_path, $resource_type->getEntityTypeId()), $defaults))
…
        ->setMethods(['GET']);
…
      // Related endpoint, like /jsonapi/file/photo/123/relationships/comments.
      $route_relationship = (new Route(sprintf('%s/{%s}/relationships/{related}', $route_base_path, $resource_type->getEntityTypeId()), $defaults + ['_on_relationship' => TRUE]))
…
        ->setMethods(['GET', 'POST', 'PATCH', 'DELETE']);
…

IOW: many routes support multiple methods. Which means we can't do the same as we do in core REST and its test coverage. Quoting the test coverage there (from \Drupal\Tests\rest\Functional\EntityResource\Message\MessageResourceTestBase):


  /**
   * {@inheritdoc}
   */
  public function testGet() {
    // Contact Message entities are not stored, so they cannot be retrieved.
    $this->setExpectedException(RouteNotFoundException::class, 'Route "rest.entity.contact_message.GET" does not exist.');

    $this->provisionEntityResource();
    Url::fromRoute('rest.entity.contact_message.GET')->toString(TRUE);
  }

  /**
   * {@inheritdoc}
   */
  public function testPatch() {
    // Contact Message entities are not stored, so they cannot be modified.
    $this->setExpectedException(RouteNotFoundException::class, 'Route "rest.entity.contact_message.PATCH" does not exist.');

    $this->provisionEntityResource();
    Url::fromRoute('rest.entity.contact_message.PATCH')->toString(TRUE);
  }

  /**
   * {@inheritdoc}
   */
  public function testDelete() {
    // Contact Message entities are not stored, so they cannot be deleted.
    $this->setExpectedException(RouteNotFoundException::class, 'Route "rest.entity.contact_message.DELETE" does not exist.');

    $this->provisionEntityResource();
    Url::fromRoute('rest.entity.contact_message.DELETE')->toString(TRUE);
  }

It's okay that not exactly the same solution (and test coverage) will work. But it does mean it merits further discussion. That's what this issue is for :)

wim leers’s picture

Status: Active » Needs review
StatusFileSize
new14.05 KB

Et voila. This does what we need to happen, to prevent WTFs when trying to interact with contact_message entities. It's very similar to what #2843755: EntityResource: Provide comprehensive test coverage for Message entity did for rest.module.

gabesullice’s picture

Issue tags: +API-First Initiative
  1. +++ b/src/ResourceType/ResourceTypeRepository.php
    @@ -168,6 +170,19 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
    +  protected static function isLocatableResourceType(EntityTypeInterface $entity_type) {
    

    ❤️

  2. +++ b/src/Routing/Routes.php
    @@ -119,7 +119,10 @@ class Routes implements ContainerInjectionInterface {
    +    $allowed_methods = $resource_type->isLocatable()
    +      ? ['GET', 'POST']
    +      : ['POST'];
    +    $collection_route->setMethods($allowed_methods);
    

    Nit: this can be on one line and it saves an extra variable floating around.
    $collection_route->setMethods($resource_type->isLocatable() ? ['GET', 'POST'] : ['POST']);

gabesullice’s picture

Status: Needs review » Needs work
wim leers’s picture

Status: Needs work » Needs review
StatusFileSize
new945 bytes
new13.99 KB

#6.2: ✔️

gabesullice’s picture

Status: Needs review » Reviewed & tested by the community
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

YAY for fewer @todos!

  • Wim Leers committed 6e386c0 on 8.x-1.x
    Issue #2944977 by Wim Leers, gabesullice: Message entities can only be...
  • Wim Leers committed 7bca582 on 8.x-2.x
    Issue #2944977 by Wim Leers, gabesullice: Message entities can only be...
wim leers’s picture

Oops, saved my comment before pushing 😊

wim leers’s picture

Category: Task » Bug report

JSON API was setting a Location header even for unlocatable resource types. That was the bug. i.e. it was generating a link with a URL that is never going to return anything but an error.

So, recategorizing this as a bug.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.