UHF-9239: Make ApiClient non-abstract

documentation/api-client.md

@@ -1,6 +1,6 @@
 # API client
 
-Base service for HTTP JSON APIs.
+Service for HTTP JSON APIs.
 
 Features:
  - Simple caching.
@@ -9,57 +9,42 @@ Features:
 
 ## Usage
 
-Extend `\Drupal\helfi_api_base\ApiClient\ApiClient`. Requests are made in the callbacks of `cache()` method.
-
-```php
-namespace Drupal\my_module;
-
-use Drupal\helfi_api_base\ApiClient\ApiClient;
-use Drupal\helfi_api_base\ApiClient\CacheValue;
-
-class MyApi extends ApiClient {
-
-  const TTL = 180;
-
-  public function getFoo(string $id) {
-    return $this->cache($id, fn () => new CacheValue(
-      $this->makeRequest('GET', 'https://example.com/api/v1/foo'),
-      $this->time->getRequestTime() + self::TTL,
-      ['user:1']
-    ))->response;
-  }
-
-}
-```
-
-### Service
-
-Extend your service from the abstract service `helfi_api_base.api_client_base`. You must provide your own logger. Optionally you can provide default request parameters and headers.
+Create your own client service from abstract service `helfi_api_base.api_client_base`. You must provide your own logger. Optionally you can provide default request parameters.
 
 ```yaml
 # my_module.services.yml
 my_module.my_api:
   parent: helfi_api_base.api_manager
-  class: Drupal\my_module\MyApi
   arguments:
     - '@logger.channel.my_module'
+    # Optional:
     - { timeout: 30 }
 ```
 
-### Mocking
+*Warning*: The client fail any further requests instantly after one failed requests. This is to prevent blocking the rendering process and cause the site to time-out. You should not share the client for different purposes that need fault tolerance.
 
-In local environment, the `makeRequestWithFixture` method returns response from JSON file if the response fails.
+Actual requests are usually made in the callback of `cache()` method. The callback must return `CacheValue`.
 
 ```php
-class MyApi extends ApiClientBase {
+use Drupal\helfi_api_base\ApiClient\CacheValue;
+
+/** @var Drupal\helfi_api_base\ApiClient\ApiClient $client */
+$client = \Drupal::service('my_module.my_api');
+
+$response = $client->cache($id, fn () => new CacheValue(
+  // Actual HTTP response.
+  $client->makeRequest('GET', 'https://example.com/api/v1/foo'),
+  // TTL.
+  $client->cacheMaxAge(ttl: 180),
+  // Custom cache tags.
+  ['user:1']
+));
+```
 
-  public function getFoo(string $id) {
-    return $this->cache($id, fn () => new CacheValue(
-      $this->makeRequestWithFixture('path-to-fixture.json', 'GET', 'https://example.com/fail'),
-      $this->time->getRequestTime() + self::TTL,
-      ['user:1']
-    ))->response;
-  }
+### Mocking
 
-}
+In local environment, the `makeRequestWithFixture` method returns response from JSON file if the response fails.
+
+```php
+$client->makeRequestWithFixture('path-to-fixture.json', 'GET', 'https://example.com/fails-in-local'),
 ```

helfi_api_base.services.yml

@@ -144,7 +144,7 @@ services:
       - '@database'
 
   helfi_api_base.api_client_base:
-    class: Drupal\helfi_api_base\ApiClient\ApiClientBase
+    class: Drupal\helfi_api_base\ApiClient\ApiClient
     abstract: true
     arguments:
       - '@http_client'

src/ApiClient/ApiClientBase.php → src/ApiClient/ApiClient.php

@@ -17,13 +17,11 @@
 use Psr\Log\LoggerInterface;
 
 /**
- * Base class for services that fetch data from HTTP API.
+ * Fetch data from HTTP API.
  *
- * Provides caching and fixtures (for local environment).
+ * Provides simple caching and fixtures (for local environment).
  */
