From 7b75235e8badd61d906cd8456cf174f1d23c6982 Mon Sep 17 00:00:00 2001 From: shemogumbe Date: Tue, 18 Feb 2025 16:06:52 +0300 Subject: [PATCH 1/5] handle 3XX responses more gracefully --- .../http/guzzle/src/GuzzleRequestAdapter.php | 134 +++++++++++++++++- 1 file changed, 133 insertions(+), 1 deletion(-) diff --git a/packages/http/guzzle/src/GuzzleRequestAdapter.php b/packages/http/guzzle/src/GuzzleRequestAdapter.php index 0b66640..ca16eb5 100644 --- a/packages/http/guzzle/src/GuzzleRequestAdapter.php +++ b/packages/http/guzzle/src/GuzzleRequestAdapter.php @@ -158,6 +158,9 @@ function (ResponseInterface $result) use ($targetCallable, $requestInfo, $errorM } $this->throwFailedResponse($result, $errorMappings, $span); + if ($this->shouldReturnNull($result)) { + return null; + } $span->setStatus(StatusCode::STATUS_OK, 'response_handle_success'); if ($this->is204NoContentResponse($result)) { return null; @@ -229,6 +232,9 @@ function (ResponseInterface $result) use ($targetCallable, $requestInfo, $errorM ); } $this->throwFailedResponse($result, $errorMappings, $span); + if ($this->shouldReturnNull($result)) { + return null; + } if ($this->is204NoContentResponse($result)) { return null; } @@ -268,6 +274,9 @@ function (ResponseInterface $result) use ($primitiveType, $requestInfo, $errorMa return $response->wait(); } $this->throwFailedResponse($result, $errorMappings, $span); + if ($this->shouldReturnNull($result)) { + return null; + } $this->setResponseType($primitiveType, $span); if ($this->is204NoContentResponse($result)) { return null; @@ -342,6 +351,9 @@ function (ResponseInterface $result) use ($primitiveType, $requestInfo, $errorMa ); } $this->throwFailedResponse($result, $errorMappings, $span); + if ($this->shouldReturnNull($result)) { + return null; + } if ($this->is204NoContentResponse($result)) { return null; } @@ -510,6 +522,115 @@ private function getRootParseNode(ResponseInterface $response, SpanInterface $sp } } + /** + * Helper function to check if the response should return null. + * + * Conditions: + * - The response status code is 204 or 304 + * - The response content is empty + * - The response status code is 301 or 302 and the location header is not present + * + * @param ResponseInterface $response The HTTP response + * @return bool True if the response should return null, False otherwise + */ + private function shouldReturnNull(ResponseInterface $response): bool + { + return $response->getStatusCode() === 204 + || $response->getStatusCode() === 304 + || empty((string)$response->getBody()) + || (!$response->hasHeader('Location') && in_array($response->getStatusCode(), [301, 302])); + } + + /** + * Checks if a redirect response is missing the Location header + * + * @param ResponseInterface $response The HTTP response + * @param SpanInterface $parentSpan + * @param SpanInterface $attributeSpan + * @return bool + * @throws ApiException + */ + private function isRedirectMissingLocation( + ResponseInterface $response, + SpanInterface $parentSpan, + SpanInterface $attributeSpan + ): bool { + $statusCode = $response->getStatusCode(); + + // Check if it's a redirect status code (3xx) + if ($statusCode >= 300 && $statusCode < 400) { + if (!$response->hasHeader('Location')) { + $parentSpan->setStatus(StatusCode::STATUS_ERROR); + $attributeSpan->setStatus(StatusCode::STATUS_ERROR); + + throw new ApiException( + sprintf( + 'Redirect status code %d returned without a Location header', + $statusCode + ) + ); + } + return false; + } + + return true; + } + + /** + * Gets the appropriate error from the response + * + * @param ResponseInterface $response The HTTP response + * @param array $errorMap Mapping of status codes to error types + * @param string $responseStatusCodeStr Status code as string + * @param int $responseStatusCode Status code as integer + * @param SpanInterface $attributeSpan Attribute tracing span + * @param SpanInterface $throwFailedRespSpan Failed response tracing span + * @return object|null The parsed error object or null + */ + private function getErrorFromResponse( + ResponseInterface $response, + array $errorMap, + string $responseStatusCodeStr, + int $responseStatusCode, + SpanInterface $attributeSpan, + SpanInterface $throwFailedRespSpan + ): ?object { + // Try exact status code match + $errorClass = $errorMap[$responseStatusCodeStr] ?? null; + + if (!$errorClass) { + // Try range match (4XX, 5XX) + $statusCodeRange = floor($responseStatusCode / 100) . 'XX'; + $errorClass = $errorMap[$statusCodeRange] ?? null; + + // Fallback to generic error (XXX) + if (!$errorClass) { + $errorClass = $errorMap['XXX'] ?? null; + } + } + + if ($errorClass) { + try { + $responseBody = (string)$response->getBody(); + if (empty($responseBody)) { + return null; + } + + // Assuming we have a factory method to create the error object + return $errorClass::createFromDiscriminatorValue( + json_decode($responseBody, true) + ); + } catch (Exception $e) { + $attributeSpan->setStatus(StatusCode::STATUS_ERROR); + $throwFailedRespSpan->setStatus(StatusCode::STATUS_ERROR); + // Log the error if needed + return null; + } + } + + return null; + } + /** * Authenticates and executes the request * @@ -620,6 +741,9 @@ private function throwFailedResponse(ResponseInterface $response, ?array $errorM if ($statusCode >= 200 && $statusCode < 400) { return; } + if (!$this->isRedirectMissingLocation($response, $span, $span)) { + return; + } $errorSpan = $this->tracer->spanBuilder('throwFailedResponses') ->setParent($currentContext) ->startSpan(); @@ -681,7 +805,14 @@ private function throwFailedResponse(ResponseInterface $response, ?array $errorM $spanForDeserialization->end(); } - + $error ?? $this->getErrorFromResponse( + $response, + $errorMappings, + $statusCodeAsString, + $statusCode, + $errorSpan, + $span + ); if ($error && is_subclass_of($error, ApiException::class)) { $span->setAttribute(self::ERROR_BODY_FOUND_ATTRIBUTE_NAME, true); $error->setResponseStatusCode($response->getStatusCode()); @@ -689,6 +820,7 @@ private function throwFailedResponse(ResponseInterface $response, ?array $errorM $errorSpan->recordException($error, ['know_error' => true, 'message' => $responseBodyContents]); throw $error; } + $otherwise = new ApiException("Unsupported error type " . get_debug_type($error)); $otherwise->setResponseStatusCode($response->getStatusCode()); $otherwise->setResponseHeaders($response->getHeaders()); From b131701c03ce2c72c5f87977b9e28c70a403ca14 Mon Sep 17 00:00:00 2001 From: shemogumbe Date: Wed, 19 Feb 2025 15:20:34 +0300 Subject: [PATCH 2/5] allow complex error mappings --- packages/http/guzzle/src/GuzzleRequestAdapter.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/http/guzzle/src/GuzzleRequestAdapter.php b/packages/http/guzzle/src/GuzzleRequestAdapter.php index ca16eb5..cebc7af 100644 --- a/packages/http/guzzle/src/GuzzleRequestAdapter.php +++ b/packages/http/guzzle/src/GuzzleRequestAdapter.php @@ -580,7 +580,7 @@ private function isRedirectMissingLocation( * Gets the appropriate error from the response * * @param ResponseInterface $response The HTTP response - * @param array $errorMap Mapping of status codes to error types + * @param array> $errorMap Mapping of status codes to error types * @param string $responseStatusCodeStr Status code as string * @param int $responseStatusCode Status code as integer * @param SpanInterface $attributeSpan Attribute tracing span From ae6aa477b7dc8b59849bbd33a2ac578cc2763047 Mon Sep 17 00:00:00 2001 From: shemogumbe Date: Wed, 19 Feb 2025 15:24:57 +0300 Subject: [PATCH 3/5] remove call to get error from response --- packages/http/guzzle/src/GuzzleRequestAdapter.php | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/packages/http/guzzle/src/GuzzleRequestAdapter.php b/packages/http/guzzle/src/GuzzleRequestAdapter.php index cebc7af..3c24cbb 100644 --- a/packages/http/guzzle/src/GuzzleRequestAdapter.php +++ b/packages/http/guzzle/src/GuzzleRequestAdapter.php @@ -580,7 +580,7 @@ private function isRedirectMissingLocation( * Gets the appropriate error from the response * * @param ResponseInterface $response The HTTP response - * @param array> $errorMap Mapping of status codes to error types + * @param array $errorMap Mapping of status codes to error types * @param string $responseStatusCodeStr Status code as string * @param int $responseStatusCode Status code as integer * @param SpanInterface $attributeSpan Attribute tracing span @@ -805,14 +805,6 @@ private function throwFailedResponse(ResponseInterface $response, ?array $errorM $spanForDeserialization->end(); } - $error ?? $this->getErrorFromResponse( - $response, - $errorMappings, - $statusCodeAsString, - $statusCode, - $errorSpan, - $span - ); if ($error && is_subclass_of($error, ApiException::class)) { $span->setAttribute(self::ERROR_BODY_FOUND_ATTRIBUTE_NAME, true); $error->setResponseStatusCode($response->getStatusCode()); From c974218d6397f9c66ef1a59d2e6c1acfccc824a0 Mon Sep 17 00:00:00 2001 From: shemogumbe Date: Wed, 19 Feb 2025 15:28:23 +0300 Subject: [PATCH 4/5] removed unused method --- .../http/guzzle/src/GuzzleRequestAdapter.php | 56 +------------------ 1 file changed, 1 insertion(+), 55 deletions(-) diff --git a/packages/http/guzzle/src/GuzzleRequestAdapter.php b/packages/http/guzzle/src/GuzzleRequestAdapter.php index 3c24cbb..46db329 100644 --- a/packages/http/guzzle/src/GuzzleRequestAdapter.php +++ b/packages/http/guzzle/src/GuzzleRequestAdapter.php @@ -576,61 +576,7 @@ private function isRedirectMissingLocation( return true; } - /** - * Gets the appropriate error from the response - * - * @param ResponseInterface $response The HTTP response - * @param array $errorMap Mapping of status codes to error types - * @param string $responseStatusCodeStr Status code as string - * @param int $responseStatusCode Status code as integer - * @param SpanInterface $attributeSpan Attribute tracing span - * @param SpanInterface $throwFailedRespSpan Failed response tracing span - * @return object|null The parsed error object or null - */ - private function getErrorFromResponse( - ResponseInterface $response, - array $errorMap, - string $responseStatusCodeStr, - int $responseStatusCode, - SpanInterface $attributeSpan, - SpanInterface $throwFailedRespSpan - ): ?object { - // Try exact status code match - $errorClass = $errorMap[$responseStatusCodeStr] ?? null; - - if (!$errorClass) { - // Try range match (4XX, 5XX) - $statusCodeRange = floor($responseStatusCode / 100) . 'XX'; - $errorClass = $errorMap[$statusCodeRange] ?? null; - - // Fallback to generic error (XXX) - if (!$errorClass) { - $errorClass = $errorMap['XXX'] ?? null; - } - } - - if ($errorClass) { - try { - $responseBody = (string)$response->getBody(); - if (empty($responseBody)) { - return null; - } - - // Assuming we have a factory method to create the error object - return $errorClass::createFromDiscriminatorValue( - json_decode($responseBody, true) - ); - } catch (Exception $e) { - $attributeSpan->setStatus(StatusCode::STATUS_ERROR); - $throwFailedRespSpan->setStatus(StatusCode::STATUS_ERROR); - // Log the error if needed - return null; - } - } - - return null; - } - + /** * Authenticates and executes the request * From 563aa3ff92120dfe49b88319fbec469e1915743b Mon Sep 17 00:00:00 2001 From: shemogumbe Date: Wed, 19 Feb 2025 15:50:29 +0300 Subject: [PATCH 5/5] fix send async test --- .../guzzle/tests/GuzzleRequestAdapterTest.php | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/http/guzzle/tests/GuzzleRequestAdapterTest.php b/packages/http/guzzle/tests/GuzzleRequestAdapterTest.php index 36d96e7..d29445c 100644 --- a/packages/http/guzzle/tests/GuzzleRequestAdapterTest.php +++ b/packages/http/guzzle/tests/GuzzleRequestAdapterTest.php @@ -113,7 +113,24 @@ public function testGetPsrRequestFromRequestInformation(): void public function testSendAsync(): void { - $requestAdapter = $this->mockRequestAdapter([new Response(200, ['Content-Type' => $this->contentType])]); + $requestAdapter = $this->mockRequestAdapter([new Response(200, ['Content-Type' => $this->contentType], '{"id": 1}')]); + + $parseNode = $this->createStub(ParseNode::class); + $parseNode->method('getObjectValue') + ->willReturn(new TestUser(1)); + + $parseNodeFactory = $this->createStub(ParseNodeFactory::class); + $parseNodeFactory->method('getRootParseNode') + ->willReturn($parseNode); + + $requestAdapter = new GuzzleRequestAdapter( + $this->authenticationProvider, + $parseNodeFactory, + $this->createMock(SerializationWriterFactory::class), + new Client(['handler' => new MockHandler([new Response(200, ['Content-Type' => $this->contentType], '{"id": 1}')])]) + ); + $requestAdapter->setBaseUrl($this->baseUrl); + $promise = $requestAdapter->sendAsync($this->requestInformation, array(TestUser::class, 'createFromDiscriminatorValue')); $this->assertInstanceOf(TestUser::class, $promise->wait()); }