From a05143bfbe0dd9f0279265194e09c23f70bbede7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Sat, 19 Sep 2026 11:34:58 +0200 Subject: [PATCH] fix: make BusProNet response failures diagnosable and non-fatal --- src/BusProNet/ApiClient.php | 29 +++++++-- src/BusProNet/ApiClientTrait.php | 51 ++++++++++++++-- .../DataProvider/CountryDataProvider.php | 4 +- .../DataProvider/HotelDataProvider.php | 4 +- .../DataProvider/PickupDataProvider.php | 4 +- src/BusProNet/ResponseParser.php | 51 ++++++++++++++-- src/Command/BpnImportCommand.php | 23 ++++++- tests/BusProNet/ResponseParserTest.php | 60 +++++++++++++++++++ 8 files changed, 207 insertions(+), 19 deletions(-) diff --git a/src/BusProNet/ApiClient.php b/src/BusProNet/ApiClient.php index 16bcf00..e9068ff 100644 --- a/src/BusProNet/ApiClient.php +++ b/src/BusProNet/ApiClient.php @@ -162,17 +162,36 @@ class ApiClient $this->config['max_retries'] ); $this->send($socket, $body); - $response = $this->receive($socket); - $this->disconnect($socket); - // message length (10 bytes) is prepended to actual message - $xml = substr($response, 10); + try { + // the length header is consumed by receive(), this is the payload + $xml = $this->receive($socket); + } finally { + $this->disconnect($socket); + } if (true === $this->config['debug']) { $this->dumpXmlToFile('response', $requestId, $xml); } - return $this->responseParser->parseXmlString($type, $xml); + try { + return $this->responseParser->parseXmlString($type, $xml); + } catch (ResponseParserException $e) { + // Keep the evidence even outside debug mode: without the raw response a parse + // failure is not diagnosable after the fact. app:cleanup:xml-dumps prunes it. + if (true !== $this->config['debug']) { + $this->dumpXmlToFile('response', $requestId, $xml); + } + + $this->logger->error('Unable to parse BusProNet response', [ + 'request_id' => $requestId, + 'type' => $type, + 'response_length' => strlen($xml), + 'error_message' => $e->getMessage(), + ]); + + throw $e; + } } private function dumpXmlToFile(string $type, string $requestId, string $body): void diff --git a/src/BusProNet/ApiClientTrait.php b/src/BusProNet/ApiClientTrait.php index b629659..56edf86 100644 --- a/src/BusProNet/ApiClientTrait.php +++ b/src/BusProNet/ApiClientTrait.php @@ -58,15 +58,58 @@ trait ApiClientTrait fwrite($socket, $send); } + /** + * Reads a single response message. The protocol prepends the payload length as a + * 10 byte header, so read exactly that many bytes rather than guessing at EOF: + * a peer that closes mid-stream would otherwise yield a silently truncated body. + * + * @throws ApiClientException + */ private function receive($socket): string { - $response = ''; + $header = $this->readBytes($socket, 10); - while (false === feof($socket)) { - $response .= fread($socket, 4096); + if (10 !== strlen($header)) { + throw new ApiClientException(sprintf('Incomplete response header, got %d of 10 bytes', strlen($header))); } - return $response; + $expectedLength = (int) trim($header); + + if (1 > $expectedLength) { + throw new ApiClientException(sprintf('Response announced an empty body (header "%s")', trim($header))); + } + + $body = $this->readBytes($socket, $expectedLength); + + if (strlen($body) !== $expectedLength) { + throw new ApiClientException(sprintf('Truncated response, got %d of %d announced bytes', strlen($body), $expectedLength)); + } + + return $body; + } + + /** + * @throws ApiClientException + */ + private function readBytes($socket, int $length): string + { + $buffer = ''; + + while (strlen($buffer) < $length && false === feof($socket)) { + $chunk = fread($socket, min(4096, $length - strlen($buffer))); + + if (false === $chunk || '' === $chunk) { + if (true === (stream_get_meta_data($socket)['timed_out'] ?? false)) { + throw new ApiClientException(sprintf('Timed out reading response after %d of %d bytes', strlen($buffer), $length)); + } + + break; + } + + $buffer .= $chunk; + } + + return $buffer; } private function disconnect($socket): void diff --git a/src/BusProNet/DataProvider/CountryDataProvider.php b/src/BusProNet/DataProvider/CountryDataProvider.php index a944c1d..ee43460 100644 --- a/src/BusProNet/DataProvider/CountryDataProvider.php +++ b/src/BusProNet/DataProvider/CountryDataProvider.php @@ -6,6 +6,7 @@ use App\BusProNet\ApiClient; use App\BusProNet\ApiClientException; use App\BusProNet\Model\Country; use App\BusProNet\Model\NotificationResponse; +use App\BusProNet\ResponseParserException; use Psr\Log\LoggerInterface; use Symfony\Contracts\Cache\CacheInterface; use Symfony\Contracts\Cache\ItemInterface; @@ -35,7 +36,8 @@ class CountryDataProvider return $response->getItems(); }); - } catch (ApiClientException $e) { + } catch (ApiClientException|ResponseParserException $e) { + $this->logger->error('Unable to fetch country base data from BusProNet: '.$e->getMessage()); $countries = []; } diff --git a/src/BusProNet/DataProvider/HotelDataProvider.php b/src/BusProNet/DataProvider/HotelDataProvider.php index 923b5ca..6a33096 100644 --- a/src/BusProNet/DataProvider/HotelDataProvider.php +++ b/src/BusProNet/DataProvider/HotelDataProvider.php @@ -6,6 +6,7 @@ use App\BusProNet\ApiClient; use App\BusProNet\ApiClientException; use App\BusProNet\Model\Hotel; use App\BusProNet\Model\NotificationResponse; +use App\BusProNet\ResponseParserException; use Psr\Cache\InvalidArgumentException; use Psr\Log\LoggerInterface; use Symfony\Contracts\Cache\CacheInterface; @@ -36,7 +37,8 @@ class HotelDataProvider return $response->getItems(); }); - } catch (ApiClientException|InvalidArgumentException $e) { + } catch (ApiClientException|InvalidArgumentException|ResponseParserException $e) { + $this->logger->error('Unable to fetch hotel base data from BusProNet: '.$e->getMessage()); $hotels = []; } diff --git a/src/BusProNet/DataProvider/PickupDataProvider.php b/src/BusProNet/DataProvider/PickupDataProvider.php index be9e94c..8ed4d92 100644 --- a/src/BusProNet/DataProvider/PickupDataProvider.php +++ b/src/BusProNet/DataProvider/PickupDataProvider.php @@ -6,6 +6,7 @@ use App\BusProNet\ApiClient; use App\BusProNet\ApiClientException; use App\BusProNet\Model\NotificationResponse; use App\BusProNet\Model\Pickup; +use App\BusProNet\ResponseParserException; use Psr\Log\LoggerInterface; use Symfony\Contracts\Cache\CacheInterface; use Symfony\Contracts\Cache\ItemInterface; @@ -35,7 +36,8 @@ class PickupDataProvider return $response->getItems(); }); - } catch (ApiClientException $e) { + } catch (ApiClientException|ResponseParserException $e) { + $this->logger->error('Unable to fetch pickup base data from BusProNet: '.$e->getMessage()); $pickups = []; } diff --git a/src/BusProNet/ResponseParser.php b/src/BusProNet/ResponseParser.php index 2af29bf..2c5cf0f 100644 --- a/src/BusProNet/ResponseParser.php +++ b/src/BusProNet/ResponseParser.php @@ -30,10 +30,7 @@ class ResponseParser */ public function parseXmlString(string $type, string $content): mixed { - $xml = simplexml_load_string($content); - if (false === $xml) { - throw new ResponseParserException('Unable to parse XML response'); - } + $xml = $this->loadXml($content); // Override type when present in XML to catch error responses $responseType = $type; @@ -65,7 +62,7 @@ class ResponseParser return $this->createHotelsResponse($xml); } - throw new ResponseParserException('Unable to parse XML response'); + throw new ResponseParserException(sprintf('Unrecognised BusProNet response type "%s"', $responseType)); } public function createNotificationResponse(\SimpleXMLElement $xml): NotificationResponse @@ -302,6 +299,50 @@ class ResponseParser return new BaseDataResponse($hotels); } + /** + * The BusProNet endpoint can answer with an empty or truncated body. Keep the libxml + * reason instead of collapsing every shape of broken response into one message. + * + * @throws ResponseParserException + */ + private function loadXml(string $content): \SimpleXMLElement + { + if ('' === trim($content)) { + throw new ResponseParserException(sprintf('Unable to parse XML response (%d bytes): empty response', strlen($content))); + } + + $previousUseErrors = libxml_use_internal_errors(true); + libxml_clear_errors(); + + try { + $xml = simplexml_load_string($content); + + if (false === $xml) { + throw new ResponseParserException(sprintf('Unable to parse XML response (%d bytes): %s', strlen($content), $this->describeLibxmlErrors())); + } + + return $xml; + } finally { + libxml_clear_errors(); + libxml_use_internal_errors($previousUseErrors); + } + } + + private function describeLibxmlErrors(): string + { + $messages = []; + + foreach (libxml_get_errors() as $error) { + $messages[] = sprintf('%s (line %d, column %d)', trim($error->message), $error->line, $error->column); + } + + if ([] === $messages) { + return 'unknown XML error'; + } + + return implode('; ', array_unique($messages)); + } + private function resolveOptions(array $options): array { $optionsResolver = new OptionsResolver(); diff --git a/src/Command/BpnImportCommand.php b/src/Command/BpnImportCommand.php index 5f362fc..b278b57 100644 --- a/src/Command/BpnImportCommand.php +++ b/src/Command/BpnImportCommand.php @@ -56,11 +56,30 @@ class BpnImportCommand extends Command return Command::FAILURE; } + // Resolve the BusProNet base data once, before touching a single row. A destination + // cannot be written without its hotel, so an unavailable hotel list would otherwise + // skip every record and still report success. Holding both lists locally also keeps + // the loop off the providers: on a failed fetch nothing is cached, and a per-lookup + // ->get() would re-open the socket for every pickup of all 174 files. + $hotels = $this->hotelDataProvider->getAll(); + $pickups = $this->pickupDataProvider->getAll(); + + if ([] === $hotels) { + $io->error('Hotel-Stammdaten konnten nicht von BusProNet geladen werden – Import abgebrochen.'); + $this->logger->error('BPN import aborted: hotel base data unavailable'); + + return Command::FAILURE; + } + $io->info('Found '.$totalCount.' XML files'); $addedCount = 0; $updatedCount = 0; $warnings = []; + if ([] === $pickups) { + $warnings[] = 'Zustiegs-Stammdaten konnten nicht von BusProNet geladen werden'; + } + $progressBar = $io->createProgressBar($totalCount); $progressBar->setFormat(" %current%/%max% [%bar%] %percent:3s%% %elapsed:6s%/%estimated:-6s%\n %message%"); $progressBar->setMessage('Starting'); @@ -92,7 +111,7 @@ class BpnImportCommand extends Command foreach ($destinationXml->xpath('zustiege/zustieg') as $pickupXml) { $pickupBusProId = (int) $pickupXml->attributes()['idbuspro']; if ($pickupBusProId) { - if (null === $pickup = $this->pickupDataProvider->get($pickupBusProId)) { + if (null === $pickup = $pickups[$pickupBusProId] ?? null) { $warnings[] = 'Pickup with busProId '.$pickupBusProId.' not found'; continue; } @@ -109,7 +128,7 @@ class BpnImportCommand extends Command // Iterate over all hotel entries foreach ($destinationXml->xpath('hotel') as $hotelXml) { $hotelBusProId = (int) $hotelXml->attributes()['idbuspro']; - if (null === $hotel = $this->hotelDataProvider->get($hotelBusProId)) { + if (null === $hotel = $hotels[$hotelBusProId] ?? null) { $warnings[] = 'Hotel with busProId '.$hotelBusProId.' not found'; continue; } diff --git a/tests/BusProNet/ResponseParserTest.php b/tests/BusProNet/ResponseParserTest.php index f78b907..c06911f 100644 --- a/tests/BusProNet/ResponseParserTest.php +++ b/tests/BusProNet/ResponseParserTest.php @@ -10,6 +10,7 @@ use App\BusProNet\Model\Pickup; use App\BusProNet\Model\ProfileResponse; use App\BusProNet\Model\ProfileUpdateResponse; use App\BusProNet\ResponseParser; +use App\BusProNet\ResponseParserException; use PHPUnit\Framework\TestCase; class ResponseParserTest extends TestCase @@ -178,6 +179,65 @@ class ResponseParserTest extends TestCase $this->assertEquals('Gaststätte', $hotel->getType()); } + /** + * A broken response used to collapse into one opaque "Unable to parse XML response", + * which is what made the production import failure undiagnosable. Each shape must now + * name itself. + */ + public function testParseEmptyResponse(): void + { + $parser = $this->getParserInstance(); + + $this->expectException(ResponseParserException::class); + $this->expectExceptionMessage('empty response'); + + $parser->parseXmlString(ApiClient::TYPE_BASE_DATA_PICKUPS, ''); + } + + public function testParseTruncatedResponse(): void + { + $content = 'getParserInstance(); + + try { + $parser->parseXmlString(ApiClient::TYPE_BASE_DATA_PICKUPS, $content); + $this->fail('Expected a ResponseParserException'); + } catch (ResponseParserException $e) { + $this->assertStringContainsString('('.strlen($content).' bytes)', $e->getMessage()); + $this->assertStringContainsString('line 1', $e->getMessage()); + } + } + + public function testParseWellFormedResponseOfAnUnknownType(): void + { + $content = ''; + + $parser = $this->getParserInstance(); + + $this->expectException(ResponseParserException::class); + $this->expectExceptionMessage('Unrecognised BusProNet response type "STAMMIRGENDWAS"'); + + $parser->parseXmlString(ApiClient::TYPE_BASE_DATA_PICKUPS, $content); + } + + public function testParsingDoesNotLeakLibxmlErrorState(): void + { + $previous = libxml_use_internal_errors(false); + + try { + try { + $this->getParserInstance()->parseXmlString(ApiClient::TYPE_BASE_DATA_PICKUPS, 'assertFalse(libxml_use_internal_errors(false)); + $this->assertSame([], libxml_get_errors()); + } finally { + libxml_use_internal_errors($previous); + } + } + private function loadFixture(string $filename): string { return file_get_contents(__DIR__.'/../Resources/'.$filename);