Skip to content

Commit 9c49a16

Browse files
julien-ncjulien-nc
committed
feat(IClient): let guzzle choose the handler for http requests so it can write a streamed response body progressively
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
1 parent 7304083 commit 9c49a16

4 files changed

Lines changed: 159 additions & 55 deletions

File tree

‎lib/private/Http/Client/Client.php‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,15 +41,19 @@ public function __construct(
4141
}
4242

4343
private function buildRequestOptions(array $options): array {
44+
$streamResponse = !empty($options[RequestOptions::STREAM]);
4445
$proxy = $this->getProxyUri();
4546

4647
$defaults = [
4748
RequestOptions::VERIFY => $this->getCertBundle(),
4849
RequestOptions::TIMEOUT => IClient::DEFAULT_REQUEST_TIMEOUT,
49-
// Prefer HTTP/2 globally (PSR-7 request version)
50-
RequestOptions::VERSION => '2.0',
50+
// Guzzle's StreamHandler only supports HTTP/1.x, so streamed
51+
// responses must not force the default HTTP/2 transport settings.
52+
RequestOptions::VERSION => $streamResponse ? '1.1' : '2.0',
5153
];
52-
$defaults['curl'][\CURLOPT_HTTP_VERSION] = \CURL_HTTP_VERSION_2TLS;
54+
$defaults['curl'][\CURLOPT_HTTP_VERSION] = $streamResponse
55+
? \CURL_HTTP_VERSION_1_1
56+
: \CURL_HTTP_VERSION_2TLS;
5357

5458
$options['nextcloud']['allow_local_address'] = $this->isLocalAddressAllowed($options);
5559
if ($options['nextcloud']['allow_local_address'] === false) {
@@ -75,6 +79,12 @@ private function buildRequestOptions(array $options): array {
7579

7680
$options = array_merge($defaults, $options);
7781

82+
if ($streamResponse) {
83+
$options[RequestOptions::VERSION] = '1.1';
84+
$options['curl'] ??= [];
85+
$options['curl'][\CURLOPT_HTTP_VERSION] = \CURL_HTTP_VERSION_1_1;
86+
}
87+
7888
if (!isset($options[RequestOptions::HEADERS]['User-Agent'])) {
7989
$userAgent = 'Nextcloud-Server-Crawler/' . $this->serverVersion->getVersionString();
8090
$overwriteCliUrl = $this->config->getSystemValueString('overwrite.cli.url');

‎lib/private/Http/Client/ClientService.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@
99
namespace OC\Http\Client;
1010

1111
use GuzzleHttp\Client as GuzzleClient;
12-
use GuzzleHttp\Handler\CurlHandler;
1312
use GuzzleHttp\HandlerStack;
1413
use GuzzleHttp\Middleware;
14+
use GuzzleHttp\Utils;
1515
use OCP\Diagnostics\IEventLogger;
1616
use OCP\Http\Client\IClient;
1717
use OCP\Http\Client\IClientService;
@@ -41,7 +41,7 @@ public function __construct(
4141

4242
#[\Override]
4343
public function newClient(): IClient {
44-
$handler = new CurlHandler();
44+
$handler = Utils::chooseHandler();
4545
$stack = HandlerStack::create($handler);
4646
if ($this->config->getSystemValueBool('dns_pinning', true)) {
4747
$stack->push($this->dnsPinMiddleware->addDnsPinning());

‎tests/lib/Http/Client/ClientServiceTest.php‎

Lines changed: 76 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,9 @@
1010

1111
namespace Test\Http\Client;
1212

13-
use GuzzleHttp\Client as GuzzleClient;
14-
use GuzzleHttp\Handler\CurlHandler;
13+
use GuzzleHttp\Handler\MockHandler;
1514
use GuzzleHttp\HandlerStack;
16-
use GuzzleHttp\Middleware;
15+
use GuzzleHttp\Psr7\Response;
1716
use OC\Http\Client\Client;
1817
use OC\Http\Client\ClientService;
1918
use OC\Http\Client\DnsPinMiddleware;
@@ -22,7 +21,6 @@
2221
use OCP\IConfig;
2322
use OCP\Security\IRemoteHostValidator;
2423
use OCP\ServerVersion;
25-
use Psr\Http\Message\RequestInterface;
2624
use Psr\Log\LoggerInterface;
2725

2826
/**
@@ -32,21 +30,38 @@ class ClientServiceTest extends \Test\TestCase {
3230
public function testNewClient(): void {
3331
/** @var IConfig $config */
3432
$config = $this->createMock(IConfig::class);
35-
$config->method('getSystemValueBool')
36-
->with('dns_pinning', true)
37-
->willReturn(true);
33+
$config->method('getSystemValueBool')->willReturnMap([
34+
['dns_pinning', true, true],
35+
['installed', false, false],
36+
['allow_local_remote_servers', false, false],
37+
['http_client_add_user_agent_url', false, false],
38+
]);
3839
/** @var ICertificateManager $certificateManager */
3940
$certificateManager = $this->createMock(ICertificateManager::class);
4041
$dnsPinMiddleware = $this->createMock(DnsPinMiddleware::class);
42+
$dnsMiddleware = static fn (callable $handler): callable => $handler;
4143
$dnsPinMiddleware
4244
->expects($this->atLeastOnce())
4345
->method('addDnsPinning')
44-
->willReturn(function (): void {
45-
});
46+
->willReturn($dnsMiddleware);
4647
$remoteHostValidator = $this->createMock(IRemoteHostValidator::class);
48+
$remoteHostValidator->method('isValid')->willReturn(true);
4749
$eventLogger = $this->createMock(IEventLogger::class);
50+
$eventLogger->expects($this->once())
51+
->method('start')
52+
->with('http:request', 'GET request to /');
53+
$eventLogger->expects($this->once())
54+
->method('end')
55+
->with('http:request');
4856
$logger = $this->createMock(LoggerInterface::class);
4957
$serverVersion = $this->createMock(ServerVersion::class);
58+
$serverVersion->method('getVersionString')->willReturn('1.0.0');
59+
$config->method('getSystemValueString')->willReturnMap([
60+
['proxy', '', ''],
61+
['overwrite.cli.url', '', ''],
62+
]);
63+
$config->method('getSystemValue')->with('proxyexclude', [])->willReturn([]);
64+
$certificateManager->method('getDefaultCertificatesBundlePath')->willReturn('/tmp/certificates.crt');
5065

5166
$clientService = new ClientService(
5267
$config,
@@ -58,27 +73,22 @@ public function testNewClient(): void {
5873
$serverVersion,
5974
);
6075

61-
$handler = new CurlHandler();
62-
$stack = HandlerStack::create($handler);
63-
$stack->push($dnsPinMiddleware->addDnsPinning());
64-
$stack->push(Middleware::tap(function (RequestInterface $request) use ($eventLogger): void {
65-
$eventLogger->start('http:request', $request->getMethod() . ' request to ' . $request->getRequestTarget());
66-
}, function () use ($eventLogger): void {
67-
$eventLogger->end('http:request');
68-
}), 'event logger');
69-
$guzzleClient = new GuzzleClient(['handler' => $stack]);
76+
$client = $clientService->newClient();
77+
$this->assertEquals(new Client(
78+
$config,
79+
$certificateManager,
80+
$this->getGuzzleClient($client),
81+
$remoteHostValidator,
82+
$logger,
83+
$serverVersion,
84+
), $client);
85+
86+
$stack = $this->getHandlerStack($client);
87+
$this->assertStringContainsString("Name: ''", (string)$stack);
88+
$this->assertStringContainsString("Name: 'event logger'", (string)$stack);
7089

71-
$this->assertEquals(
72-
new Client(
73-
$config,
74-
$certificateManager,
75-
$guzzleClient,
76-
$remoteHostValidator,
77-
$logger,
78-
$serverVersion,
79-
),
80-
$clientService->newClient()
81-
);
90+
$stack->setHandler(new MockHandler([new Response(200)]));
91+
$this->assertSame(200, $client->get('https://example.com')->getStatusCode());
8292
}
8393

8494
public function testDisableDnsPinning(): void {
@@ -93,12 +103,25 @@ public function testDisableDnsPinning(): void {
93103
$dnsPinMiddleware
94104
->expects($this->never())
95105
->method('addDnsPinning')
96-
->willReturn(function (): void {
97-
});
106+
->willReturn(static fn (callable $handler): callable => $handler);
98107
$remoteHostValidator = $this->createMock(IRemoteHostValidator::class);
108+
$remoteHostValidator->method('isValid')->willReturn(true);
99109
$eventLogger = $this->createMock(IEventLogger::class);
100110
$logger = $this->createMock(LoggerInterface::class);
101111
$serverVersion = $this->createMock(ServerVersion::class);
112+
$serverVersion->method('getVersionString')->willReturn('1.0.0');
113+
$config->method('getSystemValueBool')->willReturnMap([
114+
['dns_pinning', true, false],
115+
['installed', false, false],
116+
['allow_local_remote_servers', false, false],
117+
['http_client_add_user_agent_url', false, false],
118+
]);
119+
$config->method('getSystemValueString')->willReturnMap([
120+
['proxy', '', ''],
121+
['overwrite.cli.url', '', ''],
122+
]);
123+
$config->method('getSystemValue')->with('proxyexclude', [])->willReturn([]);
124+
$certificateManager->method('getDefaultCertificatesBundlePath')->willReturn('/tmp/certificates.crt');
102125

103126
$clientService = new ClientService(
104127
$config,
@@ -110,25 +133,28 @@ public function testDisableDnsPinning(): void {
110133
$serverVersion,
111134
);
112135

113-
$handler = new CurlHandler();
114-
$stack = HandlerStack::create($handler);
115-
$stack->push(Middleware::tap(function (RequestInterface $request) use ($eventLogger): void {
116-
$eventLogger->start('http:request', $request->getMethod() . ' request to ' . $request->getRequestTarget());
117-
}, function () use ($eventLogger): void {
118-
$eventLogger->end('http:request');
119-
}), 'event logger');
120-
$guzzleClient = new GuzzleClient(['handler' => $stack]);
136+
$client = $clientService->newClient();
137+
$this->assertEquals(new Client(
138+
$config,
139+
$certificateManager,
140+
$this->getGuzzleClient($client),
141+
$remoteHostValidator,
142+
$logger,
143+
$serverVersion,
144+
), $client);
121145

122-
$this->assertEquals(
123-
new Client(
124-
$config,
125-
$certificateManager,
126-
$guzzleClient,
127-
$remoteHostValidator,
128-
$logger,
129-
$serverVersion,
130-
),
131-
$clientService->newClient()
132-
);
146+
$stack = $this->getHandlerStack($client);
147+
$this->assertStringNotContainsString("Name: ''", (string)$stack);
148+
$this->assertStringContainsString("Name: 'event logger'", (string)$stack);
149+
}
150+
151+
private function getGuzzleClient(Client $client): \GuzzleHttp\Client {
152+
return self::invokePrivate($client, 'client');
153+
}
154+
155+
private function getHandlerStack(Client $client): HandlerStack {
156+
/** @var HandlerStack $stack */
157+
$stack = $this->getGuzzleClient($client)->getConfig('handler');
158+
return $stack;
133159
}
134160
}

‎tests/lib/Http/Client/ClientTest.php‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,23 @@ public function testGet(): void {
321321
$this->assertEquals(418, $this->client->get('http://localhost/', [])->getStatusCode());
322322
}
323323

324+
public function testGetStreamUsesHttp11(): void {
325+
$this->setUpDefaultRequestOptions();
326+
327+
$options = array_merge($this->defaultRequestOptions, [
328+
'stream' => true,
329+
'version' => '1.1',
330+
'curl' => [
331+
\CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_1_1,
332+
],
333+
]);
334+
335+
$this->guzzleClient->method('request')
336+
->with('get', 'http://localhost/', $options)
337+
->willReturn(new Response(418));
338+
$this->assertEquals(418, $this->client->get('http://localhost/', ['stream' => true])->getStatusCode());
339+
}
340+
324341
public function testGetWithOptions(): void {
325342
$this->setUpDefaultRequestOptions();
326343

@@ -522,6 +539,57 @@ public function testSetDefaultOptionsWithNotInstalled(): void {
522539
], self::invokePrivate($this->client, 'buildRequestOptions', [[]]));
523540
}
524541

542+
public function testSetDefaultOptionsWithStream(): void {
543+
$this->config
544+
->expects($this->exactly(3))
545+
->method('getSystemValueBool')
546+
->willReturnMap([
547+
['installed', false, true],
548+
['allow_local_remote_servers', false, false],
549+
['http_client_add_user_agent_url', false, false],
550+
]);
551+
$this->config
552+
->expects($this->exactly(2))
553+
->method('getSystemValueString')
554+
->willReturnMap([
555+
['proxy', '', ''],
556+
['overwrite.cli.url', '', ''],
557+
]);
558+
$this->certificateManager
559+
->expects($this->once())
560+
->method('getAbsoluteBundlePath')
561+
->with()
562+
->willReturn('/my/path.crt');
563+
564+
$this->serverVersion->method('getVersionString')
565+
->willReturn('123.45.6');
566+
567+
$this->assertEquals([
568+
'verify' => '/my/path.crt',
569+
'headers' => [
570+
'User-Agent' => 'Nextcloud-Server-Crawler/123.45.6',
571+
'Accept-Encoding' => 'gzip',
572+
],
573+
'timeout' => 30,
574+
'nextcloud' => [
575+
'allow_local_address' => false,
576+
],
577+
'allow_redirects' => [
578+
'on_redirect' => function (
579+
\Psr\Http\Message\RequestInterface $request,
580+
\Psr\Http\Message\ResponseInterface $response,
581+
\Psr\Http\Message\UriInterface $uri,
582+
): void {
583+
},
584+
],
585+
'stream' => true,
586+
'version' => '1.1',
587+
'curl' => [
588+
\CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_1_1,
589+
],
590+
], self::invokePrivate($this->client, 'buildRequestOptions', [['stream' => true]]));
591+
}
592+
525593
public function testSetDefaultOptionsWithProxy(): void {
526594
$this->config
527595
->expects($this->exactly(3))

0 commit comments

Comments
 (0)