Add tests for REST support of our entities

Tagging this as novice, as there isn't really an "intermediate"-type of tag. Because of all the examples in core, it should be easy to resolve this.

We can only do this for content entities, so that means only for the task entity type. We should be able to do this for config entities in the future.

This doesn't work yet, and I don't know why that is, but at least this is a start.

Looks like that last-minute change before the patch upload got us some messages that were easier to test. Fixing this now

I can't reproduce those fails locally, I keep getting this:

-{"message":"No route found for \u0022GET entity\/search_api_task\/1\u0022"}
+{"message":"No route found for \u0022GET \/entity\/search_api_task\/1\u0022"}

 <?xml version="1.0"?>
-<response><message>No route found for "GET entity/search_api_task/1"</message></response>
+<response><message>No route found for "GET /entity/search_api_task/1"</message></response>

I don't understand why that is happening.

We should be getting more errors now, I copied these tests from media, and looks like I didn't rename all of them correctly.

I also added 2 @group annotations to the tests, because that makes it a little easier to run these: ./vendor/bin/phpunit -c core/ modules/globalignore/search_api --group rest

Makes sense for now, but would you say we should also commit it like this?

Probably not.

We can only do this for content entities, so that means only for the task entity type. We should be able to do this for config entities in the future.

Would be a nice simplification – but are you sure about that? It seems like Core provides tests for all config entity types as well.

I was wrong, this can be done for config as well, but let's fix Task first, before we do the other entities.

I don't really think tagging this as "Novice" is fair, I'd feel bad for any actual novice trying to take this on. It seems more like we'd need a REST expert to work on this, not a Search API expert.

I agree, if both of us can't figure it out yet, asking a novice to work on this wouldn't be great. I'll did review a couple of the issues that went into drupal core regarding this, but even that knowledge isn't enough to help me figure out the fails.

❤️

We should at least make sure they have proper access restrictions in place

+1. Especially when using https://www.drupal.org/project/jsonapi this is important (and we want to move that into core in 8.6), because that exposes all entities by default, and relies on entity access to ensure access is given to only those who should be allowed to access it!

We can only do this for content entities, so that means only for the task entity type. We should be able to do this for config entities in the future.

That's not quite true — it's possible to view (GET) config entities. Run e.g. \Drupal\Tests\rest\Functional\EntityResource\NodeType\NodeTypeJsonBasicAuthTest and observe :)

#7: I cannot reproduce those failures locally (on Drupal 8.6.x + PHP 7 + latest search_api 8.x-1.x). I can reproduce the failures that testbot is showing.

Patch review:

1. +++ b/tests/src/Functional/EntityResource/TaskJsonAnonTest.php
   @@ -0,0 +1,27 @@
   +namespace Drupal\Tests\search_api\Functional\EntityResource;
   

   Nit: I'd suggest moving these into a Rest directory rather than EntityResource. That'll make 10x more sense for you as a contrib module :)

   (That's also the location that \Drupal\Tests\rest\Functional\EntityResource\EntityResourceRestTestCoverageTest checks, in case this module ever makes into core, or in case that test gets somehow generalized to run for contrib modules.)

2. +++ b/tests/src/Functional/EntityResource/TaskResourceTestBase.php
   @@ -0,0 +1,132 @@
   +use Drupal\Tests\rest\Functional\BcTimestampNormalizerUnixTestTrait;
   ...
   +  use BcTimestampNormalizerUnixTestTrait;
   

   Unused, can be removed. (There are no timestamp fields in the Task content entity type.)

3. +++ b/tests/src/Functional/EntityResource/TaskResourceTestBase.php
   @@ -0,0 +1,132 @@
   +      ->set('type', 'barn_owl')
   

   😀

4. +++ b/tests/src/Functional/EntityResource/TaskResourceTestBase.php
   @@ -0,0 +1,132 @@
   +  protected function getNormalizedPostEntity() {
   +    return [
   +      'title' => [
   +        [
   +          'value' => 'Feed Resource Post Test',
   +        ],
   +      ],
   +      'url' => [
   +        [
   +          'value' => 'http://example.com/feed',
   +        ],
   +      ],
   +      'refresh' => [
   +        [
   +          'value' => 900,
   +        ],
   +      ],
   +      'description' => [
   +        [
   +          'value' => 'Feed Resource Post Test Description',
   +        ],
   +      ],
   +    ];
   +  }
   

   C/P remnant.

5. +++ b/tests/src/Functional/EntityResource/TaskResourceTestBase.php
   @@ -0,0 +1,132 @@
   +    switch ($method) {
   +      case 'GET':
   +      case 'POST':
   +      case 'PATCH':
   +      case 'DELETE':
   +        return "You are not authorized to view this search_api_task entity.";
   +      default:
   +        return parent::getExpectedUnauthorizedAccessMessage($method);
   +    }
   

   These need to be updated too.

Test failures:

Failed asserting that Array &0 (
    0 => ''
) is identical to Array &0 (
    0 => 'user.permissions'
).

Seems to be the most complex/puzzling test failure. It's caused by the fact that the Task entity type … does NOT specify an admin_permission annotation key! So this is pointless:

+++ b/tests/src/Functional/EntityResource/TaskResourceTestBase.php
@@ -0,0 +1,132 @@
+  protected function setUpAuthorization($method) {
+    $this->grantPermissionsToTestedRole(['administer search_api']);
+  }

The reason can be found in \Drupal\Core\Entity\EntityAccessControlHandler::checkAccess():

  protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
    if ($operation == 'delete' && $entity->isNew()) {
      return AccessResult::forbidden()->addCacheableDependency($entity);
    }
    if ($admin_permission = $this->entityType->getAdminPermission()) {
      return AccessResult::allowedIfHasPermission($account, $admin_permission);
    }
    else {
      // No opinion.
      return AccessResult::neutral();
    }
  }

