Skip to content

Commit 06490f6

Browse files
ssddanbrownssddanbrown
committed
Removed use of HttpFetcher
- Fixed some existing issues in new aligned process. - Manually tested each external call scenario.
1 parent a8b5652 commit 06490f6

7 files changed

Lines changed: 54 additions & 104 deletions

File tree

‎app/Activity/DispatchWebhookJob.php‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
use Illuminate\Queue\InteractsWithQueue;
1717
use Illuminate\Queue\SerializesModels;
1818
use Illuminate\Support\Facades\Log;
19-
use Psr\Http\Client\ClientExceptionInterface;
2019

2120
class DispatchWebhookJob implements ShouldQueue
2221
{
@@ -69,7 +68,7 @@ public function handle(HttpRequestService $http)
6968
$lastError = "Response status from endpoint was {$statusCode}";
7069
Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with status {$statusCode}");
7170
}
72-
} catch (ClientExceptionInterface $error) {
71+
} catch (\Exception $error) {
7372
$lastError = $error->getMessage();
7473
Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with error \"{$lastError}\"");
7574
}

‎app/Http/HttpClientHistory.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,4 +25,9 @@ public function latestRequest(): ?GuzzleRequest
2525
{
2626
return $this->requestAt($this->requestCount() - 1);
2727
}
28+
29+
public function all(): array
30+
{
31+
return $this->container;
32+
}
2833
}

‎app/Http/HttpRequestService.php‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use GuzzleHttp\HandlerStack;
88
use GuzzleHttp\Middleware;
99
use GuzzleHttp\Psr7\Request as GuzzleRequest;
10+
use GuzzleHttp\Psr7\Response;
1011
use Psr\Http\Client\ClientInterface;
1112

