Skip to content

Commit d97a553

Browse files
NdirituNdiritu
authored
Merge pull request #453 from guilliamxavier/fix-451
HOT FIX: Fix BC break in "Fix truncated error messages"
2 parents 1e24d86 + 0f5d915 commit d97a553

File tree

6 files changed

+129
-34
lines changed

6 files changed

+129
-34
lines changed

‎src/Core/ExceptionWrapper.php‎

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
namespace Microsoft\Graph\Core;
1919

2020
use GuzzleHttp\Exception\BadResponseException;
21-
use Microsoft\Graph\Exception\GraphException;
2221

2322
/**
2423
* Class ExceptionWrapper
@@ -35,12 +34,44 @@ class ExceptionWrapper
3534
* Adds response body to the exception message.
3635
*
3736
* @param BadResponseException $ex
38-
* @return GraphException containing HTTP response from Graph API
39-
*
37+
* @return BadResponseException containing HTTP response from Graph API
4038
*/
4139
public static function wrapGuzzleBadResponseException(BadResponseException $ex)
4240
{
43-
$errMsg = "Received {$ex->getResponse()->getStatusCode()} for call to {$ex->getRequest()->getUri()}\nAPI response: {$ex->getResponse()->getBody()->getContents()}";
44-
return new GraphException($errMsg);
41+
$response = $ex->getResponse();
42+
43+
// Safety check for Guzzle < 7.0
44+
if (!$response) {
45+
return $ex;
46+
}
47+
48+
/** @see \GuzzleHttp\Exception\RequestException::create() */
49+
if (preg_match('/^(.+: `.+ .+` resulted in a `.+ .+` response):\n/U', $ex->getMessage(), $match)) {
50+
$message = $match[1];
51+
52+
$body = $response->getBody();
53+
54+
if (!$body->isSeekable() || !$body->isReadable()) {
55+
return $ex;
56+
}
57+
58+
$summary = $body->getContents();
59+
$body->rewind();
60+
61+
if ($summary !== '') {
62+
$message .= ":\n{$summary}\n";
63+
64+
//return new $ex($message, $ex->getRequest(), $ex->getResponse(), $ex, $ex->getHandlerContext());
65+
// Better: modify internal message inside original exception object (preserves the stack trace)
66+
(new class() extends \Exception {
67+
public static function overwriteProtectedMessage(\Exception $ex, $message)
68+
{
69+
$ex->message = $message;
70+
}
71+
})::overwriteProtectedMessage($ex, $message);
72+
}
73+
}
74+
75+
return $ex;
4576
}
46-
}
77+
}

‎src/Http/GraphCollectionRequest.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ public function __construct($requestType, $endpoint, $accessToken, $baseUrl, $ap
9797
*
9898
* @return int the number of entries
9999
* @throws GraphException
100+
* @throws \GuzzleHttp\Exception\GuzzleException
100101
*/
101102
public function count()
102103
{
@@ -143,7 +144,7 @@ public function setPageSize($pageSize)
143144
* Gets the next page of results
144145
*
145146
* @return array of objects of class $returnType
146-
* @throws GraphException
147+
* @throws \GuzzleHttp\Exception\GuzzleException
147148
*/
148149
public function getPage()
149150
{
@@ -236,4 +237,4 @@ public function getDeltaLink()
236237
{
237238
return $this->deltaLink;
238239
}
239-
}
240+
}

‎src/Http/GraphRequest.php‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,7 @@ public function getTimeout()
293293
*
294294
* @param mixed $client The client to use in the request
295295
*
296-
* @throws GraphException if response is invalid; if 4xx/5xx is returned and $http_errors is true
296+
* @throws \GuzzleHttp\Exception\GuzzleException
297297
*
298298
* @return mixed object or array of objects
299299
* of class $returnType
@@ -401,8 +401,9 @@ function ($reason) {
401401
* @param string $path The path to download the file to
402402
* @param mixed $client The client to use in the request
403403
*
404-
* @throws GraphException if file path is invalid; if \GuzzleHttp\Exception\BadResponseException is thrown for 4xx/5xx responses
405-
*
404+
* @throws GraphException if file path is invalid
405+
* @throws \GuzzleHttp\Exception\GuzzleException
406+
*
406407
* @return null
407408
*/
408409
public function download($path, $client = null)
@@ -444,8 +445,9 @@ public function download($path, $client = null)
444445
* @param string $path The path of the file to upload
445446
* @param mixed $client The client to use in the request
446447
*
447-
* @throws GraphException if file is invalid
448-
*
448+
* @throws GraphException if file is invalid
449+
* @throws \GuzzleHttp\Exception\GuzzleException
450+
*
449451
* @return mixed DriveItem or array of DriveItems
450452
*/
451453
public function upload($path, $client = null)
@@ -540,4 +542,4 @@ protected function createGuzzleClient()
540542

541543
return $client;
542544
}
543-
}
545+
}

