From b9d0bf363ccafc2c6ca849c801b1dcd159e49ca1 Mon Sep 17 00:00:00 2001 From: Santeri Hurnanen Date: Mon, 15 Jan 2024 08:03:05 +0200 Subject: [PATCH] UHF-9239: Make ApiClient non-abstract --- documentation/api-client.md | 63 +++++++------------ helfi_api_base.services.yml | 2 +- .../{ApiClientBase.php => ApiClient.php} | 27 +++++--- .../helfi_api_client_test.info.yml | 6 -- .../helfi_api_client_test/src/ApiClient.php | 43 ------------- ...piClientBaseTest.php => ApiClientTest.php} | 42 +++++++------ 6 files changed, 66 insertions(+), 117 deletions(-) rename src/ApiClient/{ApiClientBase.php => ApiClient.php} (93%) delete mode 100644 tests/modules/helfi_api_client_test/helfi_api_client_test.info.yml delete mode 100644 tests/modules/helfi_api_client_test/src/ApiClient.php rename tests/src/Unit/ApiClient/{ApiClientBaseTest.php => ApiClientTest.php} (91%) diff --git a/documentation/api-client.md b/documentation/api-client.md index bc29b55d..b42e202a 100644 --- a/documentation/api-client.md +++ b/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'), ``` diff --git a/helfi_api_base.services.yml b/helfi_api_base.services.yml index 8232b1ee..c9590c92 100644 --- a/helfi_api_base.services.yml +++ b/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' diff --git a/src/ApiClient/ApiClientBase.php b/src/ApiClient/ApiClient.php similarity index 93% rename from src/ApiClient/ApiClientBase.php rename to src/ApiClient/ApiClient.php index a93919b9..b7ae6697 100644 --- a/src/ApiClient/ApiClientBase.php +++ b/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; + } + } diff --git a/tests/modules/helfi_api_client_test/helfi_api_client_test.info.yml b/tests/modules/helfi_api_client_test/helfi_api_client_test.info.yml deleted file mode 100644 index 4042b4d9..00000000 --- a/tests/modules/helfi_api_client_test/helfi_api_client_test.info.yml +++ /dev/null @@ -1,6 +0,0 @@ -name: HELfi api client test -type: module -package: Custom -core_version_requirement: ^9 || ^10 -dependencies: - - helfi_api_base diff --git a/tests/modules/helfi_api_client_test/src/ApiClient.php b/tests/modules/helfi_api_client_test/src/ApiClient.php deleted file mode 100644 index 58d21c39..00000000 --- a/tests/modules/helfi_api_client_test/src/ApiClient.php +++ /dev/null @@ -1,43 +0,0 @@ -makeRequest($method, $url, $options); - } - - /** - * Expose protected method for testing. - * - * @throws \GuzzleHttp\Exception\GuzzleException - */ - public function exposedMakeRequestWithFixture(string $fixture, string $method, string $url, array $options = []): ApiResponse { - return $this->makeRequestWithFixture($fixture, $method, $url, $options); - } - - /** - * Expose protected method for testing. - * - * @throws \GuzzleHttp\Exception\GuzzleException - */ - public function exposedCache(string $key, callable $callback): CacheValue { - return $this->cache($key, $callback); - } - -} diff --git a/tests/src/Unit/ApiClient/ApiClientBaseTest.php b/tests/src/Unit/ApiClient/ApiClientTest.php similarity index 91% rename from tests/src/Unit/ApiClient/ApiClientBaseTest.php rename to tests/src/Unit/ApiClient/ApiClientTest.php index dceb6f6e..1523a985 100644 --- a/tests/src/Unit/ApiClient/ApiClientBaseTest.php +++ b/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;