1213
class HttpRequestService
@@ -16,7 +17,7 @@ class HttpRequestService
1617
/**
1718
* Build a new http client for sending requests on.
1819
*/
19-
public function buildClient(int $timeout, array $options): ClientInterface
20+
public function buildClient(int $timeout, array $options = []): ClientInterface
2021
{
2122
$defaultOptions = [
2223
'timeout' => $timeout,
@@ -40,8 +41,16 @@ public function jsonRequest(string $method, string $uri, array $data): GuzzleReq
4041
* Returns history which can then be queried.
4142
* @link https://docs.guzzlephp.org/en/stable/testing.html#history-middleware
4243
*/
43-
public function mockClient(array $responses = []): HttpClientHistory
44+
public function mockClient(array $responses = [], bool $pad = true): HttpClientHistory
4445
{
46+
// By default, we pad out the responses with 10 successful values so that requests will be
47+
// properly recorded for inspection. Otherwise, we can't later check if we're received
48+
// too many requests.
49+
if ($pad) {
50+
$response = new Response(200, [], 'success');
51+
$responses = array_merge($responses, array_fill(0, 10, $response));
52+
}
53+
4554
$container = [];
4655
$history = Middleware::history($container);
4756
$mock = new MockHandler($responses);

‎app/Uploads/HttpFetcher.php‎

Lines changed: 0 additions & 38 deletions
This file was deleted.

‎app/Uploads/UserAvatars.php‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,20 @@
33
namespace BookStack\Uploads;
44

55
use BookStack\Exceptions\HttpFetchException;
6+
use BookStack\Http\HttpRequestService;
67
use BookStack\Users\Models\User;
78
use Exception;
9+
use GuzzleHttp\Psr7\Request;
810
use Illuminate\Support\Facades\Log;
911
use Illuminate\Support\Str;
12+
use Psr\Http\Client\ClientExceptionInterface;
1013

1114
class UserAvatars
1215
{
13-
protected $imageService;
14-
protected $http;
15-
16-
public function __construct(ImageService $imageService, HttpFetcher $http)
17-
{
18-
$this->imageService = $imageService;
19-
$this->http = $http;
16+
public function __construct(
17+
protected ImageService $imageService,
18+
protected HttpRequestService $http
19+
) {
2020
}
2121

2222
/**
@@ -112,8 +112,10 @@ protected function createAvatarImageFromData(User $user, string $imageData, stri
112112
protected function getAvatarImageData(string $url): string
113113
{
114114
try {
115-
$imageData = $this->http->fetch($url);
116-
} catch (HttpFetchException $exception) {
115+
$client = $this->http->buildClient(5);
116+
$response = $client->sendRequest(new Request('GET', $url));
117+
$imageData = (string) $response->getBody();
118+
} catch (ClientExceptionInterface $exception) {
117119
throw new HttpFetchException(trans('errors.cannot_get_image_from_url', ['url' => $url]), $exception->getCode(), $exception);
118120
}
119121

@@ -127,7 +129,7 @@ protected function avatarFetchEnabled(): bool
127129
{
128130
$fetchUrl = $this->getAvatarUrl();
129131

130-
return is_string($fetchUrl) && strpos($fetchUrl, 'http') === 0;
132+
return str_starts_with($fetchUrl, 'http');
131133
}
132134

133135
/**

‎tests/TestCase.php‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
use BookStack\Http\HttpClientHistory;
77
use BookStack\Http\HttpRequestService;
88
use BookStack\Settings\SettingService;
9-
use BookStack\Uploads\HttpFetcher;
109
use BookStack\Users\Models\User;
1110
use Illuminate\Contracts\Console\Kernel;
1211
use Illuminate\Foundation\Testing\DatabaseTransactions;
@@ -16,7 +15,6 @@
1615
use Illuminate\Support\Facades\DB;
1716
use Illuminate\Support\Facades\Log;
1817
use Illuminate\Testing\Assert as PHPUnit;
19-
use Mockery;
2018
use Monolog\Handler\TestHandler;
2119
use Monolog\Logger;
2220
use Ssddanbrown\AssertHtml\TestsHtml;
@@ -107,19 +105,6 @@ protected function setSettings(array $settingsArray): void
107105
}
108106
}
109107

110-
/**
111-
* Mock the HttpFetcher service and return the given data on fetch.
112-
*/
113-
protected function mockHttpFetch($returnData, int $times = 1)
114-
{
115-
// TODO - Remove
116-
$mockHttp = Mockery::mock(HttpFetcher::class);
117-
$this->app[HttpFetcher::class] = $mockHttp;
118-
$mockHttp->shouldReceive('fetch')
119-
->times($times)
120-
->andReturn($returnData);
121-
}
122-
123108
/**
124109
* Mock the http client used in BookStack http calls.
125110
*/

‎tests/Uploads/AvatarTest.php‎

Lines changed: 25 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,11 @@
33
namespace Tests\Uploads;
44

55
use BookStack\Exceptions\HttpFetchException;
6-
use BookStack\Uploads\HttpFetcher;
76
use BookStack\Uploads\UserAvatars;
87
use BookStack\Users\Models\User;
8+
use GuzzleHttp\Exception\ConnectException;
9+
use GuzzleHttp\Psr7\Request;
10+
use GuzzleHttp\Psr7\Response;
911
use Tests\TestCase;
1012

1113
class AvatarTest extends TestCase
@@ -22,34 +24,26 @@ protected function createUserRequest($user): User
2224
return User::query()->where('email', '=', $user->email)->first();
2325
}
2426

25-
protected function assertImageFetchFrom(string $url)
26-
{
27-
$http = $this->mock(HttpFetcher::class);
28-
29-
$http->shouldReceive('fetch')
30-
->once()->with($url)
31-
->andReturn($this->files->pngImageData());
32-
}
33-
34-
protected function deleteUserImage(User $user)
27+
protected function deleteUserImage(User $user): void
3528
{
3629
$this->files->deleteAtRelativePath($user->avatar->path);
3730
}
3831

3932
public function test_gravatar_fetched_on_user_create()
4033
{
41-
config()->set([
42-
'services.disable_services' => false,
43-
]);
34+
$requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]);
35+
config()->set(['services.disable_services' => false]);
4436
$user = User::factory()->make();
45-
$this->assertImageFetchFrom('https://www.gravatar.com/avatar/' . md5(strtolower($user->email)) . '?s=500&d=identicon');
4637

4738
$user = $this->createUserRequest($user);
4839
$this->assertDatabaseHas('images', [
4940
'type' => 'user',
5041
'created_by' => $user->id,
5142
]);
5243
$this->deleteUserImage($user);
44+
45+
$expectedUri = 'https://www.gravatar.com/avatar/' . md5(strtolower($user->email)) . '?s=500&d=identicon';
46+
$this->assertEquals($expectedUri, $requests->latestRequest()->getUri());
5347
}
5448

5549
public function test_custom_url_used_if_set()
@@ -61,24 +55,22 @@ public function test_custom_url_used_if_set()
6155

6256
$user = User::factory()->make();
6357
$url = 'https://example.com/' . urlencode(strtolower($user->email)) . '/' . md5(strtolower($user->email)) . '/500';
64-
$this->assertImageFetchFrom($url);
58+
$requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]);
6559