‎tests/Core/ExceptionWrapperTest.php‎

Lines changed: 71 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,91 @@
11
<?php
2-
32
use GuzzleHttp\Exception\BadResponseException;
43
use GuzzleHttp\Psr7\Request;
54
use GuzzleHttp\Psr7\Response;
65
use Microsoft\Graph\Core\ExceptionWrapper;
7-
use Microsoft\Graph\Exception\GraphException;
86
use PHPUnit\Framework\TestCase;
97

108
class ExceptionWrapperTest extends TestCase
119
{
12-
public function testWrapBadResponseExceptionReturnsGraphException()
10+
protected $responseBodies;
11+
protected $autoBadResponseExceptions;
12+
protected $manualBadResponseExceptions;
13+
14+
public function setUp(): void
1315
{
14-
$responseBody = json_encode(array('body' => 'content'));
15-
$ex = new BadResponseException("Error: API returned 400", new Request("GET", "/endpoint"), new Response(400, [], $responseBody));
16-
$graphException = ExceptionWrapper::wrapGuzzleBadResponseException($ex);
17-
$this->assertInstanceOf(GraphException::class, $graphException);
16+
$this->responseBodies = array(
17+
'short' => json_encode(array('body' => 'content')), // not truncated by Guzzle
18+
'long' => json_encode(array('body' => base64_encode(random_bytes(120)) . '.')), // truncated by Guzzle
19+
);
20+
21+
$this->autoBadResponseExceptions = array();
22+
$this->manualBadResponseExceptions = array();
23+
foreach ($this->responseBodies as $name => $responseBody) {
24+
$autoBadResponseException = GuzzleHttp\Exception\RequestException::create(new Request("GET", "/endpoint"), new Response(400, [], $responseBody));
25+
assert($autoBadResponseException instanceof BadResponseException);
26+
$this->autoBadResponseExceptions[$name] = $autoBadResponseException;
27+
28+
$manualBadResponseException = new BadResponseException("Error: API returned 400", new Request("GET", "/endpoint"), new Response(400, [], $responseBody));
29+
$this->manualBadResponseExceptions[$name] = $manualBadResponseException;
30+
}
1831
}
1932

20-
public function testWrapBadResponseExceptionHasResponseBody()
33+
public function testWrapBadResponseExceptionReturnsInstanceOfSameClass()
2134
{
22-
$responseBody = json_encode(array('body' => 'content'));
23-
$ex = new BadResponseException("Error: API returned 400", new Request("GET", "/endpoint"), new Response(400, [], $responseBody));
24-
$graphException = ExceptionWrapper::wrapGuzzleBadResponseException($ex);
25-
$this->assertStringContainsString($responseBody, $graphException->getMessage());
35+
$name = 'short';
36+
37+
$ex = $this->autoBadResponseExceptions[$name];
38+
$wrappedException = ExceptionWrapper::wrapGuzzleBadResponseException($ex);
39+
$this->assertInstanceOf(get_class($ex), $wrappedException);
40+
41+
$ex = $this->manualBadResponseExceptions[$name];
42+
$wrappedException = ExceptionWrapper::wrapGuzzleBadResponseException($ex);
43+
$this->assertInstanceOf(get_class($ex), $wrappedException);
44+
45+
$name = 'long';
46+
47+
$ex = $this->autoBadResponseExceptions[$name];
48+
$wrappedException = ExceptionWrapper::wrapGuzzleBadResponseException($ex);
49+
$this->assertInstanceOf(get_class($ex), $wrappedException);
50+
51+
$ex = $this->manualBadResponseExceptions[$name];
52+
$wrappedException = ExceptionWrapper::wrapGuzzleBadResponseException($ex);
53+
$this->assertInstanceOf(get_class($ex), $wrappedException);
54+
}
55+
56+
public function testWrapAutoBadResponseExceptionHasResponseBody()
57+
{
58+
$name = 'short';
59+
$responseBody = $this->responseBodies[$name];
60+
$ex = $this->autoBadResponseExceptions[$name];
61+
$wrappedException = ExceptionWrapper::wrapGuzzleBadResponseException($ex);
62+
$this->assertStringContainsString($responseBody, $wrappedException->getMessage());
63+
64+
$name = 'long';
65+
$responseBody = $this->responseBodies[$name];
66+
$ex = $this->autoBadResponseExceptions[$name];
67+
$wrappedException = ExceptionWrapper::wrapGuzzleBadResponseException($ex);
68+
$this->assertStringContainsString($responseBody, $wrappedException->getMessage());
69+
}
70+
71+
public function testWrapManualBadResponseExceptionHasNotResponseBody()
72+
{
73+
$name = 'short';
74+
$responseBody = $this->responseBodies[$name];
75+
$ex = $this->manualBadResponseExceptions[$name];
76+
$wrappedException = ExceptionWrapper::wrapGuzzleBadResponseException($ex);
77+
$this->assertStringNotContainsString($responseBody, $wrappedException->getMessage());
78+
79+
$name = 'long';
80+
$responseBody = $this->responseBodies[$name];
81+
$ex = $this->manualBadResponseExceptions[$name];
82+
$wrappedException = ExceptionWrapper::wrapGuzzleBadResponseException($ex);
83+
$this->assertStringNotContainsString($responseBody, $wrappedException->getMessage());
2684
}
2785

2886
public function testWrapBadResponseExceptionWithInvalidInput()
2987
{
3088
$this->expectException(TypeError::class);
3189
ExceptionWrapper::wrapGuzzleBadResponseException(null);
3290
}
33-
}
91+
}