It ends up executing the last branch, and hence there's no cacheability to be associated, hence no user.permissions cache context.

Adding an admin_permission annotation key fixes that. Tests then fail just a little bit further, because then both url.site and url.permissions are expected. url.site is expected by default because 99% of entity types specify link relations. And link relations cause Link response headers to be generated, containing absolute URLs, and those then of course vary by which site in a multi-site setup is being accessed. That's not the case here, so we need to override that default expectation.

From then onward, the failures are more obvious, and I think you'll be easily able to drive them to completion :)

I still have those same failures locally - which makes it really hard to see if the things I'm doing are actually improving the patch. (php7.1, drupal 8.6 and latest search api 8.x-1.x)

I reinstalled mamp and now am running on php7.2, this made it so that I could reproduce the errors and I fixed them.

I'm not sure if we should add new issues for the other entities or if we should do that in one go.

Not sure if that was the big thing, but at least that made it so that I could reproduce the failures.

I added the work I did on the Index and Server, but this needs a fix in core tests.

@Wim Leers, do you think this is something sane to be added in core tests or should we try to fix this in Search API? The problem is that the not all links that we add to the Index class trough the annotation don't exist in the link relation type plugin manager.

1) Drupal\Tests\search_api\Functional\Rest\IndexXmlCookieTest::testGet
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "fields" plugin does not exist.

That was the problem that is fixed with the core patch.

The problem that we see with the Anon test is something that I don't know how to fix:

1) Drupal\Tests\search_api\Functional\Rest\ServerXmlAnonTest::testGet
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@

-The used authentication method is not allowed on this route.
+The 'administer search_api' permission is required.

The core patch I added in #21 was wrong and I fixed that by adding the correct link_relation_types in #26. Now all we need to to is figure out the 6 remaining failures, but I don't have a clue why they are happening and how to resolve them.

I asked @tedbow for help on this in slack.

tedbow [4:32 PM]
@borisson_ so first 1 seems related to missing cache tags on when you don’t send auth

As expected, his assesment is correct, the expected cache tags are:

array(3) {
  [0]=>
  string(12) "4xx-response"
  [1]=>
  string(26) "config:user.role.anonymous"
  [2]=>
  string(13) "http_response"
}

The actual cache tags ($response->getHeader('X-Drupal-Cache-Tags')) are empty (array(0) {}).

I've tried figuring out why this happens, but I can't seem to find the reason.
I tried doing this by just getting the same page with Insomnia, but it looks like not getting the correct result is actually the correct response. At least it looks like that is what is happening.

I tried all afternoon to figure out why these failures were happening, but I didn't get one any closer. We're going to need help on this one.