PAT-1866 Add php-api-client-base shared base for service API clients#513
Conversation
…in KeboolaServiceAccountAuthenticatorTest tempnam() returns string, not non-empty-string; add @phpstan-ignore-next-line on the three constructor call-sites that pass tempnam results so level-max CI passes.
…Decider; add retry-path tests
…ientOptions Remove the authenticator field from the options object in preparation for making it a first-class ApiClient constructor argument. Rename the test file to match the new class name and drop the authenticator-related assertions.
…ient argument ApiClient now accepts the authenticator as an explicit second constructor parameter rather than a field on ApiClientOptions. This makes auth intent clear at the call site and decouples the options bag from authentication concerns. Middleware push order (retry → auth → log) is preserved so the authenticator re-runs on every retry attempt.
…AuthAuthenticator Make `RequestAuthenticatorInterface $authenticator` a mandatory second constructor argument on `ApiClient` (removing the `= null` default and the null-guard around the mapRequest push). Add `NoAuthAuthenticator` as the explicit no-op authenticator for unauthenticated clients, covered by a new TDD test. Update all existing tests to pass `new NoAuthAuthenticator()` where the authenticator was previously omitted, and document the new signature in the README.
…eStatusCodes ApiClient args These two knobs describe the service's API contract (error shape and which status codes are retryable), not per-call caller preferences. Move them from ApiClientOptions to explicit constructor parameters on ApiClient so that the service facade (not the library consumer) controls them.
…essageResolverInterface with DefaultErrorMessageResolver
…nseData take an untyped array Decoded JSON is untyped; an explicit array<...> value type forces every model to re-annotate $data to dodge phpstan's cast-from-mixed rule. An untyped array lets implementers cast the values they need without per-method annotations.
…(backoff, timeouts)
…lesce to NullLogger in ApiClient
|
A zvažovals možnost založit to Symfony\HTTPClientovi ? To by totiž imho spoustu věcí zjednodušilo |
…ervice The test pipeline runs 'docker compose run dev-php-api-client-base', which needs a matching compose service; every other lib has a dev-<lib> block but the new base lib's was missing (CI failed with 'no such service').
keboola-pr-reviewer-bot
left a comment
keboola-pr-reviewer-bot
left a comment
There was a problem hiding this comment.
Verdict: needs_human (risk 2/5) · profile ajda
This PR adds new classes whose names contain Authenticator and that handle Keboola auth tokens, triggering the always-needs-human auth & credentials rule.
Concerns:
libs/php-api-client-base/src/Auth/StorageApiTokenAuthenticator.php: Class name containsAuthenticatorand wraps Storage API token — always-needs-human per policy auth rule.libs/php-api-client-base/src/Auth/ManageApiTokenAuthenticator.php: Class name containsAuthenticatorand wraps Manage API token — always-needs-human per policy auth rule.libs/php-api-client-base/src/Auth/KeboolaServiceAccountAuthenticator.php: Class name containsAuthenticatorand reads/passes a projected SA token — always-needs-human per policy auth rule.
|
https://keboolaglobal.slack.com/archives/C055HSMKX51/p1781089998100289 Zkusil jsem pripravit i variantu, ktera pouziva Symfony HTTP Client misto Guzzle #517, ale na prvni dobrou se nezda tam byt nejaky killer vylepsovak a naopak je tam riziko, ze se bude v nekterych situacich chovat jinak (timeouty, retries, erro handling, Exception chain), takze prozatim bych zustal u Guzzle. Nicmene tu experimentalni vetev zatim nechavam a muzem s ni pak zkusit treba jednoho klienta na jednu service nahodit, jak se bude chovat. |
| private function doSendRequest(RequestInterface $request, array $options = []): ResponseInterface | ||
| { | ||
| try { | ||
| return $this->httpClient->send($request, $options); |
There was a problem hiding this comment.
Když např. KeboolaServiceAccountAuthenticator nebude moct přečíst token soubor, tak nepůjde GuzzleException, nahoru projde RuntimeException namísto ClientException, chytáme ale ClientException, takže tohle selhání se nezachytíme v services.
A pak se taky u toho nedělá retry, to vyhození chyby z Auth middlewaru se do retry nedostane. Auth chyby teda "obchází retry i error" handling naráz.
Vidím to na dvě věci:
- catch a konverze na
ClientException, - throw Auth exception uvnitř stacku převést na promise rejection, pak ho uvidí i
RetryDecider, např. dává smysl při rotaci SA tokenu
There was a problem hiding this comment.
Fixed in cb1352c. Auth is now a middleware that turns a thrown authenticator error into a rejected promise, so it flows through RetryDecider (a transient projected-SA-token read during rotation gets retried) and doSendRequest maps it to ClientException. Added a regression test asserting the auth throw surfaces as ClientException and the authenticator is re-invoked per attempt.
| $code !== null => 'HTTP ' . $code, | ||
| default => 'unknown', | ||
| }, | ||
| $retries, |
There was a problem hiding this comment.
| $retries, | |
| $retries + 1, |
There was a problem hiding this comment.
Fixed in cb1352c — $retries + 1, so the retry log is 1-based.
| Pick the authenticator matching the service's scheme, or implement | ||
| `RequestAuthenticatorInterface` for a service-specific scheme (e.g. azure's | ||
| OAuth). `Content-Type` is set per request on calls with a body; the only Guzzle | ||
| default header is `User-Agent` (set via `ApiClientOptions::$userAgent`). |
There was a problem hiding this comment.
Tohle asi jen pro další migrace klientů, ale pohlídal bych, že ty hlavičky se nastavují, aby náhodou neodešel request bez content-type.
There was a problem hiding this comment.
Dropped the Content-Type note entirely in cb1352c. It was leftover from the clients' earlier state; setting per-request headers is just normal client hygiene, not worth documenting in the base.
…+ ClientException Review on #513: an authenticator that throws (e.g. a projected SA token unreadable during rotation) escaped the handler stack as a raw RuntimeException, bypassing both retry and error handling. Auth is now a middleware that converts the throw into a rejected promise (so RetryDecider sees it) and doSendRequest maps it to ClientException. Also fix the off-by-one in the RetryDecider retry log (1-based), and drop the Content-Type note from the README.
4 participants
Release Notes
https://linear.app/keboola/issue/PAT-1866/unified-api-client-base
TL;DR Vytahl jsem "base clienta", co jsme rozkopirovavali mezi poslednima API client libkama do sdilenyho package. Ukazka rebase koncovych API klientu:
Introduces
keboola/php-api-client-base(Keboola\ApiClientBase), a shared base for Keboola service API clients. The platform-libraries API clients (git-service,vault,sandboxes-service,sync-actions) each re-implement the same skeleton — a Guzzle wrapper, a config object, a retry decider, a JSON helper, a response-model contract, a client exception, and an auth abstraction — with no shared dependency (the utility classes are near-identical across clients). This lib extracts that skeleton so the mechanics live, and get fixed and tested, in one place.git-servicewas the reference implementation.The base provides
ApiClient(per-request auth, retry, logging, response-to-model mapping, error normalization),ApiClientConfiguration,RetryDecider(with configurable retryable status codes),Json,ResponseModelInterface,Exception\ClientException, and anAuth\RequestAuthenticatorInterface(a PSR-7 request-decorator contract) with ready authenticators for the Keboola auth schemes (X-StorageApi-Token,X-KBC-ManageApiToken, and a projected service-account token →X-Kubernetes-Authorization). Auth runs inside the retry loop, so file-backed tokens are re-resolved on every attempt.Key changes:
libs/php-api-client-base(PHP 8.2, Guzzle 7); 32 tests, PHPStan level max, Keboola coding standard.keboola/php-api-client-baserepo and Azure endpoint to exist first.This is the first of a stacked series; follow-up PRs migrate the five clients onto it (starting with
vault).Plans for customer communication
None.
Impact analysis
No end-user impact. New library with no consumers yet. Note: the follow-up client migrations will
require keboola/php-api-client-base, so the base must be split-published (new read-only repo + Azure endpoint +split-libraryjob) before any migrated client can be released. This PR only registers the lib for CI testing.Change type
Refactoring
Justification
Deduplicate the copy-pasted HTTP/auth/retry skeleton across the platform-libraries API clients (PAT-1866).
Deployment
Merge & automatic deploy.
Rollback plan
Revert of this PR.
Post release support plan
None.