‎tests/Http/GraphRequestTest.php‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
<?php
2-
32
use GuzzleHttp\Psr7\Response;
43
use PHPUnit\Framework\TestCase;
54
use Microsoft\Graph\Core\GraphConstants;
6-
use Microsoft\Graph\Exception\GraphException;
75
use Microsoft\Graph\Graph;
86
use Microsoft\Graph\Http\GraphRequest;
97
use Microsoft\Graph\Http\Test\MockClientFactory;
@@ -170,6 +168,11 @@ public function testExecuteAsync()
170168
->executeAsync($this->client);
171169
$this->assertInstanceOf(GuzzleHttp\Promise\PromiseInterface::class, $promise);
172170

171+
$promise = $this->requests[0]
172+
->executeAsync($this->client);
173+
$promise2 = $this->requests[2]
174+
->executeAsync($this->client);
175+
173176
$response = \GuzzleHttp\Promise\unwrap(array($promise));
174177
foreach ($response as $responseItem) {
175178
$this->assertInstanceOf(Microsoft\Graph\Http\GraphResponse::class, $responseItem);
@@ -204,20 +207,20 @@ public function testGetConcatenator()
204207

205208
public function testExecuteWith4xxResponse()
206209
{
207-
$this->expectException(GraphException::class);
210+
$this->expectException(GuzzleHttp\Exception\ClientException::class);
208211
$mockResponse = array(new Response(400));
209212
$client = MockClientFactory::create(['http_errors' => true], $mockResponse);
210213
$this->requests[0]->execute($client);
211214
}
212215

213216
public function testExecuteWith5xxResponse()
214217
{
215-
$this->expectException(GraphException::class);
218+
$this->expectException(GuzzleHttp\Exception\ServerException::class);
216219
$mockResponse = array(new Response(500));
217220
$client = MockClientFactory::create(['http_errors' => true], $mockResponse);
218221
$this->requests[0]->execute($client);
219222
}
220-
223+
221224
public function testExecuteAsyncWithBadResponseTriggersNotice()
222225
{
223226
$this->expectNotice();

‎tests/Http/HttpTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ public function testDelete()
8484

8585
public function testInvalidVerb()
8686
{
87-
$this->expectException(GraphException::class);
87+
$this->expectException(GuzzleHttp\Exception\ClientException::class);
8888

8989
$mock = new MockHandler([
9090
new Response(400, ['foo' => 'bar'])
@@ -156,4 +156,4 @@ public function testSendStream()
156156
$this->assertInstanceOf(Microsoft\Graph\Http\GraphResponse::class, $response);
157157
$this->assertEquals($body, $this->container[0]['request']->getBody()->getContents());
158158
}
159-
}
159+
}

0 commit comments

Comments
 (0)