diff --git a/src/BusProNet/ApiClient.php b/src/BusProNet/ApiClient.php index 5429e30..20154a6 100644 --- a/src/BusProNet/ApiClient.php +++ b/src/BusProNet/ApiClient.php @@ -602,10 +602,12 @@ class ApiClient 'port' => $this->selectedPort, ]); $this->send($socket, $body); - $response = $this->receive($socket); - $this->disconnect($socket); - $responseXml = substr($response, 10); + try { + $responseXml = $this->receive($socket); + } finally { + $this->disconnect($socket); + } if (true === $debug || true === $this->config['debug']) { $this->dumpXmlToFile('response', $requestId, $responseXml); @@ -714,18 +716,12 @@ class ApiClient ]); $this->send($socket, $body); - $response = $this->receive($socket); - $this->disconnect($socket); - // message length (10 bytes) is prepended to actual message - $xml = substr($response, 10); - - if ('' === $xml) { - $this->logger->error('Empty response body received from API (header-only response)', [ - 'request_id' => $requestId, - 'raw_length' => strlen($response), - ]); - throw new ApiClientException('Empty response body received from API'); + try { + // the length header is consumed and verified by receive(), this is the payload + $xml = $this->receive($socket); + } finally { + $this->disconnect($socket); } if (true === $debug || true === $this->config['debug']) { @@ -735,14 +731,19 @@ class ApiClient try { return $this->responseParser->parseXmlString($type, $xml, $additionalArgs); } 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. $this->dumpXmlToFile('response', $requestId, $xml); + + $this->logger->error('Unable to parse response received from API', [ + 'request_id' => $requestId, + 'type' => $type, + 'response_length' => strlen($xml), + 'error_message' => $e->getMessage(), + ]); + + throw new ApiClientException('Unable to parse response received from API: '.$e->getMessage(), 0, $e); } - - $this->logger->error('Unexpected response received from API', [ - 'request_id' => $requestId, - ]); - - throw new ApiClientException('Unexpected response received from API'); } /** @@ -773,10 +774,12 @@ class ApiClient ]); $this->send($socket, $body); - $response = $this->receive($socket); - $this->disconnect($socket); - $xml = substr($response, 10); + try { + $xml = $this->receive($socket); + } finally { + $this->disconnect($socket); + } if (true === $this->config['debug']) { $this->dumpXmlToFile('response', $requestId, $xml); @@ -898,8 +901,14 @@ class ApiClient } /** + * Reads a single response message and returns its payload. The protocol prepends the + * payload length as a 10 byte header, so the announced length is compared against what + * actually arrived: a peer closing mid-stream would otherwise yield a silently + * truncated body that only surfaces later as an unexplained parse failure. + * * @param resource $socket * + * @throws ApiClientException * @throws TimeoutException * @throws ImmediateConnectionCloseException */ @@ -963,7 +972,47 @@ class ApiClient throw new ImmediateConnectionCloseException('Server closed connection without sending data'); } - return $response; + return $this->extractPayload($response); + } + + /** + * @throws ApiClientException + */ + private function extractPayload(string $response): string + { + if (10 > strlen($response)) { + $this->logger->error('Incomplete response header received from API', [ + 'bytes_received' => strlen($response), + 'port' => $this->selectedPort, + ]); + + throw new ApiClientException(sprintf('Incomplete response header, got %d of 10 bytes', strlen($response))); + } + + $header = substr($response, 0, 10); + $announcedLength = (int) trim($header); + $payload = substr($response, 10); + + if (1 > $announcedLength) { + $this->logger->error('Empty response body announced by API', [ + 'header' => trim($header), + 'port' => $this->selectedPort, + ]); + + throw new ApiClientException(sprintf('Response announced an empty body (header "%s")', trim($header))); + } + + if (strlen($payload) !== $announcedLength) { + $this->logger->error('Truncated response received from API', [ + 'bytes_received' => strlen($payload), + 'announced_length' => $announcedLength, + 'port' => $this->selectedPort, + ]); + + throw new ApiClientException(sprintf('Truncated response, got %d of %d announced bytes', strlen($payload), $announcedLength)); + } + + return $payload; } /** @param resource $socket */ diff --git a/src/BusProNet/XmlCrawlerFactory.php b/src/BusProNet/XmlCrawlerFactory.php index 7c1dd09..ec5388a 100644 --- a/src/BusProNet/XmlCrawlerFactory.php +++ b/src/BusProNet/XmlCrawlerFactory.php @@ -20,4 +20,38 @@ final class XmlCrawlerFactory return $crawler; } + + /** + * Explains why create() returned an empty Crawler. addXmlContent() discards the + * libxml errors, so an empty, a malformed and a truncated payload are + * indistinguishable afterwards; this re-parses to recover the reason. Only worth + * calling on the failure path. + */ + public static function diagnose(string $xml): string + { + if ('' === trim($xml)) { + return sprintf('empty response (%d bytes)', strlen($xml)); + } + + $previousUseErrors = libxml_use_internal_errors(true); + libxml_clear_errors(); + + try { + simplexml_load_string($xml); + + $messages = []; + foreach (libxml_get_errors() as $error) { + $messages[] = sprintf('%s (line %d, column %d)', trim($error->message), $error->line, $error->column); + } + + if ([] === $messages) { + return sprintf('unknown XML error (%d bytes)', strlen($xml)); + } + + return sprintf('%s (%d bytes)', implode('; ', array_unique($messages)), strlen($xml)); + } finally { + libxml_clear_errors(); + libxml_use_internal_errors($previousUseErrors); + } + } } diff --git a/src/BusProNet/XmlParser/ApiResponseParser.php b/src/BusProNet/XmlParser/ApiResponseParser.php index 54b16f0..df893b9 100644 --- a/src/BusProNet/XmlParser/ApiResponseParser.php +++ b/src/BusProNet/XmlParser/ApiResponseParser.php @@ -17,7 +17,7 @@ class ApiResponseParser extends AbstractParser $crawler = XmlCrawlerFactory::create($xml); if (0 === $crawler->count()) { - throw new ResponseParserException('Empty response received from server'); + throw new ResponseParserException('Unable to parse XML response: '.XmlCrawlerFactory::diagnose($xml)); } // Override type unless present in XML to catch error responses @@ -56,7 +56,8 @@ class ApiResponseParser extends AbstractParser case 'Vorgangdruck': return (new DocumentsParser())->parseConfirmation($resultNode); } - break; + + throw new ResponseParserException(sprintf('Unrecognised customer data subtype "%s"', $subType)); case ApiClient::TYPE_BASE_DATA_COUNTRIES: return (new CountriesParser())->parse($resultNode); case ApiClient::TYPE_MUTABLE_DATA: @@ -92,6 +93,6 @@ class ApiResponseParser extends AbstractParser throw new ResponseParserException('Invalid XML response structure: '.$e->getMessage(), 0, $e); } - throw new ResponseParserException('Unable to parse XML response'); + throw new ResponseParserException(sprintf('Unrecognised BusProNet response type "%s"', $responseType)); } } diff --git a/tests/BusProNet/ApiClientReceiveTest.php b/tests/BusProNet/ApiClientReceiveTest.php index 519c7fe..1b254cf 100644 --- a/tests/BusProNet/ApiClientReceiveTest.php +++ b/tests/BusProNet/ApiClientReceiveTest.php @@ -6,6 +6,7 @@ namespace App\Tests\BusProNet; use App\BusProNet\ApiClient; use App\BusProNet\DataProcessor\BookingDataProcessor; +use App\BusProNet\Exception\ApiClientException; use App\BusProNet\Exception\ImmediateConnectionCloseException; use App\BusProNet\XmlParser\ApiResponseParser; use App\Service\RequestIdGenerator; @@ -70,7 +71,7 @@ class ApiClientReceiveTest extends TestCase $this->invokeReceive($stream); } - public function testNonEmptyResponseReturnsData(): void + public function testNonEmptyResponseReturnsPayloadWithoutTheLengthHeader(): void { $stream = fopen('php://memory', 'r+'); fwrite($stream, '0000000005Hello'); @@ -80,7 +81,56 @@ class ApiClientReceiveTest extends TestCase $result = $this->invokeReceive($stream); - self::assertSame('0000000005Hello', $result); + self::assertSame('Hello', $result); + } + + /** + * A peer closing mid-stream used to yield a silently shortened payload, which only + * surfaced later as an unexplained parse failure. The announced length catches it here. + */ + public function testTruncatedResponseThrows(): void + { + $stream = fopen('php://memory', 'r+'); + fwrite($stream, '0000000050Hello'); + rewind($stream); + + $this->setOperationStartTime(); + $this->setSelectedPort(9000); + + $this->expectException(ApiClientException::class); + $this->expectExceptionMessage('Truncated response, got 5 of 50 announced bytes'); + + $this->invokeReceive($stream); + } + + public function testResponseShorterThanTheLengthHeaderThrows(): void + { + $stream = fopen('php://memory', 'r+'); + fwrite($stream, '00000'); + rewind($stream); + + $this->setOperationStartTime(); + $this->setSelectedPort(9000); + + $this->expectException(ApiClientException::class); + $this->expectExceptionMessage('Incomplete response header, got 5 of 10 bytes'); + + $this->invokeReceive($stream); + } + + public function testHeaderOnlyResponseThrows(): void + { + $stream = fopen('php://memory', 'r+'); + fwrite($stream, '0000000000'); + rewind($stream); + + $this->setOperationStartTime(); + $this->setSelectedPort(9000); + + $this->expectException(ApiClientException::class); + $this->expectExceptionMessage('Response announced an empty body (header "0000000000")'); + + $this->invokeReceive($stream); } private function invokeReceive($socket): string diff --git a/tests/BusProNet/XmlParser/ApiResponseParserFailureTest.php b/tests/BusProNet/XmlParser/ApiResponseParserFailureTest.php new file mode 100644 index 0000000..4e4738f --- /dev/null +++ b/tests/BusProNet/XmlParser/ApiResponseParserFailureTest.php @@ -0,0 +1,77 @@ +expectException(ResponseParserException::class); + $this->expectExceptionMessage('empty response (0 bytes)'); + + (new ApiResponseParser())->parseXmlString(ApiClient::TYPE_NOTIFICATION, ''); + } + + public function testTruncatedResponseNamesTheLibxmlReason(): void + { + $xml = 'parseXmlString(ApiClient::TYPE_NOTIFICATION, $xml); + $this->fail('Expected a ResponseParserException'); + } catch (ResponseParserException $e) { + self::assertStringContainsString('('.strlen($xml).' bytes)', $e->getMessage()); + self::assertStringContainsString('line 1', $e->getMessage()); + } + } + + public function testWellFormedResponseOfAnUnknownType(): void + { + $xml = ''; + + $this->expectException(ResponseParserException::class); + $this->expectExceptionMessage('Unrecognised BusProNet response type "IRGENDWAS"'); + + (new ApiResponseParser())->parseXmlString(ApiClient::TYPE_NOTIFICATION, $xml); + } + + public function testUnknownCustomerDataSubtype(): void + { + $xml = 'Irgendwas'; + + $this->expectException(ResponseParserException::class); + $this->expectExceptionMessage('Unrecognised customer data subtype "Irgendwas"'); + + (new ApiResponseParser())->parseXmlString(ApiClient::TYPE_CUSTOMER_DATA, $xml); + } + + public function testDiagnosingDoesNotLeakLibxmlErrorState(): void + { + $previous = libxml_use_internal_errors(false); + + try { + try { + (new ApiResponseParser())->parseXmlString(ApiClient::TYPE_NOTIFICATION, '