-abstract class ApiClientBase {
-
-  use CacheKeyTrait;
+class ApiClient {
 
   /**
    * Whether to bypass cache or not.
@@ -119,7 +117,7 @@ protected function getRequestOptions(string $environmentName, array $options = [
    *
    * @throws \GuzzleHttp\Exception\GuzzleException
    */
-  protected function makeRequest(
+  public function makeRequest(
     string $method,
     string $url,
     array $options = [],
@@ -145,7 +143,7 @@ protected function makeRequest(
    *
    * @throws \GuzzleHttp\Exception\GuzzleException
    */
-  protected function makeRequestWithFixture(
+  public function makeRequestWithFixture(
     string $fixture,
     string $method,
     string $url,
@@ -232,7 +230,7 @@ private function makeRequestInternal(
    *
    * @throws \GuzzleHttp\Exception\GuzzleException
    */
-  protected function cache(string $key, callable $callback) : ?CacheValue {
+  public function cache(string $key, callable $callback) : ?CacheValue {
     $exception = new TransferException();
     $value = ($cache = $this->cache->get($key)) ? $cache->data : NULL;
 
@@ -266,4 +264,17 @@ protected function cache(string $key, callable $callback) : ?CacheValue {
     throw $exception;
   }
 
+  /**
+   * Helper method for calculating cache max age.
+   *
+   * @param int $ttl
+   *   Time to live in seconds.
+   *
+   * @return int
+   *   Expires timestamp.
+   */
+  public function cacheMaxAge(int $ttl): int {
+    return $this->time->getRequestTime() + $ttl;
+  }
+
 }

tests/src/Unit/ApiClient/ApiClientBaseTest.php → tests/src/Unit/ApiClient/ApiClientTest.php

@@ -8,12 +8,12 @@
 use Drupal\Core\Cache\CacheBackendInterface;
 use Drupal\Core\Cache\MemoryBackend;
 use Drupal\Core\File\Exception\FileNotExistsException;
+use Drupal\helfi_api_base\ApiClient\ApiClient;
 use Drupal\helfi_api_base\ApiClient\ApiResponse;
 use Drupal\helfi_api_base\ApiClient\CacheValue;
 use Drupal\helfi_api_base\Environment\EnvironmentResolver;
 use Drupal\helfi_api_base\Environment\EnvironmentResolverInterface;
 use Drupal\helfi_api_base\Environment\Project;
-use Drupal\helfi_api_client_test\ApiClient;
 use Drupal\Tests\helfi_api_base\Traits\ApiTestTrait;
 use Drupal\Tests\UnitTestCase;
 use GuzzleHttp\ClientInterface;
@@ -30,10 +30,10 @@
 use Psr\Log\LoggerInterface;
 
 /**
- * @coversDefaultClass \Drupal\helfi_api_base\ApiClient\ApiClientBase
+ * @coversDefaultClass \Drupal\helfi_api_base\ApiClient\ApiClient
  * @group helfi_api_base
  */
-class ApiClientBaseTest extends UnitTestCase {
+class ApiClientTest extends UnitTestCase {
 
   use ApiTestTrait;
   use ProphecyTrait;
@@ -94,7 +94,7 @@ private function getTimeMock(int $expectedTime) : ObjectProphecy {
    * @param array $requestOptions
    *   The default request options.
    *
-   * @return \Drupal\helfi_api_client_test\ApiClient
+   * @return \Drupal\helfi_api_base\ApiClient\ApiClient
    *   The api client instance.
    */
   private function getSut(
@@ -150,7 +150,7 @@ public function testMakeRequest() {
 
     // Test empty and non-empty response.
     for ($i = 0; $i < 2; $i++) {
-      $response = $sut->exposedMakeRequest('GET', '/foo');
+      $response = $sut->makeRequest('GET', '/foo');
       $this->assertInstanceOf(ApiResponse::class, $response);
       $this->assertInstanceOf(RequestInterface::class, $requests[0]['request']);
     }
@@ -165,7 +165,7 @@ public function testMakeRequest() {
   public function testCacheExceptionOnFailure() : void {
     $this->expectException(GuzzleException::class);
 
-    $this->getSut()->exposedCache(
+    $this->getSut()->cache(
       'nonexistent:fi',
       fn () => throw new RequestException(
         'Test',
@@ -195,7 +195,7 @@ public function testStaleCacheOnFailure() : void {
     $sut = $this->getSut(
       time: $this->getTimeMock($time)->reveal(),
     );
-    $response = $sut->exposedCache(
+    $response = $sut->cache(
       'external_menu:main:fi',
       fn () => throw new RequestException(
         'Test',
@@ -210,6 +210,7 @@ public function testStaleCacheOnFailure() : void {
    *
    * @covers ::__construct
    * @covers ::cache
+   * @covers ::cacheMaxAge
    * @covers \Drupal\helfi_api_base\ApiClient\CacheValue::hasExpired
    * @covers \Drupal\helfi_api_base\ApiClient\CacheValue::__construct
    * @covers \Drupal\helfi_api_base\ApiClient\ApiResponse::__construct
@@ -229,19 +230,20 @@ public function testStaleCacheUpdate() : void {
     $sut = $this->getSut(
       time: $this->getTimeMock($time)->reveal(),
     );
-    $value = $sut->exposedCache('external_menu:main:en', static fn () => new CacheValue(
+    $value = $sut->cache('external_menu:main:en', static fn () => new CacheValue(
       new ApiResponse((object) ['value' => 'value']),
-      $time + 10,
+      $sut->cacheMaxAge(10),
       [],
     ));
     $this->assertInstanceOf(CacheValue::class, $value);
     $this->assertInstanceOf(ApiResponse::class, $value->response);
     // Make sure cache was updated.
     $this->assertInstanceOf(\stdClass::class, $value->response->data);
+    $this->assertEquals($time + 10, $value->expires);
     $this->assertEquals('value', $value->response->data->value);
     // Re-fetch the data to make sure we still get updated data and make sure
     // no further requests are made.
-    $value = $sut->exposedCache('external_menu:main:en', fn() => $this->fail('Data should be cached'));
+    $value = $sut->cache('external_menu:main:en', fn() => $this->fail('Data should be cached'));
     $this->assertInstanceOf(\stdClass::class, $value->response->data);
     $this->assertEquals('value', $value->response->data->value);
   }
@@ -265,7 +267,7 @@ public function testRequestLoggingException() : void {
       ->shouldBeCalled();
 
     $sut = $this->getSut($client, logger: $logger->reveal());
-    $sut->exposedMakeRequest('GET', '/foo');
+    $sut->makeRequest('GET', '/foo');
   }
 
   /**
@@ -290,7 +292,7 @@ public function testMockFallbackException() : void {
     ]);
     $sut = $this->getSut($client);
     // Test with non-existent menu to make sure no mock file exist.
-    $sut->exposedMakeRequestWithFixture(
+    $sut->makeRequestWithFixture(
       sprintf('%d/should-not-exists.txt', __DIR__),
       'GET',
       '/foo'
@@ -322,7 +324,7 @@ public function testMockFallback() : void {
       $client,
       logger: $logger->reveal(),
     );
-    $response = $sut->exposedMakeRequestWithFixture(
+    $response = $sut->makeRequestWithFixture(
       sprintf('%s/../../../fixtures/environments.json', __DIR__),
       'GET',
       '/foo',
@@ -353,7 +355,7 @@ public function testFastRequestFailure() : void {
     // if more than one request is sent.
     for ($i = 0; $i < 50; $i++) {
       try {
-        $sut->exposedMakeRequest('GET', '/foo');
+        $sut->makeRequest('GET', '/foo');
       }
       catch (ConnectException) {
         $attempts++;
@@ -388,8 +390,8 @@ public function testCacheBypass() : void {
     );
     // Make sure cache is used for all requests.
     for ($i = 0; $i < 3; $i++) {
-      $response = $sut->exposedCache('cache_key', fn () => new CacheValue(
-        $sut->exposedMakeRequest('GET', '/foo'),
+      $response = $sut->cache('cache_key', fn () => new CacheValue(
+        $sut->makeRequest('GET', '/foo'),
         $time + 100,
         [],
       ))->response;
@@ -398,8 +400,8 @@ public function testCacheBypass() : void {
     }
     // Make sure cache is bypassed when configured so and the cached content
     // is updated.
-    $response = $sut->withBypassCache()->exposedCache('cache_key', fn () => new CacheValue(
-      $sut->exposedMakeRequest('GET', '/foo'),
+    $response = $sut->withBypassCache()->cache('cache_key', fn () => new CacheValue(
+      $sut->makeRequest('GET', '/foo'),
       $time + 100,
       []
     ))->response;
@@ -411,8 +413,8 @@ public function testCacheBypass() : void {
     // We defined only two responses, so this should fail to OutOfBoundException
     // if cache was bypassed here.
     for ($i = 0; $i < 3; $i++) {
-      $response = $sut->exposedCache('cache_key', fn () => new CacheValue(
-        $sut->exposedMakeRequest('GET', '/foo'),
+      $response = $sut->cache('cache_key', fn () => new CacheValue(
+        $sut->makeRequest('GET', '/foo'),
         $time + 100,
         [],
       ))->response;