Merge branch 'master' into facet

CHANGELOG.md

@@ -6,6 +6,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 ### Fixed
+- ParallelExecution adds a HttpException instead of an empty Result in case of an endpoint failure
 - Facet pivot stats contain all fields (previously only the last field was present in the result)
 - Facet pivot stats return `NAN` for a mean value that's NaN
 - Facet pivot stats return an associative array for percentiles

docs/client-and-adapters.md

@@ -198,7 +198,6 @@ If your application does many Solr requests during a single PHP process, conside
 Below example registers such a PSR-18 Client with a timeout of 120 seconds.
 
 ```sh
-composer require psr/http-client
 composer require nyholm/psr7
 composer require symfony/http-client
 ```

docs/exceptions.md

@@ -126,6 +126,8 @@ Those that extend their SPL counterparts do so to implement the marker interface
 
 This exception indicates that a problem occurred in the communication with the Solr server. You should catch this (or a more generic exception) for every request that is executed.
 
+When using the [ParallelExecution plugin](plugins.md#parallelexecution-plugin), this exception isn't thrown. Instead it's added to the result array and you have to check the type in your code.
+
 
 #### `StreamException`
 

docs/plugins.md

@@ -450,7 +450,7 @@ This plugin makes it possible to execute multiple Solr queries at the same time,
 -   If you construct the client with a different adapter, this plugin will replace it with a cURL adapter. Construct the client with a properly configured cURL adapter if you need proxy support.
 -   Only request execution is parallel, request preparation and result parsing cannot be done parallelly. Luckily these parts cost very little time, far more time is in the requests.
 -   The execution time is limited by the slowest request. If you execute 3 queries with timings of 0.2, 0.4 and 1.2 seconds the execution time for all will be (near) 1.2 seconds.
--   If one of the requests fails the other requests will still be executed and the results parsed. In the result array the entry for the failed query will contain an exception instead of a result object. It's your own responsibility to check the result type.
+-   If one of the requests fails the other requests will still be executed and the results parsed. In the result array the entry for the failed query will contain an `HttpException` instead of a `Result` object. It's your own responsibility to check the result type.
 -   All query types are supported, and you can even mix query types in the same `execute` call.
 -   For testing this plugin you can use a special Solr delay component I’ve created (and used to develop the plugin). For more info see [this archived blog post](https://web.archive.org/web/20170904162800/http://www.raspberry.nl/2012/01/04/solr-delay-component/).
 -   Add queries using the `addQuery` method. Supply at least a key and a query instance. Optionally you can supply a client instance as third argument. This can be used to execute queries on different cores or even servers. If omitted the plugin will use its own client instance.

src/Core/Client/Adapter/Curl.php

@@ -29,7 +29,7 @@ class Curl extends Configurable implements AdapterInterface, TimeoutAwareInterfa
     use ProxyAwareTrait;
 
     /**
-     * Execute a Solr request using the cURL Http.
+     * Execute a Solr request using the cURL library.
      *
      * @param Request  $request
      * @param Endpoint $endpoint
@@ -44,27 +44,26 @@ public function execute(Request $request, Endpoint $endpoint): Response
     /**
      * Get the response for a cURL handle.
      *
-     * @param \CurlHandle       $handle
-     * @param string|false|null $httpResponse
+     * @param \CurlHandle  $handle
+     * @param string|false $httpResponse
+     *
+     * @throws HttpException
      *
      * @return Response
      */
     public function getResponse(\CurlHandle $handle, $httpResponse): Response
     {
-        if (false !== $httpResponse && null !== $httpResponse) {
-            $data = $httpResponse;
-            $info = curl_getinfo($handle);
-            $headers = [];
-            $headers[] = 'HTTP/1.1 '.$info['http_code'].' OK';
-        } else {
-            $headers = [];
-            $data = '';
+        if (CURLE_OK !== curl_errno($handle)) {
+            throw new HttpException(sprintf('HTTP request failed, %s', curl_error($handle)));
         }
 
-        $this->check($data, $headers, $handle);
+        $httpCode = curl_getinfo($handle, CURLINFO_RESPONSE_CODE);
+        $headers = [];
+        $headers[] = 'HTTP/1.1 '.$httpCode.' OK';
+
         curl_close($handle);
 
-        return new Response($data, $headers);
+        return new Response($httpResponse, $headers);
     }
 
     /**
@@ -154,24 +153,6 @@ public function createHandle(Request $request, Endpoint $endpoint): \CurlHandle
         return $handler;
     }
 
-    /**
-     * Check result of a request.
-     *
-     * @param string      $data
-     * @param array       $headers
-     * @param \CurlHandle $handle
-     *
-     * @throws HttpException
-     */
-    public function check($data, $headers, \CurlHandle $handle): void
-    {
-        // if there is no data and there are no headers it's a total failure,
-        // a connection to the host was impossible.
-        if (empty($data) && 0 === \count($headers)) {
-            throw new HttpException(sprintf('HTTP request failed, %s', curl_error($handle)));
-        }
-    }
-
     public function setOption(string $name, $value): Configurable
     {
         if ('proxy' === $name) {

src/Core/Client/Adapter/Http.php

@@ -39,26 +39,12 @@ public function execute(Request $request, Endpoint $endpoint): Response
 
         list($data, $headers) = $this->getData($uri, $context);
 
-        $this->check($data, $headers);
-
-        return new Response($data, $headers);
-    }
-
-    /**
-     * Check result of a request.
-     *
-     * @param string $data
-     * @param array  $headers
-     *
-     * @throws HttpException
-     */
-    public function check($data, $headers): void
-    {
-        // if there is no data and there are no headers it's a total failure,
-        // a connection to the host was impossible.
-        if (false === $data && 0 === \count($headers)) {
+        // if false instead of response data it's a total failure
+        if (false === $data) {
             throw new HttpException('HTTP request failed');
         }
+
+        return new Response($data, $headers);
     }
 
     /**
@@ -171,7 +157,7 @@ public function createContext(Request $request, Endpoint $endpoint)
      *
      * @return array
      */
-    protected function getData($uri, $context)
+    protected function getData(string $uri, $context): array
     {
         $data = @file_get_contents($uri, false, $context);
 

src/Plugin/ParallelExecution/ParallelExecution.php

@@ -19,6 +19,7 @@
 use Solarium\Core\Event\PreExecuteRequest as PreExecuteRequestEvent;
 use Solarium\Core\Plugin\AbstractPlugin;
 use Solarium\Core\Query\QueryInterface;
+use Solarium\Core\Query\Result\Result;
 use Solarium\Exception\HttpException;
 use Solarium\Exception\RuntimeException;
 use Solarium\Plugin\ParallelExecution\Event\ExecuteEnd as ExecuteEndEvent;
@@ -105,7 +106,7 @@ public function clearQueries(): self
      *
      * @throws RuntimeException
      *
-     * @return \Solarium\Core\Query\Result\Result[]
+     * @return (Result|HttpException)[]
      */
     public function execute(): array
     {
@@ -167,6 +168,10 @@ public function execute(): array
             } while (CURLM_CALL_MULTI_PERFORM === $mrc);
         }
 
+        while (false !== curl_multi_info_read($multiHandle)) {
+            // ↑ this loops over messages from the individual transfers so we can get curl_errno() for each handle
+        }
+
         $event = new ExecuteEndEvent();
         $this->client->getEventDispatcher()->dispatch($event);
 

tests/Core/Client/Adapter/CurlTest.php

@@ -79,24 +79,6 @@ public function testSetNonProxyOption()
         $this->assertSame('bar', $this->adapter->getOption('foo'));
     }
 
-    public function testCheck()
-    {
-        $data = 'data';
-        $headers = ['X-dummy: data'];
-        $handle = curl_init();
-
-        // this should be ok, no exception
-        $this->adapter->check($data, $headers, $handle);
-
-        $data = '';
-        $headers = [];
-
-        $this->expectException(HttpException::class);
-        $this->adapter->check($data, $headers, $handle);
-
-        curl_close($handle);
-    }
-
     public function testExecute()
     {
         $headers = ['HTTP/1.0 200 OK'];
@@ -121,22 +103,15 @@ public function testExecute()
         $this->assertSame($data, $response);
     }
 
-    /**
-     * @testWith [false]
-     *           [null]
-     *
-     * @param mixed $httpResponse
-     */
-    public function testGetResponseWithEmptyHttpResponse($httpResponse)
+    public function testGetResponseThrowsOnFailure()
     {
-        $handle = curl_init();
+        $handle = curl_init('example.invalid');
+        curl_exec($handle);
 
         $this->expectException(HttpException::class);
-        $response = $this->adapter->getResponse($handle, $httpResponse);
+        $this->adapter->getResponse($handle, false);
 
         curl_close($handle);
-
-        $this->assertEquals(new Response('', []), $response);
     }
 
     /**
@@ -185,6 +160,20 @@ public function testCreateHandleForPostRequestWithFileUpload()
         curl_close($handle);
     }
 
+    public function testCreateHandleWithCustomRequestHeaders()
+    {
+        $request = new Request();
+        $request->addHeader('X-Header: value');
+        $request->setIsServerRequest(true);
+        $endpoint = new Endpoint();
+
+        $handle = $this->adapter->createHandle($request, $endpoint);
+
+        $this->assertInstanceOf(\CurlHandle::class, $handle);
+
+        curl_close($handle);
+    }
+
     public function testCreateHandleWithUnknownMethod()
     {
         $request = new Request();

tests/Core/Client/Adapter/HttpTest.php

@@ -35,7 +35,7 @@ public function testExecute()
 
         /** @var Http|MockObject $mock */
         $mock = $this->getMockBuilder(Http::class)
-            ->onlyMethods(['getData', 'check'])
+            ->onlyMethods(['getData'])
             ->getMock();
 
         $mock->expects($this->once())
@@ -48,41 +48,24 @@ public function testExecute()
 
     public function testExecuteErrorResponse()
     {
-        $data = 'test123';
-
         $request = new Request();
         $request->setIsServerRequest(true);
         $endpoint = new Endpoint();
 
         /** @var Http|MockObject $mock */
         $mock = $this->getMockBuilder(Http::class)
-            ->onlyMethods(['getData', 'check'])
+            ->onlyMethods(['getData'])
             ->getMock();
 
         $mock->expects($this->once())
              ->method('getData')
              ->with($this->equalTo('http://127.0.0.1:8983/solr/'), $this->isType('resource'))
-             ->willReturn([$data, ['HTTP 1.1 200 OK']]);
-        $mock->expects($this->once())
-             ->method('check')
-             ->will($this->throwException(new HttpException('HTTP request failed')));
+             ->willReturn([false, []]);
 
         $this->expectException(HttpException::class);
         $mock->execute($request, $endpoint);
     }
 
-    public function testCheckError()
-    {
-        $this->expectException(HttpException::class);
-        $this->adapter->check(false, []);
-    }
-
-    public function testCheckOk()
-    {
-        $this->expectNotToPerformAssertions();
-        $this->adapter->check('dummydata', ['HTTP 1.1 200 OK']);
-    }
-
     public function testCreateContextGetRequest()
     {
         $timeout = 13;

tests/Core/Client/ResponseTest.php

@@ -19,7 +19,10 @@ class ResponseTest extends TestCase
 
     public function setUp(): void
     {
-        $this->headers = ['HTTP/1.0 304 Not Modified'];
+        $this->headers = [
+            'HTTP/1.0 304 Not Modified',
+            'X-Header: value',
+        ];
         $this->data = '{"responseHeader":{"status":0,"QTime":1,"params":{"wt":"json","q":"mdsfgdsfgdf"}},'.
             '"response":{"numFound":0,"start":0,"docs":[]}}';
         $this->response = new Response($this->data, $this->headers);

tests/Integration/AbstractTechproductsTestCase.php

@@ -37,6 +37,7 @@
 use Solarium\Plugin\Loadbalancer\Event\EndpointFailure as LoadbalancerEndpointFailureEvent;
 use Solarium\Plugin\Loadbalancer\Event\Events as LoadbalancerEvents;
 use Solarium\Plugin\Loadbalancer\Loadbalancer;
+use Solarium\Plugin\ParallelExecution\ParallelExecution;
 use Solarium\Plugin\PrefetchIterator;
 use Solarium\QueryType\Luke\Query as LukeQuery;
 use Solarium\QueryType\Luke\Result\Doc\DocFieldInfo as LukeDocFieldInfo;
@@ -3690,13 +3691,20 @@ public function testParallelExecution()
             }
         );
 
+        $invalidEndpointConfig = self::$config['endpoint']['localhost'];
+        $invalidEndpointConfig['host'] = 'server.invalid';
+        $invalidEndpointConfig['key'] = 'invalid';
+        $invalidEndpoint = self::$client->createEndpoint($invalidEndpointConfig);
+
+        /** @var ParallelExecution $parallel */
         $parallel = self::$client->getPlugin('parallelexecution');
         $parallel->addQuery('instock', $queryInStock);
         $parallel->addQuery('lowprice', $queryLowPrice);
         $parallel->addQuery('bigrequest', $queryBigRequest);
         $parallel->addQuery('overrideresult', $queryOverrideResult);
         $parallel->addQuery('overrideresponse', $queryOverrideResponse);
         $parallel->addQuery('error', $queryError);
+        $parallel->addQuery('endpointfailure', $queryInStock, $invalidEndpoint);
         $parallel->addQuery('server', $serverQuery);
         $results = $parallel->execute();
 
@@ -3708,6 +3716,7 @@ public function testParallelExecution()
             'overrideresult',
             'overrideresponse',
             'error',
+            'endpointfailure',
             'server',
         ];
         $this->assertSame($expectedKeys, array_keys($results));
@@ -3718,13 +3727,16 @@ public function testParallelExecution()
         $this->assertEquals($resultOverrideResult, $results['overrideresult']);
         $this->assertEquals($resultOverrideResponse, $results['overrideresponse']);
         $this->assertInstanceOf(HttpException::class, $results['error']);
+        $this->assertInstanceOf(HttpException::class, $results['endpointfailure']);
+        $this->assertStringContainsString('HTTP request failed, ', $results['endpointfailure']->getMessage());
         $this->assertEquals($serverResult, $results['server']);
 
         // cleanup
         self::$client->getEventDispatcher()->removeListener(Events::PRE_EXECUTE, $overrideResult);
         self::$client->getEventDispatcher()->removeListener(Events::PRE_EXECUTE_REQUEST, $overrideResponse);
         self::$client->removePlugin('parallelexecution');
         self::$client->removePlugin('postbigrequest');
+        self::$client->removeEndpoint('invalid');
     }
 
     public function testPrefetchIterator()