diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index e32d78f..62a9b69 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,3 +1,38 @@ +# Alfa Bank SDK - Release Notes v1.1.0 + +## โš ๏ธ Behaviour change: safe retries + +Saloon retries a request whenever an exception is thrown (`handleRetry` returns `true` by +default) and both SDK connectors set `tries = 3`. As a result, non-idempotent operations +(`POST /payoffs`, `PATCH /payoffs/{id}/accept`, SBP `QrcReversal`) were sent to the bank up to +three times while the calling code believed it had made a single request. For payouts a repeated +POST is a duplicate-payment risk, not a harmless retry โ€” the SDK now stops this at the connector +level, because Saloon only retries when the per-send callable, the request **and** the connector +all allow it. + +The new trait `Apoloz\AlfaBank\Traits\HasSafeRetries` replaces the default with an explicit policy: + +1. A deterministic bank verdict (4xx/5xx) is never retried. +2. An explicit deny-list of money operations is never retried. +3. An explicit allow-list keeps retries for read-only SBP operations (that API uses POST for reads). +4. By default only read-only HTTP methods (GET/HEAD/OPTIONS) are retried. + +Every decision is reported through the connector's PSR-3 logger (`[alfa-sdk][retry]`), which makes +the number of physical requests countable per operation via the `X-Request-ID` header. + +## ๐Ÿž Fixes + +- Restored the missing `HasStringBody` and `CmsSigner` imports in + `Sbp\Requests\DigitalRuble\CashLink\*` requests: the classes failed with a fatal error when loaded. + +## ๐Ÿงช Tests + +- Added `tests/Feature/RetryPolicyTest.php` (23 tests): money operations and bank verdicts are never + repeated, read-only operations keep their retries, and the real Saloon retry loop is asserted + (a failing non-idempotent POST reaches the wire exactly once). + +--- + # Alfa Bank SDK - Release Notes v1.0.0 ## ๐Ÿš€ Overview diff --git a/src/Core/AlfaBankConnector.php b/src/Core/AlfaBankConnector.php index 039cee0..bc09e78 100644 --- a/src/Core/AlfaBankConnector.php +++ b/src/Core/AlfaBankConnector.php @@ -13,7 +13,12 @@ use Apoloz\AlfaBank\Http\Middleware\LoggingResponseMiddleware; use Apoloz\AlfaBank\Http\Middleware\RequestIdMiddleware; use Apoloz\AlfaBank\Http\Middleware\VerifyJwsResponseMiddleware; use Apoloz\AlfaBank\Http\Pagination\AlfaOffsetPaginator; +use Apoloz\AlfaBank\Requests\NominalAccounts\Payouts\CreatePayoffRequest; +use Apoloz\AlfaBank\Requests\NominalAccounts\Payouts\CreateTaxRefundRequest; +use Apoloz\AlfaBank\Requests\NominalAccounts\Payouts\PayoffAcceptRequest; +use Apoloz\AlfaBank\Requests\NominalAccounts\Payouts\TaxRefundAcceptRequest; use Apoloz\AlfaBank\Traits\HasAlfaErrorHandling; +use Apoloz\AlfaBank\Traits\HasSafeRetries; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; use Saloon\Helpers\OAuth2\OAuthConfig; @@ -26,7 +31,6 @@ use Saloon\PaginationPlugin\OffsetPaginator; use Saloon\Traits\OAuth2\AuthorizationCodeGrant; use Saloon\Traits\Plugins\AlwaysThrowOnErrors; use Saloon\Traits\Plugins\HasTimeout; -use Saloon\Traits\RequestProperties\HasTries; class AlfaBankConnector extends Connector implements HasPagination { @@ -34,7 +38,9 @@ class AlfaBankConnector extends Connector implements HasPagination use AuthorizationCodeGrant; use HasAlfaErrorHandling; use HasTimeout; - use HasTries; + // Replaces the HasTries default handleRetry (always retry) with a safe policy: + // transport failures are retried for read-only requests only, bank verdicts never. + use HasSafeRetries; public function __construct( protected AlfaConfig $alfaConfig, @@ -45,6 +51,15 @@ class AlfaBankConnector extends Connector implements HasPagination $this->tries = 3; $this->retryInterval = 500; $this->logger = $this->logger ?? new NullLogger; + + // Money operations: repeating them is a duplicate-payment risk, so they are + // denied explicitly in addition to the read-only-method rule. + $this->denyRetriesFor([ + CreatePayoffRequest::class, + PayoffAcceptRequest::class, + CreateTaxRefundRequest::class, + TaxRefundAcceptRequest::class, + ]); } public function resolveBaseUrl(): string diff --git a/src/Sbp/SbpConnector.php b/src/Sbp/SbpConnector.php index 0dff45d..ccb99b1 100644 --- a/src/Sbp/SbpConnector.php +++ b/src/Sbp/SbpConnector.php @@ -9,7 +9,20 @@ use Apoloz\AlfaBank\Http\Middleware\LoggingRequestMiddleware; use Apoloz\AlfaBank\Http\Middleware\LoggingResponseMiddleware; use Apoloz\AlfaBank\Http\Middleware\RequestIdMiddleware; use Apoloz\AlfaBank\Http\Pagination\AlfaOffsetPaginator; +use Apoloz\AlfaBank\Sbp\Requests\CashLink\GetCashQrHistoryRequest; +use Apoloz\AlfaBank\Sbp\Requests\CashLink\GetCashQrStatusRequest; +use Apoloz\AlfaBank\Sbp\Requests\DigitalRuble\CashLink\GetCashUniQrStatusRequest; +use Apoloz\AlfaBank\Sbp\Requests\DigitalRuble\GetUniQrStatusRequest; +use Apoloz\AlfaBank\Sbp\Requests\Management\GetBlockedQrListRequest; +use Apoloz\AlfaBank\Sbp\Requests\Registry\GetBankListRequest; +use Apoloz\AlfaBank\Sbp\Requests\Reversal\GetQrcReversalDataRequest; +use Apoloz\AlfaBank\Sbp\Requests\Reversal\GetQrcReversalHistoryRequest; +use Apoloz\AlfaBank\Sbp\Requests\Reversal\GetQrcReversalStatusRequest; +use Apoloz\AlfaBank\Sbp\Requests\Reversal\QrcReversalRequest; +use Apoloz\AlfaBank\Sbp\Requests\Status\GetQrcStatusRequest; +use Apoloz\AlfaBank\Sbp\Requests\Subscription\GetQrSubStatusRequest; use Apoloz\AlfaBank\Traits\HasAlfaErrorHandling; +use Apoloz\AlfaBank\Traits\HasSafeRetries; use Apoloz\AlfaBank\Utils\CmsSigner; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; @@ -20,14 +33,14 @@ use Saloon\PaginationPlugin\Contracts\HasPagination; use Saloon\PaginationPlugin\OffsetPaginator; use Saloon\Traits\Plugins\AlwaysThrowOnErrors; use Saloon\Traits\Plugins\HasTimeout; -use Saloon\Traits\RequestProperties\HasTries; class SbpConnector extends Connector implements HasPagination { use AlwaysThrowOnErrors; use HasAlfaErrorHandling; use HasTimeout; - use HasTries; + // Replaces the HasTries default handleRetry (always retry) with a safe policy. + use HasSafeRetries; public function __construct( protected AlfaConfig $alfaConfig, @@ -38,6 +51,32 @@ class SbpConnector extends Connector implements HasPagination $this->tries = 3; $this->retryInterval = 500; $this->logger = $this->logger ?? new NullLogger; + + // The SBP API uses POST even for read-only operations, so the read-only method + // rule alone would silently disable retries for status/list queries. These are + // pure reads: repeating them creates no entity at the bank. + $this->allowRetriesFor([ + GetQrcStatusRequest::class, + GetQrcReversalStatusRequest::class, + GetQrcReversalHistoryRequest::class, + GetCashQrStatusRequest::class, + GetCashQrHistoryRequest::class, + GetQrSubStatusRequest::class, + GetBlockedQrListRequest::class, + GetBankListRequest::class, + GetUniQrStatusRequest::class, + GetCashUniQrStatusRequest::class, + ]); + + // Money-mutating operations and calls that create/modify a document on the + // bank side are denied explicitly. GetQrcReversalDataRequest is deliberately + // NOT retried: the bank treats its message as the "previous message" the + // following QRCreversal is matched against (EBD 44 / I14044), so it is not a + // pure read despite its name. + $this->denyRetriesFor([ + QrcReversalRequest::class, + GetQrcReversalDataRequest::class, + ]); } public function resolveBaseUrl(): string diff --git a/src/Traits/HasSafeRetries.php b/src/Traits/HasSafeRetries.php new file mode 100644 index 0000000..33eef96 --- /dev/null +++ b/src/Traits/HasSafeRetries.php @@ -0,0 +1,147 @@ + +// +trait HasSafeRetries +{ + // HasTries provides $tries, $retryInterval and the retry loop contract. + // We replace its handleRetry default (always true) with the policy below. + use HasTries; + + /** + * HTTP methods whose repetition is idempotent by specification: they read state + * and never create or mutate a bank entity. + * + * @var list<\Saloon\Enums\Method> + */ + protected const RETRYABLE_METHODS = [Method::GET, Method::HEAD, Method::OPTIONS]; + + /** + * Explicit deny-list. Wins over every other rule: keeps a non-idempotent + * operation safe even if it is ever declared with a read-only HTTP method. + * + * @var list> + */ + protected array $nonRetryableRequests = []; + + /** + * Explicit allow-list for safe operations that are not read-only by method. + * + * @var list> + */ + protected array $retryableRequests = []; + + /** + * Retry decision, evaluated by Saloon inside the retry loop. + * + * Called for every caught FatalRequestException (transport: timeout, DNS, TLS) + * and RequestException (HTTP verdict: 4xx/5xx). + */ + public function handleRetry(FatalRequestException|RequestException $exception, Request $request): bool + { + $decision = $this->shouldRetryRequest($exception, $request); + + $this->logRetryDecision($exception, $request, $decision); + + return $decision; + } + + /** + * Policy itself, free of side effects (used directly by tests and subclasses). + * + * Rules, in priority order: + * 1. The bank answered with a verdict (4xx/5xx) โ€” never retry: repeating a + * deterministic rejection only multiplies requests. + * 2. Explicit deny-list โ€” never retry. + * 3. Explicit allow-list โ€” retry on transport failure. + * 4. Default โ€” retry read-only methods only (GET/HEAD/OPTIONS). + */ + public function shouldRetryRequest(FatalRequestException|RequestException $exception, Request $request): bool + { + // Rule 1: bank verdict, not a transport failure. + if ($exception instanceof RequestException) { + return false; + } + + $requestClass = $request::class; + + // Rule 2: explicit deny-list wins over everything. + if (in_array($requestClass, $this->nonRetryableRequests, true)) { + return false; + } + + // Rule 3: explicit allow-list. + if (in_array($requestClass, $this->retryableRequests, true)) { + return true; + } + + // Rule 4: read-only methods only. + return in_array($request->getMethod(), self::RETRYABLE_METHODS, true); + } + + /** + * Mark requests as non-retryable (operations that create or mutate bank entities). + * + * @param list> $requestClasses + */ + public function denyRetriesFor(array $requestClasses): static + { + $this->nonRetryableRequests = array_values(array_unique([...$this->nonRetryableRequests, ...$requestClasses])); + + return $this; + } + + /** + * Mark requests as retryable even when their HTTP method is not read-only. + * + * @param list> $requestClasses + */ + public function allowRetriesFor(array $requestClasses): static + { + $this->retryableRequests = array_values(array_unique([...$this->retryableRequests, ...$requestClasses])); + + return $this; + } + + /** + * Observability: record every retry decision through the connector's PSR-3 logger + * (in the Laravel application this is the finance_alfabank channel). + * + * X-Request-ID is stable across the attempts of one operation, so the number of + * physical requests per operation is 1 + the number of records sharing that id. + * + * Requires the using connector to expose $logger (both SDK connectors do). + */ + protected function logRetryDecision( + FatalRequestException|RequestException $exception, + Request $request, + bool $decision + ): void { + if ($this->logger === null) { + return; + } + + $this->logger->warning('[alfa-sdk][retry]', [ + 'connector' => static::class, + 'request' => $request::class, + 'http_method' => $request->getMethod()->value, + 'exception' => $exception::class, + 'http_status' => $exception instanceof RequestException ? $exception->getResponse()->status() : null, + 'decision' => $decision ? 'retry' : 'stop', + 'request_id' => $exception instanceof RequestException + ? $exception->getResponse()->header('X-Request-ID') + : $exception->getPendingRequest()->headers()->get('X-Request-ID'), + ]); + } +} +// diff --git a/tests/Feature/RetryPolicyTest.php b/tests/Feature/RetryPolicyTest.php new file mode 100644 index 0000000..651b65b --- /dev/null +++ b/tests/Feature/RetryPolicyTest.php @@ -0,0 +1,407 @@ + + +/** + * PSR-3 logger double: the SDK is framework-agnostic, so retry decisions are asserted + * on the records the connector sends to its logger (in the app: channel finance_alfabank). + */ +class RetryRecordingLogger extends AbstractLogger +{ + /** @var list}> */ + public array $records = []; + + public function log($level, string|Stringable $message, array $context = []): void + { + $this->records[] = [ + 'level' => $level, + 'message' => (string) $message, + 'context' => $context, + ]; + } +} + +/** + * Sender that fails on every attempt with a transport error (timeout/DNS/TLS), + * which is exactly the case where Saloon's retry loop asks the connector. + * Counts physical attempts so a test can prove how many requests reached the wire. + */ +class RetryCountingFailingSender extends GuzzleSender +{ + public static int $attempts = 0; + + public function send(PendingRequest $pendingRequest): Response + { + self::$attempts++; + + throw new FatalRequestException(new RuntimeException('Connection timed out'), $pendingRequest); + } +} + +/** + * Requests that create or move money. Repeating any of them is a duplicate-payment + * risk, so this list is enumerated on purpose: if a money operation ever becomes + * retryable, this test must fail and force a conscious review. + * + * @return array}> + */ +function retryMoneyOperations(): array +{ + return [ + 'BaaS: POST /payoffs' => [CreatePayoffRequest::class], + 'BaaS: PATCH accept' => [PayoffAcceptRequest::class], + 'BaaS: POST tax refund' => [CreateTaxRefundRequest::class], + 'BaaS: PATCH tax refund accept' => [TaxRefundAcceptRequest::class], + 'SBP: QRCreversal' => [QrcReversalRequest::class], + ]; +} + +/** + * SBP uses POST even for read-only endpoints. These calls are safe to repeat because + * they create nothing at the bank; the list is duplicated here deliberately so that + * widening the connector's allow-list requires updating the test too. + * + * @return list> + */ +function retrySbpReadOnlyPostOperations(): array +{ + return [ + Apoloz\AlfaBank\Sbp\Requests\Status\GetQrcStatusRequest::class, + Apoloz\AlfaBank\Sbp\Requests\Reversal\GetQrcReversalStatusRequest::class, + Apoloz\AlfaBank\Sbp\Requests\Reversal\GetQrcReversalHistoryRequest::class, + Apoloz\AlfaBank\Sbp\Requests\CashLink\GetCashQrStatusRequest::class, + Apoloz\AlfaBank\Sbp\Requests\CashLink\GetCashQrHistoryRequest::class, + Apoloz\AlfaBank\Sbp\Requests\Subscription\GetQrSubStatusRequest::class, + Apoloz\AlfaBank\Sbp\Requests\Management\GetBlockedQrListRequest::class, + Apoloz\AlfaBank\Sbp\Requests\Registry\GetBankListRequest::class, + Apoloz\AlfaBank\Sbp\Requests\DigitalRuble\GetUniQrStatusRequest::class, + Apoloz\AlfaBank\Sbp\Requests\DigitalRuble\CashLink\GetCashUniQrStatusRequest::class, + ]; +} + +/** + * All request classes shipped by the SDK with the HTTP method declared in their defaults. + * + * @return array, Method> + */ +function retryAllRequestClasses(): array +{ + $sourceRoot = dirname(__DIR__, 2).'/src'; + $classes = []; + + foreach (['Requests', 'Sbp/Requests'] as $directory) { + $path = $sourceRoot.'/'.$directory; + + if (! is_dir($path)) { + continue; + } + + $files = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($path)); + + foreach ($files as $file) { + if ($file->getExtension() !== 'php') { + continue; + } + + $relative = substr($file->getPathname(), strlen($sourceRoot) + 1, -4); + $class = 'Apoloz\\AlfaBank\\'.str_replace('/', '\\', $relative); + + if (! class_exists($class)) { + continue; + } + + $reflection = new ReflectionClass($class); + + if ($reflection->isAbstract() || ! $reflection->isSubclassOf(Request::class)) { + continue; + } + + $classes[$class] = $reflection->getDefaultProperties()['method'] ?? Method::GET; + } + } + + ksort($classes); + + return $classes; +} + +function retryConnectorFor(string $requestClass): Connector +{ + return str_starts_with($requestClass, 'Apoloz\\AlfaBank\\Sbp\\') + ? new SbpConnector(createTestConfig(), 'test-key-alias') + : new AlfaBankConnector(createTestConfig()); +} + +function retryRequestInstance(string $requestClass): Request +{ + return (new ReflectionClass($requestClass))->newInstanceWithoutConstructor(); +} + +/** Pending request built from a harmless GET so that no body is signed or merged. */ +function retryNeutralPendingRequest(Connector $connector): PendingRequest +{ + $neutral = new class extends Request + { + protected Method $method = Method::GET; + + public function resolveEndpoint(): string + { + return '/'; + } + }; + + return new PendingRequest($connector, $neutral); +} + +function retryTransportFailure(Connector $connector): FatalRequestException +{ + return new FatalRequestException( + new RuntimeException('Connection timed out'), + retryNeutralPendingRequest($connector), + ); +} + +function retryBankVerdict(Connector $connector, int $status = 400): RequestException +{ + $pending = retryNeutralPendingRequest($connector); + + $response = new Response( + new PsrResponse($status, ['Content-Type' => 'application/json', 'X-Request-ID' => 'test-request-id'], '{"error":"bad_request"}'), + $pending, + new PsrRequest('GET', 'https://test.alfabank.ru/'), + ); + + return new RequestException($response); +} + +// --- money operations are never repeated ------------------------------------- + +test('money operations are never retried on transport failure', function (string $requestClass): void { + $connector = retryConnectorFor($requestClass); + + $decision = $connector->handleRetry( + retryTransportFailure($connector), + retryRequestInstance($requestClass), + ); + + expect($decision)->toBeFalse(); +})->with(retryMoneyOperations()); + +test('QRCreversalData is never retried: the bank treats it as the previous message of a reversal', function (): void { + $connector = new SbpConnector(createTestConfig(), 'test-key-alias'); + + $decision = $connector->handleRetry( + retryTransportFailure($connector), + retryRequestInstance(GetQrcReversalDataRequest::class), + ); + + expect($decision)->toBeFalse(); +}); + +// --- read-only operations keep their resilience ------------------------------ + +test('read-only GET requests are retried on transport failure', function (): void { + $connector = new AlfaBankConnector(createTestConfig()); + + $getRequest = new class extends Request + { + protected Method $method = Method::GET; + + public function resolveEndpoint(): string + { + return '/payoffs'; + } + }; + + expect($connector->handleRetry(retryTransportFailure($connector), $getRequest))->toBeTrue(); +}); + +test('SBP read-only POST operations are still retried on transport failure', function (string $requestClass): void { + $connector = new SbpConnector(createTestConfig(), 'test-key-alias'); + + $decision = $connector->handleRetry( + retryTransportFailure($connector), + retryRequestInstance($requestClass), + ); + + expect($decision)->toBeTrue(); +})->with(retrySbpReadOnlyPostOperations()); + +test('every mutating request class in the SDK is non-retryable by default', function (): void { + $sbpReadOnlyPost = retrySbpReadOnlyPostOperations(); + + $mutating = array_filter( + retryAllRequestClasses(), + static fn (Method $method, string $class): bool => ! in_array($method, [Method::GET, Method::HEAD, Method::OPTIONS], true) + && ! in_array($class, $sbpReadOnlyPost, true), + ARRAY_FILTER_USE_BOTH, + ); + + expect($mutating)->not->toBeEmpty(); + + foreach ($mutating as $class => $method) { + $connector = retryConnectorFor($class); + + $decision = $connector->handleRetry( + retryTransportFailure($connector), + retryRequestInstance($class), + ); + + expect($decision)->toBeFalse("{$method->value} {$class} must not be retried"); + } +}); + +// --- deterministic bank verdicts --------------------------------------------- + +test('bank verdict responses are never retried, even for read-only requests', function (): void { + $connector = new AlfaBankConnector(createTestConfig()); + + $getRequest = new class extends Request + { + protected Method $method = Method::GET; + + public function resolveEndpoint(): string + { + return '/payoffs'; + } + }; + + expect($connector->handleRetry(retryBankVerdict($connector, 400), $getRequest))->toBeFalse() + ->and($connector->handleRetry(retryBankVerdict($connector, 500), $getRequest))->toBeFalse(); +}); + +// --- the decision is actually enforced by Saloon's retry loop ---------------- + +test('a failing non-idempotent POST is sent exactly once', function (): void { + $connector = new class(createTestConfig()) extends AlfaBankConnector + { + protected string $defaultSender = RetryCountingFailingSender::class; + }; + $connector->retryInterval = 0; + + $request = new class extends Request + { + protected Method $method = Method::POST; + + public function resolveEndpoint(): string + { + return '/payoffs'; + } + }; + + RetryCountingFailingSender::$attempts = 0; + + try { + $connector->send($request); + } catch (FatalRequestException) { + // expected: the original transport failure is surfaced to the caller + } + + expect(RetryCountingFailingSender::$attempts)->toBe(1); +}); + +test('a failing read-only GET still uses all configured attempts', function (): void { + $connector = new class(createTestConfig()) extends AlfaBankConnector + { + protected string $defaultSender = RetryCountingFailingSender::class; + }; + $connector->retryInterval = 0; + + $request = new class extends Request + { + protected Method $method = Method::GET; + + public function resolveEndpoint(): string + { + return '/payoffs'; + } + }; + + RetryCountingFailingSender::$attempts = 0; + + try { + $connector->send($request); + } catch (FatalRequestException) { + // expected: retries exhausted, original failure surfaced + } + + expect(RetryCountingFailingSender::$attempts)->toBe(3); +}); + +// --- observability ----------------------------------------------------------- + +test('the retry decision is logged with the request, method and verdict', function (): void { + $logger = new RetryRecordingLogger; + $connector = new AlfaBankConnector(createTestConfig()); + $connector->withLogging($logger); + + $getRequest = new class extends Request + { + protected Method $method = Method::GET; + + public function resolveEndpoint(): string + { + return '/payoffs'; + } + }; + + $connector->handleRetry(retryTransportFailure($connector), $getRequest); + $connector->handleRetry(retryBankVerdict($connector, 400), $getRequest); + + // The connector's logging middleware shares this logger, so keep only retry records. + $records = array_values(array_filter( + $logger->records, + static fn (array $record): bool => $record['message'] === '[alfa-sdk][retry]', + )); + + expect($records)->toHaveCount(2) + ->and($records[0]['context']['decision'])->toBe('retry') + ->and($records[0]['context']['http_method'])->toBe('GET') + ->and($records[1]['context']['decision'])->toBe('stop') + ->and($records[1]['context']['http_status'])->toBe(400) + ->and($records[1]['context']['request_id'])->toBe('test-request-id'); +}); + +// --- explicit policy overrides ------------------------------------------------ + +test('the deny-list overrides the read-only default', function (): void { + $readOnly = new class extends Request + { + protected Method $method = Method::GET; + + public function resolveEndpoint(): string + { + return '/'; + } + }; + + $connector = new AlfaBankConnector(createTestConfig()); + expect($connector->handleRetry(retryTransportFailure($connector), $readOnly))->toBeTrue(); + + $connector->denyRetriesFor([$readOnly::class]); + expect($connector->handleRetry(retryTransportFailure($connector), $readOnly))->toBeFalse(); +}); + +//