fix: make BusProNet response failures diagnosable and non-fatal
This commit is contained in:
@@ -162,17 +162,36 @@ class ApiClient
|
|||||||
$this->config['max_retries']
|
$this->config['max_retries']
|
||||||
);
|
);
|
||||||
$this->send($socket, $body);
|
$this->send($socket, $body);
|
||||||
$response = $this->receive($socket);
|
|
||||||
$this->disconnect($socket);
|
|
||||||
|
|
||||||
// message length (10 bytes) is prepended to actual message
|
try {
|
||||||
$xml = substr($response, 10);
|
// the length header is consumed by receive(), this is the payload
|
||||||
|
$xml = $this->receive($socket);
|
||||||
|
} finally {
|
||||||
|
$this->disconnect($socket);
|
||||||
|
}
|
||||||
|
|
||||||
if (true === $this->config['debug']) {
|
if (true === $this->config['debug']) {
|
||||||
$this->dumpXmlToFile('response', $requestId, $xml);
|
$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
|
private function dumpXmlToFile(string $type, string $requestId, string $body): void
|
||||||
|
|||||||
@@ -58,15 +58,58 @@ trait ApiClientTrait
|
|||||||
fwrite($socket, $send);
|
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
|
private function receive($socket): string
|
||||||
{
|
{
|
||||||
$response = '';
|
$header = $this->readBytes($socket, 10);
|
||||||
|
|
||||||
while (false === feof($socket)) {
|
if (10 !== strlen($header)) {
|
||||||
$response .= fread($socket, 4096);
|
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
|
private function disconnect($socket): void
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ use App\BusProNet\ApiClient;
|
|||||||
use App\BusProNet\ApiClientException;
|
use App\BusProNet\ApiClientException;
|
||||||
use App\BusProNet\Model\Country;
|
use App\BusProNet\Model\Country;
|
||||||
use App\BusProNet\Model\NotificationResponse;
|
use App\BusProNet\Model\NotificationResponse;
|
||||||
|
use App\BusProNet\ResponseParserException;
|
||||||
use Psr\Log\LoggerInterface;
|
use Psr\Log\LoggerInterface;
|
||||||
use Symfony\Contracts\Cache\CacheInterface;
|
use Symfony\Contracts\Cache\CacheInterface;
|
||||||
use Symfony\Contracts\Cache\ItemInterface;
|
use Symfony\Contracts\Cache\ItemInterface;
|
||||||
@@ -35,7 +36,8 @@ class CountryDataProvider
|
|||||||
|
|
||||||
return $response->getItems();
|
return $response->getItems();
|
||||||
});
|
});
|
||||||
} catch (ApiClientException $e) {
|
} catch (ApiClientException|ResponseParserException $e) {
|
||||||
|
$this->logger->error('Unable to fetch country base data from BusProNet: '.$e->getMessage());
|
||||||
$countries = [];
|
$countries = [];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ use App\BusProNet\ApiClient;
|
|||||||
use App\BusProNet\ApiClientException;
|
use App\BusProNet\ApiClientException;
|
||||||
use App\BusProNet\Model\Hotel;
|
use App\BusProNet\Model\Hotel;
|
||||||
use App\BusProNet\Model\NotificationResponse;
|
use App\BusProNet\Model\NotificationResponse;
|
||||||
|
use App\BusProNet\ResponseParserException;
|
||||||
use Psr\Cache\InvalidArgumentException;
|
use Psr\Cache\InvalidArgumentException;
|
||||||
use Psr\Log\LoggerInterface;
|
use Psr\Log\LoggerInterface;
|
||||||
use Symfony\Contracts\Cache\CacheInterface;
|
use Symfony\Contracts\Cache\CacheInterface;
|
||||||
@@ -36,7 +37,8 @@ class HotelDataProvider
|
|||||||
|
|
||||||
return $response->getItems();
|
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 = [];
|
$hotels = [];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ use App\BusProNet\ApiClient;
|
|||||||
use App\BusProNet\ApiClientException;
|
use App\BusProNet\ApiClientException;
|
||||||
use App\BusProNet\Model\NotificationResponse;
|
use App\BusProNet\Model\NotificationResponse;
|
||||||
use App\BusProNet\Model\Pickup;
|
use App\BusProNet\Model\Pickup;
|
||||||
|
use App\BusProNet\ResponseParserException;
|
||||||
use Psr\Log\LoggerInterface;
|
use Psr\Log\LoggerInterface;
|
||||||
use Symfony\Contracts\Cache\CacheInterface;
|
use Symfony\Contracts\Cache\CacheInterface;
|
||||||
use Symfony\Contracts\Cache\ItemInterface;
|
use Symfony\Contracts\Cache\ItemInterface;
|
||||||
@@ -35,7 +36,8 @@ class PickupDataProvider
|
|||||||
|
|
||||||
return $response->getItems();
|
return $response->getItems();
|
||||||
});
|
});
|
||||||
} catch (ApiClientException $e) {
|
} catch (ApiClientException|ResponseParserException $e) {
|
||||||
|
$this->logger->error('Unable to fetch pickup base data from BusProNet: '.$e->getMessage());
|
||||||
$pickups = [];
|
$pickups = [];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -30,10 +30,7 @@ class ResponseParser
|
|||||||
*/
|
*/
|
||||||
public function parseXmlString(string $type, string $content): mixed
|
public function parseXmlString(string $type, string $content): mixed
|
||||||
{
|
{
|
||||||
$xml = simplexml_load_string($content);
|
$xml = $this->loadXml($content);
|
||||||
if (false === $xml) {
|
|
||||||
throw new ResponseParserException('Unable to parse XML response');
|
|
||||||
}
|
|
||||||
|
|
||||||
// Override type when present in XML to catch error responses
|
// Override type when present in XML to catch error responses
|
||||||
$responseType = $type;
|
$responseType = $type;
|
||||||
@@ -65,7 +62,7 @@ class ResponseParser
|
|||||||
return $this->createHotelsResponse($xml);
|
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
|
public function createNotificationResponse(\SimpleXMLElement $xml): NotificationResponse
|
||||||
@@ -302,6 +299,50 @@ class ResponseParser
|
|||||||
return new BaseDataResponse($hotels);
|
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
|
private function resolveOptions(array $options): array
|
||||||
{
|
{
|
||||||
$optionsResolver = new OptionsResolver();
|
$optionsResolver = new OptionsResolver();
|
||||||
|
|||||||
@@ -56,11 +56,30 @@ class BpnImportCommand extends Command
|
|||||||
return Command::FAILURE;
|
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');
|
$io->info('Found '.$totalCount.' XML files');
|
||||||
$addedCount = 0;
|
$addedCount = 0;
|
||||||
$updatedCount = 0;
|
$updatedCount = 0;
|
||||||
$warnings = [];
|
$warnings = [];
|
||||||
|
|
||||||
|
if ([] === $pickups) {
|
||||||
|
$warnings[] = 'Zustiegs-Stammdaten konnten nicht von BusProNet geladen werden';
|
||||||
|
}
|
||||||
|
|
||||||
$progressBar = $io->createProgressBar($totalCount);
|
$progressBar = $io->createProgressBar($totalCount);
|
||||||
$progressBar->setFormat(" %current%/%max% [%bar%] %percent:3s%% %elapsed:6s%/%estimated:-6s%\n %message%");
|
$progressBar->setFormat(" %current%/%max% [%bar%] %percent:3s%% %elapsed:6s%/%estimated:-6s%\n %message%");
|
||||||
$progressBar->setMessage('Starting');
|
$progressBar->setMessage('Starting');
|
||||||
@@ -92,7 +111,7 @@ class BpnImportCommand extends Command
|
|||||||
foreach ($destinationXml->xpath('zustiege/zustieg') as $pickupXml) {
|
foreach ($destinationXml->xpath('zustiege/zustieg') as $pickupXml) {
|
||||||
$pickupBusProId = (int) $pickupXml->attributes()['idbuspro'];
|
$pickupBusProId = (int) $pickupXml->attributes()['idbuspro'];
|
||||||
if ($pickupBusProId) {
|
if ($pickupBusProId) {
|
||||||
if (null === $pickup = $this->pickupDataProvider->get($pickupBusProId)) {
|
if (null === $pickup = $pickups[$pickupBusProId] ?? null) {
|
||||||
$warnings[] = 'Pickup with busProId '.$pickupBusProId.' not found';
|
$warnings[] = 'Pickup with busProId '.$pickupBusProId.' not found';
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
@@ -109,7 +128,7 @@ class BpnImportCommand extends Command
|
|||||||
// Iterate over all hotel entries
|
// Iterate over all hotel entries
|
||||||
foreach ($destinationXml->xpath('hotel') as $hotelXml) {
|
foreach ($destinationXml->xpath('hotel') as $hotelXml) {
|
||||||
$hotelBusProId = (int) $hotelXml->attributes()['idbuspro'];
|
$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';
|
$warnings[] = 'Hotel with busProId '.$hotelBusProId.' not found';
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -10,6 +10,7 @@ use App\BusProNet\Model\Pickup;
|
|||||||
use App\BusProNet\Model\ProfileResponse;
|
use App\BusProNet\Model\ProfileResponse;
|
||||||
use App\BusProNet\Model\ProfileUpdateResponse;
|
use App\BusProNet\Model\ProfileUpdateResponse;
|
||||||
use App\BusProNet\ResponseParser;
|
use App\BusProNet\ResponseParser;
|
||||||
|
use App\BusProNet\ResponseParserException;
|
||||||
use PHPUnit\Framework\TestCase;
|
use PHPUnit\Framework\TestCase;
|
||||||
|
|
||||||
class ResponseParserTest extends TestCase
|
class ResponseParserTest extends TestCase
|
||||||
@@ -178,6 +179,65 @@ class ResponseParserTest extends TestCase
|
|||||||
$this->assertEquals('Gaststätte', $hotel->getType());
|
$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 = '<?xml version="1.0" encoding="utf-8"?><ergebnis><satz typ="STAMMZUSTIEGE"></satz><zustieg id="1"';
|
||||||
|
|
||||||
|
$parser = $this->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 = '<?xml version="1.0" encoding="utf-8"?><ergebnis><satz typ="STAMMIRGENDWAS"></satz></ergebnis>';
|
||||||
|
|
||||||
|
$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, '<ergebnis');
|
||||||
|
} catch (ResponseParserException) {
|
||||||
|
}
|
||||||
|
|
||||||
|
$this->assertFalse(libxml_use_internal_errors(false));
|
||||||
|
$this->assertSame([], libxml_get_errors());
|
||||||
|
} finally {
|
||||||
|
libxml_use_internal_errors($previous);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
private function loadFixture(string $filename): string
|
private function loadFixture(string $filename): string
|
||||||
{
|
{
|
||||||
return file_get_contents(__DIR__.'/../Resources/'.$filename);
|
return file_get_contents(__DIR__.'/../Resources/'.$filename);
|
||||||
|
|||||||
Reference in New Issue
Block a user