6660
$user = $this->createUserRequest($user);
61+
$this->assertEquals($url, $requests->latestRequest()->getUri());
6762
$this->deleteUserImage($user);
6863
}
6964

7065
public function test_avatar_not_fetched_if_no_custom_url_and_services_disabled()
7166
{
72-
config()->set([
73-
'services.disable_services' => true,
74-
]);
75-
67+
config()->set(['services.disable_services' => true]);
7668
$user = User::factory()->make();
77-
78-
$http = $this->mock(HttpFetcher::class);
79-
$http->shouldNotReceive('fetch');
69+
$requests = $this->mockHttpClient([new Response()]);
8070

8171
$this->createUserRequest($user);
72+
73+
$this->assertEquals(0, $requests->requestCount());
8274
}
8375

8476
public function test_avatar_not_fetched_if_avatar_url_option_set_to_false()
@@ -89,21 +81,18 @@ public function test_avatar_not_fetched_if_avatar_url_option_set_to_false()
8981
]);
9082

9183
$user = User::factory()->make();
92-
93-
$http = $this->mock(HttpFetcher::class);
94-
$http->shouldNotReceive('fetch');
84+
$requests = $this->mockHttpClient([new Response()]);
9585

9686
$this->createUserRequest($user);
87+
88+
$this->assertEquals(0, $requests->requestCount());
9789
}
9890

9991
public function test_no_failure_but_error_logged_on_failed_avatar_fetch()
10092
{
101-
config()->set([
102-
'services.disable_services' => false,
103-
]);
93+
config()->set(['services.disable_services' => false]);
10494

105-
$http = $this->mock(HttpFetcher::class);
106-
$http->shouldReceive('fetch')->andThrow(new HttpFetchException());
95+
$this->mockHttpClient([new ConnectException('Failed to connect', new Request('GET', ''))]);
10796

10897
$logger = $this->withTestLogger();
10998

@@ -122,17 +111,16 @@ public function test_exception_message_on_failed_fetch()
122111

123112
$user = User::factory()->make();
124113
$avatar = app()->make(UserAvatars::class);
125-
$url = 'http_malformed_url/' . urlencode(strtolower($user->email)) . '/' . md5(strtolower($user->email)) . '/500';
126114
$logger = $this->withTestLogger();
115+
$this->mockHttpClient([new ConnectException('Could not resolve host http_malformed_url', new Request('GET', ''))]);
127116

128117
$avatar->fetchAndAssignToUser($user);
129118

119+
$url = 'http_malformed_url/' . urlencode(strtolower($user->email)) . '/' . md5(strtolower($user->email)) . '/500';
130120
$this->assertTrue($logger->hasError('Failed to save user avatar image'));
131121
$exception = $logger->getRecords()[0]['context']['exception'];
132-
$this->assertEquals(new HttpFetchException(
133-
'Cannot get image from ' . $url,
134-
6,
135-
(new HttpFetchException('Could not resolve host: http_malformed_url', 6))
136-
), $exception);
122+
$this->assertInstanceOf(HttpFetchException::class, $exception);
123+
$this->assertEquals('Cannot get image from ' . $url, $exception->getMessage());
124+
$this->assertEquals('Could not resolve host http_malformed_url', $exception->getPrevious()->getMessage());
137125
}
138126
}

0 commit comments

Comments
 (0)