From c850a203fb12958df67981ddb59823a87ab025b0 Mon Sep 17 00:00:00 2001 From: Joe Huss Date: Sun, 28 Jun 2026 12:46:15 -0400 Subject: [PATCH] feat(arr): inject async HTTP transport via ArrTransportInterface (F2b/B1/P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Decouple the four *arr clients from blocking cURL so event-loop consumers can inject a non-blocking transport. Closes findings B1/P1/CQ1/F2 (lib side). - New Arr\Transport\ArrTransportInterface: request(method,url,headers,?body) returns array{status:int, body:string}. - New default Arr\Transport\CurlArrTransport implementing the prior blocking cURL behaviour (cURL call moved out of AbstractArrClient). Documented as CLI/test only. - AbstractArrClient: appended OPTIONAL ?ArrTransportInterface $transport = null ctor param (BC-safe); all I/O flows through the transport; null falls back to CurlArrTransport so direct instantiation is unchanged. When a transport is injected, no curl_exec is reached. - ArrClientFactory: accepts + propagates an optional transport to every client. - composer.json: keep "zero I/O by charter" honest — ext-curl moved to suggest/ require-dev (only the bundled CurlArrTransport needs it); suggest workerman/http-client for event-loop consumers. - Tests: deterministic in-memory FakeArrTransport + InjectedTransportTest asserting the injected transport short-circuits cURL entirely (canned response against an unroutable URL), POST body encoding, status-error mapping, and a control test proving the default transport performs real cURL. Existing tests/Arr/* still pass with the default transport. No required method added to ArrClientInterface (transport is a ctor concern) — additive/minor, not breaking. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 30 +++++ composer.json | 8 +- src/Arr/AbstractArrClient.php | 63 ++++------ src/Arr/ArrClientFactory.php | 15 ++- src/Arr/Transport/ArrTransportInterface.php | 39 ++++++ src/Arr/Transport/CurlArrTransport.php | 94 ++++++++++++++ tests/Arr/Transport/FakeArrTransport.php | 51 ++++++++ tests/Arr/Transport/InjectedTransportTest.php | 119 ++++++++++++++++++ 8 files changed, 376 insertions(+), 43 deletions(-) create mode 100644 src/Arr/Transport/ArrTransportInterface.php create mode 100644 src/Arr/Transport/CurlArrTransport.php create mode 100644 tests/Arr/Transport/FakeArrTransport.php create mode 100644 tests/Arr/Transport/InjectedTransportTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 532d54b..0eb37d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,36 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm v0.10.x backward-compat default for legacy tokens (existing behaviour unchanged); consumers can migrate to the strict variant once every issuer emits `aud`. Additive, BC-safe. +- **`Arr` async-transport seam** (findings B1/P1, CQ1, F2) — the *arr clients now route + all HTTP I/O through an injectable transport, so the blocking cURL call lives behind a + seam and event-loop (Workerman/Webman) consumers can avoid it entirely, honouring the + package's "zero I/O" charter: + - New `interface Arr\Transport\ArrTransportInterface` with a single + `request(string $method, string $url, array $headers, ?string $body): array{status:int, body:string}` + method. Implementations return the status + raw body (they do NOT throw on non-2xx; + the client maps status codes). + - New default `final Arr\Transport\CurlArrTransport` carrying the **blocking** cURL + behaviour moved out of `AbstractArrClient::request()`. Documented as **CLI/test only**; + event-loop consumers MUST inject an async, non-blocking transport. + - `Arr\AbstractArrClient` gains an **optional, appended** constructor parameter + `?ArrTransportInterface $transport = null`; when null it falls back to + `new CurlArrTransport($timeout)`, so existing direct instantiation keeps working + unchanged. All requests are dispatched through the transport — no `curl_exec()` runs + when a transport is injected. + - `Arr\ArrClientFactory` gains an optional appended `?ArrTransportInterface $transport` + constructor parameter, propagated to every client it creates. + - **`ArrClientInterface` is unchanged** — the transport is a constructor concern only, + not a new interface method, so this is NOT a breaking change. + - `composer.json`: the misleading absolute "zero I/O" claim is reconciled — the + description now states the only bundled network code is the blocking `CurlArrTransport` + (CLI/test only) and event-loop consumers must inject an async transport. `ext-curl` + moved from `require` to `require-dev` + `suggest` (needed only for the default cURL + transport); added a `suggest` for `workerman/http-client` for consumers wiring an + async transport. + - **Consumer follow-up (Wave 1+):** phlix-server will add a `workerman/http-client`-backed + `WorkermanArrTransport` and inject it (via `ArrClientFactory`/direct construction) so + *arr calls stop stalling the worker; phlix-hub audits its `RequestManager` usage the + same way. Additive / BC-safe here. ### Changed - **`Arr\AbstractArrClient` extraction** (findings CQ2/CQ5) — the four near-identical diff --git a/composer.json b/composer.json index a056f26..056bf82 100644 --- a/composer.json +++ b/composer.json @@ -1,21 +1,25 @@ { "name": "detain/phlix-shared", - "description": "Shared interfaces, DTOs, event names, and protocol types used by both phlix-server and phlix-hub. Composer-installable, PHP 8.3+, zero I/O.", + "description": "Shared interfaces, DTOs, event names, and protocol types used by both phlix-server and phlix-hub. Composer-installable, PHP 8.3+, zero I/O by charter — the only bundled network code is the blocking CurlArrTransport (CLI/test only); event-loop consumers MUST inject an async ArrTransportInterface.", "type": "library", "license": "MIT", "require": { "php": "^8.3", - "ext-curl": "*", "psr/container": "^2.0", "psr/event-dispatcher": "^1.0", "psr/log": "^3.0" }, "require-dev": { + "ext-curl": "*", "phpunit/phpunit": "^10.0", "phpstan/phpstan": "^2.0", "squizlabs/php_codesniffer": "^3.10", "vimeo/psalm": "^5.0" }, + "suggest": { + "ext-curl": "Required only for the bundled blocking Arr\\Transport\\CurlArrTransport (CLI/test use). Event-loop (Workerman/Webman) consumers should inject an async ArrTransportInterface instead and do not need ext-curl.", + "workerman/http-client": "Event-loop (Workerman/Webman) consumers should inject a workerman/http-client-backed Arr\\Transport\\ArrTransportInterface so *arr API calls do not block the worker; the bundled CurlArrTransport is blocking and for CLI/test only." + }, "autoload": { "psr-4": { "Phlix\\Shared\\": "src/" diff --git a/src/Arr/AbstractArrClient.php b/src/Arr/AbstractArrClient.php index 82d9e77..6151398 100644 --- a/src/Arr/AbstractArrClient.php +++ b/src/Arr/AbstractArrClient.php @@ -4,21 +4,25 @@ namespace Phlix\Shared\Arr; +use Phlix\Shared\Arr\Transport\ArrTransportInterface; +use Phlix\Shared\Arr\Transport\CurlArrTransport; use Psr\Log\LoggerInterface; use RuntimeException; /** * Shared base for the *arr HTTP API clients (Radarr/Sonarr/Prowlarr/Bazarr). * - * Centralises the constructor, header building, the GET/POST/PUT/DELETE cURL + * Centralises the constructor, header building, the GET/POST/PUT/DELETE request * methods, and the per-status-code error mapping that were previously * duplicated across each client. Subclasses provide only their * endpoint-specific methods plus {@see AbstractArrClient::vendorName()} used in * error messages (e.g. "Radarr API error: HTTP 500"). * - * NOTE: This is intentionally still blocking cURL. A later step (F2b) swaps the - * transport behind an injected seam; this class only removes the duplication so - * that swap happens in one place. + * All HTTP I/O is delegated to an injected {@see ArrTransportInterface}. When none + * is supplied the class falls back to the bundled, **blocking** {@see CurlArrTransport} + * so direct instantiation in CLI scripts/tests keeps working unchanged. Event-loop + * consumers (Workerman/Webman) MUST inject an async, non-blocking transport so a slow + * *arr instance never stalls the worker — see {@see ArrTransportInterface}. * * @package Phlix\Shared\Arr * @since 0.4.0 @@ -29,6 +33,7 @@ abstract class AbstractArrClient protected string $apiKey; protected ?LoggerInterface $logger; protected int $timeout; + protected ArrTransportInterface $transport; /** * Creates a new *arr client. @@ -37,17 +42,22 @@ abstract class AbstractArrClient * @param string $apiKey API key for authentication. * @param LoggerInterface|null $logger Optional logger instance. * @param int $timeout Request timeout in seconds (default 30). + * @param ArrTransportInterface|null $transport Optional HTTP transport. When null, + * a blocking {@see CurlArrTransport} (CLI/test only) is used. Event-loop + * consumers MUST inject an async, non-blocking transport. */ public function __construct( string $baseUrl, string $apiKey, ?LoggerInterface $logger = null, - int $timeout = 30 + int $timeout = 30, + ?ArrTransportInterface $transport = null ) { $this->baseUrl = rtrim($baseUrl, '/'); $this->apiKey = $apiKey; $this->logger = $logger; $this->timeout = $timeout; + $this->transport = $transport ?? new CurlArrTransport($timeout); } /** @@ -108,6 +118,10 @@ protected function delete(string $path): array /** * Executes an HTTP request against the *arr instance and decodes the JSON body. * + * The wire I/O is delegated to the injected {@see ArrTransportInterface}; this + * method only builds the request, maps status codes to exceptions, and decodes + * the JSON body. No cURL call happens here when a non-default transport is injected. + * * @param string $method One of GET/POST/PUT/DELETE. * @param string $path Request path. * @param array|null $body JSON-serializable body for POST/PUT; null otherwise. @@ -117,42 +131,15 @@ protected function delete(string $path): array private function request(string $method, string $path, ?array $body): array { $url = $this->baseUrl . $path; - assert($url !== ''); - - $options = [ - CURLOPT_URL => $url, - CURLOPT_RETURNTRANSFER => true, - CURLOPT_TIMEOUT => $this->timeout, - CURLOPT_HTTPHEADER => $this->buildHeaders(), - ]; - - if ($method === 'POST') { - $options[CURLOPT_POST] = true; - $options[CURLOPT_POSTFIELDS] = $this->encodeBody($body ?? []); - } elseif ($method === 'PUT') { - $options[CURLOPT_CUSTOMREQUEST] = 'PUT'; - $options[CURLOPT_POSTFIELDS] = $this->encodeBody($body ?? []); - } elseif ($method === 'DELETE') { - $options[CURLOPT_CUSTOMREQUEST] = 'DELETE'; - } - $ch = curl_init(); - if ($ch === false) { - throw new RuntimeException('curl_init() failed'); + $encodedBody = null; + if ($method === 'POST' || $method === 'PUT') { + $encodedBody = $this->encodeBody($body ?? []); } - curl_setopt_array($ch, $options); - - /** @var string|false */ - $responseBody = curl_exec($ch); - $httpCode = (int) curl_getinfo($ch, CURLINFO_HTTP_CODE); - $curlErrno = curl_errno($ch); - $curlError = curl_error($ch); - curl_close($ch); - - if ($responseBody === false || $curlErrno !== 0) { - throw new RuntimeException('cURL error: ' . $curlError, $curlErrno); - } + $response = $this->transport->request($method, $url, $this->buildHeaders(), $encodedBody); + $httpCode = $response['status']; + $responseBody = $response['body']; $vendor = $this->vendorName(); diff --git a/src/Arr/ArrClientFactory.php b/src/Arr/ArrClientFactory.php index 185fa15..04928f1 100644 --- a/src/Arr/ArrClientFactory.php +++ b/src/Arr/ArrClientFactory.php @@ -4,11 +4,17 @@ namespace Phlix\Shared\Arr; +use Phlix\Shared\Arr\Transport\ArrTransportInterface; use Psr\Log\LoggerInterface; /** * Factory for creating Sonarr/Radarr API clients from config. * + * An optional {@see ArrTransportInterface} may be supplied; when present it is + * propagated to every created client so event-loop consumers can wire a single + * async, non-blocking transport once. When omitted, clients fall back to the + * bundled blocking {@see \Phlix\Shared\Arr\Transport\CurlArrTransport} (CLI/test only). + * * @package Phlix\Shared\Arr * @since 0.4.0 */ @@ -19,9 +25,12 @@ class ArrClientFactory * sonarr?: array{url?: string, api_key?: string, enabled?: bool}, * radarr?: array{url?: string, api_key?: string, enabled?: bool} * } $config Configuration array with sonarr/radarr sections. + * @param ArrTransportInterface|null $transport Optional HTTP transport propagated to + * every created client. When null, clients use the default blocking cURL transport. */ public function __construct( - private readonly array $config + private readonly array $config, + private readonly ?ArrTransportInterface $transport = null ) { } @@ -47,7 +56,7 @@ public function createSonarrClient(?LoggerInterface $logger = null): ?SonarrClie return null; } - return new SonarrClient($url, $apiKey, $logger); + return new SonarrClient($url, $apiKey, $logger, 30, $this->transport); } /** @@ -72,6 +81,6 @@ public function createRadarrClient(?LoggerInterface $logger = null): ?RadarrClie return null; } - return new RadarrClient($url, $apiKey, $logger); + return new RadarrClient($url, $apiKey, $logger, 30, $this->transport); } } diff --git a/src/Arr/Transport/ArrTransportInterface.php b/src/Arr/Transport/ArrTransportInterface.php new file mode 100644 index 0000000..61ea8b0 --- /dev/null +++ b/src/Arr/Transport/ArrTransportInterface.php @@ -0,0 +1,39 @@ + $headers Headers as raw `Name: value` strings. + * @param string|null $body Raw request body for write methods; null for none. + * @return array{status:int, body:string} HTTP status code and raw response body. + */ + public function request(string $method, string $url, array $headers, ?string $body): array; +} diff --git a/src/Arr/Transport/CurlArrTransport.php b/src/Arr/Transport/CurlArrTransport.php new file mode 100644 index 0000000..fc397e5 --- /dev/null +++ b/src/Arr/Transport/CurlArrTransport.php @@ -0,0 +1,94 @@ + $headers + * @return array{status:int, body:string} + * @throws RuntimeException On a transport-level cURL failure. + */ + public function request(string $method, string $url, array $headers, ?string $body): array + { + if ($url === '') { + throw new RuntimeException('CurlArrTransport: empty request URL'); + } + if ($method === '') { + throw new RuntimeException('CurlArrTransport: empty request method'); + } + + $options = [ + CURLOPT_URL => $url, + CURLOPT_RETURNTRANSFER => true, + CURLOPT_TIMEOUT => $this->timeout, + CURLOPT_HTTPHEADER => $headers, + ]; + + if ($method === 'POST') { + $options[CURLOPT_POST] = true; + $options[CURLOPT_POSTFIELDS] = (string) $body; + } elseif ($method === 'PUT') { + $options[CURLOPT_CUSTOMREQUEST] = 'PUT'; + $options[CURLOPT_POSTFIELDS] = (string) $body; + } elseif ($method === 'DELETE') { + $options[CURLOPT_CUSTOMREQUEST] = 'DELETE'; + } elseif ($method !== 'GET') { + $options[CURLOPT_CUSTOMREQUEST] = $method; + if ($body !== null && $body !== '') { + $options[CURLOPT_POSTFIELDS] = $body; + } + } + + $ch = curl_init(); + if ($ch === false) { + throw new RuntimeException('curl_init() failed'); + } + + curl_setopt_array($ch, $options); + + /** @var string|false $responseBody */ + $responseBody = curl_exec($ch); + $httpCode = (int) curl_getinfo($ch, CURLINFO_HTTP_CODE); + $curlErrno = curl_errno($ch); + $curlError = curl_error($ch); + curl_close($ch); + + if ($responseBody === false || $curlErrno !== 0) { + throw new RuntimeException('cURL error: ' . $curlError, $curlErrno); + } + + return ['status' => $httpCode, 'body' => $responseBody]; + } +} diff --git a/tests/Arr/Transport/FakeArrTransport.php b/tests/Arr/Transport/FakeArrTransport.php new file mode 100644 index 0000000..4df94bc --- /dev/null +++ b/tests/Arr/Transport/FakeArrTransport.php @@ -0,0 +1,51 @@ +, body:string|null}> */ + public array $calls = []; + + /** + * @param int $status Canned HTTP status to return. + * @param string $body Canned response body to return. + */ + public function __construct( + private readonly int $status = 200, + private readonly string $body = '[]' + ) { + } + + /** + * {@inheritDoc} + * + * @param array $headers + * @return array{status:int, body:string} + */ + public function request(string $method, string $url, array $headers, ?string $body): array + { + $this->calls[] = [ + 'method' => $method, + 'url' => $url, + 'headers' => array_values($headers), + 'body' => $body, + ]; + + return ['status' => $this->status, 'body' => $this->body]; + } +} diff --git a/tests/Arr/Transport/InjectedTransportTest.php b/tests/Arr/Transport/InjectedTransportTest.php new file mode 100644 index 0000000..7c5da15 --- /dev/null +++ b/tests/Arr/Transport/InjectedTransportTest.php @@ -0,0 +1,119 @@ +getMovies(); + + $this->assertSame([['id' => 1, 'title' => 'Test Movie']], $result); + $this->assertCount(1, $fake->calls, 'Exactly one request must flow through the injected transport.'); + $this->assertSame('GET', $fake->calls[0]['method']); + $this->assertSame(self::UNROUTABLE_BASE_URL . '/api/v3/movie', $fake->calls[0]['url']); + $this->assertNull($fake->calls[0]['body']); + $this->assertContains('X-Api-Key: test-api-key', $fake->calls[0]['headers']); + } + + public function testInjectedTransportReceivesEncodedPostBody(): void + { + $fake = new FakeArrTransport(201, '{"id":10,"tmdbId":123456}'); + + $client = new RadarrClient( + self::UNROUTABLE_BASE_URL, + 'test-api-key', + null, + 30, + $fake + ); + + $result = $client->addMovie(123456, 2, '/movies', true); + + $this->assertSame(10, $result['id']); + $this->assertSame('POST', $fake->calls[0]['method']); + $this->assertNotNull($fake->calls[0]['body']); + /** @var string $sentBody */ + $sentBody = $fake->calls[0]['body']; + $decoded = json_decode($sentBody, true); + $this->assertIsArray($decoded); + $this->assertSame(123456, $decoded['tmdbId']); + } + + public function testInjectedTransportStatusErrorsStillMap(): void + { + $fake = new FakeArrTransport(401, ''); + + $client = new RadarrClient( + self::UNROUTABLE_BASE_URL, + 'test-api-key', + null, + 30, + $fake + ); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('Radarr API authentication failed (401)'); + $client->getMovies(); + } + + /** + * Control: with NO injected transport the client falls back to the blocking + * {@see CurlArrTransport}, which DOES perform real cURL — so an unroutable URL + * raises a transport-level cURL error. This proves the fake genuinely replaces + * cURL rather than the call being a no-op either way. + */ + public function testDefaultTransportPerformsRealCurl(): void + { + $client = new RadarrClient(self::UNROUTABLE_BASE_URL, 'test-api-key', null, 1); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('cURL error'); + $client->getMovies(); + } + + public function testDefaultTransportIsCurlArrTransport(): void + { + $client = new RadarrClient('http://localhost:7878', 'k'); + + $reflection = new \ReflectionClass($client); + $transportProp = $reflection->getProperty('transport'); + $transportProp->setAccessible(true); + + $this->assertInstanceOf(CurlArrTransport::class, $transportProp->getValue($client)); + } +}